Implement Webauthn #8393
Implement Webauthn #8393
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis update introduces comprehensive WebAuthn (passkey) support across the backend and frontend. New backend models, DAOs, and controller endpoints are implemented for WebAuthn credential management, authentication, and registration. The database schema is extended with a Changes
Suggested labels
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
frontend/javascripts/admin/auth/change_password_view.tsx (4)
72-83: Unused variable in registerNewPasskey functionThe result from doWebAuthnRegistration is assigned but never used.
- const result = await doWebAuthnRegistration(newPasskeyName); + await doWebAuthnRegistration(newPasskeyName);Alternatively, if the result is needed for debugging:
- const result = await doWebAuthnRegistration(newPasskeyName); + const _result = await doWebAuthnRegistration(newPasskeyName);🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: This variable is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
Unsafe fix: If this is intentional, prepend result with an underscore.(lint/correctness/noUnusedVariables)
197-232: Improve UI for passkey listingThe passkey list items could benefit from improved styling and layout. The current implementation has the delete button directly next to the passkey name without proper spacing or alignment.
Consider using Ant Design's List component or improving the Row structure:
- {passkeys.map((passkey) => ( - <Row key={passkey.id}> - {passkey.name} - <Button - onClick={async () => { - await removeWebAuthnKey(passkey); - await fetchData(); - }} - > - Delete - </Button> - </Row> - ))} + {passkeys.map((passkey) => ( + <Row key={passkey.id} justify="space-between" align="middle" style={{ marginBottom: 8 }}> + <span>{passkey.name}</span> + <Button + danger + size="small" + onClick={async () => { + await removeWebAuthnKey(passkey); + await fetchPasskeys(); + }} + > + Delete + </Button> + </Row> + ))}
233-244: Consider adding validation for passkey nameThe modal allows submitting empty passkey names. Consider adding validation to prevent empty names or adding a disabled state to the OK button when the name is empty.
<Modal title="Enter a name for the new Passkey" open={isPasskeyNameModalOpen} onOk={registerNewPasskey} + okButtonProps={{ disabled: !newPasskeyName.trim() }} onCancel={() => setIsPasskeyNameModalOpen(false)} >
227-229: Consider adding state for empty passkeys listThe component doesn't show any special indication when the passkeys list is empty. Consider adding a message to guide users when they have no passkeys yet.
Add a conditional before the button:
+ {passkeys.length === 0 && ( + <Alert + type="info" + message="You don't have any passkeys registered yet. Click the button below to add your first passkey." + style={{ marginBottom: 16 }} + /> + )} <div style={{paddingTop: 10}}> <Button onClick={() => setIsPasskeyNameModalOpen(true)} type="primary"> Register new Passkey
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/admin/auth/change_password_view.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/javascripts/admin/auth/change_password_view.tsx
[error] 75-75: This variable is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
Unsafe fix: If this is intentional, prepend result with an underscore.
(lint/correctness/noUnusedVariables)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (4)
frontend/javascripts/admin/auth/change_password_view.tsx (4)
11-17: Well-structured import organizationThe imports have been properly organized to include the necessary WebAuthn-related functions and React hooks needed for the new passkey functionality.
29-33: Good state management setupThe state variables are well-defined with appropriate types and naming conventions. Using the underscore prefix for
_isLoadingPasskeysproperly indicates it's currently not used in the render, which is a good practice.
35-37: Appropriate use of useEffect for initializationThe useEffect hook with an empty dependency array ensures that passkeys are fetched only once when the component mounts.
65-70: Well-implemented fetch functionThe fetchPasskeys function appropriately manages loading state and handles the async API call correctly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
frontend/javascripts/admin/auth/change_password_view.tsx (2)
32-32:⚠️ Potential issueMissing WebAuthnKeyDescriptor type import.
The code uses WebAuthnKeyDescriptor type but doesn't import it.
Add the import at the top of the file:
import { useEffect, useState } from "react"; + import { WebAuthnKeyDescriptor } from "types/api_flow_types"; import { doWebAuthnRegistration, listWebAuthnKeys, removeWebAuthnKey, revokeAuthToken, } from "admin/admin_rest_api";
219-219:⚠️ Potential issueBug: Undefined function call.
There's a call to an undefined
fetchData()function, which will cause a runtime error.- await fetchData(); + await fetchPasskeys();
🧹 Nitpick comments (5)
tools/proxy/proxy.js (1)
2-7: Update Node.js module imports with the node: protocolThe new imports for
fsandhttpsshould use the node: protocol prefix for better clarity and to explicitly indicate these are Node.js built-in modules.-const fs = require("fs"); +const fs = require("node:fs"); const httpProxy = require("http-proxy"); const { spawn, exec } = require("child_process"); -const path = require("path"); +const path = require("node:path"); const prefixLines = require("prefix-stream-lines"); -const https = require("https"); +const https = require("node:https");While we're at it, we should update the other Node.js built-in modules too.
const httpProxy = require("http-proxy"); -const { spawn, exec } = require("child_process"); +const { spawn, exec } = require("node:child_process");🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 4-4: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 5-5: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 7-7: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
frontend/javascripts/admin/auth/login_form.tsx (1)
154-157: Improve error message for better user experience.The current error message is generic and doesn't provide enough context to help users understand what went wrong.
- console.error("WebAuthn login failed", error); - Toast.errror("Login with Passkey failed"); + console.error("WebAuthn login failed", error); + Toast.error(`Login with Passkey failed: ${error.message || "Unknown error"}`);frontend/javascripts/admin/auth/change_password_view.tsx (3)
213-225: Improve passkey row layout and feedback.The current row layout for passkeys lacks proper spacing and alignment between the name and delete button.
- {passkeys.map((passkey) => ( - <Row key={passkey.id}> - {passkey.name} - <Button - onClick={async () => { - await removeWebAuthnKey(passkey); - await fetchData(); - }} - > - Delete - </Button> - </Row> + {passkeys.map((passkey) => ( + <Row key={passkey.id} justify="space-between" align="middle" style={{ marginBottom: 8 }}> + <Col>{passkey.name}</Col> + <Col> + <Button + danger + onClick={async () => { + try { + await removeWebAuthnKey(passkey); + await fetchPasskeys(); + Toast.success("Passkey deleted successfully"); + } catch (error) { + console.error("Failed to delete passkey", error); + Toast.error("Failed to delete passkey"); + } + }} + > + Delete + </Button> + </Col> + </Row>
65-70: Add loading state indicator for passkeys.The component tracks loading state but doesn't show any loading indicator to the user.
Consider adding a loading indicator when fetching passkeys:
async function fetchPasskeys(): Promise<void> { setIsLoadingPasskeys(true); const keys = await listWebAuthnKeys(); setPasskeys(keys); setIsLoadingPasskeys(false); }And in the JSX part where the passkeys are displayed:
<h3>Your Passkeys</h3> + {_isLoadingPasskeys && <div><Spin size="small" /> Loading passkeys...</div>} + {!_isLoadingPasskeys && passkeys.length === 0 && <div>No passkeys registered yet.</div>} {passkeys.map((passkey) => ( <Row key={passkey.id}> // ... rest of the codeDon't forget to add the Spin import:
- import { Alert, Button, Col, Form, Input, Modal, Row } from "antd"; + import { Alert, Button, Col, Form, Input, Modal, Row, Spin } from "antd";
237-243: Add validation for empty passkey name.The modal allows submitting an empty passkey name, which may cause issues or confusion.
<Modal title="Enter a name for the new Passkey" open={isPasskeyNameModalOpen} - onOk={registerNewPasskey} + onOk={() => { + if (!newPasskeyName.trim()) { + Toast.error("Please enter a name for your passkey"); + return; + } + registerNewPasskey(); + }} onCancel={() => setIsPasskeyNameModalOpen(false)} > <Input placeholder="Passkey name" value={newPasskeyName} onChange={(e) => setNewPasskeyName(e.target.value)} + onPressEnter={(e) => { + if (!newPasskeyName.trim()) { + Toast.error("Please enter a name for your passkey"); + return; + } + registerNewPasskey(); + }} /> </Modal>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/admin/auth/change_password_view.tsx(4 hunks)frontend/javascripts/admin/auth/login_form.tsx(2 hunks)tools/postgres/schema.sql(3 hunks)tools/proxy/proxy.js(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
tools/proxy/proxy.js
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 4-4: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 5-5: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 7-7: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (9)
tools/proxy/proxy.js (4)
9-9: Great use of a named constant for timeout valuesUsing a named constant for the timeout value improves code maintainability and prevents inconsistencies.
12-14: Consistent timeout settings using the constantGood refactoring to use the named constant for both timeout settings.
132-139: Ensure you understand the host header override implicationsYou're now explicitly setting the
hostheader to "localhost" when proxying to the Webpack dev server. This could affect how the server interprets or handles requests, especially if hostname-based routing logic exists.Verify that this change doesn't break any functionality that might depend on the original host header value, particularly in development environments with different hostnames.
11-14: Switch from var to const.Using
varcan lead to unintentional scope leakage. Preferconstwhen the reference never changes orletwhen it does. This aligns better with modern JS best practices.-var proxy = httpProxy.createProxyServer({ +const proxy = httpProxy.createProxyServer({ proxyTimeout: time5min, // 5 min timeout: time5min, // 5 min });frontend/javascripts/admin/auth/login_form.tsx (1)
142-163: WebAuthn login implementation looks good.The implementation correctly handles WebAuthn login with proper error handling and user flow. It dispatches appropriate actions on successful login.
tools/postgres/schema.sql (3)
421-430: Well-structured WebAuthn credentials table.The table definition is properly structured with appropriate data types, a primary key, foreign key constraint and a unique constraint to prevent duplicate credential keys for the same user.
23-23: Schema version update looks good.Schema version has been properly incremented from 125 to 126 to reflect the addition of the WebAuthn credentials table.
885-886: Foreign key constraint looks good.The foreign key constraint properly links the WebAuthn credentials to user accounts and includes appropriate cascade options.
frontend/javascripts/admin/auth/change_password_view.tsx (1)
72-83: Error handling in registerNewPasskey looks good.The function properly handles errors and provides appropriate feedback to the user.
| if (process.argv.includes("--tls")) { | ||
| console.log(loggingPrefix, "Using TLS") | ||
| https.createServer({ | ||
| key: fs.readFileSync("target/dev.key.pem"), | ||
| cert: fs.readFileSync("target/dev.cert.pem"), | ||
| }, app).listen(PORT) | ||
| } else { | ||
| app.listen(PORT); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for TLS certificate file reading
The implementation for TLS support is good, but it's missing error handling for the file reading operations. If the certificate files don't exist or are inaccessible, the application will crash without a helpful error message.
if (process.argv.includes("--tls")) {
console.log(loggingPrefix, "Using TLS")
- https.createServer({
- key: fs.readFileSync("target/dev.key.pem"),
- cert: fs.readFileSync("target/dev.cert.pem"),
- }, app).listen(PORT)
+ try {
+ const key = fs.readFileSync("target/dev.key.pem");
+ const cert = fs.readFileSync("target/dev.cert.pem");
+ https.createServer({ key, cert }, app).listen(PORT);
+ console.log(loggingPrefix, "HTTPS server started successfully");
+ } catch (error) {
+ console.error(loggingPrefix, "Failed to start HTTPS server:", error.message);
+ console.error(loggingPrefix, "Make sure you've generated SSL certificates using the gen-ssl-dev-certs.sh script");
+ process.exit(1);
+ }
} else {
app.listen(PORT);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (process.argv.includes("--tls")) { | |
| console.log(loggingPrefix, "Using TLS") | |
| https.createServer({ | |
| key: fs.readFileSync("target/dev.key.pem"), | |
| cert: fs.readFileSync("target/dev.cert.pem"), | |
| }, app).listen(PORT) | |
| } else { | |
| app.listen(PORT); | |
| } | |
| if (process.argv.includes("--tls")) { | |
| console.log(loggingPrefix, "Using TLS") | |
| try { | |
| const key = fs.readFileSync("target/dev.key.pem"); | |
| const cert = fs.readFileSync("target/dev.cert.pem"); | |
| https.createServer({ key, cert }, app).listen(PORT); | |
| console.log(loggingPrefix, "HTTPS server started successfully"); | |
| } catch (error) { | |
| console.error(loggingPrefix, "Failed to start HTTPS server:", error.message); | |
| console.error(loggingPrefix, "Make sure you've generated SSL certificates using the gen-ssl-dev-certs.sh script"); | |
| process.exit(1); | |
| } | |
| } else { | |
| app.listen(PORT); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/javascripts/admin/auth/change_password_view.tsx (6)
66-71: Add error handling to fetchPasskeys functionThe current implementation doesn't handle potential failures when fetching passkeys. If the API call fails, the loading state will remain true.
async function fetchPasskeys(): Promise<void> { setIsLoadingPasskeys(true); - const keys = await listWebAuthnKeys(); - setPasskeys(keys); - setIsLoadingPasskeys(false); + try { + const keys = await listWebAuthnKeys(); + setPasskeys(keys); + } catch (error) { + console.error("Failed to fetch passkeys:", error); + Toast.error("Failed to load passkeys"); + } finally { + setIsLoadingPasskeys(false); + } }
73-84: Add validation for empty passkey nameThe registerNewPasskey function doesn't validate if the passkey name is empty before attempting registration.
const registerNewPasskey = async () => { + if (!newPasskeyName.trim()) { + Toast.error("Passkey name cannot be empty"); + return; + } try { setIsPasskeyNameModalOpen(false); await doWebAuthnRegistration(newPasskeyName); Toast.success("Passkey registered successfully"); setNewPasskeyName(""); await fetchPasskeys(); } catch (e) { Toast.error(`Registering new Passkey '${newPasskeyName}' failed`); console.error("Could not register new Passkey", e); } };
214-226: Improve passkey list UI and add empty stateThe current UI for displaying passkeys lacks proper spacing and alignment. Also, there's no handling for when the passkeys list is empty.
-{passkeys.map((passkey) => ( - <Row key={passkey.id}> - {passkey.name} - <Button - onClick={async () => { - await removeWebAuthnKey(passkey); - await fetchPasskeys(); - }} - > - Delete - </Button> - </Row> -))} +{_isLoadingPasskeys ? ( + <div>Loading passkeys...</div> +) : passkeys.length === 0 ? ( + <div>No passkeys registered yet.</div> +) : ( + passkeys.map((passkey) => ( + <Row key={passkey.id} justify="space-between" align="middle" style={{ marginBottom: 8 }}> + <Col>{passkey.name}</Col> + <Col> + <Button + danger + onClick={async () => { + await removeWebAuthnKey(passkey); + await fetchPasskeys(); + }} + > + Delete + </Button> + </Col> + </Row> + )) +)}
240-244: Add keyboard support for passkey name inputThe input field for the passkey name should support submitting via the Enter key for better user experience.
<Input placeholder="Passkey name" value={newPasskeyName} onChange={(e) => setNewPasskeyName(e.target.value)} + onPressEnter={registerNewPasskey} />
207-212: Consider rewording the passkeys explanationThe explanation of passkeys could be slightly improved for clarity.
<p> - Passkeys are a new web authentication method that allows you to log in without a - password in a secured way. Microsoft Hello and Apple FaceID are examples of technologies - that can be used as passkeys to log in in WEBKNOSSOS. If you want to add a new passkey - to your account use the button below. + Passkeys are a secure web authentication method that allows you to log in without a + password. Technologies like Microsoft Windows Hello and Apple FaceID can be used as + passkeys to log in to WEBKNOSSOS. To add a new passkey to your account, use the + button below. </p>
34-34: Remove underscore prefix if the variable is usedThe
_isLoadingPasskeysvariable has an underscore prefix suggesting it might be unused, but it is actually used in the component.-const [_isLoadingPasskeys, setIsLoadingPasskeys] = useState(false); +const [isLoadingPasskeys, setIsLoadingPasskeys] = useState(false);And update all references to this variable accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/admin/auth/change_password_view.tsx(4 hunks)frontend/javascripts/admin/auth/login_form.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/admin/auth/login_form.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
There was a problem hiding this comment.
Thanks a lot for digging through this 🎉. Looking good already ✨
I left some comments regarding the backend code (did not test anything).
Beside these comments, I noticed that you tended to write some java like code with semicolons. Scala does usually not need semicolons. So please remove them. Moreover, some brackets are also not needed and we usually also delete them. Could you do this here as well please? At some occurences I commented this in the code :)
| @@ -11,6 +15,7 @@ import net.liftweb.common.{Box, Empty, Failure, Full} | |||
| import org.apache.commons.codec.binary.Base64 | |||
| import org.apache.commons.codec.digest.{HmacAlgorithms, HmacUtils} | |||
| import org.apache.pekko.actor.ActorSystem | |||
| import play.api.Configuration | |||
There was a problem hiding this comment.
Please import WkConf as this already parses the config and might also make some adjusted to some config fields
|
|
||
| class AuthenticationController @Inject()( | ||
| configuration: Configuration, |
There was a problem hiding this comment.
And as you imported WkConf, please use it here as well:
| configuration: Configuration, | |
| conf: WkConf, |
| @@ -66,6 +94,13 @@ class AuthenticationController @Inject()( | |||
| private lazy val ssoKey = | |||
| conf.WebKnossos.User.ssoKey | |||
|
|
|||
| private lazy val relyingParty = { | |||
| val origin = configuration.get[String]("http.uri").split("/")(2); | |||
There was a problem hiding this comment.
And now you can use conf here with something like:
| val origin = configuration.get[String]("http.uri").split("/")(2); | |
| val origin = conf.http.uri.split("/")(2); |
There was a problem hiding this comment.
But maybe using the java URI class to get the host might be even better than manually splitting the string.
So something like this might be better:
val uri = new java.net.URI(conf.http.uri)
val origin = uri.getHost
And in case any of these functions can throw an error please also add a try and catch :)
| _multiUser: ObjectId, | ||
| keyId: Array[Byte], | ||
| name: String, | ||
| publicKeyCose: Array[Byte], |
There was a problem hiding this comment.
I think there is a typo here. Should it be publicKeyCode?
There was a problem hiding this comment.
Ah I see cose is actually a term: CBOR Object Signing and Encryption
Well then nvm
There was a problem hiding this comment.
Could you maybe add a comment here that COSE is an actual term and maybe write that in caps? -> publicKeyCOSE
| def listKeys(multiUser: ObjectId)(implicit ctx: DBAccessContext): Fox[List[WebAuthnCredential]] = | ||
| for { | ||
| accessQuery <- readAccessQuery | ||
| r <- run( | ||
| q"""SELECT $columns FROM webknossos.webauthncredentials WHERE _multiUser = $multiUser AND $accessQuery""" | ||
| .as[WebauthncredentialsRow]) | ||
| parsed <- parseAll(r) | ||
| } yield parsed |
There was a problem hiding this comment.
This is exactly the same as findAllForUser above. Please remove this implementation here as I think findAllForUser is a better name :)
| .builder() | ||
| .credentialId(WebAuthnCredentialRepository.bytesToByteArray(credential.keyId)) | ||
| .userHandle(WebAuthnCredentialRepository.objectIdToByteArray(credential._multiUser)) | ||
| .publicKeyCose(new ByteArray((credential.publicKeyCose))) |
There was a problem hiding this comment.
There are too many parantheses here:
| .publicKeyCose(new ByteArray((credential.publicKeyCose))) | |
| .publicKeyCose(new ByteArray(credential.publicKeyCose)) |
| do $$ begin ASSERT (select schemaVersion from webknossos.releaseInformation) = 126, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql; | ||
|
|
||
| DROP TABLE webknossos.webauthnCredentials; | ||
|
|
||
| UPDATE webknossos.releaseInformation SET schemaVersion = 125; | ||
|
|
There was a problem hiding this comment.
Just a reminder: the version numbers here need to be bumped up as the master schema version is currently 128 and maybe even higher when this pr is merged
| @@ -0,0 +1,21 @@ | |||
| START TRANSACTION; | |||
|
|
|||
| do $$ begin ASSERT (select schemaVersion from webknossos.releaseInformation) = 125, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql; | |||
There was a problem hiding this comment.
same here for the version numbers
| _ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser) | ||
| } yield Ok(Json.obj()) | ||
| } | ||
| } |
There was a problem hiding this comment.
It is totally fine to pass the entry that should be deleted in the request body. But I think usually when we send only an ID, this is done via url params. But as your approach is totally fine, feel free to choose one. Just wanted to leave that thought here :D
| #!/usr/bin/env bash | ||
|
|
||
| mkdir -p target/ | ||
| openssl req -x509 -newkey rsa:2048 \ | ||
| -keyout target/dev.key.pem \ | ||
| -out target/dev.cert.pem \ | ||
| -sha256 -days 365 \ | ||
| -nodes \ | ||
| -subj "/C=XX/ST=Dev/O=Dev/OU=Dev/CN=webknossos.local" |
There was a problem hiding this comment.
🤔 As this is used for the proxy anyway and there is a tools/proxy dir, maybe move that file here?
| user <- multiUserDAO.findOneByEmail(email)(GlobalAccessContext).await("Java interop") | ||
| keys <- webAuthnCredentialDAO.findAllForUser(user._id)(GlobalAccessContext).await("Java interop") |
There was a problem hiding this comment.
Any chance of a way to use an asynchronous variant of getCredentialIdsForUsername with the webauthn libraries? Maybe you could have another look if something exists there. It would be great if we could avoid the await. Currently we have none of them outside of tests, and it would certainly be a step back if we would have to add them here.
There was a problem hiding this comment.
I can try https://github.com/webauthn4j/webauthn4j. I do not see any Java interface forcing awaits there.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
app/controllers/AuthenticationController.scala (5)
516-523: 🛠️ Refactor suggestionAdd error handling to WebAuthn authentication start
The current implementation doesn't handle potential exceptions from the RelyingParty creation or token generation.
def webauthnAuthStart(): Action[AnyContent] = Action { - val opts = StartAssertionOptions.builder().build(); - val assertion = relyingParty.startAssertion(opts); - val sessionId = UUID.randomUUID().toString; - val cookie = Cookie("webauthn-session", sessionId, maxAge = Some(120), httpOnly = true, secure = true) - temporaryAssertionStore.insert(sessionId, assertion, Some(2 minutes)); - Ok(Json.toJson(Json.parse(assertion.toCredentialsGetJson))).withCookies(cookie) + try { + val opts = StartAssertionOptions.builder().build() + val assertion = relyingParty.startAssertion(opts) + val sessionId = UUID.randomUUID().toString + val cookie = Cookie("webauthn-session", sessionId, maxAge = Some(120), httpOnly = true, secure = true) + temporaryAssertionStore.insert(sessionId, assertion, Some(2 minutes)) + logger.debug("Started WebAuthn authentication") + Ok(Json.toJson(Json.parse(assertion.toCredentialsGetJson))).withCookies(cookie) + } catch { + case e: Exception => + logger.error(s"Failed to start WebAuthn authentication: ${e.getMessage}") + InternalServerError(Json.obj("error" -> "Failed to start authentication")) + } }
605-641: 🛠️ Refactor suggestionImprove error handling in WebAuthn registration finalization
The current implementation has nested match expressions and a try-catch that only catches RegistrationFailedException. Use Fox's monadic composition for a cleaner flow and better error handling.
def webauthnRegisterFinalize(): Action[WebAuthnRegistration] = sil.SecuredAction.async(validateJson[WebAuthnRegistration]) { implicit request => - { - val creationOpts = temporaryRegistrationStore.get(request.identity._multiUser) - temporaryRegistrationStore.remove(request.identity._multiUser) - creationOpts match { - case Some(data) => { - val response = PublicKeyCredential.parseRegistrationResponseJson(request.body.key); - val opts = FinishRegistrationOptions.builder().request(data).response(response).build(); - try { - for { - preKey <- Fox.future2Fox(Future { Try(relyingParty.finishRegistration(opts)) })(blockingContext); - key <- preKey match { - case scala.util.Success(key) => Fox.successful(key) - case scala.util.Failure(e) => Fox.failure("Registration failed", Full(e)) - }; - result <- { - val credential = WebAuthnCredential( - ObjectId.generate, - request.identity._multiUser, - WebAuthnCredentialRepository.byteArrayToBytes(key.getKeyId.getId), - request.body.name, - key.getPublicKeyCose.getBytes, - key.getSignatureCount.toInt, - isDeleted = false, - ) - webAuthnCredentialDAO.insertOne(credential).map(_ => Ok("")) - } - } yield result; - } catch { - case e: RegistrationFailedException => Future.successful(BadRequest("Failed to register key")) - } - } - case None => Future.successful(BadRequest("Challenge not found or expired")) - } - } + for { + creationOpts <- Fox.option2Fox(temporaryRegistrationStore.get(request.identity._multiUser)) ?~> + "Challenge not found or expired" ~> BAD_REQUEST + _ = temporaryRegistrationStore.remove(request.identity._multiUser) + + response = PublicKeyCredential.parseRegistrationResponseJson(request.body.key) + opts = FinishRegistrationOptions.builder().request(creationOpts).response(response).build() + + key <- Fox.tryo(relyingParty.finishRegistration(opts))(blockingContext) ?~> "Registration failed" + + // Check if a passkey with this name already exists + existingKeys <- webAuthnCredentialDAO.listKeys(request.identity._multiUser) + _ <- Fox.bool2Fox(!existingKeys.exists(_.name == request.body.name)) ?~> + "A passkey with this name already exists" ~> BAD_REQUEST + + credential = WebAuthnCredential( + ObjectId.generate, + request.identity._multiUser, + WebAuthnCredentialRepository.byteArrayToBytes(key.getKeyId.getId), + request.body.name, + key.getPublicKeyCose.getBytes, + key.getSignatureCount.toInt, + isDeleted = false + ) + _ <- webAuthnCredentialDAO.insertOne(credential) + _ = logger.info(s"User ${request.identity._id} registered new passkey: ${request.body.name}") + } yield Ok(Json.obj("message" -> "Webauthn registration successful")) }
652-659: 🛠️ Refactor suggestionImprove error handling in WebAuthn remove key endpoint
The current implementation doesn't validate that the key belongs to the user nor does it use the
?~>operator for error handling as per the project conventions.def webauthnRemoveKey: Action[WebAuthnKeyDescriptor] = sil.SecuredAction.async(validateJson[WebAuthnKeyDescriptor]) { implicit request => { for { - _ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser) + _ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser) ?~> "webauthn.deletionFailed" } yield Ok(Json.obj()) } }
525-561: 🛠️ Refactor suggestionRefactor WebAuthn authentication finalization for better error handling
The current implementation has nested match statements which reduce readability. Consider using Fox's monadic composition for a cleaner flow and better error handling.
def webauthnAuthFinalize(): Action[WebAuthnAuthentication] = Action.async(validateJson[WebAuthnAuthentication]) { implicit request => - { - request.cookies.get("webauthn-session") match { - case None => - Future.successful(BadRequest("Authentication took too long, please try again.")) - case Some(cookie) => - val sessionId = cookie.value - val challengeData = temporaryAssertionStore.get(sessionId) - temporaryAssertionStore.remove(sessionId) - challengeData match { - case None => Future.successful(Unauthorized("Authentication timeout.")) - case Some(data) => { - val keyCredential = PublicKeyCredential.parseAssertionResponseJson(request.body.key); - val opts = FinishAssertionOptions.builder().request(data).response(keyCredential).build(); - for { - result <- Fox - .future2Fox(Future { Try(relyingParty.finishAssertion(opts)) })(blockingContext); // NOTE: Prevent blocking on HTTP handler - assertion <- result match { - case scala.util.Success(assertion) => Fox.successful(assertion); - case scala.util.Failure(e) => Fox.failure("Authentication failed.", Full(e)); - }; - userId = WebAuthnCredentialRepository.byteArrayToObjectId(assertion.getCredential.getUserHandle); - multiUser <- multiUserDAO.findOne(userId)(GlobalAccessContext); - result <- multiUser._lastLoggedInIdentity match { - case None => Future.successful(InternalServerError("user never logged in")) - case Some(userId) => { - val loginInfo = LoginInfo("credentials", userId.toString); - authenticateInner(loginInfo) - } - } - } yield result; - } - } - } - } + for { + sessionCookie <- Fox.option2Fox(request.cookies.get("webauthn-session")) ?~> "Authentication took too long, please try again." ~> BAD_REQUEST + sessionId = sessionCookie.value + challengeData <- Fox.option2Fox(temporaryAssertionStore.get(sessionId)) ?~> "Authentication timeout." ~> UNAUTHORIZED + _ = temporaryAssertionStore.remove(sessionId) + keyCredential = PublicKeyCredential.parseAssertionResponseJson(request.body.key) + opts = FinishAssertionOptions.builder().request(challengeData).response(keyCredential).build() + assertion <- Fox.tryo(relyingParty.finishAssertion(opts))(blockingContext) ?~> "Authentication failed." + userId = WebAuthnCredentialRepository.byteArrayToObjectId(assertion.getCredential.getUserHandle) + multiUser <- multiUserDAO.findOne(userId)(GlobalAccessContext) ?~> "User not found" ~> NOT_FOUND + lastLoggedInId <- Fox.option2Fox(multiUser._lastLoggedInIdentity) ?~> "User has never logged in before" ~> INTERNAL_SERVER_ERROR + loginInfo = LoginInfo("credentials", lastLoggedInId.toString) + result <- authenticateInner(loginInfo) + _ = logger.info(s"User authenticated with WebAuthn: ${lastLoggedInId}") + } yield result }
165-169: 🛠️ Refactor suggestionImprove RelyingParty initialization with robust URI parsing
The current implementation uses string splitting which is fragile. A Java URI object would be more reliable for extracting the host.
private lazy val relyingParty = { - val origin = configuration.get[String]("http.uri").split("/")(2); + try { + val uri = new java.net.URI(conf.http.uri) + val origin = uri.getHost + logger.info(s"Initializing WebAuthn with relying party ID: $origin") + val identity = RelyingPartyIdentity.builder().id(origin).name("WebKnossos").build(); + RelyingParty.builder().identity(identity).credentialRepository(webAuthnCredentialRepository).build() + } catch { + case e: Exception => + logger.error(s"Failed to initialize WebAuthn RelyingParty: ${e.getMessage}") + throw e + } }
🧹 Nitpick comments (2)
app/controllers/AuthenticationController.scala (2)
69-72: Consider expanding authenticator selection optionsWhile the current implementation covers basic WebAuthn functionality, the
WebAuthnCreationOptionsAuthenticatorSelectionclass could be enhanced in the future to support more options likeauthenticatorAttachmentanduserVerification.
631-631: Add a success message to the registration responseThe current implementation returns an empty response on successful registration. Consider returning a more informative message.
- webAuthnCredentialDAO.insertOne(credential).map(_ => Ok("")) + webAuthnCredentialDAO.insertOne(credential).map(_ => Ok(Json.obj("message" -> "Webauthn registration successful")))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/AuthenticationController.scala(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
app/controllers/AuthenticationController.scala (1)
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8393
File: app/controllers/AuthenticationController.scala:566-573
Timestamp: 2025-03-26T13:22:21.455Z
Learning: In the webknossos project, prefer using the ?~> operator for error handling with message keys stored in conf/messages rather than extensive custom error handling.
🧬 Code Definitions (1)
app/controllers/AuthenticationController.scala (3)
app/security/WebAuthnCredentialRepository.scala (4)
WebAuthnCredentialRepository(16-22)WebAuthnCredentialRepository(29-122)byteArrayToObjectId(21-21)byteArrayToBytes(17-17)app/models/user/MultiUser.scala (2)
updateLastLoggedInIdentity(106-114)parse(45-64)app/models/user/WebAuthnCredentials.scala (3)
parse(31-44)findAllForUser(44-53)WebAuthnCredential(13-21)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (4)
app/controllers/AuthenticationController.scala (4)
44-59: Case classes accurately model WebAuthn PublicKeyCredentialCreationOptionsThe case class structure properly represents the WebAuthn PublicKeyCredentialCreationOptions interface with appropriate documentation referencing Mozilla's standards. The code clearly documents what has been omitted and why.
111-124: Well-structured case classes with proper JSON serializationThe supporting case classes (WebAuthnRegistration, WebAuthnAuthentication, WebAuthnKeyDescriptor) are well-defined with appropriate JSON format instances for serialization/deserialization.
147-150: Appropriate dependencies for WebAuthn functionalityThe controller properly injects the necessary WebAuthn-related dependencies. The use of TemporaryStore for managing session state is a good approach.
249-268: Well-structured authentication method with proper error handlingThe
authenticateInnermethod is well-designed, abstracting the authentication logic that can be reused by both traditional and WebAuthn authentication flows. It properly handles error cases and updates the user's last activity.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/models/user/WebAuthnCredentials.scala (1)
59-59: Fix the spelling in the method name.The current method
deseriaizeAttestationDatacontains a typo. Consider renaming it todeserializeAttestationData.- def deseriaizeAttestationData(serializedData: Array[Byte]): AttestedCredentialData = { + def deserializeAttestationData(serializedData: Array[Byte]): AttestedCredentialData = {app/controllers/AuthenticationController.scala (2)
556-563: Add error handling for thestartAssertioncall.Consider wrapping
relyingParty.startAssertion(opts)in a try-catch or a?~>construct to avoid unhandled exceptions, ensuring the user is informed if the operation fails.
603-642: Reuse a singleSecureRandominstance.Initializing a new
SecureRandomeach time can affect performance and adds duplicate code. Move it to a private val to align with the existing TODO.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/AuthenticationController.scala(7 hunks)app/models/user/WebAuthnCredentials.scala(1 hunks)project/Dependencies.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- project/Dependencies.scala
🧰 Additional context used
🧠 Learnings (1)
app/controllers/AuthenticationController.scala (1)
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8393
File: app/controllers/AuthenticationController.scala:566-573
Timestamp: 2025-03-26T13:22:21.455Z
Learning: In the webknossos project, prefer using the ?~> operator for error handling with message keys stored in conf/messages rather than extensive custom error handling.
🧬 Code Definitions (2)
app/models/user/WebAuthnCredentials.scala (1)
app/models/user/MultiUser.scala (4)
idColumn(42-42)isDeletedColumn(43-43)parse(45-64)insertOne(64-77)
app/controllers/AuthenticationController.scala (2)
frontend/javascripts/types/api_flow_types.ts (1)
WebAuthnKeyDescriptor(1213-1216)app/models/user/WebAuthnCredentials.scala (3)
WebAuthnCredential2(32-49)WebAuthnCredential2(50-63)insertOne2(122-130)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (5)
app/models/user/WebAuthnCredentials.scala (2)
23-23: Consider renaming topublicKeyCOSEor clarifying with a comment.A previous review comment noted that “COSE” is a standard acronym (CBOR Object Signing and Encryption). Either rename
publicKeyCoseto emphasize the acronym or add an inline comment indicating that “COSE” is an actual term.
133-140: Remove duplication or rename one of the methods.
listKeysduplicates the logic offindAllForUser. To avoid confusion and code duplication, consider removing one of them or merging their functionality.app/controllers/AuthenticationController.scala (3)
565-601: Prefer functional error handling with?~>rather than nested matches.Using
?~>would make the error paths clearer and consistent with your codebase’s style. This was recommended in a previous review.
676-683: LGTM!The flow for listing keys appears straightforward and correct.
685-692: Consider adding ownership checks and user feedback.The method removes the key without verifying the user actually owns the key. Past suggestions advocated for more robust error handling. If you see a need to validate ownership or provide better messages, please do so.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
frontend/javascripts/admin/admin_rest_api.ts (1)
167-179:⚠️ Potential issueRemove redundant JSON parsing in WebAuthn registration.
The function contains redundant JSON parsing that could lead to errors. The
Request.receiveJSONalready returns parsed JSON, so the additional.then((body) => JSON.parse(body))is unnecessary.Apply this diff to fix the issue:
- const webAuthnRegistrationAssertion = await Request.receiveJSON( - "/api/auth/webauthn/register/start", - { - method: "POST", - }, - ).then((body) => JSON.parse(body)); + const webAuthnRegistrationAssertion = await Request.receiveJSON( + "/api/auth/webauthn/register/start", + { + method: "POST", + }, + );app/models/user/WebAuthnCredentials.scala (2)
129-136: Remove duplicate method listKeysThis method is identical to
findAllForUserbut with a different name. Consider removing it and usingfindAllForUserconsistently throughout the codebase.This is exactly the same as
findAllForUserabove. Please remove this implementation here as I thinkfindAllForUseris a better name :)
120-126:⚠️ Potential issueComplete the insertOne2 method implementation
The method creates serialization variables but never uses them, and the SQL query has an incomplete VALUES clause marked with TODO. This method is called during credential registration and will fail in its current state.
def insertOne2(c: WebAuthnCredential2): Fox[Unit] = { + val keyId = c.record.getAttestedCredentialData.getCredentialId val serializedAttestationStatement = c.serializeAttestationStatement() val serializedAttestationData = c.serializableAttestationData() for { _ <- run( - q"""INSERT INTO webknossos.webauthncredentials(_id, _multiUser, keyId, name) VALUES ()""".asUpdate // TODO + q"""INSERT INTO webknossos.webauthncredentials( + _id, _multiUser, keyId, name, publicKeyCose, signatureCount, + attestationStatement, attestationData) + VALUES(${c._id}, ${c._multiUser}, $keyId, ${c.name}, + ${c.record.getAttestedCredentialData.getCOSEKey.getBytes}, + ${c.record.getSignatureCounter}, + $serializedAttestationStatement, $serializedAttestationData)""".asUpdate ) } yield () }app/controllers/AuthenticationController.scala (4)
556-563: 🛠️ Refactor suggestionAdd error handling to webauthnAuthStart
The method doesn't handle potential exceptions when starting authentication.
- def webauthnAuthStart(): Action[AnyContent] = Action { - val opts = StartAssertionOptions.builder().build(); - val assertion = relyingParty.startAssertion(opts); - val sessionId = UUID.randomUUID().toString; - val cookie = Cookie("webauthn-session", sessionId, maxAge = Some(120), httpOnly = true, secure = true) - temporaryAssertionStore.insert(sessionId, assertion, Some(2 minutes)); - Ok(Json.toJson(Json.parse(assertion.toCredentialsGetJson))).withCookies(cookie) + def webauthnAuthStart(): Action[AnyContent] = Action { + try { + val opts = StartAssertionOptions.builder().build() + val assertion = relyingParty.startAssertion(opts) + val sessionId = UUID.randomUUID().toString + val cookie = Cookie("webauthn-session", sessionId, maxAge = Some(120), httpOnly = true, secure = true) + temporaryAssertionStore.insert(sessionId, assertion, Some(2 minutes)) + logger.debug("Started WebAuthn authentication") + Ok(Json.toJson(Json.parse(assertion.toCredentialsGetJson))).withCookies(cookie) + } catch { + case e: Exception => + logger.error(s"Failed to start WebAuthn authentication: ${e.getMessage}") + InternalServerError(Json.obj("error" -> "Failed to start authentication")) + } }
196-200: 🛠️ Refactor suggestionImprove RelyingParty configuration with error handling
The current implementation uses string splitting to extract the host, which is fragile and could lead to runtime errors.
private lazy val relyingParty = { - val origin = configuration.get[String]("http.uri").split("/")(2); - val identity = RelyingPartyIdentity.builder().id(origin).name("WebKnossos").build(); - RelyingParty.builder().identity(identity).credentialRepository(webAuthnCredentialRepository).build() + try { + val uri = new java.net.URI(conf.http.uri) + val origin = uri.getHost + logger.info(s"Initializing WebAuthn with relying party ID: $origin") + val identity = RelyingPartyIdentity.builder().id(origin).name("WebKnossos").build() + RelyingParty.builder() + .identity(identity) + .credentialRepository(webAuthnCredentialRepository) + .build() + } catch { + case e: Exception => + logger.error(s"Failed to initialize WebAuthn RelyingParty: ${e.getMessage}") + throw e + } }
565-601: 🛠️ Refactor suggestionRefactor webauthnAuthFinalize to use Fox's functional style
The current implementation has deeply nested conditionals and manual error handling. Use Fox's functional style with the ?~> operator as recommended in the project learnings.
def webauthnAuthFinalize(): Action[WebAuthnAuthentication] = Action.async(validateJson[WebAuthnAuthentication]) { implicit request => - { - request.cookies.get("webauthn-session") match { - case None => - Future.successful(BadRequest("Authentication took too long, please try again.")) - case Some(cookie) => - val sessionId = cookie.value - val challengeData = temporaryAssertionStore.get(sessionId) - temporaryAssertionStore.remove(sessionId) - challengeData match { - case None => Future.successful(Unauthorized("Authentication timeout.")) - case Some(data) => { - val keyCredential = PublicKeyCredential.parseAssertionResponseJson(request.body.key); - val opts = FinishAssertionOptions.builder().request(data).response(keyCredential).build(); - for { - result <- Fox - .future2Fox(Future { Try(relyingParty.finishAssertion(opts)) })(blockingContext); // NOTE: Prevent blocking on HTTP handler - assertion <- result match { - case scala.util.Success(assertion) => Fox.successful(assertion); - case scala.util.Failure(e) => Fox.failure("Authentication failed.", Full(e)); - }; - userId = WebAuthnCredentialRepository.byteArrayToObjectId(assertion.getCredential.getUserHandle); - multiUser <- multiUserDAO.findOne(userId)(GlobalAccessContext); - result <- multiUser._lastLoggedInIdentity match { - case None => Future.successful(InternalServerError("user never logged in")) - case Some(userId) => { - val loginInfo = LoginInfo("credentials", userId.toString); - authenticateInner(loginInfo) - } - } - } yield result; - } - } - } - } + for { + sessionCookie <- Fox.option2Fox(request.cookies.get("webauthn-session")) ?~> "Authentication took too long, please try again." ~> BAD_REQUEST + sessionId = sessionCookie.value + challengeData <- Fox.option2Fox(temporaryAssertionStore.get(sessionId)) ?~> "Authentication timeout." ~> UNAUTHORIZED + _ = temporaryAssertionStore.remove(sessionId) + keyCredential <- tryo(PublicKeyCredential.parseAssertionResponseJson(request.body.key)).toFox ?~> "Invalid authentication response" + opts = FinishAssertionOptions.builder().request(challengeData).response(keyCredential).build() + assertion <- Fox.future2Fox(Future { tryo(relyingParty.finishAssertion(opts)).toOption.get })(blockingContext) ?~> "Authentication failed." + userId = WebAuthnCredentialRepository.byteArrayToObjectId(assertion.getCredential.getUserHandle) + multiUser <- multiUserDAO.findOne(userId)(GlobalAccessContext) ?~> "User not found" ~> NOT_FOUND + lastLoggedInId <- Fox.option2Fox(multiUser._lastLoggedInIdentity) ?~> "User has never logged in before" ~> INTERNAL_SERVER_ERROR + loginInfo = LoginInfo("credentials", lastLoggedInId.toString) + result <- authenticateInner(loginInfo) + _ = logger.info(s"User authenticated with WebAuthn: ${lastLoggedInId}") + } yield result }
685-692: 🛠️ Refactor suggestionAdd validation to webauthnRemoveKey
The current implementation doesn't validate that the key belongs to the user before removing it.
def webauthnRemoveKey: Action[WebAuthnKeyDescriptor] = sil.SecuredAction.async(validateJson[WebAuthnKeyDescriptor]) { implicit request => - { - for { - _ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser) - } yield Ok(Json.obj()) - } + for { + _ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser) ?~> "webauthn.deletionFailed" + } yield Ok(Json.obj("message" -> "Passkey removed successfully")) }
🧹 Nitpick comments (3)
app/models/user/WebAuthnCredentials.scala (2)
28-28: Consider adding a comment explaining COSE termFollowing up on the previous review discussions, it would be helpful to add a comment explaining that COSE stands for "CBOR Object Signing and Encryption" and is an actual term, not a typo.
- publicKeyCose: Array[Byte], + publicKeyCOSE: Array[Byte], // COSE = CBOR Object Signing and Encryption
54-61: Add documentation for the WebAuthnCredential2 classThe class is currently empty with no implementation or documentation. Add a comment explaining its purpose and how it differs from the WebAuthnCredential class.
case class WebAuthnCredential2( _id: ObjectId, _multiUser: ObjectId, name: String, record: CredentialRecordImpl, isDeleted: Boolean, -) { +) { + // This class represents a WebAuthn credential that uses the CredentialRecordImpl from webauthn4j + // It's used for credential registration where we need to store the full attestation information }app/controllers/AuthenticationController.scala (1)
616-617: Move SecureRandom to class level variableCreating a new SecureRandom instance for each request is inefficient. Move it to a class-level variable.
+ private val random = new SecureRandom() + def webauthnRegisterStart(): Action[AnyContent] = sil.SecuredAction.async { implicit request => for { email <- userService.emailFor(request.identity) // ... - random = new SecureRandom() // TODO: Initialize once challenge = {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
MIGRATIONS.unreleased.md(1 hunks)app/controllers/AuthenticationController.scala(7 hunks)app/models/user/WebAuthnCredentials.scala(1 hunks)conf/application.conf(1 hunks)conf/evolutions/131-add-webauthn-credentials.sql(1 hunks)conf/evolutions/reversions/131-add-webauthn-credentials.sql(1 hunks)conf/webknossos.latest.routes(1 hunks)frontend/javascripts/admin/admin_rest_api.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- MIGRATIONS.unreleased.md
- conf/application.conf
🧰 Additional context used
🧠 Learnings (1)
app/controllers/AuthenticationController.scala (1)
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8393
File: app/controllers/AuthenticationController.scala:566-573
Timestamp: 2025-04-09T13:32:39.564Z
Learning: In the webknossos project, prefer using the ?~> operator for error handling with message keys stored in conf/messages rather than extensive custom error handling.
🧬 Code Graph Analysis (3)
frontend/javascripts/admin/admin_rest_api.ts (2)
frontend/javascripts/types/api_flow_types.ts (1)
WebAuthnKeyDescriptor(1237-1240)app/controllers/AuthenticationController.scala (2)
WebAuthnKeyDescriptor(152-152)WebAuthnKeyDescriptor(153-155)
app/models/user/WebAuthnCredentials.scala (3)
util/src/main/scala/com/scalableminds/util/accesscontext/AccessContext.scala (1)
DBAccessContext(31-37)app/utils/sql/SQLDAO.scala (2)
SQLDAO(14-99)parseAll(40-43)app/utils/sql/SecuredSQLDAO.scala (1)
readAccessQuery(23-36)
app/controllers/AuthenticationController.scala (5)
app/models/organization/Organization.scala (4)
Organization(18-34)OrganizationDAO(36-243)parse(44-67)findOne(93-102)app/models/user/UserService.scala (2)
retrieve(269-272)insert(92-136)app/models/user/User.scala (4)
User(24-26)User(28-62)parse(98-122)findOne(149-156)app/models/user/MultiUser.scala (2)
updateLastLoggedInIdentity(106-114)parse(45-64)app/models/user/WebAuthnCredentials.scala (4)
parse(71-84)WebAuthnCredential2(54-61)insertOne2(120-126)listKeys(129-138)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: backend-tests
🔇 Additional comments (6)
conf/evolutions/reversions/131-add-webauthn-credentials.sql (1)
1-10: Well-structured reversion script for WebAuthn credentials.The script follows best practices with transaction wrapping, schema version validation, and proper cleanup by dropping the table. This ensures reliable reversion of the database schema changes.
conf/webknossos.latest.routes (1)
41-47: Well-organized WebAuthn route definitions.The routes follow RESTful conventions with appropriate HTTP methods:
- POST for authentication actions (start/finalize)
- POST for registration actions (start/finalize)
- GET for retrieving keys
- DELETE for removing keys
The route naming is consistent and clearly indicates their purpose.
conf/evolutions/131-add-webauthn-credentials.sql (2)
5-14: Well-designed WebAuthn credentials table schema.The table structure includes all necessary fields for WebAuthn with appropriate types:
- Primary key with TEXT type for credential ID
- Foreign key to multiUsers for user association
- BYTEA type for binary data (keyId and publicKeyCose)
- Unique constraint on user+keyId combination to prevent duplicates
- Soft deletion support with isDeleted flag
This schema appropriately supports the WebAuthn credential storage requirements.
16-17: Proper foreign key constraint with cascade options.The foreign key relationship to the multiUsers table includes appropriate CASCADE options for both DELETE and UPDATE operations, ensuring referential integrity while automatically handling user deletion.
frontend/javascripts/admin/admin_rest_api.ts (2)
152-165: Well-implemented WebAuthn login flow.The login flow correctly follows the WebAuthn authentication process by:
- Requesting authentication options from the server
- Processing them with the WebAuthn API
- Sending the result back to the server for verification
- Retrieving user and organization information after successful authentication
181-190: Appropriate implementation of WebAuthn key management operations.The functions for listing and removing WebAuthn keys follow best practices by:
- Using appropriate HTTP methods (GET for retrieval, DELETE for removal)
- Properly typing the response with WebAuthnKeyDescriptor
- Sending the key data in the DELETE request body for precise key removal
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
tools/postgres/schema.sql (2)
456-465: Consider adding a check constraint for_multiUser.To maintain consistency with other tables, you might add a check constraint ensuring
_multiUserfollows the 24-hex format, for example:CONSTRAINT _multiUser_objectId CHECK (_multiUser ~ '^[0-9a-f]{24}$')Additionally, consider whether
( _multiUser, name )should be unique if the same user must not register multiple credentials under the exact same name.
936-937: Add a dedicated view for webauthnCredentials.To stay consistent with similar tables, you might create a view filtering
WHERE NOT isDeletedforwebauthnCredentials, akin to the_-suffixed views for other tables (e.g.,annotations_,meshes_).app/models/user/WebAuthnCredentials.scala (2)
25-44: Encapsulate or document the multiplenullreferences inCredentialRecordImpl.Repeated
nullparameters may hinder readability or future migrations. Consider documenting or wrapping them in optional fields if feasible.
75-98: Consider adding null-safe validations.Although
tryo(...)is used, ensure you handle cases whereserializedattestationstatement,serializedattestedcredential, orserializedextensionsmight be empty or corrupted.app/controllers/AuthenticationController.scala (5)
213-217: Use a safer approach to parse the relying party domain.Manually splitting the URI may fail with schemas that include subdirectories or ports. Consider a
java.net.URIapproach to extract the host.- val origin = configuration.get[String]("http.uri").split("/")(2); - val identity = RelyingPartyIdentity.builder().id(origin).name("WebKnossos").build(); + val uri = new java.net.URI(configuration.get[String]("http.uri")) + val host = uri.getHost + val identity = RelyingPartyIdentity.builder().id(host).name("WebKnossos").build();
219-220: Load the origin from configuration.The TODO implies it should not remain hardcoded. A mismatch could cause WebAuthn domain errors.
591-627: Improve error feedback to the client.Currently, the user sees "Authentication took too long" or "Authentication timeout." Consider returning consistent, localized messages or codes.
629-667: Reuse a single SecureRandom instance if possible.Creating a new
SecureRandomeach time is safe, but it’s more typical to keep one instance for multiple calls.
707-723: Listing and removing keys logic is straightforward.One small note: for better user experience, consider verifying the user truly owns the key before removal, or log potential mismatches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/controllers/AuthenticationController.scala(7 hunks)app/models/user/WebAuthnCredentials.scala(1 hunks)conf/evolutions/131-add-webauthn-credentials.sql(1 hunks)tools/postgres/schema.sql(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- conf/evolutions/131-add-webauthn-credentials.sql
🧰 Additional context used
🧠 Learnings (1)
app/controllers/AuthenticationController.scala (1)
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8393
File: app/controllers/AuthenticationController.scala:566-573
Timestamp: 2025-04-09T13:32:39.564Z
Learning: In the webknossos project, prefer using the ?~> operator for error handling with message keys stored in conf/messages rather than extensive custom error handling.
🧬 Code Graph Analysis (2)
app/models/user/WebAuthnCredentials.scala (1)
app/utils/sql/SQLDAO.scala (2)
SQLDAO(14-99)parseAll(40-43)
app/controllers/AuthenticationController.scala (5)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/TemporaryStore.scala (3)
TemporaryStore(9-78)insert(55-60)remove(69-74)app/models/user/UserService.scala (2)
retrieve(269-272)insert(92-136)app/models/user/User.scala (3)
User(24-26)User(28-62)findOne(149-156)app/models/user/MultiUser.scala (1)
updateLastLoggedInIdentity(106-114)app/models/user/WebAuthnCredentials.scala (2)
findAllForUser(99-108)WebAuthnCredential(25-44)
🔇 Additional comments (10)
tools/postgres/schema.sql (1)
24-24: No concerns with the schema version increment.The insertion of the new schema version (131) is consistent with the addition of a new table.
app/models/user/WebAuthnCredentials.scala (5)
46-65: Looks good and clear.The
AttestationStatementEnvelopeclass is clearly handling attestation statements. Serde logic looks well structured.
67-75: No issues with DAO structure so far.The class extension from
SQLDAOis set up correctly.
99-107: Query logic is straightforward.Filtering by both
_multiUserand a read-access query aligns with your usual pattern.
108-119: Insert logic is consistent with the schema.The code properly converts and stores attestation data, plus counters.
120-125:❓ Verification inconclusive
Revisit soft-deletion vs. physical deletion.
The table includes
isDeleted, yetremoveByIdphysically deletes the row. Confirm this aligns with your desired lifecycle for passkeys.Would you like to verify if the code references
isDeletedin other queries, or if a truly hard delete is desired? You can run:
🏁 Script executed:
#!/bin/bash # Searching for usage of `isDeleted` in the DAO or service layer rg "isDeleted" -A 5Length of output: 79039
Review Reassessment: Revisit the Deletion Strategy for Passkeys
In
app/models/user/WebAuthnCredentials.scala(lines 120–125), theremoveByIdmethod issues a hard delete (using a DELETE statement) on thewebauthncredentialstable. However, the table schema (see migration131-add-webauthn-credentials.sql) and similar modules across the codebase consistently use anisDeletedflag to implement soft deletion.
- Confirm whether passkeys (WebAuthn credentials) are intended to be permanently removed or merely marked as deleted.
- If consistent soft deletion is desired (as in other models), consider updating
removeByIdto setisDeleted = truerather than physically deleting the record.- Otherwise, please provide justification for why a hard delete is acceptable here despite the soft-delete pattern used elsewhere.
app/controllers/AuthenticationController.scala (4)
6-7: Imports for new WebAuthn features are fine.No immediate issues with the additional imports for
TemporaryStoreand WebAuthn dependencies.
44-45: SecureRandom usage is acceptable.Using a secure random source is appropriate here.
573-589: Ok approach for generating challenge.Storing the request options in the
TemporaryStorewith a short timeout is correct for ephemeral WebAuthn challenges.
669-705: Robust registration finalization flow.Kudos for removing the challenge from the store after retrieval. The usage of the manager’s parse and verify methods is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
tools/postgres/schema.sql (1)
456-467: Ensure consistency of column definitions with objectId usage.While creating the
webauthnCredentialstable,_multiUseris defined withCHAR(24)and_idis defined asTEXT. In other tables,_multiUseroften usesTEXTwith a correspondingCHECKconstraint for 24-hex objectId formats. Consider making these definitions consistent across the schema (e.g., usingTEXT CONSTRAINT _multiUser_objectId CHECK) or adding an equivalent constraint for_multiUserto ensure uniform validations.- _multiUser CHAR(24) NOT NULL, + _multiUser TEXT CONSTRAINT _multiUser_objectId CHECK (_multiUser ~ '^[0-9a-f]{24}$') NOT NULL,app/models/user/WebAuthnCredentials.scala (3)
22-41: Consider clarifying purpose of theisDeletedflag.The
WebAuthnCredentialcase class includes anisDeletedflag. Ensure that the DAO or other services respect it (e.g., queries filtering out deleted entries). Otherwise, consider removing the flag to simplify the model if soft-deletion is not actually used.
72-94: Validate partial fields inparse.In the
parsemethod, the code gracefully converts and handles typed fields likeserializedattestationstatementandserializedattestedcredential. This is correct. However, consider adding explicit logging or error details if these conversions fail, to help with debugging invalid or corrupted data.
127-132: Signature counter is updated properly.The simple approach to increment or set the signCount is correct. If concurrency may cause race conditions, consider a more secure approach or row-level locking to avoid overwriting updates from multiple authentication events in parallel.
app/controllers/AuthenticationController.scala (4)
6-15: Imports for TemporaryStore and WebAuthn libraries.The newly added imports for WebAuthn functionality and the
SecureRandom/UUIDusage are relevant. If repeated across multiple files, consider consolidating them in a shared utility or at least ensuring standard usage patterns (e.g., a single sharedSecureRandominstance) to avoid potential performance overhead.Also applies to: 44-50
213-213: Use config for theorigin.The hard-coded
"https://webknossos.local:9000"includes a TODO to load from config. Addressing this would improve deployment flexibility, especially when running in different environments.- private lazy val origin = new Origin("https://webknossos.local:9000") // TODO: Load from config + private lazy val origin = { + val url = conf.Http.uri + new Origin(url) + }
566-581: WebAuthn authentication start.
- A new
SecureRandominstance is created for each call. This might be okay for short bursts but consider a single shared instance.- Storing
webauthn-sessioncookie in a short-lived temporary store works well.- val random = new SecureRandom() // TODO: Initialize once + // Possibly define one SecureRandom at the class level or use a specialized random source
619-657: Starting the WebAuthn registration flow.
- Excluding credentials that match existing
_multiUseris correct, but note that the code uses_multiUser.id.getBytesfor the ID rather than the actual credential ID. Double-check if that’s intended.- Again, a new
SecureRandomis created for each request.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/controllers/AuthenticationController.scala(7 hunks)app/models/user/WebAuthnCredentials.scala(1 hunks)conf/evolutions/131-add-webauthn-credentials.sql(1 hunks)tools/postgres/schema.sql(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
tools/postgres/schema.sql (1)
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransaction.scala:80-88
Timestamp: 2025-01-27T11:53:16.873Z
Learning: Credit balance consistency in WEBKNOSSOS is maintained through a PostgreSQL BEFORE trigger that calculates the total balance using SUM(credit_change) and prevents negative balances. The implementation includes DEFERRABLE foreign key constraints for flexibility, while the trigger ensures atomic balance checks within transactions.
🔇 Additional comments (19)
tools/postgres/schema.sql (2)
24-24: Confirm schema version bump to 131.You are incrementing the schema version from 130 to 131. This looks intentional and necessary for the new WebAuthn changes introduced in this file.
938-939: Foreign key reference is consistent.Defining
ON DELETE CASCADEandON UPDATE CASCADEwithDEFERRABLEensures that credential records are removed along with their associatedmultiUserswhile preserving transactional integrity.conf/evolutions/131-add-webauthn-credentials.sql (5)
1-3: Enforce precondition checks.The
ASSERTstatement ensures the schema is currently at version 130, preventing accidental application of this migration out of sequence. This is a good practice for safety.
5-16: Table creation for WebAuthn credentials.The table schema aligns with typical WebAuthn storage requirements, including attestation statements and signature counters. Consider adding a
_idformat check if consistency with other_idcolumns is required.
18-19: Foreign key ensures user linkage.The added foreign key referencing
multiUsers(_id)withON DELETE CASCADE ON UPDATE CASCADE DEFERRABLEis appropriate, ensuring credentials are cleaned up when the user is removed.
21-21: Update schema version to 131.This evolution step completes the migration by bumping the schema version.
23-23: Wrapping up with transaction commit.Finalizing with a
COMMITafter the schema update is standard practice. No issues here.app/models/user/WebAuthnCredentials.scala (5)
43-62: Well-structured attestation statement wrapper.The
AttestationStatementEnvelopeprovides a clear, polymorphic JSON serialization approach. Including thefmtproperty externally is a good match for WebAuthn’s spec.
96-104:findAllForUsermethod reads well.The method fetches credential rows by
_multiUserand parses them. Ensure thereadAccessQueryyields the correct row-level security in multi-tenant scenarios. If other checks or filters (e.g.isDeleted) are intended, add them to the SQL.
105-112: Ensure handling of not-found credentials by ID.
findByCredentialIdstrictly returns the first matching row or fails. This is correct if only one credential can share the samecredentialIdunder a single_multiUser. If collisions or multiple matches are possible, consider returning a list or a more controlled resolution.
113-125: Insert routine is complete and consistent.All serialized data fields (
serializedAttestationStatement,serializedAttestedCredential,serializedExtensions) are correctly inserted, addressing the known issue of incomplete storage. The code also sets thesignatureCountproperly.
134-138: Credential removal is straightforward.
removeByIdlogically deletes a record by_idand_multiUser. This matches the single-user usage pattern.app/controllers/AuthenticationController.scala (7)
174-174: Dependency injection for WebAuthn.Injecting
webAuthnCredentialDAO,temporaryAssertionStore, andtemporaryRegistrationStoreis consistent with the existing pattern. This design fosters testability by allowing these dependencies to be mocked.
300-318: RefactoredauthenticateInnerfor code clarity.Splitting out credential-based login logic into
authenticateInneris a clean separation of concerns. Mind checking for concurrency issues if multiple login attempts update the same user simultaneously (e.g., user’s lastLogin).
333-333: Inline usage ofauthenticateInner.This call is straightforward and aligns well with the form-based authentication flow.
583-617: WebAuthn authentication finalize.
- The challenge is retrieved once and then removed, preventing replay.
- The DB lookup via
findByCredentialIdensures the user’s passkey belongs to them.- The sign count is updated, reflecting replay protection.
659-696: Completing WebAuthn registration.
- The verification with
webAuthnManager.verify(...)covers attestation logic.- The
CredentialRecordImplis constructed with minimal needed fields, which is sufficient for typical passkey usage.- Insert logic to the DAO is consistent.
698-705: Listing keys viawebauthnListKeys.Returning only
_idand name is good for a minimal response. Confirm any UI references of the credential ID use_id, not the underlying raw credential ID bytes.
707-714: Removing a key by_id.
- Method directly calls
removeById.- No additional user verification is needed if the DAO call ensures
_multiUsercorrectness.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
app/controllers/AuthenticationController.scala (3)
212-212:⚠️ Potential issueReplace hardcoded origin with configuration
You're using a hardcoded origin URL with a TODO comment. This should be replaced with a proper configuration-based value.
-private lazy val origin = new Origin("https://webknossos.local:9000") // TODO: Load from config +private lazy val origin = { + try { + val uri = conf.http.uri + new Origin(uri) + } catch { + case e: Exception => + logger.error(s"Failed to create Origin from URI: ${e.getMessage}") + throw e + } +}
705-712: 🛠️ Refactor suggestionAdd validation for WebAuthn key removal
The current implementation doesn't validate that the key belongs to the user before removing it. This could be a security issue.
def webauthnRemoveKey: Action[WebAuthnKeyDescriptor] = sil.SecuredAction.async(validateJson[WebAuthnKeyDescriptor]) { implicit request => { for { - _ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser) + _ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser) ?~> "webauthn.deletionFailed" } yield Ok(Json.obj()) } }Remember to add the "webauthn.deletionFailed" message to your conf/messages file.
659-666: 🛠️ Refactor suggestionImprove challenge handling in registration finalization
There's a potential issue with how challenges are handled. The challenge is retrieved, removed, and then used. Make sure this is atomic to prevent race conditions.
registrationData <- tryo(webAuthnManager.parseRegistrationResponseJSON(Json.stringify(request.body.key))).toFox cookie <- Fox.option2Fox(request.cookies.get("webauthn-registration")) sessionId = cookie.value challenge <- { val challenge = temporaryRegistrationStore.get(sessionId) temporaryRegistrationStore.remove(sessionId) - Fox.option2Fox(challenge) + Fox.option2Fox(challenge) ?~> "Challenge not found or expired" ~> BAD_REQUEST }
🧹 Nitpick comments (4)
app/controllers/AuthenticationController.scala (4)
50-50: Remove unused importThe pipeline shows this import is unused.
-import scala.util.Try🧰 Tools
🪛 GitHub Actions: CI Pipeline
[error] 50-50: Unused import: 'scala.util.Try'.
564-579: Add logging to WebAuthn authentication startAdding logging would help with debugging authentication issues.
def webauthnAuthStart(): Action[AnyContent] = Action { val sessionId = UUID.randomUUID().toString; val cookie = Cookie("webauthn-session", sessionId, maxAge = Some(120), httpOnly = true, secure = true) val random = new SecureRandom() // TODO: Initialize once val challenge = new Array[Byte](32) // NOTE: Minimum required length are 16 bytes. random.nextBytes(challenge) val assertion = WebAuthnPublicKeyCredentialRequestOptions( challenge = challenge, timeout = Some(120000), rpId = Some(origin.getHost), userVerification = None, hints = None ) temporaryAssertionStore.insert(sessionId, assertion, Some(2 minutes)); + logger.debug("Started WebAuthn authentication") Ok(Json.toJson(assertion)).withCookies(cookie) }
696-703: Simplify webauthnListKeys methodThe method has unnecessary braces that can be removed for cleaner code.
def webauthnListKeys: Action[AnyContent] = sil.SecuredAction.async { implicit request => - { for { keys <- webAuthnCredentialDAO.findAllForUser(request.identity._multiUser) reducedKeys = keys.map(credential => WebAuthnKeyDescriptor(credential._id, credential.name)) } yield Ok(Json.toJson(reducedKeys)) - } }
691-693: Add logging for successful credential registrationAdding logging for successful credential registration would help with auditing and debugging.
) _ <- webAuthnCredentialDAO.insertOne(credential) + logger.info(s"User ${request.identity._id} registered new WebAuthn credential: ${request.body.name}") } yield Ok(Json.obj("message" -> "Key registered successfully"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
MIGRATIONS.unreleased.md(1 hunks)app/controllers/AuthenticationController.scala(7 hunks)conf/webknossos.latest.routes(1 hunks)frontend/javascripts/admin/admin_rest_api.ts(3 hunks)project/Dependencies.scala(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- conf/webknossos.latest.routes
🚧 Files skipped from review as they are similar to previous changes (2)
- MIGRATIONS.unreleased.md
- frontend/javascripts/admin/admin_rest_api.ts
🧰 Additional context used
🪛 GitHub Actions: CI Pipeline
app/controllers/AuthenticationController.scala
[error] 7-8: Compilation error: object 'yubico' is not a member of package 'com' in import statements.
[error] 50-50: Unused import: 'scala.util.Try'.
🔇 Additional comments (5)
project/Dependencies.scala (2)
108-109: WebAuthn dependency added correctlyThe WebAuthn4J library is properly added with version 0.28.6.RELEASE and correctly excludes jackson-databind to avoid dependency conflicts.
115-115:❓ Verification inconclusive
Jackson-databind override is appropriate
The explicit override of jackson-databind to version 2.14.3 ensures compatibility with the WebAuthn4J library while maintaining consistency with other dependencies in the project.
🏁 Script executed:
#!/bin/bash # Check if there are any known vulnerabilities in this jackson-databind version echo "Checking for vulnerabilities in jackson-databind 2.14.3..." curl -s "https://nvd.nist.gov/vuln/search/results?form_type=Basic&results_type=overview&query=jackson-databind+2.14.3&search_type=all" | grep -i CVELength of output: 277
Dependency Override Appears Appropriate – Manual Verification Recommended
The explicit override of jackson-databind to version 2.14.3 effectively maintains compatibility with WebAuthn4J and keeps our dependency versions consistent. Note that the automated vulnerability check did not return any output. Please manually verify that no critical vulnerabilities affect version 2.14.3 before finalizing the update.
- Confirm vulnerability status through another trusted source (e.g., CVE databases or security advisories).
- Ensure ongoing monitoring for any future security issues related to jackson-databind.
app/controllers/AuthenticationController.scala (3)
296-316: Well-implemented authentication inner methodThe extraction of common authentication logic into a separate method is a good refactoring. This allows for code reuse between traditional and WebAuthn authentication flows.
52-172: Well-structured WebAuthn data modelsThe WebAuthn data models are well-defined with proper JSON serialization. The code is well-commented with references to the WebAuthn specification.
213-218: Well-configured WebAuthn parametersThe WebAuthn public key parameters are correctly configured to support multiple algorithms (Ed25519, ES256, RS256), providing good compatibility with different authenticators.
| val random = new SecureRandom() // TODO: Initialize once | ||
| val challenge = new Array[Byte](32) // NOTE: Minimum required length are 16 bytes. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move SecureRandom initialization to class level
Creating a new SecureRandom instance for each request is inefficient. Move this to a class-level field.
- val random = new SecureRandom() // TODO: Initialize once
- val challenge = new Array[Byte](32) // NOTE: Minimum required length are 16 bytes.
+ val challenge = new Array[Byte](32)
+ secureRandom.nextBytes(challenge)Add this at the class level:
private val secureRandom = new SecureRandom()| // TODO: Validate | ||
| userId <- Fox.option2Fox(multiUser._lastLoggedInIdentity) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address TODOs in WebAuthn authentication
There are TODOs in the authentication parameters that should be addressed before deploying to production.
params = new AuthenticationParameters(
serverProperty,
credential.credentialRecord,
null,
- true, // TODO
- false // TODO
+ true, // Require user verification
+ false // Don't require backup eligibility
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: Validate | |
| userId <- Fox.option2Fox(multiUser._lastLoggedInIdentity) | |
| params = new AuthenticationParameters( | |
| serverProperty, | |
| credential.credentialRecord, | |
| null, | |
| true, // Require user verification | |
| false // Don't require backup eligibility | |
| ) |
| import com.yubico.webauthn._ | ||
| import com.yubico.webauthn.data._ |
There was a problem hiding this comment.
Fix the Yubico WebAuthn import
The pipeline is failing because the Yubico WebAuthn library is not correctly imported. The error suggests the library might not be included in the dependencies.
-import com.yubico.webauthn._
-import com.yubico.webauthn.data._
+import com.webauthn4j.authenticator._
+import com.webauthn4j.converter.util._Alternatively, add the Yubico WebAuthn library to your dependencies in project/Dependencies.scala:
"com.webauthn4j" % "webauthn4j-core" % "0.28.6.RELEASE" exclude("com.fasterxml.jackson.core", "jackson-databind"),
+"com.yubico" % "webauthn-server-core" % "2.5.0",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import com.yubico.webauthn._ | |
| import com.yubico.webauthn.data._ | |
| import com.webauthn4j.authenticator._ | |
| import com.webauthn4j.converter.util._ |
🧰 Tools
🪛 GitHub Actions: CI Pipeline
[error] 7-8: Compilation error: object 'yubico' is not a member of package 'com' in import statements.
| credentialId = authData.getCredentialId | ||
| multiUserId = ObjectId(new String(authData.getUserHandle)) | ||
| multiUser <- multiUserDAO.findOne(multiUserId)(GlobalAccessContext) | ||
| credential <- webAuthnCredentialDAO.findByCredentialId(multiUser._id, credentialId)(GlobalAccessContext) | ||
| serverProperty = new ServerProperty(origin, origin.getHost, challenge) | ||
|
|
||
| params = new AuthenticationParameters( | ||
| serverProperty, | ||
| credential.credentialRecord, | ||
| null, | ||
| true, // TODO | ||
| false // TODO | ||
| ) | ||
| _ <- Fox.box2Fox(tryo(webAuthnManager.verify(authData, params))) | ||
| _ = credential.credentialRecord.setCounter(authData.getAuthenticatorData.getSignCount) | ||
| _ <- webAuthnCredentialDAO.updateSignCount(credential) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in WebAuthn authentication
The current error handling in the authentication finalization is minimal. Add more specific error messages to help debugging.
authData <- Fox.box2Fox(tryo(webAuthnManager.parseAuthenticationResponseJSON(Json.stringify(request.body.key))))
credentialId = authData.getCredentialId
multiUserId = ObjectId(new String(authData.getUserHandle))
- multiUser <- multiUserDAO.findOne(multiUserId)(GlobalAccessContext)
- credential <- webAuthnCredentialDAO.findByCredentialId(multiUser._id, credentialId)(GlobalAccessContext)
+ multiUser <- multiUserDAO.findOne(multiUserId)(GlobalAccessContext) ?~> "User not found" ~> NOT_FOUND
+ credential <- webAuthnCredentialDAO.findByCredentialId(multiUser._id, credentialId)(GlobalAccessContext) ?~> "Credential not found" ~> NOT_FOUND
serverProperty = new ServerProperty(origin, origin.getHost, challenge)
params = new AuthenticationParameters(
serverProperty,
credential.credentialRecord,
null,
true, // TODO
false // TODO
)
- _ <- Fox.box2Fox(tryo(webAuthnManager.verify(authData, params)))
+ _ <- Fox.box2Fox(tryo(webAuthnManager.verify(authData, params))) ?~> "Authentication verification failed" ~> UNAUTHORIZED📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| credentialId = authData.getCredentialId | |
| multiUserId = ObjectId(new String(authData.getUserHandle)) | |
| multiUser <- multiUserDAO.findOne(multiUserId)(GlobalAccessContext) | |
| credential <- webAuthnCredentialDAO.findByCredentialId(multiUser._id, credentialId)(GlobalAccessContext) | |
| serverProperty = new ServerProperty(origin, origin.getHost, challenge) | |
| params = new AuthenticationParameters( | |
| serverProperty, | |
| credential.credentialRecord, | |
| null, | |
| true, // TODO | |
| false // TODO | |
| ) | |
| _ <- Fox.box2Fox(tryo(webAuthnManager.verify(authData, params))) | |
| _ = credential.credentialRecord.setCounter(authData.getAuthenticatorData.getSignCount) | |
| _ <- webAuthnCredentialDAO.updateSignCount(credential) | |
| authData <- Fox.box2Fox(tryo(webAuthnManager.parseAuthenticationResponseJSON(Json.stringify(request.body.key)))) | |
| credentialId = authData.getCredentialId | |
| multiUserId = ObjectId(new String(authData.getUserHandle)) | |
| multiUser <- multiUserDAO.findOne(multiUserId)(GlobalAccessContext) ?~> "User not found" ~> NOT_FOUND | |
| credential <- webAuthnCredentialDAO.findByCredentialId(multiUser._id, credentialId)(GlobalAccessContext) ?~> "Credential not found" ~> NOT_FOUND | |
| serverProperty = new ServerProperty(origin, origin.getHost, challenge) | |
| params = new AuthenticationParameters( | |
| serverProperty, | |
| credential.credentialRecord, | |
| null, | |
| true, // TODO | |
| false // TODO | |
| ) | |
| _ <- Fox.box2Fox(tryo(webAuthnManager.verify(authData, params))) ?~> "Authentication verification failed" ~> UNAUTHORIZED | |
| _ = credential.credentialRecord.setCounter(authData.getAuthenticatorData.getSignCount) | |
| _ <- webAuthnCredentialDAO.updateSignCount(credential) |
| `type`="public-key" | ||
| )).toArray | ||
| random = new SecureRandom() // TODO: Initialize once | ||
| challenge = { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reuse the SecureRandom instance
This is another instance where SecureRandom is created unnecessarily. Use the class-level instance.
- random = new SecureRandom() // TODO: Initialize once
+ // Using the class-level secureRandom📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| challenge = { | |
| challenge = { | |
| // Using the class-level secureRandom | |
| // ... additional logic to generate challenge | |
| } |
| id = origin.getHost(), | ||
| name = "Webknossos", |
There was a problem hiding this comment.
Origin host mismatch with RelyingParty ID
The relying party ID should match the origin host configured earlier. Using the hardcoded value can lead to security issues and authentication failures.
rp = WebAuthnCreationOptionsRelyingParty(
- id = origin.getHost(),
+ id = origin.getHost,
name = "Webknossos",
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| id = origin.getHost(), | |
| name = "Webknossos", | |
| id = origin.getHost, | |
| name = "Webknossos", |
URL of deployed dev instance (used for testing):
Steps to test:
If your browser requires TLS:
gen-ssl-dev-certs.shyarn start-tlsTODOs:
(Please delete unneeded items, merge only when none are left open)