Implement Webauthn #8393
Implement Webauthn #8393
Conversation
📝 WalkthroughWalkthroughThis update introduces comprehensive support for passkey-based authentication using WebAuthn across the backend, frontend, and database. It adds new API endpoints, data models, DAOs, frontend components, configuration flags, and documentation to enable users to register, authenticate, list, and remove passkeys. Schema migrations and build changes are included. Changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
Security Considerationshttps://w3c.github.io/webauthn/#sctn-security-considerations-rp
Privacy Considerationshttps://w3c.github.io/webauthn/#sctn-privacy-considerations-rp
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tools/proxy/proxy.js (1)
2-2: Use Node.js import protocol for built-in modules.Follow modern Node.js conventions by using the
node:protocol for built-in module imports.-const fs = require("fs"); +const fs = require("node:fs");-const https = require("https"); +const https = require("node:https");Also applies to: 7-7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/javascripts/test/backend-snapshot-tests/__snapshots__/misc.e2e.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
app/controllers/AuthenticationController.scala(7 hunks)conf/evolutions/reversions/136-add-webauthn-credentials.sql(1 hunks)project/Dependencies.scala(1 hunks)tools/proxy/proxy.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- conf/evolutions/reversions/136-add-webauthn-credentials.sql
- project/Dependencies.scala
🧰 Additional context used
🧠 Learnings (1)
app/controllers/AuthenticationController.scala (6)
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/NeuroglancerPrecomputedMeshFileService.scala:161-166
Timestamp: 2025-04-28T14:18:04.368Z
Learning: In Scala for-comprehensions with the Fox error handling monad, `Fox.fromBool()` expressions should use the `<-` binding operator instead of the `=` assignment operator to properly propagate error conditions. Using `=` will cause validation failures to be silently ignored.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8221
File: app/controllers/JobController.scala:226-232
Timestamp: 2025-01-13T09:06:15.202Z
Learning: In the JobController of webknossos, numeric parameters should use `Option[Double]` instead of `Option[String]` for better type safety. Additionally, when adding new job parameters that are conditionally required (like evaluation settings), proper validation should be added in the `for` comprehension block before creating the `commandArgs`.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:06:42.865Z
Learning: In Scala's for-comprehension with Fox (Future-like type), the `<-` operator ensures sequential execution. If any step fails, the entire chain short-circuits and returns early, preventing subsequent operations from executing. This makes it safe to perform validation checks before database operations.
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala:170-173
Timestamp: 2025-04-23T08:51:57.756Z
Learning: In the webknossos codebase, classes extending `FoxImplicits` have access to an implicit conversion from `Option[A]` to `Fox[A]`, where `None` is converted to an empty Fox that fails gracefully in for-comprehensions.
Learnt from: frcroth
PR: scalableminds/webknossos#8657
File: app/models/dataset/DatasetService.scala:380-392
Timestamp: 2025-06-16T09:04:40.603Z
Learning: Trailing commas are supported in Scala starting from version 2.12.2 as part of SIP-27. They are valid in function calls, function definitions, import statements, and other contexts. This means trailing commas after the last parameter or argument do not cause compilation errors in Scala 2.12.2+.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8393
File: app/controllers/AuthenticationController.scala:566-573
Timestamp: 2025-03-21T16:49:17.956Z
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.
🪛 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] 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (7)
tools/proxy/proxy.js (1)
174-189: Excellent TLS implementation with proper error handling.The conditional TLS server startup is well-implemented with comprehensive error handling and helpful error messages. The try-catch block properly guides users to generate SSL certificates when needed.
app/controllers/AuthenticationController.scala (6)
53-189: Well-structured WebAuthn data models with comprehensive documentation.The WebAuthn case classes are well-defined with proper JSON serialization and excellent documentation referencing the official WebAuthn specification. The clear documentation of omitted fields helps future maintainers understand the implementation decisions.
585-606: Secure WebAuthn authentication start implementation.The authentication start method properly implements:
- Feature flag validation
- Secure random challenge generation
- Secure cookie configuration with
SameSite.Strict- Consistent timeout handling using the
webauthnTimeoutconstant- Proper temporary storage management
The implementation follows security best practices for WebAuthn.
608-646: Robust WebAuthn authentication finalization with proper validation.The authentication finalization properly handles:
- Challenge retrieval and validation using the
popmethod- Comprehensive error handling with appropriate HTTP status codes
- Signature counter validation to prevent replay attacks
- Secure user lookup and authentication flow
The implementation correctly follows the WebAuthn specification and security requirements.
648-690: Comprehensive WebAuthn registration start implementation.The registration start method includes:
- Proper user data encoding
- Credential exclusion to prevent duplicate registrations
- Secure challenge generation and storage
- Consistent timeout and cookie security settings
The implementation follows WebAuthn best practices for registration initiation.
692-730: Secure WebAuthn registration finalization with proper verification.The registration finalization correctly implements:
- Challenge validation using the
popmethod- Proper WebAuthn verification parameters
- Secure credential record creation
- Database persistence with error handling
The implementation follows the WebAuthn specification and security requirements.
732-749: Clean WebAuthn key management implementation.The key listing and removal methods are straightforward and secure:
- Proper feature flag validation
- Secure key listing with reduced data exposure
- Safe key removal with user validation
The implementation provides a clean API for passkey management.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
app/controllers/AuthenticationController.scala (1)
622-622: Critical: Fix userHandle decoding issueThe userHandle is base64-encoded during registration (line 658) but treated as a plain string during authentication. This will cause authentication failures.
The userHandle needs to be decoded from base64:
- multiUserId <- ObjectId.fromString(new String(authData.getUserHandle)) ~> "Passkey Authentication Failed" + multiUserId <- ObjectId.fromString(new String(Base64.decodeBase64(authData.getUserHandle), StandardCharsets.UTF_8)) ~> "Passkey Authentication Failed"This issue was previously identified in past reviews and prevents WebAuthn authentication from working.
🧹 Nitpick comments (5)
tools/proxy/proxy.js (2)
2-2: Add node: protocol for fs import.Consider using the
node:protocol for Node.js builtin modules to be more explicit.-const fs = require("fs"); +const fs = require("node:fs");
7-7: Add node: protocol for https import.Consider using the
node:protocol for Node.js builtin modules to be more explicit.-const https = require("https"); +const https = require("node:https");frontend/javascripts/admin/auth/passkeys_view.tsx (3)
2-2: Remove unused imports.Several imports are unused and should be removed to clean up the code.
-import { Alert, Button, Col, Form, Input, Modal, Row, Table } from "antd"; +import { Button, Input, Modal, Table } from "antd";
3-10: Remove unused imports and variables.Multiple imports and variables are unused and should be removed.
-import Request from "libs/request"; -import Toast from "libs/toast"; -import messages from "messages"; -import { type RouteComponentProps, withRouter } from "react-router-dom"; -import { logoutUserAction } from "viewer/model/actions/user_actions"; -import Store from "viewer/store"; -const FormItem = Form.Item; -const { Password } = Input;Keep only the necessary imports:
+import Toast from "libs/toast"; +import { type RouteComponentProps, withRouter } from "react-router-dom";
23-23: Remove unused constant.The
MIN_PASSWORD_LENGTHconstant is not used in this component and should be removed.-const MIN_PASSWORD_LENGTH = 8;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/controllers/AuthenticationController.scala(7 hunks)frontend/javascripts/admin/account/account_password_view.tsx(4 hunks)frontend/javascripts/admin/auth/passkeys_view.tsx(1 hunks)frontend/javascripts/types/api_types.ts(1 hunks)tools/proxy/proxy.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/types/api_types.ts
🧰 Additional context used
🧠 Learnings (1)
app/controllers/AuthenticationController.scala (6)
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/NeuroglancerPrecomputedMeshFileService.scala:161-166
Timestamp: 2025-04-28T14:18:04.368Z
Learning: In Scala for-comprehensions with the Fox error handling monad, `Fox.fromBool()` expressions should use the `<-` binding operator instead of the `=` assignment operator to properly propagate error conditions. Using `=` will cause validation failures to be silently ignored.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8221
File: app/controllers/JobController.scala:226-232
Timestamp: 2025-01-13T09:06:15.202Z
Learning: In the JobController of webknossos, numeric parameters should use `Option[Double]` instead of `Option[String]` for better type safety. Additionally, when adding new job parameters that are conditionally required (like evaluation settings), proper validation should be added in the `for` comprehension block before creating the `commandArgs`.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:06:42.865Z
Learning: In Scala's for-comprehension with Fox (Future-like type), the `<-` operator ensures sequential execution. If any step fails, the entire chain short-circuits and returns early, preventing subsequent operations from executing. This makes it safe to perform validation checks before database operations.
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala:170-173
Timestamp: 2025-04-23T08:51:57.756Z
Learning: In the webknossos codebase, classes extending `FoxImplicits` have access to an implicit conversion from `Option[A]` to `Fox[A]`, where `None` is converted to an empty Fox that fails gracefully in for-comprehensions.
Learnt from: frcroth
PR: scalableminds/webknossos#8657
File: app/models/dataset/DatasetService.scala:380-392
Timestamp: 2025-06-16T09:04:40.603Z
Learning: Trailing commas are supported in Scala starting from version 2.12.2 as part of SIP-27. They are valid in function calls, function definitions, import statements, and other contexts. This means trailing commas after the last parameter or argument do not cause compilation errors in Scala 2.12.2+.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8393
File: app/controllers/AuthenticationController.scala:566-573
Timestamp: 2025-03-21T16:49:17.956Z
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 (1)
frontend/javascripts/admin/account/account_password_view.tsx (2)
frontend/javascripts/admin/account/helpers/settings_title.tsx (1)
SettingsTitle(5-21)frontend/javascripts/admin/account/helpers/settings_card.tsx (1)
SettingsCard(11-31)
🪛 Biome (1.9.4)
frontend/javascripts/admin/auth/passkeys_view.tsx
[error] 78-78: Expected an expression but instead found ')'.
Expected an expression here.
(parse)
[error] 77-77: The comma operator is disallowed.
Its use is often confusing and obscures side effects.
(lint/style/noCommaOperator)
[error] 2-2: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 2-2: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 2-2: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 3-3: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 5-5: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 7-7: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 8-8: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Safe fix: Remove the unused import.
(lint/correctness/noUnusedImports)
[error] 9-9: 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 FormItem with an underscore.
(lint/correctness/noUnusedVariables)
[error] 10-10: This variable is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
(lint/correctness/noUnusedVariables)
[error] 23-23: 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 MIN_PASSWORD_LENGTH with an underscore.
(lint/correctness/noUnusedVariables)
[error] 25-25: This variable is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
(lint/correctness/noUnusedVariables)
[error] 41-41: This function is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
(lint/correctness/noUnusedVariables)
[error] 76-76: This parameter is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
Unsafe fix: If this is intentional, prepend id with an underscore.
(lint/correctness/noUnusedVariables)
[error] 76-76: This parameter is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
Unsafe fix: If this is intentional, prepend passkey with an underscore.
(lint/correctness/noUnusedVariables)
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] 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (20)
frontend/javascripts/admin/account/account_password_view.tsx (4)
3-3: LGTM: Clean feature flag import.The import of the
featuresmodule is properly placed and follows the existing import structure.
15-15: LGTM: PasskeysView import is correctly placed.The import statement is properly structured and follows TypeScript conventions with the
.tsxextension.
169-169: LGTM: Feature flag extraction is clean.The destructuring of
passkeysEnabledfrom the features function is well-placed and follows the existing pattern.
192-204: LGTM: Conditional rendering implementation is clean and well-structured.The conditional rendering approach using the feature flag is well-implemented. The UI structure mirrors the password section above, maintaining consistency in the interface design.
tools/proxy/proxy.js (6)
9-9: LGTM: Time constant extraction improves readability.The
time5minconstant makes the timeout values more readable and maintainable.
12-13: LGTM: Timeout configuration uses the new constant.The timeout values are now using the extracted constant, which improves maintainability.
32-37: LGTM: TLS webpack arguments are properly structured.The TLS arguments array is well-formatted and properly structured for webpack-dev-server configuration.
50-50: LGTM: Conditional webpack arguments based on TLS flag.The conditional logic for including TLS arguments is clean and follows the expected pattern.
140-155: LGTM: Conditional TLS proxying is well-implemented.The conditional logic for proxying to webpack-dev-server with TLS support is properly structured. The
secure: falseoption is appropriate for development environments with self-signed certificates.
174-188: LGTM: TLS server creation with proper error handling.The TLS server creation includes proper error handling and informative error messages, which addresses the previous review feedback about missing error handling.
app/controllers/AuthenticationController.scala (10)
6-52: LGTM: Clean imports and dependenciesThe new imports are well-organized and appropriate for the WebAuthn functionality. The addition of
TemporaryStore, WebAuthn4J library components, and utility classes supports the implementation correctly.
53-189: Well-documented WebAuthn data structuresThe case classes accurately represent the WebAuthn API objects with proper documentation references. The structure follows the Web API specifications closely and includes appropriate JSON serialization support.
211-213: Proper dependency injection for WebAuthn componentsThe addition of WebAuthn-specific dependencies (
webAuthnCredentialDAO,temporaryAssertionStore,temporaryRegistrationStore) is correctly implemented and supports the new functionality.
222-242: Good security practices in WebAuthn configurationThe secure random generator, origin handling, and timeout configuration follow security best practices. The HTTPS check and proper algorithm support are correctly implemented.
319-337: Excellent refactoring of authentication logicThe extraction of common authentication logic into
authenticateInnerimproves code reusability and maintainability. The implementation correctly handles user retrieval, deactivation checks, and logging.
586-608: Secure WebAuthn authentication start implementationThe authentication start flow properly implements security measures including feature flag checks, HTTPS requirements, secure cookie settings with
SameSite.Strict, and proper challenge generation usingSecureRandom.
610-649: Robust WebAuthn authentication finalizationThe authentication finalization properly implements security checks, signature count validation, and credential verification. The use of
temporaryAssertionStore.pop()correctly handles challenge cleanup.
651-694: Secure WebAuthn registration start flowThe registration start properly excludes existing credentials, generates secure challenges, and sets appropriate cookie security attributes. The user encoding matches the authentication expectations.
696-735: Comprehensive WebAuthn registration finalizationThe registration finalization correctly verifies the attestation, creates credential records, and persists to the database. The error handling using
?~>operator aligns with codebase conventions.
737-754: Appropriate WebAuthn key management endpointsThe key listing and removal endpoints are correctly implemented with proper security checks and error handling. The simplified response formats are appropriate for the UI needs.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/admin/account/account_password_view.tsx(4 hunks)frontend/javascripts/admin/auth/passkeys_view.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/admin/auth/passkeys_view.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/admin/account/account_password_view.tsx (2)
frontend/javascripts/admin/account/helpers/settings_title.tsx (1)
SettingsTitle(5-21)frontend/javascripts/admin/account/helpers/settings_card.tsx (1)
SettingsCard(11-31)
🪛 GitHub Check: frontend-tests
frontend/javascripts/admin/account/account_password_view.tsx
[failure] 15-15:
An import path can only end with a '.tsx' extension when 'allowImportingTsExtensions' is enabled.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (3)
frontend/javascripts/admin/account/account_password_view.tsx (3)
4-4: LGTM: Feature flag import added correctly.The import follows the existing pattern and enables feature flag functionality for the passkeys feature.
160-160: LGTM: Feature flag extraction is correctly implemented.The destructuring of the
passkeysEnabledflag follows React best practices and enables conditional rendering based on the feature flag.
183-192: LGTM: Conditional passkeys section follows existing patterns.The implementation correctly:
- Uses feature flag to conditionally render the passkeys section
- Follows the same structure as the password section with SettingsTitle and SettingsCard
- Maintains consistent styling and layout with the existing component
- Properly integrates the PasskeysView component
This provides a clean, consistent user experience for the new passkeys functionality.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/models/user/WebAuthnCredentials.scala (2)
138-138: Remove debug print statement.Debug print statements should not be present in production code as they can clutter logs and potentially expose sensitive information.
- _ = println(attestationStatement)
147-152: Fix race condition in signature counter update.The current implementation unconditionally updates the signature count, which can lead to race conditions if multiple WebAuthn authentications occur concurrently. This could cause the counter to regress, potentially allowing replay attacks.
def updateSignCount(c: WebAuthnCredential): Fox[Unit] = { val signatureCount = c.credentialRecord.getCounter for { - _ <- run(q"""UPDATE $existingCollectionName SET signatureCount = $signatureCount WHERE _id = ${c._id}""".asUpdate) + rowsUpdated <- run(q"""UPDATE $existingCollectionName SET signatureCount = $signatureCount + WHERE _id = ${c._id} AND signatureCount < $signatureCount""".asUpdate) + _ <- if (rowsUpdated == 1) Fox.successful(()) else Fox.failure("Failed to update signature count - potential replay attack or concurrent update") } yield () }This ensures the counter only increases and fails if a concurrent update occurs, requiring the caller to reload and retry.
🧹 Nitpick comments (2)
conf/evolutions/136-add-webauthn-credentials.sql (1)
18-18: Consider removing redundant unique constraint.The unique constraint on
(_id, credentialId)appears redundant since_idis already the primary key. If the intent is to ensurecredentialIduniqueness within the table, consider usingUNIQUE (credentialId)instead.- UNIQUE (_id, credentialId) + UNIQUE (credentialId)However, if there's a specific business requirement for this composite constraint, please add a comment explaining the rationale.
app/controllers/AuthenticationController.scala (1)
662-663: Reconsider error status code for credential fetching.Using
INTERNAL_SERVER_ERRORfor credential fetching might be too harsh. This could be a legitimate case where the query succeeds but returns no results.- credentials <- webAuthnCredentialDAO - .findAllForUser(request.identity._multiUser) ?~> "Failed to fetch Passkeys" ~> INTERNAL_SERVER_ERROR + credentials <- webAuthnCredentialDAO + .findAllForUser(request.identity._multiUser) ?~> "Failed to fetch Passkeys" ~> BAD_REQUEST
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/controllers/AuthenticationController.scala(7 hunks)app/models/user/WebAuthnCredentials.scala(1 hunks)conf/evolutions/136-add-webauthn-credentials.sql(1 hunks)tools/postgres/schema.sql(4 hunks)util/src/main/scala/com/scalableminds/util/tools/Fox.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/postgres/schema.sql
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:29-35
Timestamp: 2025-07-09T07:30:57.149Z
Learning: In the webknossos project, error handling and user notifications are handled at the API function level (e.g., `removeWebAuthnKey`, `fetchPasskeys`) rather than in the UI components, so additional try-catch blocks in the UI layer are unnecessary.
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (3)
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala:170-173
Timestamp: 2025-04-23T08:51:57.756Z
Learning: In the webknossos codebase, classes extending `FoxImplicits` have access to an implicit conversion from `Option[A]` to `Fox[A]`, where `None` is converted to an empty Fox that fails gracefully in for-comprehensions.
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/NeuroglancerPrecomputedMeshFileService.scala:161-166
Timestamp: 2025-04-28T14:18:04.368Z
Learning: In Scala for-comprehensions with the Fox error handling monad, `Fox.fromBool()` expressions should use the `<-` binding operator instead of the `=` assignment operator to properly propagate error conditions. Using `=` will cause validation failures to be silently ignored.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:06:42.865Z
Learning: In Scala's for-comprehension with Fox (Future-like type), the `<-` operator ensures sequential execution. If any step fails, the entire chain short-circuits and returns early, preventing subsequent operations from executing. This makes it safe to perform validation checks before database operations.
conf/evolutions/136-add-webauthn-credentials.sql (2)
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.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransaction.scala:80-88
Timestamp: 2025-01-27T11:53:16.873Z
Learning: The credit balance consistency in WEBKNOSSOS is maintained through a PostgreSQL BEFORE trigger that checks for negative balances. The trigger runs within the same transaction as credit updates, ensuring atomicity and preventing race conditions.
app/models/user/WebAuthnCredentials.scala (1)
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8221
File: app/controllers/JobController.scala:226-232
Timestamp: 2025-01-13T09:06:15.202Z
Learning: In the JobController of webknossos, numeric parameters should use `Option[Double]` instead of `Option[String]` for better type safety. Additionally, when adding new job parameters that are conditionally required (like evaluation settings), proper validation should be added in the `for` comprehension block before creating the `commandArgs`.
app/controllers/AuthenticationController.scala (11)
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:29-35
Timestamp: 2025-07-09T07:30:57.149Z
Learning: In the webknossos project, error handling and user notifications are handled at the API function level (e.g., `removeWebAuthnKey`, `fetchPasskeys`) rather than in the UI components, so additional try-catch blocks in the UI layer are unnecessary.
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.669Z
Learning: In the webknossos project, the Request module has comprehensive built-in error handling. The triggerRequest method defaults to showErrorToast: true, automatically catches errors with .catch(), and displays user notifications via Toast.error() or Toast.messages() in the handleError method. Functions using Request methods (like doWebAuthnRegistration and listWebAuthnKeys) do not need additional error handling for user notifications.
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.669Z
Learning: In the webknossos project, the Request module's sendJSONReceiveJSON function already includes comprehensive error handling that displays Toast.error() notifications for HTTP errors. Functions using this module (like doWebAuthnRegistration and listWebAuthnKeys) do not need additional error handling for user notifications.
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/NeuroglancerPrecomputedMeshFileService.scala:161-166
Timestamp: 2025-04-28T14:18:04.368Z
Learning: In Scala for-comprehensions with the Fox error handling monad, `Fox.fromBool()` expressions should use the `<-` binding operator instead of the `=` assignment operator to properly propagate error conditions. Using `=` will cause validation failures to be silently ignored.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8221
File: app/controllers/JobController.scala:226-232
Timestamp: 2025-01-13T09:06:15.202Z
Learning: In the JobController of webknossos, numeric parameters should use `Option[Double]` instead of `Option[String]` for better type safety. Additionally, when adding new job parameters that are conditionally required (like evaluation settings), proper validation should be added in the `for` comprehension block before creating the `commandArgs`.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:06:42.865Z
Learning: In Scala's for-comprehension with Fox (Future-like type), the `<-` operator ensures sequential execution. If any step fails, the entire chain short-circuits and returns early, preventing subsequent operations from executing. This makes it safe to perform validation checks before database operations.
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala:170-173
Timestamp: 2025-04-23T08:51:57.756Z
Learning: In the webknossos codebase, classes extending `FoxImplicits` have access to an implicit conversion from `Option[A]` to `Fox[A]`, where `None` is converted to an empty Fox that fails gracefully in for-comprehensions.
Learnt from: frcroth
PR: scalableminds/webknossos#8657
File: app/models/dataset/DatasetService.scala:380-392
Timestamp: 2025-06-16T09:04:40.603Z
Learning: Trailing commas are supported in Scala starting from version 2.12.2 as part of SIP-27. They are valid in function calls, function definitions, import statements, and other contexts. This means trailing commas after the last parameter or argument do not cause compilation errors in Scala 2.12.2+.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8393
File: app/controllers/AuthenticationController.scala:566-573
Timestamp: 2025-03-21T16:49:17.956Z
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.
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.669Z
Learning: In the webknossos project, the Request module's receiveJSON and sendJSONReceiveJSON methods already include comprehensive error handling that displays user notifications for HTTP errors. Functions using these methods (like doWebAuthnRegistration and listWebAuthnKeys) do not need additional error handling for user notifications.
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.669Z
Learning: In the webknossos project, the Request module's receiveJSON and sendJSONReceiveJSON methods already include comprehensive error handling that displays user notifications for HTTP errors. Functions using these methods (like doWebAuthnRegistration and listWebAuthnKeys) do not need additional error handling for user notifications.
🧬 Code Graph Analysis (1)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (1)
util/src/main/scala/com/scalableminds/util/tools/Box.scala (1)
Full(661-699)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
🔇 Additional comments (3)
app/controllers/AuthenticationController.scala (3)
319-338: LGTM: Well-structured authentication refactoring.Good refactoring to extract the core authentication logic into a reusable
authenticateInnermethod. This allows both traditional and WebAuthn authentication to use the same core logic while maintaining consistency.
586-608: LGTM: Secure authentication start implementation.The implementation correctly:
- Validates feature flags and HTTPS requirement
- Generates cryptographically secure random challenges
- Sets proper security attributes on cookies (httpOnly, secure, sameSite)
- Uses consistent timeout handling
610-650: LGTM: Comprehensive authentication finalization.The implementation properly:
- Validates the authentication response using WebAuthn4J
- Implements signature counter validation for replay protection
- Uses atomic pop operation for challenge retrieval
- Follows the codebase error handling patterns with
?~>and??~>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/AuthenticationController.scala(7 hunks)util/src/main/scala/com/scalableminds/util/tools/Fox.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- util/src/main/scala/com/scalableminds/util/tools/Fox.scala
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:29-35
Timestamp: 2025-07-09T07:30:57.149Z
Learning: In the webknossos project, error handling and user notifications are handled at the API function level (e.g., `removeWebAuthnKey`, `fetchPasskeys`) rather than in the UI components, so additional try-catch blocks in the UI layer are unnecessary.
app/controllers/AuthenticationController.scala (11)
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:29-35
Timestamp: 2025-07-09T07:30:57.149Z
Learning: In the webknossos project, error handling and user notifications are handled at the API function level (e.g., `removeWebAuthnKey`, `fetchPasskeys`) rather than in the UI components, so additional try-catch blocks in the UI layer are unnecessary.
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.669Z
Learning: In the webknossos project, the Request module has comprehensive built-in error handling. The triggerRequest method defaults to showErrorToast: true, automatically catches errors with .catch(), and displays user notifications via Toast.error() or Toast.messages() in the handleError method. Functions using Request methods (like doWebAuthnRegistration and listWebAuthnKeys) do not need additional error handling for user notifications.
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.669Z
Learning: In the webknossos project, the Request module's sendJSONReceiveJSON function already includes comprehensive error handling that displays Toast.error() notifications for HTTP errors. Functions using this module (like doWebAuthnRegistration and listWebAuthnKeys) do not need additional error handling for user notifications.
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/NeuroglancerPrecomputedMeshFileService.scala:161-166
Timestamp: 2025-04-28T14:18:04.368Z
Learning: In Scala for-comprehensions with the Fox error handling monad, `Fox.fromBool()` expressions should use the `<-` binding operator instead of the `=` assignment operator to properly propagate error conditions. Using `=` will cause validation failures to be silently ignored.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8221
File: app/controllers/JobController.scala:226-232
Timestamp: 2025-01-13T09:06:15.202Z
Learning: In the JobController of webknossos, numeric parameters should use `Option[Double]` instead of `Option[String]` for better type safety. Additionally, when adding new job parameters that are conditionally required (like evaluation settings), proper validation should be added in the `for` comprehension block before creating the `commandArgs`.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:06:42.865Z
Learning: In Scala's for-comprehension with Fox (Future-like type), the `<-` operator ensures sequential execution. If any step fails, the entire chain short-circuits and returns early, preventing subsequent operations from executing. This makes it safe to perform validation checks before database operations.
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala:170-173
Timestamp: 2025-04-23T08:51:57.756Z
Learning: In the webknossos codebase, classes extending `FoxImplicits` have access to an implicit conversion from `Option[A]` to `Fox[A]`, where `None` is converted to an empty Fox that fails gracefully in for-comprehensions.
Learnt from: frcroth
PR: scalableminds/webknossos#8657
File: app/models/dataset/DatasetService.scala:380-392
Timestamp: 2025-06-16T09:04:40.603Z
Learning: Trailing commas are supported in Scala starting from version 2.12.2 as part of SIP-27. They are valid in function calls, function definitions, import statements, and other contexts. This means trailing commas after the last parameter or argument do not cause compilation errors in Scala 2.12.2+.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8393
File: app/controllers/AuthenticationController.scala:566-573
Timestamp: 2025-03-21T16:49:17.956Z
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.
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.669Z
Learning: In the webknossos project, the Request module's receiveJSON and sendJSONReceiveJSON methods already include comprehensive error handling that displays user notifications for HTTP errors. Functions using these methods (like doWebAuthnRegistration and listWebAuthnKeys) do not need additional error handling for user notifications.
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.669Z
Learning: In the webknossos project, the Request module's receiveJSON and sendJSONReceiveJSON methods already include comprehensive error handling that displays user notifications for HTTP errors. Functions using these methods (like doWebAuthnRegistration and listWebAuthnKeys) do not need additional error handling for user notifications.
🧬 Code Graph Analysis (1)
app/controllers/AuthenticationController.scala (10)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/TemporaryStore.scala (4)
TemporaryStore(9-83)insert(55-60)get(13-18)pop(74-79)app/utils/WkConf.scala (5)
User(45-60)User(75-77)User(85-91)EmailVerification(52-58)Features(128-147)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 (11)
flatMap(277-287)flatMap(372-372)Fox(30-223)Fox(225-298)runIf(169-178)successful(53-56)fromBool(32-36)toFox(14-14)fromFuture(42-53)map(274-277)map(370-370)app/models/user/EmailVerificationService.scala (1)
assertEmailVerifiedOrResendVerificationMail(66-76)app/models/user/MultiUser.scala (3)
updateLastLoggedInIdentity(106-114)findOneById(139-146)insertOne(64-77)util/src/main/scala/com/scalableminds/util/objectid/ObjectId.scala (5)
toString(12-12)ObjectId(11-13)ObjectId(15-55)fromString(17-19)generate(16-16)util/src/main/scala/com/scalableminds/util/mvc/ExtendedController.scala (1)
validateJson(204-209)app/models/user/WebAuthnCredentials.scala (6)
findByCredentialId(118-128)updateSignCount(147-152)findAllForUser(109-118)WebAuthnCredential(25-49)insertOne(128-145)removeById(154-160)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (8)
app/controllers/AuthenticationController.scala (8)
53-189: Well-structured WebAuthn data modelsThe WebAuthn case classes are well-designed with proper documentation references to MDN and clear field explanations. The JSON serialization is correctly implemented.
222-242: Good security configuration setupThe WebAuthn configuration is properly implemented with:
SecureRandominitialized once at class level- Origin derived from configuration
- HTTPS requirement checking
- Appropriate algorithm support and timeout settings
319-338: Smart refactoring of authentication logicThe extraction of
authenticateInnermethod is a good refactoring that allows reuse between regular authentication and WebAuthn authentication flows while maintaining consistent behavior.
586-608: Proper authentication start implementationThe WebAuthn authentication start method correctly:
- Validates feature flags and HTTPS requirements
- Generates secure random challenges
- Sets appropriate cookie security attributes including
SameSite.Strict- Uses consistent timeout handling
638-645: Appropriate signature counter validationThe signature counter validation correctly handles both cases where the counter is unused (0) and where it should be strictly increasing, following WebAuthn security requirements.
652-695: Well-implemented registration startThe registration start method properly:
- Validates permissions and requirements
- Uses privacy-preserving user handle (multiUser ID instead of email)
- Correctly excludes existing credentials
- Implements proper challenge generation and storage
697-730: Solid registration finalizationThe registration finalization correctly:
- Validates the registration response
- Creates proper credential records
- Handles timeouts and errors appropriately
- Stores credentials securely
742-749: Key removal has proper access controlThe key removal method correctly filters by multiUser in the DAO, ensuring users can only remove their own keys. The error handling with
?~>follows the codebase conventions.
There was a problem hiding this comment.
Neat, thanks for keeping digging throught this 💪 🙏 🧑💼
I found a few left over suggestions regarding your latest changes. These are only minor things like concistent naming & extracting error messages.
I think after this, this PR should be ready to go after. At least code-wise. I have never done any testing
| @@ -105,7 +105,7 @@ object Dependencies { | |||
| // SQL Queries postgres specifics. not imported. | |||
| "org.postgresql" % "postgresql" % "42.7.5", | |||
| /// WebAuthn Dependencies | |||
| "com.webauthn4j" % "webauthn4j-core" % "0.29.2.RELEASE" exclude("com.fasterxml.jackson.core", "jackson-databind"), | |||
| "com.webauthn4j" % "webauthn4j-core" % "0.29.3.RELEASE" exclude("com.fasterxml.jackson.core", "jackson-databind"), | |||
| ) | |||
|
|
|||
| val dependencyOverrides: Seq[ModuleID] = Seq( | |||
There was a problem hiding this comment.
@fm3 / @robert-oleynik I am not familiar with what dependencyOverrides does, but if I understand the comment correctly, this overwrite is no longer needed as we do no longer have netlift common as a dependency. Thus, can you please delete this and test whether everything still works?
val dependencyOverrides: Seq[ModuleID] = Seq(
// liftweb-commons (used by us for Box/tryo) depends on older scala-xml, but we do not use its xml-related features
"com.fasterxml.jackson.core" % "jackson-databind" % "2.14.3" "com.fasterxml.jackson.core" % "jackson-databind" % "2.14.3"
) )
There was a problem hiding this comment.
It is necessary, due to a conflict with the Play framework:
backend: [error] com.fasterxml.jackson.databind.JsonMappingException: Scala module 2.14.3 requires Jackson Databind version >= 2.14.0 and < 2.15.0 - Found jackson-databind version 2.19.0
backend: [error] at com.fasterxml.jackson.module.scala.JacksonModule.setupModule(JacksonModule.scala:61)
backend: [error] at com.fasterxml.jackson.module.scala.JacksonModule.setupModule$(JacksonModule.scala:46)
backend: [error] at com.fasterxml.jackson.module.scala.DefaultScalaModule.setupModule(DefaultScalaModule.scala:17
...
I will update the comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/controllers/AuthenticationController.scala (1)
610-653: Critical bug: User handle must be base64 decoded before ObjectId conversion.The user handle is base64 encoded during registration (line 662) but being treated as plain string during authentication. This will cause authentication to always fail.
- multiUserId <- ObjectId.fromString(new String(authData.getUserHandle)) ??~> Messages( - "auth.passkeys.unauthorized") ~> UNAUTHORIZED + multiUserId <- ObjectId.fromString(new String(Base64.decodeBase64(authData.getUserHandle), StandardCharsets.UTF_8)) ??~> Messages( + "auth.passkeys.unauthorized") ~> UNAUTHORIZED
🧹 Nitpick comments (1)
app/controllers/AuthenticationController.scala (1)
746-754: Consider adding explicit validation for key removal.While the DAO's
removeByIdmethod filters by_multiUser(preventing cross-user deletions), it may silently succeed when no matching key is found. Consider adding explicit validation to return a proper 404 response.def webauthnRemoveKey(id: ObjectId): Action[AnyContent] = sil.SecuredAction.async { implicit request => { for { _ <- Fox.fromBool(conf.Features.passkeysEnabled) ?~> Messages("auth.passkeys.disabled") _ <- Fox.fromBool(usesHttps) ?~> Messages("auth.passkeys.requiresHttps") + // Verify the key exists and belongs to the user + _ <- webAuthnCredentialDAO.findOneById(id, request.identity._multiUser) ?~> "Passkey not found" ~> NOT_FOUND _ <- webAuthnCredentialDAO.removeById(id, request.identity._multiUser) ?~> "Passkey not found" ~> NOT_FOUND } yield Ok(Json.obj()) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/AuthenticationController.scala(7 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 (2)
📓 Common learnings
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:29-35
Timestamp: 2025-07-09T07:30:57.149Z
Learning: In the webknossos project, error handling and user notifications are handled at the API function level (e.g., `removeWebAuthnKey`, `fetchPasskeys`) rather than in the UI components, so additional try-catch blocks in the UI layer are unnecessary.
app/controllers/AuthenticationController.scala (11)
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:29-35
Timestamp: 2025-07-09T07:30:57.149Z
Learning: In the webknossos project, error handling and user notifications are handled at the API function level (e.g., `removeWebAuthnKey`, `fetchPasskeys`) rather than in the UI components, so additional try-catch blocks in the UI layer are unnecessary.
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.669Z
Learning: In the webknossos project, the Request module has comprehensive built-in error handling. The triggerRequest method defaults to showErrorToast: true, automatically catches errors with .catch(), and displays user notifications via Toast.error() or Toast.messages() in the handleError method. Functions using Request methods (like doWebAuthnRegistration and listWebAuthnKeys) do not need additional error handling for user notifications.
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.669Z
Learning: In the webknossos project, the Request module's sendJSONReceiveJSON function already includes comprehensive error handling that displays Toast.error() notifications for HTTP errors. Functions using this module (like doWebAuthnRegistration and listWebAuthnKeys) do not need additional error handling for user notifications.
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/NeuroglancerPrecomputedMeshFileService.scala:161-166
Timestamp: 2025-04-28T14:18:04.368Z
Learning: In Scala for-comprehensions with the Fox error handling monad, `Fox.fromBool()` expressions should use the `<-` binding operator instead of the `=` assignment operator to properly propagate error conditions. Using `=` will cause validation failures to be silently ignored.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8221
File: app/controllers/JobController.scala:226-232
Timestamp: 2025-01-13T09:06:15.202Z
Learning: In the JobController of webknossos, numeric parameters should use `Option[Double]` instead of `Option[String]` for better type safety. Additionally, when adding new job parameters that are conditionally required (like evaluation settings), proper validation should be added in the `for` comprehension block before creating the `commandArgs`.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:06:42.865Z
Learning: In Scala's for-comprehension with Fox (Future-like type), the `<-` operator ensures sequential execution. If any step fails, the entire chain short-circuits and returns early, preventing subsequent operations from executing. This makes it safe to perform validation checks before database operations.
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala:170-173
Timestamp: 2025-04-23T08:51:57.756Z
Learning: In the webknossos codebase, classes extending `FoxImplicits` have access to an implicit conversion from `Option[A]` to `Fox[A]`, where `None` is converted to an empty Fox that fails gracefully in for-comprehensions.
Learnt from: frcroth
PR: scalableminds/webknossos#8657
File: app/models/dataset/DatasetService.scala:380-392
Timestamp: 2025-06-16T09:04:40.603Z
Learning: Trailing commas are supported in Scala starting from version 2.12.2 as part of SIP-27. They are valid in function calls, function definitions, import statements, and other contexts. This means trailing commas after the last parameter or argument do not cause compilation errors in Scala 2.12.2+.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8393
File: app/controllers/AuthenticationController.scala:566-573
Timestamp: 2025-03-21T16:49:17.956Z
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.
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.669Z
Learning: In the webknossos project, the Request module's receiveJSON and sendJSONReceiveJSON methods already include comprehensive error handling that displays user notifications for HTTP errors. Functions using these methods (like doWebAuthnRegistration and listWebAuthnKeys) do not need additional error handling for user notifications.
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.669Z
Learning: In the webknossos project, the Request module's receiveJSON and sendJSONReceiveJSON methods already include comprehensive error handling that displays user notifications for HTTP errors. Functions using these methods (like doWebAuthnRegistration and listWebAuthnKeys) do not need additional error handling for user notifications.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (8)
app/controllers/AuthenticationController.scala (8)
6-51: WebAuthn imports and dependencies look comprehensive.The imports include all necessary components for WebAuthn implementation using the webauthn4j library, which is a good choice for server-side WebAuthn support.
53-189: Well-structured WebAuthn case classes with proper documentation.The case classes accurately model the WebAuthn API specification with clear documentation referencing the MDN Web API docs. Good practice omitting unnecessary fields with explanatory comments.
211-213: Constructor properly updated with WebAuthn dependencies.The new dependencies are correctly added and properly typed. The temporary stores for assertion and registration challenges are appropriately separated.
222-241: WebAuthn configuration and initialization looks secure.Good practices include:
- SecureRandom initialized at class level to avoid per-request overhead
- Proper origin configuration from application config
- HTTPS requirement enforced
- Standard cryptographic algorithms (EdDSA, ES256, RS256) supported
586-608: WebAuthn authentication start method is well-implemented.Good security practices:
- Proper feature flag and HTTPS validation
- Secure random challenge generation
- Cookie configured with SameSite.Strict for CSRF protection
- Consistent timeout handling
655-698: WebAuthn registration start method is correctly implemented.The method properly:
- Validates prerequisites (feature flags, HTTPS)
- Creates user object with base64 encoded ID (WebAuthn spec requirement)
- Excludes existing credentials to prevent re-registration
- Generates secure challenge and stores it temporarily
700-733: Registration finalization method is correctly implemented.The method properly validates the registration response, creates the credential record, and stores it in the database with appropriate error handling.
735-744: Key listing method is straightforward and correct.The method properly retrieves keys for the user and returns simplified descriptors containing only the necessary information (ID and name).
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)