Implement Webauthn #8393
Implement Webauthn #8393
Conversation
📝 WalkthroughWalkthroughThis update introduces comprehensive WebAuthn (passkey) support across the backend and frontend. It adds new database tables and migrations, backend models and controllers for credential management, frontend UI for passkey login and management, REST API endpoints and client functions, and required dependencies for both Scala and JavaScript. TLS support for development is also enhanced. Changes
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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: 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.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
conf/evolutions/135-add-webauthn-credentials.sql (1)
14-14: Clarify the intention of the UNIQUE constraint.The UNIQUE constraint on
(_id, credentialId)appears redundant since_idis already the PRIMARY KEY. Consider:
- If ensuring
credentialIduniqueness across all users:UNIQUE (credentialId)- If ensuring
credentialIduniqueness per user:UNIQUE (_multiUser, credentialId)- If this constraint serves a specific purpose, please add a comment explaining it.
frontend/javascripts/admin/rest_api.ts (1)
194-199: Use appropriate response type for DELETE operation.The function uses
sendJSONReceiveArraybufferbut DELETE operations typically return JSON responses. Consider usingsendJSONReceiveJSONinstead for consistency with other DELETE operations in this file.-export async function removeWebAuthnKey(key: WebAuthnKeyDescriptor): Promise<any> { - return await Request.sendJSONReceiveArraybuffer("/api/auth/webauthn/keys", { +export async function removeWebAuthnKey(key: WebAuthnKeyDescriptor): Promise<any> { + return await Request.sendJSONReceiveJSON("/api/auth/webauthn/keys", { method: "DELETE", data: key, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
MIGRATIONS.unreleased.md(1 hunks)app/controllers/AuthenticationController.scala(7 hunks)app/models/user/MultiUser.scala(1 hunks)app/models/user/WebAuthnCredentials.scala(1 hunks)conf/evolutions/135-add-webauthn-credentials.sql(1 hunks)conf/evolutions/reversions/135-add-webauthn-credentials.sql(1 hunks)conf/webknossos.latest.routes(1 hunks)frontend/javascripts/admin/auth/change_password_view.tsx(4 hunks)frontend/javascripts/admin/auth/login_form.tsx(2 hunks)frontend/javascripts/admin/rest_api.ts(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- conf/evolutions/reversions/135-add-webauthn-credentials.sql
- conf/webknossos.latest.routes
🚧 Files skipped from review as they are similar to previous changes (4)
- MIGRATIONS.unreleased.md
- frontend/javascripts/admin/auth/login_form.tsx
- app/models/user/MultiUser.scala
- frontend/javascripts/admin/auth/change_password_view.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/controllers/AuthenticationController.scala (9)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/TemporaryStore.scala (4)
TemporaryStore(9-78)insert(55-60)get(13-18)remove(69-74)app/security/CombinedAuthenticatorService.scala (7)
id(20-20)loginInfo(24-24)retrieve(71-80)create(55-58)init(84-88)embed(88-92)embed(92-95)frontend/javascripts/types/api_types.ts (1)
WebAuthnKeyDescriptor(1250-1253)app/models/user/User.scala (4)
User(23-25)User(27-61)updateLastActivity(490-497)insertOne(472-490)app/models/user/UserService.scala (3)
retrieve(269-272)insert(92-136)emailFor(179-184)util/src/main/scala/com/scalableminds/util/tools/Fox.scala (9)
flatMap(269-279)flatMap(364-364)Fox(30-224)Fox(226-290)runIf(170-179)successful(53-56)fromFuture(42-53)map(266-269)map(362-362)app/models/user/MultiUser.scala (2)
updateLastLoggedInIdentity(106-114)insertOne(64-77)util/src/main/scala/com/scalableminds/util/mvc/ExtendedController.scala (1)
validateJson(204-209)app/models/user/WebAuthnCredentials.scala (6)
findByCredentialId(81-91)updateSignCount(106-112)findAllForUser(72-81)WebAuthnCredential(23-37)insertOne(91-104)removeById(114-119)
🔇 Additional comments (12)
conf/evolutions/135-add-webauthn-credentials.sql (1)
1-22: LGTM! Well-structured migration script.The migration follows best practices with proper transaction management, schema version assertion, and appropriate table structure for WebAuthn credential storage.
frontend/javascripts/admin/rest_api.ts (3)
152-167: LGTM! Well-implemented WebAuthn login flow.The function follows the established pattern for login functions and correctly implements the WebAuthn authentication flow with proper API calls and return types.
169-188: LGTM! Proper WebAuthn registration implementation.The function correctly implements the WebAuthn registration flow with appropriate challenge handling and credential creation.
190-192: LGTM! Clean and simple implementation.The function properly implements the key listing functionality with appropriate typing.
app/models/user/WebAuthnCredentials.scala (2)
23-37: LGTM! Well-designed case class with proper serialization.The WebAuthnCredential case class is well-structured with appropriate fields and correct serialization methods using WebAuthn4J converters.
72-117: LGTM! Comprehensive DAO implementation with proper access control.The DAO methods provide complete CRUD operations with appropriate access control using DBAccessContext and follow established patterns in the codebase.
app/controllers/AuthenticationController.scala (6)
6-52: LGTM: Comprehensive WebAuthn importsThe imports are well-organized and include all necessary components for WebAuthn implementation using the webauthn4j library. The addition of TemporaryStore, SecureRandom, and UUID classes supports the security requirements of WebAuthn.
53-179: Excellent WebAuthn data structure implementationThe case classes accurately follow the WebAuthn specification with excellent documentation referencing MDN. The implementation includes all necessary fields while appropriately omitting unused ones as documented in comments.
201-204: Appropriate dependency injection for WebAuthnThe new constructor parameters properly inject the necessary dependencies for WebAuthn functionality while maintaining consistency with existing patterns.
212-229: Well-designed WebAuthn configurationThe private fields are properly structured with a single SecureRandom instance, appropriate algorithm choices, and correct origin configuration using the application's URI settings.
627-725: Solid WebAuthn registration and key management implementationThe registration flow properly handles challenge generation, credential exclusion, and verification. The key management endpoints are clean and follow the established codebase patterns for error handling using the
?~>operator.
590-625:❌ Incorrect review comment
Verify userHandle usage in authentication
The authentication flow implementation is solid, but line 602 treats
userHandleas an email address. According to the WebAuthn specification,userHandleshould typically contain a user identifier (like user ID), not email. This could lead to security issues if the userHandle doesn't match what was set during registration.Please verify that the userHandle contains the email and not the user ID. If this is intentional, consider adding a comment explaining why email is used instead of user ID for the userHandle.
🏁 Script executed:
#!/bin/bash # Verify how userHandle is set during registration ast-grep --pattern 'id = Base64.encodeBase64URLSafeString($_.getBytes)'Length of output: 240
userHandle encoding is intentional and correct
TheuserHandleis deliberately set to the Base64-encoded email (see line 632’s comment: “Not leak database id, but use email instead”). No further changes needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Awesome thanks for the hard work @robert-oleynik 💪 🚀
Please find my sugestions for improvements below. Shouldn't be too much I hope.
One important this would still be: I found a comment regardin "validation is TODO". This is a still something important that needs to be done, correct?
| @@ -418,6 +418,17 @@ CREATE TABLE webknossos.multiUsers( | |||
| CONSTRAINT nuxInfoIsJsonObject CHECK(jsonb_typeof(novelUserExperienceInfos) = 'object') | |||
| ); | |||
|
|
|||
| CREATE TABLE webknossos.webauthnCredentials( | |||
There was a problem hiding this comment.
thats still todo as far as I can see
| - [134-dataset-layer-attachments.sql](conf/evolutions/134-dataset-layer-attachments.sql) | ||
| - [135-add-webauthn-credentials.sql](./conf/evolutions/131-add-webauthn-credentials.sql) |
There was a problem hiding this comment.
the path here also needs to be updated
| challenge: String, | ||
| excludeCredentials: Array[WebAuthnCreationOptionsExcludeCredentials], | ||
| pubKeyCredParams: Array[WebAuthnCreationOptionsPubKeyParam], | ||
| timeout: Int, // NOTE: timeout in milliseconds |
There was a problem hiding this comment.
For all comments starting with NOTE: please remove the NOTE: part. The sole reason is that this is uncomment and usually the we do not have a "bycomment" like NOTE in our comments.
| import scala.jdk.CollectionConverters._ | ||
|
|
||
| /** | ||
| * Object reference: https://developer.mozilla.org/en-US/docs/Web/API/PublicKeyCredentialCreationOptions |
There was a problem hiding this comment.
Thanks for adding all the doc references. They might become very helpful in the future 🙏
| val cookie = Cookie("webauthn-session", sessionId, maxAge = Some(120), httpOnly = true, secure = true) | ||
| val challenge = new Array[Byte](32) // NOTE: Minimum required length are 16 bytes. | ||
| secureRandom.nextBytes(challenge) | ||
| val assertion = WebAuthnPublicKeyCredentialRequestOptions( | ||
| challenge = Base64.encodeBase64URLSafeString(challenge), | ||
| timeout = Some(120000), |
There was a problem hiding this comment.
The 120000 and 120 timeout correlate with each other, correct? One is in seconds and the other in milliseconds. Could you please extract the 120 seconds into a constant and if you take a look at this: I think you can even do something like:
val timeout: FiniteDuration = 120.seconds
val timeoutInSeconds: Int = timeout.toSeconds.toInt
val timeoutInMilliseconds: Int = timeout.toMillis.toInt
but you need to import scala.concurrent.duration._ for that.
| onLoggedIn(); | ||
| } | ||
| } catch (error) { | ||
| console.error("WebAuthn login failed", error); |
There was a problem hiding this comment.
Could you please also show an error toast here of this?
| onClick={async () => { | ||
| try { | ||
| const [user, organization] = await doWebAuthnLogin(); | ||
| Store.dispatch(setActiveUserAction(user)); | ||
| Store.dispatch(setActiveOrganizationAction(organization)); | ||
| if (onLoggedIn) { | ||
| onLoggedIn(); | ||
| } | ||
| } catch (error) { | ||
| console.error("WebAuthn login failed", error); | ||
| } | ||
| }} |
There was a problem hiding this comment.
And to make the rendering code a little more compact, could you please consider moving this into an own function which can be called here?
| @@ -147,6 +149,55 @@ export async function loginUser(formValues: { | |||
| return [activeUser, organization]; | |||
| } | |||
|
|
|||
| export async function doWebAuthnLogin(): Promise<[APIUser, APIOrganization]> { | |||
There was a problem hiding this comment.
We started to make this module / file smaller by splitting it up into multiple files. Could you please move these new functions into an own file admin/api/webauthn.ts or so? That would be greately appreciated. This file already has 2.4k lines on this branch 🙈
| export type WebAuthnKeyDescriptor = { | ||
| id: string; | ||
| name: string; | ||
| }; |
There was a problem hiding this comment.
This can then also be moved into the new admin/api/webauthn.ts file (or however you'll name it)
| @@ -463,6 +463,18 @@ CREATE TABLE webknossos.multiUsers( | |||
| CONSTRAINT nuxInfoIsJsonObject CHECK(jsonb_typeof(novelUserExperienceInfos) = 'object') | |||
| ); | |||
|
|
|||
| CREATE TABLE webknossos.webauthnCredentials( | |||
| _id TEXT PRIMARY KEY, | |||
| _multiUser CHAR(24) NOT NULL, | |||
There was a problem hiding this comment.
Please use TEXT here. See my previous comment
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)