Implement Webauthn #8393
Implement Webauthn #8393
Conversation
📝 WalkthroughWalkthroughAdds end-to-end WebAuthn (passkey) support: new backend WebAuthn models, DAO, controller endpoints and routes, DB schema/migrations, temporary stores and utilities, frontend passkey UI and client flows, TLS/dev proxy updates, docs, build dependency and config feature flag. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
conf/evolutions/137-add-webauthn-credentials.sql(1 hunks)conf/evolutions/reversions/137-add-webauthn-credentials.sql(1 hunks)frontend/javascripts/admin/account/account_password_view.tsx(3 hunks)project/Dependencies.scala(1 hunks)tools/postgres/schema.sql(4 hunks)unreleased_changes/8393.md(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- conf/evolutions/reversions/137-add-webauthn-credentials.sql
- conf/evolutions/137-add-webauthn-credentials.sql
🚧 Files skipped from review as they are similar to previous changes (3)
- unreleased_changes/8393.md
- project/Dependencies.scala
- tools/postgres/schema.sql
🧰 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.181Z
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.
frontend/javascripts/admin/account/account_password_view.tsx (1)
Learnt from: robert-oleynik
PR: #8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:29-35
Timestamp: 2025-07-09T07:30:57.181Z
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.
🧬 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(12-32)
🪛 GitHub Check: frontend-tests
frontend/javascripts/admin/account/account_password_view.tsx
[failure] 188-188:
Type '{ title: string; description: Element; }' is not assignable to type 'IntrinsicAttributes & SettingsCardProps'.
⏰ 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 (4)
frontend/javascripts/admin/account/account_password_view.tsx (4)
4-4: LGTM!The import of the
featuresmodule is appropriate for accessing feature flags.
11-11: LGTM!The import follows the existing pattern and is necessary for the settings card UI component.
15-15: LGTM!The import statement correctly excludes the .tsx extension, which resolves the previous TypeScript configuration issue.
160-160: LGTM!Clean destructuring of the passkeys feature flag from the features module.
There was a problem hiding this comment.
Awesome. Thanks for sticking with this PR 🎉
I found one nitpicky improvement. Please see below. After that I would say that this is ready to be merged code wise. Regarding testing I do not have a valid setup. So maybe ask another person to test or if you are confident that it works, I can also approve this myself :)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
conf/evolutions/138-add-webauthn-credentials.sql (1)
5-18: Consider adding timestamp columns for auditabilityStoring
createdAtandupdatedAt(UTC) greatly helps debugging, support, and compliance audits with negligible storage cost.createdAt TIMESTAMPTZ NOT NULL DEFAULT NOW(), updatedAt TIMESTAMPTZ NOT NULL DEFAULT NOW()Optionally add a trigger to update
updatedAton row changes.conf/evolutions/reversions/138-add-webauthn-credentials.sql (1)
5-7: Make the reversion idempotent withIF EXISTSIf the forward migration failed part-way, the objects might already be gone.
UsingIF EXISTSprevents the rollback from aborting unexpectedly.-DROP VIEW webknossos.webauthnCredentials_; -DROP TABLE webknossos.webauthnCredentials; +DROP VIEW IF EXISTS webknossos.webauthnCredentials_; +DROP TABLE IF EXISTS webknossos.webauthnCredentials;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
conf/evolutions/138-add-webauthn-credentials.sql(1 hunks)conf/evolutions/reversions/138-add-webauthn-credentials.sql(1 hunks)tools/postgres/schema.sql(4 hunks)unreleased_changes/8393.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- unreleased_changes/8393.md
- tools/postgres/schema.sql
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
conf/evolutions/138-add-webauthn-credentials.sql (1)
5-19: Still missing the dedicated index on_multiUserfor fast per-user look-ups
Same feedback as in the previous review: most queries filter by_multiUser; relying on the composite UNIQUE index means the planner must also keepcredentialIdin memory and cannot excludeisDeleted = truerows efficiently.
Create a partial index to keep auth latency low:CREATE INDEX idx_webauthncredentials_multiuser ON webknossos.webauthnCredentials(_multiUser) WHERE NOT isDeleted;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
conf/evolutions/138-add-webauthn-credentials.sql(1 hunks)tools/postgres/schema.sql(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/postgres/schema.sql
🧰 Additional context used
🧠 Learnings (3)
📓 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.181Z
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.
📚 Learning: credit balance consistency in webknossos is maintained through a postgresql before trigger that calc...
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.
Applied to files:
conf/evolutions/138-add-webauthn-credentials.sql
📚 Learning: the credit balance consistency in webknossos is maintained through a postgresql before trigger that ...
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.
Applied to files:
conf/evolutions/138-add-webauthn-credentials.sql
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/controllers/AuthenticationController.scala (3)
677-682: Add SameSite attribute to WebAuthn registration cookieSimilar to the authentication cookie, the registration cookie needs the
sameSiteattribute for CSRF protection.cookie = Cookie("webauthn-registration", sessionId, maxAge = Some(webauthnTimeout.toSeconds.toInt), httpOnly = true, secure = true, + sameSite = Some(Cookie.SameSite.Strict))
620-625: Critical: Fix user handle decoding for authenticationThe user handle from WebAuthn needs to be base64 decoded before conversion to ObjectId. During registration (line 661), the user.id is base64 encoded, but here it's being treated as a plain string, which 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
745-753: Improve error handling for key removalBased on past review comments, the current implementation should verify that the key exists and belongs to the user before attempting removal to provide proper 404 responses.
def webauthnRemoveKey(id: ObjectId): Action[AnyContent] = sil.SecuredAction.async { implicit request => { for { _ <- Fox.fromBool(conf.Features.passkeysEnabled) ?~> "auth.passkeys.disabled" _ <- Fox.fromBool(usesHttps) ?~> "auth.passkeys.requiresHttps" + // Verify the key exists and belongs to the user before deletion + _ <- 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 (1)
app/controllers/AuthenticationController.scala(8 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 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.181Z
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: conf/evolutions/138-add-webauthn-credentials.sql:18-18
Timestamp: 2025-08-05T09:15:50.093Z
Learning: In WebAuthn implementations, credential IDs are generated by the authenticator (user's device) and are only unique per user, not globally across the entire relying party system. Credentials are properly identified by the combination of user identifier and credential ID, so a composite unique constraint on (multiUser, credentialId) is the correct approach rather than requiring global credential ID uniqueness.
📚 Learning: in the webknossos project, error handling and user notifications are handled at the api function lev...
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:29-35
Timestamp: 2025-07-09T07:30:57.181Z
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.
Applied to files:
app/controllers/AuthenticationController.scala
📚 Learning: in the webknossos project, the request module has comprehensive built-in error handling. the trigger...
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.681Z
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.
Applied to files:
app/controllers/AuthenticationController.scala
📚 Learning: in scala for-comprehensions with the fox error handling monad, `fox.frombool()` expressions should u...
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.
Applied to files:
app/controllers/AuthenticationController.scala
📚 Learning: in the jobcontroller of webknossos, numeric parameters should use `option[double]` instead of `optio...
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`.
Applied to files:
app/controllers/AuthenticationController.scala
📚 Learning: in scala's for-comprehension with fox (future-like type), the `<-` operator ensures sequential execu...
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.
Applied to files:
app/controllers/AuthenticationController.scala
📚 Learning: in the webknossos codebase, classes extending `foximplicits` have access to an implicit conversion f...
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.
Applied to files:
app/controllers/AuthenticationController.scala
📚 Learning: trailing commas are supported in scala starting from version 2.12.2 as part of sip-27. they are vali...
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+.
Applied to files:
app/controllers/AuthenticationController.scala
📚 Learning: in webauthn implementations, credential ids are generated by the authenticator (user's device) and a...
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: conf/evolutions/138-add-webauthn-credentials.sql:18-18
Timestamp: 2025-08-05T09:15:50.093Z
Learning: In WebAuthn implementations, credential IDs are generated by the authenticator (user's device) and are only unique per user, not globally across the entire relying party system. Credentials are properly identified by the combination of user identifier and credential ID, so a composite unique constraint on (multiUser, credentialId) is the correct approach rather than requiring global credential ID uniqueness.
Applied to files:
app/controllers/AuthenticationController.scala
📚 Learning: in the webknossos project, prefer using the ?~> operator for error handling with message keys stored...
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.
Applied to files:
app/controllers/AuthenticationController.scala
📚 Learning: in the webknossos project, the request module's sendjsonreceivejson function already includes compre...
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.681Z
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.
Applied to files:
app/controllers/AuthenticationController.scala
📚 Learning: in the webknossos project, the request module's receivejson and sendjsonreceivejson methods already ...
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.681Z
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.
Applied to files:
app/controllers/AuthenticationController.scala
⏰ 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 (5)
app/controllers/AuthenticationController.scala (5)
1-189: LGTM - Well-structured WebAuthn data modelsThe new imports and case class definitions are well-organized and properly documented. The case classes accurately model the WebAuthn specification objects with appropriate JSON serialization support. The MDN documentation references are particularly helpful for future maintenance.
191-217: LGTM - Appropriate dependency injection for WebAuthnThe constructor additions properly inject the necessary dependencies for WebAuthn functionality, following the existing codebase patterns.
218-242: LGTM - Well-configured WebAuthn infrastructureThe private fields are properly initialized with good security practices:
- SecureRandom at class level for efficiency
- Origin correctly extracted from configuration
- HTTPS requirement enforced for security
- Standard cryptographic algorithms supported
- Consistent timeout configuration
319-360: LGTM - Good refactoring for code reuseThe extraction of common authentication logic into
authenticateInneris a good refactoring that follows the DRY principle and will be useful for WebAuthn authentication flows.
591-596: Add SameSite attribute to WebAuthn session cookieThe session cookie is missing the
sameSiteattribute, which should be set toStrictto prevent CSRF attacks on WebAuthn authentication flows.cookie = Cookie(name = "webauthn-session", value = sessionId, maxAge = Some(webauthnTimeout.toSeconds.toInt), httpOnly = true, secure = true, - sameSite = Some(Cookie.SameSite.Strict)) + sameSite = Some(Cookie.SameSite.Strict))⛔ Skipped due to learnings
Learnt from: robert-oleynik PR: scalableminds/webknossos#8393 File: conf/evolutions/138-add-webauthn-credentials.sql:18-18 Timestamp: 2025-08-05T09:15:50.093Z Learning: In WebAuthn implementations, credential IDs are generated by the authenticator (user's device) and are only unique per user, not globally across the entire relying party system. Credentials are properly identified by the combination of user identifier and credential ID, so a composite unique constraint on (multiUser, credentialId) is the correct approach rather than requiring global credential ID uniqueness.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/javascripts/admin/account/account_password_view.tsx (1)
15-15: Resolution note: Import path fixed per prior review.The .tsx extension was removed, resolving the TS config error.
🧹 Nitpick comments (2)
frontend/javascripts/admin/account/account_password_view.tsx (2)
15-15: Consider code-splitting PasskeysView to avoid shipping it when the feature is disabled.Passkeys UI and its dependencies will be bundled even if
passkeysEnabledis false. Lazy-load to reduce JS payload for users who don’t use passkeys.Apply this diff to lazy-load the view:
-import PasskeysView from "../auth/passkeys_view"; +const PasskeysView = lazy(() => import("../auth/passkeys_view"));Additionally, update the React import to include
lazyandSuspense(outside this hunk):// at the top where React hooks are imported import { useState, Suspense, lazy } from "react";And wrap the usage in a Suspense boundary (see suggestion on Lines 183-192).
185-185: Nit: Improve phrasing.If you keep a literal string, prefer “Log in with passkeys” or “Passwordless login with passkeys.”
📜 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 (8)
app/controllers/AuthenticationController.scala(8 hunks)app/utils/WkConf.scala(1 hunks)conf/application.conf(1 hunks)conf/messages(1 hunks)conf/webknossos.latest.routes(1 hunks)frontend/javascripts/admin/account/account_password_view.tsx(3 hunks)frontend/javascripts/admin/auth/login_form.tsx(3 hunks)frontend/javascripts/types/api_types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- conf/messages
- conf/application.conf
- app/utils/WkConf.scala
- frontend/javascripts/admin/auth/login_form.tsx
- frontend/javascripts/types/api_types.ts
- conf/webknossos.latest.routes
- app/controllers/AuthenticationController.scala
🧰 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(12-38)
⏰ 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 (3)
frontend/javascripts/admin/account/account_password_view.tsx (3)
4-4: LGTM: Feature flag plumbing looks correct.Importing the centralized features accessor is appropriate for gating UI.
11-11: LGTM: Correct component import for SettingsCard.Switching to the component import aligns with actual usage.
160-160: Confirm features() bootstrap to prevent runtime errors/flickerThe default export of
features.tsthrows if the toggles haven’t been loaded, and I didn’t find anyfeatures.load()or__setFeatures()invocation in the JS entrypoints. As written, callingfeatures()here may throw or briefly return an empty/falsey state, leading to hydration mismatches or runtime errors.• Ensure your app’s bootstrap (e.g. main entry file or server-render template) calls
await load()or__setFeatures(...)before mounting React.
• Alternatively, guard this component against uninitialized toggles—check for readiness or render a loading/fallback state before accessingpasskeysEnabled.• Location:
frontend/javascripts/admin/account/account_password_view.tsxline 160
• Module:frontend/javascripts/features.ts
fm3
dismissed
MichaelBuessemeyer’s stale review
The requested changes were adressed. @MichaelBuessemeyer is out of office this week so they can’t approve themselves.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/admin/auth/passkeys_view.tsx (1)
28-32: Aligned with centralized error handling approach.No extra UI-level try/catch here is correct given Request-layer toasts. Success toast and refresh are handled appropriately.
🧹 Nitpick comments (5)
frontend/javascripts/admin/auth/passkeys_view.tsx (5)
18-21: Avoid shadowing state name in fetchPasskeys.Using the same identifier for both local and state variable harms readability. Suggest renaming the local variable.
Apply this diff:
- const passkeys = await listWebAuthnKeys(); - setPasskeys(passkeys); + const fetchedPasskeys = await listWebAuthnKeys(); + setPasskeys(fetchedPasskeys);
44-47: Make refresh consistent with removal path (await).You await refresh after deletion but not after registration. Aligning both avoids subtle race conditions (e.g., UI re-render timing).
Apply this diff:
- setNewPasskeyName(""); - fetchPasskeys(); + setNewPasskeyName(""); + await fetchPasskeys();
50-57: Handle very long passkey names gracefully.Add ellipsis and a title tooltip for the name column to avoid layout breaks while preserving discoverability of the full value.
Apply this diff:
{ title: "Name", dataIndex: "name", key: "name", width: "100%", + ellipsis: true, + render: (name: string) => <span title={name}>{name}</span>, },
61-69: Add deletion confirmation and improve a11y for the icon-only button.Use Popconfirm to prevent accidental deletions and include an aria-label for screen readers. Also use
voidto signal intentional non-awaiting in the onConfirm.Apply this diff:
- render: (_id: string, passkey: WebAuthnKeyDescriptor) => ( - <Button - type="default" - shape="circle" - icon={<DeleteOutlined />} - onClick={() => webauthnRemoveKey(passkey)} - size="small" - /> - ), + render: (_id: string, passkey: WebAuthnKeyDescriptor) => ( + <Popconfirm + title={`Remove passkey '${passkey.name}'?`} + okText="Remove" + cancelText="Cancel" + onConfirm={() => void webauthnRemoveKey(passkey)} + > + <Button + aria-label={`Remove passkey ${passkey.name}`} + type="default" + shape="circle" + icon={<DeleteOutlined />} + size="small" + /> + </Popconfirm> + ),Additionally, add the missing import at the top of the file:
import { Button, Input, Modal, Table, Popconfirm } from "antd";
90-95: Disable “OK” when the passkey name is invalid to reduce round-trips.Prevents unnecessary submissions and aligns with the inline validation already present.
Apply this diff:
<Modal title="Enter a name for the new Passkey" open={isPasskeyNameModalOpen} onOk={registerNewPasskey} onCancel={() => setIsPasskeyNameModalOpen(false)} + okButtonProps={{ + disabled: + newPasskeyName.trim().length < 3 || + passkeys.some( + (pk) => pk.name.toLowerCase() === newPasskeyName.trim().toLowerCase(), + ), + }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/admin/auth/passkeys_view.tsx(1 hunks)project/Dependencies.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- project/Dependencies.scala
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-09T07:30:57.181Z
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:29-35
Timestamp: 2025-07-09T07:30:57.181Z
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.
Applied to files:
frontend/javascripts/admin/auth/passkeys_view.tsx
📚 Learning: 2025-07-09T07:33:45.681Z
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.681Z
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.
Applied to files:
frontend/javascripts/admin/auth/passkeys_view.tsx
📚 Learning: 2025-07-09T07:33:45.681Z
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.681Z
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.
Applied to files:
frontend/javascripts/admin/auth/passkeys_view.tsx
📚 Learning: 2025-07-09T07:33:45.681Z
Learnt from: robert-oleynik
PR: scalableminds/webknossos#8393
File: frontend/javascripts/admin/auth/passkeys_view.tsx:47-51
Timestamp: 2025-07-09T07:33:45.681Z
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.
Applied to files:
frontend/javascripts/admin/auth/passkeys_view.tsx
🧬 Code Graph Analysis (1)
frontend/javascripts/admin/auth/passkeys_view.tsx (2)
app/controllers/AuthenticationController.scala (2)
WebAuthnKeyDescriptor(186-186)WebAuthnKeyDescriptor(187-189)frontend/javascripts/admin/api/webauthn.ts (3)
listWebAuthnKeys(49-51)removeWebAuthnKey(53-55)doWebAuthnRegistration(28-47)
⏰ 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 (2)
frontend/javascripts/admin/auth/passkeys_view.tsx (2)
12-16: Component structure and state setup look solid.Clean, focused component with minimal state. Naming is clear.
73-83: Table configuration is appropriate.Good use of rowKey and hiding the header for a minimalist list view. Pagination behavior is sensible.
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)