Implement Webauthn #8393
Implement Webauthn #8393
Conversation
📝 WalkthroughWalkthroughThis pull request integrates comprehensive WebAuthn support into the project. It adds new migration scripts and schema changes to create and manage a WebAuthn credentials table, along with a corresponding reversion script. On the backend, new controller endpoints, DAOs, and case classes have been introduced to handle WebAuthn authentication and registration. The dependency list is updated to include WebAuthn libraries. Frontend components now include functions and UI elements for passkey login and key management. Additionally, configuration and routing updates enable TLS support and define new WebAuthn-related endpoints. Changes
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 5
🧹 Nitpick comments (18)
conf/evolutions/reversions/126-add-webauthn-credentials.sql (1)
3-3: Comment seems unrelated to this migration.The comment refers to replacing '$' in annotation layer names, which appears unrelated to WebAuthn credentials. It was likely copied from another migration file.
-- This reversion might take a while because it needs to search in all annotation layer names for '$' and replace it with '' +-- Reversion script for WebAuthn credentials tableapp/models/user/MultiUser.scala (1)
139-144: Implementation looks good, but consider adding a comment explaining the method's purpose.The implementation of
findOneByIdfollows the existing DAO pattern correctly. It properly handles authorization viareadAccessQueryand uses the Fox monad for error handling, similar to other finder methods in this class.Consider adding a brief comment describing the purpose of this method and its relationship to the WebAuthn implementation. This would help future developers understand why this method was added.
conf/application.conf (1)
64-66: Consider using a dedicated dispatcher for blocking WebAuthn operations.Using the default dispatcher for blocking operations could lead to thread starvation under heavy load, as WebAuthn operations might involve CPU-intensive cryptographic calculations.
Consider creating a dedicated dispatcher for blocking WebAuthn operations:
play { context { - blocking = "pekko.actor.default-dispatcher" + blocking = "webauthn-blocking-dispatcher" } } +webauthn-blocking-dispatcher { + type = Dispatcher + executor = "thread-pool-executor" + thread-pool-executor { + fixed-pool-size = 16 + } + throughput = 1 +}This would isolate WebAuthn's blocking operations from affecting the rest of the application.
frontend/javascripts/admin/auth/login_form.tsx (1)
142-158: Consider refactoring duplicate login success handling code.There's duplicated code between the email/password login and WebAuthn login for handling successful authentication.
Extract the common login success handling logic:
+const handleLoginSuccess = (user, organization) => { + Store.dispatch(setActiveUserAction(user)); + Store.dispatch(setActiveOrganizationAction(organization)); + if (onLoggedIn) { + onLoggedIn(); + } +}; // In the email/password onFinish function: const onFinish = async (formValues) => { const [user, organization] = await loginUser(formValues); - Store.dispatch(setActiveUserAction(user)); - Store.dispatch(setActiveOrganizationAction(organization)); - - if (onLoggedIn) { - onLoggedIn(); - } + handleLoginSuccess(user, organization); }; // In the WebAuthn login button: onClick={async () => { try { const [user, organization] = await doWebAuthnLogin(); - Store.dispatch(setActiveUserAction(user)); - Store.dispatch(setActiveOrganizationAction(organization)); - if (onLoggedIn) { - onLoggedIn(); - } + handleLoginSuccess(user, organization); } catch (error) { console.error("WebAuthn login failed:", error); message.error("Passkey login failed: " + (error.message || "Unknown error")); } }}frontend/javascripts/navbar.tsx (1)
678-688: Nicely integrated menu item for passkey management.Adding a dedicated menu entry for passkeys alongside the other authentication flows is a welcome enhancement. The label, link, and position in the user avatar dropdown are intuitive. Consider aligning the naming convention (e.g., "Register/Manage Passkeys") consistently across the app to help users recognize it at a glance.
frontend/javascripts/admin/auth/manage_passkeys_view.tsx (2)
16-16: Remove unused import or clarify its side effects.The empty import statement from
"@github/webauthn-json/browser-ponyfill"(if unused) should be removed. If it is intended for side effects, consider adding a comment explaining its necessity.- import {} from "@github/webauthn-json/browser-ponyfill"; + // Remove if not needed or clarify the side effect
21-22: Display loading state or remove unused_isLoading.The
_isLoadingstate is set but never utilized to display a spinner or loading placeholder. If intended, hook it into the UI (e.g., show a or disabled button while loading). Otherwise, consider removing the_isLoadinglogic to reduce confusion and unused code.- const [_isLoading, setIsLoading] = useState<boolean>(true); + // If not needed, remove: + // const [_isLoading, setIsLoading] = useState<boolean>(true);Also applies to: 48-100
tools/proxy/proxy.js (2)
2-2: Use the node: import protocol for built-in modules (per static analysis).Modules such as
fsandhttpscan be imported with thenode:protocol for clarity and consistency. This helps distinguish standard library imports from external modules.- const fs = require("fs"); - const https = require("https"); + const fs = require("node:fs"); + const https = require("node:https");Also applies to: 7-7
🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
157-165: Conditional HTTPS is a flexible, user-friendly approach.Implementing TLS through
--tlsis convenient. Some considerations for improvement:
- Error handling if the key/cert files are missing or invalid.
- Optionally allowing environment variables for custom cert paths or passphrases.
- Logging a more helpful message about secure or non-secure server usage.
app/models/user/WebAuthnCredentials.scala (3)
44-52: Search queries do not indexuserId.
Depending on query volume, consider adding an index on_multiUserto improve performance when fetching by user.
53-61: Consider an index onkeyId.
Lookups forkeyIdcould become expensive on a large table. IndexingkeyIdcan enhance performance.
72-79: IncludeisDeleteddefault when inserting.
YourinsertOnemethod doesn't explicitly setisDeleted. If you rely on a default in the DB schema or expect a non-deleted record by default, ensure consistency in the schema and code.app/security/WebAuthnCredentialRepository.scala (1)
98-120: Leverage unmodifiable sets and handle collisions gracefully.
lookupAllreturns ajava.util.Set. Consider returning an unmodifiable set to prevent unintended modifications by higher layers. Also ensure that credential collisions are handled if multiple credentials share the same ID.app/controllers/AuthenticationController.scala (5)
57-82: Injected dependencies for WebAuthn
The new WebAuthn fields andTemporaryStorereferences are sensible. Ensure that you have adequate error handling for store insertion & retrieval.
96-102: Potential ID mismatch in RelyingParty builder.
Be certain thatoriginfromconfmatches the actual domain used by clients. Otherwise, WebAuthn could fail domain verification.
447-454: Short cookie lifespan
Setting the cookie’s maxAge to 120 seconds is a good short window. Validate that it accommodates typical user interaction times.
456-491: Exception hierarchy for WebAuthn
Catching onlyAssertionFailedExceptionmay omit other possible exceptions (e.g., JSON parse errors). Consider broadening or providing an additional fallback to handle unexpected exception types.
517-549: Enforce unique key names if desired
When registering a new WebAuthn credential, you storerequest.body.namebut do not confirm uniqueness. You might want to check for duplicates to prevent confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (20)
MIGRATIONS.unreleased.md(1 hunks)app/controllers/AuthenticationController.scala(8 hunks)app/models/user/MultiUser.scala(1 hunks)app/models/user/WebAuthnCredentials.scala(1 hunks)app/security/WebAuthnCredentialRepository.scala(1 hunks)conf/application.conf(1 hunks)conf/evolutions/126-add-webauthn-credentials.sql(1 hunks)conf/evolutions/reversions/126-add-webauthn-credentials.sql(1 hunks)conf/webknossos.latest.routes(1 hunks)frontend/javascripts/admin/admin_rest_api.ts(3 hunks)frontend/javascripts/admin/auth/login_form.tsx(2 hunks)frontend/javascripts/admin/auth/manage_passkeys_view.tsx(1 hunks)frontend/javascripts/navbar.tsx(1 hunks)frontend/javascripts/router.tsx(2 hunks)frontend/javascripts/types/api_flow_types.ts(1 hunks)package.json(2 hunks)project/Dependencies.scala(1 hunks)tools/gen-ssl-dev-certs.sh(1 hunks)tools/postgres/schema.sql(3 hunks)tools/proxy/proxy.js(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/javascripts/types/api_flow_types.ts
- tools/gen-ssl-dev-certs.sh
🧰 Additional context used
🪛 Biome (1.9.4)
tools/proxy/proxy.js
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 4-4: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 5-5: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 7-7: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 10-14: Use let or const instead of var.
A variable declared with var is accessible in the whole module. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.
(lint/style/noVar)
🔇 Additional comments (30)
MIGRATIONS.unreleased.md (1)
22-22: Good addition to document database migration for WebAuthn credentials.The migration entry properly documents the addition of the WebAuthn credentials table to support the PassKey functionality being implemented.
project/Dependencies.scala (1)
107-109: Good choice of WebAuthn library.Using Yubico's WebAuthn server core library (2.6.0) is appropriate for implementing WebAuthn functionality. Yubico is a trusted provider in the authentication space with well-maintained libraries.
conf/evolutions/reversions/126-add-webauthn-credentials.sql (1)
1-10: Rollback script properly handles WebAuthn table removal.The reversion script correctly:
- Checks the schema version (126) before proceeding
- Drops the webauthnCredentials table
- Updates schema version back to 125
This ensures clean rollback capability for the WebAuthn feature.
frontend/javascripts/router.tsx (2)
66-66: Good import placement for the new component.The ManagePassKeyView import is properly placed with other authentication-related imports.
621-625: Well-structured route for managing passkeys.The new secure route for passkey management:
- Is properly secured with authentication check
- Uses the same pattern as other auth-related routes
- Is correctly placed alongside other authentication management routes
This ensures consistent user experience and security model for the new WebAuthn feature.
frontend/javascripts/admin/auth/login_form.tsx (1)
2-2: LGTM! Import added for WebAuthn login.The import statement correctly includes the new
doWebAuthnLoginfunction alongside existing authentication functions.package.json (2)
75-76: LGTM! Scripts added for TLS support.The scripts for starting the proxy with TLS and generating SSL certificates are necessary for WebAuthn, which requires a secure context.
139-139:✅ Verification successful
Verify WebAuthn library version.
Using
@github/webauthn-jsonis appropriate for WebAuthn implementation, but it's good to ensure you're using a recent version.
🌐 Web query:
What is the latest version of @github/webauthn-json?💡 Result:
The latest version of
@github/webauthn-jsonis 2.1.1, released on January 30, 2023[1][2][3]. This version includes updates such as removing an inaccurateauthenticatorAttachmentfield fromAuthenticatorAttestationResponseand maintaining compatibility with TypeScript's DOM type definitions[3][5].The library provides JSON-friendly WebAuthn API wrappers that handle base64url encoding/decoding of binary data[1][6], and its package.json explicitly lists "2.1.1" as the current version[2]. For implementation details, developers can use either:
- The original simplified API (
create()/get())- The newer ponyfill API (
parseCreationOptionsFromJSON()/parseRequestOptionsFromJSON())[1][5]Citations:
- 1: https://github.com/github/webauthn-json
- 2: https://github.com/github/webauthn-json/blob/main/package.json
- 3: https://github.com/github/webauthn-json/releases
- 4: https://github.com/go-webauthn/webauthn/releases
- 5: https://github.com/github/webauthn-json/blob/main/compat.md
- 6: https://central.sonatype.com/artifact/org.mvnpm.at.github/webauthn-json
- 7: https://security.snyk.io/package/npm/@github%2Fwebauthn-json/2.0.0-alpha4
- 8: https://github.com/github/webauthn-json/blob/main/src/webauthn-json/extended.ts
- 9: https://github.com/github/webauthn-json/blob/main/Makefile
Verified: The WebAuthn library version is up-to-date.
The version in your
package.json("@github/webauthn-json": "^2.1.1") has been confirmed as the latest release. No changes are necessary.tools/proxy/proxy.js (2)
9-9: Fine introduction of a clear timeout constant.Defining
time5minis a good readability improvement for the proxy’s timeout settings. The naming is sufficiently descriptive, and 5 minutes is a sensible default.
132-138: Check host header overrides for potential side effects.When proxying to the Webpack dev server, overriding the
hostheader to "localhost" can resolve certain mismatch issues. If you require preserving the original host, consider settingX-Forwarded-Hostor verifying how the dev server uses the host header.conf/webknossos.latest.routes (1)
39-47: Well-structured WebAuthn routesThe new WebAuthn routes are well-organized and follow the RESTful pattern already established in the codebase. The grouping under a specific comment makes the purpose clear, and the route names align well with their functions.
conf/evolutions/126-add-webauthn-credentials.sql (1)
1-21: Properly structured migration script with appropriate constraintsThis migration script correctly:
- Verifies the previous schema version
- Creates a well-designed table with all necessary WebAuthn credential fields
- Sets up appropriate constraints including a foreign key with cascade actions
- Updates the schema version
The table structure follows best practices for storing WebAuthn credentials with the required fields: credential ID, user reference, public key, signature count, and a human-readable name.
tools/postgres/schema.sql (2)
23-23: Schema version properly updatedThe schema version has been correctly updated to 126, consistent with the migration script.
885-886: Foreign key constraint properly definedThe foreign key constraint for the WebAuthn credentials table is correctly defined with appropriate CASCADE options for DELETE and UPDATE operations.
frontend/javascripts/admin/admin_rest_api.ts (4)
1-1: Appropriate WebAuthn library importThe import of
@github/webauthn-jsonis a good choice, as it's a well-maintained library that simplifies WebAuthn API interactions.
152-165: WebAuthn login implementation looks goodThe
doWebAuthnLoginfunction correctly implements the WebAuthn authentication flow:
- Starts the auth process on the server
- Uses the browser's WebAuthn API to get credentials
- Finalizes the authentication on the server
- Retrieves the user and organization data
The implementation follows the same pattern as the existing
loginUserfunction.
181-183: Concise WebAuthn key listing implementationThe
listWebAuthnKeysfunction is concise and correctly implements the API call to fetch the user's WebAuthn keys.
185-190: WebAuthn key removal implementationThe
removeWebAuthnKeyfunction correctly implements the API call to remove a WebAuthn key with the appropriate HTTP method and payload.app/models/user/WebAuthnCredentials.scala (2)
13-21: Ensure thatisDeletedis correctly enforced when retrieving credentials.
Currently, there's no filter preventing the retrieval of deleted credentials (i.e.,isDeleted = true). If your business rules require excluding them, consider updating methods likefindAllForUserorlistKeysto filter out deleted entries.
23-24: DAO naming remains consistent and clear.
UsingWebAuthnCredentialDAOconsistently reflects the domain. No issues found here.app/security/WebAuthnCredentialRepository.scala (2)
16-22: Validate string-based ObjectId conversions.
When converting betweenByteArrayandObjectId, ensure the byte array always holds a validObjectId.toString. Otherwise, parsing might fail for users with custom object IDs or invalid data.
29-32: Solid repository design.
ExtendingCredentialRepositoryto keep concerns separated is a good pattern. No immediate issues found.app/controllers/AuthenticationController.scala (8)
6-9: Imports for WebAuthn libraries look correct.
No concerns about version conflicts or missing dependencies.
39-40: Ensure blocking calls are minimal.
While referencing a blocking dispatcher, confirm that only essential blocking (e.g., I/O-bound) tasks happen inside it to avoid starved threads.
42-55: Well-defined data classes for WebAuthn payloads.
The case classes are straightforward for JSON serialization. No issues found.
180-199: Ensure consistent user deactivation checks.
authenticateInnerproperly checks for deactivated users. Confirm that other WebAuthn flows (e.g., passkey login) also disallow deactivated user signins.
213-214: Chained Inner Authentication
The chaining from the credentials provider toauthenticateInnerreduces duplication. Nicely done.
493-516: Big blocking operation on line 513
ThestartRegistrationis performed in a blocking context, which is correct. Keep an eye on performance if usage spikes.
551-558: Descriptive naming
webauthnListKeysreturns a descriptor that includes ID and name. The approach is straightforward. Looks good.
560-567: No concurrency checks on key removal
webauthnRemoveKeyis correct in deleting by_idand_multiUser. Ensure there's no stale references in partial flows if multiple processes manipulate keys concurrently.
| export async function doWebAuthnRegistration(name: string): Promise<any> { | ||
| const webAuthnRegistrationAssertion = await Request.receiveJSON( | ||
| "/api/auth/webauthn/register/start", | ||
| { | ||
| method: "POST", | ||
| }, | ||
| ).then((body) => JSON.parse(body)); | ||
| const response = JSON.stringify(await webauthn.create(webAuthnRegistrationAssertion)); | ||
| return Request.sendJSONReceiveJSON("/api/auth/webauthn/register/finalize", { | ||
| data: { name: name, key: response }, | ||
| method: "POST", | ||
| }); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potentially redundant JSON parsing in registration function
There may be redundant JSON parsing in this function. If Request.receiveJSON already returns parsed JSON, then the JSON.parse(body) call is unnecessary and could cause errors.
🏁 Script executed:
#!/bin/bash
# Check how Request.receiveJSON is implemented
rg -A 5 "receiveJSON\s*=\s*function" --type ts Length of output: 48
Let's broaden our search to locate the implementation of Request.receiveJSON and verify whether it returns already parsed JSON. We'll run an updated shell script to search for any occurrences of receiveJSON in the repository:
🏁 Script executed:
#!/bin/bash
# Search for receiveJSON implementations across all TypeScript files
rg -n "receiveJSON" --type tsLength of output: 18922
Action Required: Remove Redundant JSON Parsing in WebAuthn Registration
It appears that Request.receiveJSON consistently returns a parsed JSON object across the codebase (as evidenced by its usage elsewhere), so chaining a JSON.parse in doWebAuthnRegistration is unnecessary and could lead to errors.
- File:
frontend/javascripts/admin/admin_rest_api.ts - Function:
doWebAuthnRegistration - Suggestion: Remove the
.then((body) => JSON.parse(body))and directly await the value fromRequest.receiveJSON.
Revised Code Snippet:
export async function doWebAuthnRegistration(name: string): Promise<any> {
const webAuthnRegistrationAssertion = await Request.receiveJSON(
"/api/auth/webauthn/register/start",
{ method: "POST" }
);
const response = JSON.stringify(await webauthn.create(webAuthnRegistrationAssertion));
return Request.sendJSONReceiveJSON("/api/auth/webauthn/register/finalize", {
data: { name: name, key: response },
method: "POST",
});
}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
frontend/javascripts/admin/auth/manage_passkeys_view.tsx (5)
16-16: Specify required imports or remove empty import.The empty import from
@github/webauthn-json/browser-ponyfillsuggests you're only importing for side effects. If specific functionality is needed, import it explicitly; otherwise, add a comment explaining the side effect import.
21-22: Fix naming inconsistency in state variable.The state variable is named
_isLoadingwith an underscore prefix, but it's accessed assetIsLoadingwithout the underscore in the component. This inconsistency could lead to bugs.- const [_isLoading, setIsLoading] = useState<boolean>(true); + const [isLoading, setIsLoading] = useState<boolean>(true);
37-38: Remove debug logging before production.The console.debug statement should be removed before the code is deployed to production.
- const result = await doWebAuthnRegistration(newPassKeyName); - console.debug(result); + await doWebAuthnRegistration(newPassKeyName);
58-72: Improve passkey list UI layout and handle empty state.The current layout lacks proper alignment and spacing between the passkey name and delete button. Also, there's no handling for when no passkeys are available.
<Col span={8}> <h3>Your Passkeys</h3> - {passkeys.map((passkey) => ( - <Row key={passkey.id}> - {passkey.name} - <Button - onClick={async () => { - await removeWebAuthnKey(passkey); - await fetchData(); - }} - > - Delete - </Button> - </Row> - ))} + {passkeys.length > 0 ? ( + passkeys.map((passkey) => ( + <Row key={passkey.id} justify="space-between" align="middle" style={{ marginBottom: 8 }}> + <Col>{passkey.name}</Col> + <Col> + <Button + onClick={async () => { + await removeWebAuthnKey(passkey); + await fetchData(); + }} + > + Delete + </Button> + </Col> + </Row> + )) + ) : ( + <p>No passkeys registered yet. Add your first one below.</p> + )}
76-81: Enhance passkey explanation with more details and formatting.The explanatory text about passkeys could benefit from better formatting and more comprehensive information about the benefits of passkeys.
<p> - Passkeys are a new web authentication method that allows you to log in without a - password in a secured way. Microsoft Hello and Apple FaceID are examples of technologies - that can be used as passkeys to log in in WEBKNOSSOS. If you want to add a new passkey - to your account use the button below. + <strong>What are Passkeys?</strong><br /> + Passkeys are a secure authentication method that allows you to log in without using a + password. They offer: + <ul> + <li>Enhanced security - more resistant to phishing than passwords</li> + <li>Convenience - use biometrics like Microsoft Hello or Apple FaceID</li> + <li>Seamless experience - faster login without typing credentials</li> + </ul> + To add a new passkey to your account, use the button below. </p>app/controllers/AuthenticationController.scala (1)
558-564: Add error handling to WebAuthn list keys method.The current implementation doesn't handle potential errors from the DAO.
def webauthnListKeys: Action[AnyContent] = sil.SecuredAction.async { implicit request => - { - for { - keys <- webAuthnCredentialDAO.listKeys(request.identity._multiUser) - reducedKeys = keys.map(credential => WebAuthnKeyDescriptor(credential._id, credential.name)) - } yield Ok(Json.toJson(reducedKeys)) - } + for { + keys <- webAuthnCredentialDAO.listKeys(request.identity._multiUser) + .recoverWith { + case e: Exception => + logger.error(s"Error listing WebAuthn keys: ${e.getMessage}") + Fox.failure("Failed to list passkeys") ~> INTERNAL_SERVER_ERROR + } + reducedKeys = keys.map(credential => WebAuthnKeyDescriptor(credential._id, credential.name)) + } yield { + logger.debug(s"Listed ${keys.length} passkeys for user ${request.identity._id}") + Ok(Json.toJson(reducedKeys)) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/AuthenticationController.scala(8 hunks)frontend/javascripts/admin/auth/manage_passkeys_view.tsx(1 hunks)frontend/javascripts/navbar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/navbar.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
| <Modal | ||
| title="Enter a name for the new PassKey" | ||
| open={isPassKeyNameModalOpen} | ||
| onOk={registerNewPassKey} | ||
| onCancel={() => setIsPassKeyNameModalOpen(false)} | ||
| > | ||
| <Input | ||
| placeholder="PassKey name" | ||
| value={newPassKeyName} | ||
| onChange={(e) => setNewPassKeyName(e.target.value)} | ||
| /> | ||
| </Modal> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for passkey name input.
The current implementation doesn't validate the passkey name input. You should add validation to prevent empty names or duplicates.
<Modal
title="Enter a name for the new PassKey"
open={isPassKeyNameModalOpen}
- onOk={registerNewPassKey}
+ onOk={() => {
+ if (!newPassKeyName.trim()) {
+ Toast.error("Passkey name cannot be empty");
+ return;
+ }
+ if (passkeys.some(pk => pk.name === newPassKeyName.trim())) {
+ Toast.error("A passkey with this name already exists");
+ return;
+ }
+ registerNewPassKey();
+ }}
onCancel={() => setIsPassKeyNameModalOpen(false)}
+ okButtonProps={{ disabled: !newPassKeyName.trim() }}
>
<Input
placeholder="PassKey name"
value={newPassKeyName}
onChange={(e) => setNewPassKeyName(e.target.value)}
+ onPressEnter={(e) => {
+ if (newPassKeyName.trim()) {
+ registerNewPassKey();
+ }
+ }}
/>
</Modal>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Modal | |
| title="Enter a name for the new PassKey" | |
| open={isPassKeyNameModalOpen} | |
| onOk={registerNewPassKey} | |
| onCancel={() => setIsPassKeyNameModalOpen(false)} | |
| > | |
| <Input | |
| placeholder="PassKey name" | |
| value={newPassKeyName} | |
| onChange={(e) => setNewPassKeyName(e.target.value)} | |
| /> | |
| </Modal> | |
| <Modal | |
| title="Enter a name for the new PassKey" | |
| open={isPassKeyNameModalOpen} | |
| onOk={() => { | |
| if (!newPassKeyName.trim()) { | |
| Toast.error("Passkey name cannot be empty"); | |
| return; | |
| } | |
| if (passkeys.some(pk => pk.name === newPassKeyName.trim())) { | |
| Toast.error("A passkey with this name already exists"); | |
| return; | |
| } | |
| registerNewPassKey(); | |
| }} | |
| onCancel={() => setIsPassKeyNameModalOpen(false)} | |
| okButtonProps={{ disabled: !newPassKeyName.trim() }} | |
| > | |
| <Input | |
| placeholder="PassKey name" | |
| value={newPassKeyName} | |
| onChange={(e) => setNewPassKeyName(e.target.value)} | |
| onPressEnter={(e) => { | |
| if (newPassKeyName.trim()) { | |
| registerNewPassKey(); | |
| } | |
| }} | |
| /> | |
| </Modal> |
| 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() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve RelyingParty configuration with error handling.
The current RelyingParty initialization doesn't handle potential configuration issues or provide logging. Additionally, splitting the domain from the URL with split("/") is fragile.
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(configuration.get[String]("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
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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() | |
| } | |
| private lazy val relyingParty = { | |
| try { | |
| val uri = new java.net.URI(configuration.get[String]("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 | |
| } | |
| } |
| 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) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for WebAuthn authentication start.
The current implementation doesn't handle potential exceptions from the RelyingParty or token generation.
def webauthnAuthStart(): Action[AnyContent] = Action {
- val opts = StartAssertionOptions.builder().build();
- val assertion = relyingParty.startAssertion(opts);
- val sessionId = UUID.randomUUID().toString;
- val cookie = Cookie("webauthn-session", sessionId, maxAge = Some(120), httpOnly = true, secure = true)
- temporaryAssertionStore.insert(sessionId, assertion, Some(2 minutes));
- Ok(Json.toJson(Json.parse(assertion.toCredentialsGetJson))).withCookies(cookie)
+ try {
+ val opts = StartAssertionOptions.builder().build()
+ val assertion = relyingParty.startAssertion(opts)
+ val sessionId = UUID.randomUUID().toString
+ val cookie = Cookie("webauthn-session", sessionId, maxAge = Some(120), httpOnly = true, secure = true)
+ temporaryAssertionStore.insert(sessionId, assertion, Some(2 minutes))
+ logger.debug("Started WebAuthn authentication")
+ Ok(Json.toJson(Json.parse(assertion.toCredentialsGetJson))).withCookies(cookie)
+ } catch {
+ case e: Exception =>
+ logger.error(s"Failed to start WebAuthn authentication: ${e.getMessage}")
+ InternalServerError(Json.obj("error" -> "Failed to start authentication"))
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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")) | |
| } | |
| } |
| def webauthnRemoveKey: Action[WebAuthnKeyDescriptor] = sil.SecuredAction.async(validateJson[WebAuthnKeyDescriptor]) { | ||
| implicit request => | ||
| { | ||
| for { | ||
| _ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser) | ||
| } yield Ok(Json.obj()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and error handling to WebAuthn remove key method.
The current implementation doesn't validate that the key belongs to the user nor handle potential errors from the DAO.
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 {
+ // First verify the key exists and belongs to the user
+ keys <- webAuthnCredentialDAO.listKeys(request.identity._multiUser)
+ _ <- Fox.bool2Fox(keys.exists(_.id == request.body.id)) ?~>
+ "Passkey not found or doesn't belong to you" ~> NOT_FOUND
+ // Then remove it
+ _ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser)
+ .recoverWith {
+ case e: Exception =>
+ logger.error(s"Error removing WebAuthn key: ${e.getMessage}")
+ Fox.failure("Failed to remove passkey") ~> INTERNAL_SERVER_ERROR
+ }
+ } yield {
+ logger.info(s"User ${request.identity._id} removed passkey: ${request.body.name}")
+ Ok(Json.obj("message" -> "Passkey removed successfully"))
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def webauthnRemoveKey: Action[WebAuthnKeyDescriptor] = sil.SecuredAction.async(validateJson[WebAuthnKeyDescriptor]) { | |
| implicit request => | |
| { | |
| for { | |
| _ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser) | |
| } yield Ok(Json.obj()) | |
| } | |
| } | |
| def webauthnRemoveKey: Action[WebAuthnKeyDescriptor] = sil.SecuredAction.async(validateJson[WebAuthnKeyDescriptor]) { | |
| implicit request => | |
| for { | |
| // First verify the key exists and belongs to the user | |
| keys <- webAuthnCredentialDAO.listKeys(request.identity._multiUser) | |
| _ <- Fox.bool2Fox(keys.exists(_.id == request.body.id)) ?~> | |
| "Passkey not found or doesn't belong to you" ~> NOT_FOUND | |
| // Then remove it | |
| _ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser) | |
| .recoverWith { | |
| case e: Exception => | |
| logger.error(s"Error removing WebAuthn key: ${e.getMessage}") | |
| Fox.failure("Failed to remove passkey") ~> INTERNAL_SERVER_ERROR | |
| } | |
| } yield { | |
| logger.info(s"User ${request.identity._id} removed passkey: ${request.body.name}") | |
| Ok(Json.obj("message" -> "Passkey removed successfully")) | |
| } | |
| } |
| 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; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and refactor WebAuthn authentication finalization.
The current implementation has nested conditionals and could benefit from a more functional style. Additionally, error handling could be improved.
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)
+ result <- {
+ try {
+ val keyCredential = PublicKeyCredential.parseAssertionResponseJson(request.body.key)
+ val opts = FinishAssertionOptions.builder().request(challengeData).response(keyCredential).build()
+
+ for {
+ assertion <- Fox.future2Fox(Future {
+ Try(relyingParty.finishAssertion(opts)) match {
+ case scala.util.Success(a) => a
+ case scala.util.Failure(e) => {
+ logger.warn(s"WebAuthn authentication failed: ${e.getMessage}")
+ throw e
+ }
+ }
+ })(blockingContext)
+ 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
+ } catch {
+ case e: Exception =>
+ logger.error(s"Error finalizing WebAuthn authentication: ${e.getMessage}")
+ Fox.failure("Authentication failed: " + e.getMessage, Full(e)) ~> BAD_REQUEST
+ }
+ }
+ } yield result
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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; | |
| } | |
| } | |
| } | |
| } | |
| } | |
| def webauthnAuthFinalize(): Action[WebAuthnAuthentication] = Action.async(validateJson[WebAuthnAuthentication]) { | |
| implicit request => | |
| 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) | |
| result <- { | |
| try { | |
| val keyCredential = PublicKeyCredential.parseAssertionResponseJson(request.body.key) | |
| val opts = FinishAssertionOptions.builder().request(challengeData).response(keyCredential).build() | |
| for { | |
| assertion <- Fox.future2Fox(Future { | |
| Try(relyingParty.finishAssertion(opts)) match { | |
| case scala.util.Success(a) => a | |
| case scala.util.Failure(e) => | |
| logger.warn(s"WebAuthn authentication failed: ${e.getMessage}") | |
| throw e | |
| } | |
| })(blockingContext) | |
| 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 | |
| } catch { | |
| case e: Exception => | |
| logger.error(s"Error finalizing WebAuthn authentication: ${e.getMessage}") | |
| Fox.failure("Authentication failed: " + e.getMessage, Full(e)) ~> BAD_REQUEST | |
| } | |
| } | |
| } yield result | |
| } |
| def webauthnRegisterFinalize(): Action[WebAuthnRegistration] = | ||
| sil.SecuredAction.async(validateJson[WebAuthnRegistration]) { implicit request => | ||
| { | ||
| val creationOpts = temporaryRegistrationStore.get(request.identity._multiUser) | ||
| temporaryRegistrationStore.remove(request.identity._multiUser) | ||
| creationOpts match { | ||
| case Some(data) => { | ||
| val response = PublicKeyCredential.parseRegistrationResponseJson(request.body.key); | ||
| val opts = FinishRegistrationOptions.builder().request(data).response(response).build(); | ||
| try { | ||
| for { | ||
| preKey <- Fox.future2Fox(Future { Try(relyingParty.finishRegistration(opts)) })(blockingContext); | ||
| key <- preKey match { | ||
| case scala.util.Success(key) => Fox.successful(key) | ||
| case scala.util.Failure(e) => Fox.failure("Registration failed", Full(e)) | ||
| }; | ||
| result <- { | ||
| val credential = WebAuthnCredential( | ||
| ObjectId.generate, | ||
| request.identity._multiUser, | ||
| WebAuthnCredentialRepository.byteArrayToBytes(key.getKeyId.getId), | ||
| request.body.name, | ||
| key.getPublicKeyCose.getBytes, | ||
| key.getSignatureCount.toInt, | ||
| isDeleted = false, | ||
| ) | ||
| webAuthnCredentialDAO.insertOne(credential).map(_ => Ok("")) | ||
| } | ||
| } yield result; | ||
| } catch { | ||
| case e: RegistrationFailedException => Future.successful(BadRequest("Failed to register key")) | ||
| } | ||
| } | ||
| case None => Future.successful(BadRequest("Challenge not found or expired")) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen error handling and improve structure of WebAuthn registration finalization.
The current implementation has a try-catch block that only catches RegistrationFailedException and uses nested conditionals. The error handling could be more comprehensive and the code structure could be improved.
def webauthnRegisterFinalize(): Action[WebAuthnRegistration] =
sil.SecuredAction.async(validateJson[WebAuthnRegistration]) { implicit request =>
- {
- val creationOpts = temporaryRegistrationStore.get(request.identity._multiUser)
- temporaryRegistrationStore.remove(request.identity._multiUser)
- creationOpts match {
- case Some(data) => {
- val response = PublicKeyCredential.parseRegistrationResponseJson(request.body.key);
- val opts = FinishRegistrationOptions.builder().request(data).response(response).build();
- try {
- for {
- preKey <- Fox.future2Fox(Future { Try(relyingParty.finishRegistration(opts)) })(blockingContext);
- key <- preKey match {
- case scala.util.Success(key) => Fox.successful(key)
- case scala.util.Failure(e) => Fox.failure("Registration failed", Full(e))
- };
- result <- {
- val credential = WebAuthnCredential(
- ObjectId.generate,
- request.identity._multiUser,
- WebAuthnCredentialRepository.byteArrayToBytes(key.getKeyId.getId),
- request.body.name,
- key.getPublicKeyCose.getBytes,
- key.getSignatureCount.toInt,
- isDeleted = false,
- )
- webAuthnCredentialDAO.insertOne(credential).map(_ => Ok(""))
- }
- } yield result;
- } catch {
- case e: RegistrationFailedException => Future.successful(BadRequest("Failed to register key"))
- }
- }
- case None => Future.successful(BadRequest("Challenge not found or expired"))
- }
- }
+ for {
+ creationOpts <- Fox.option2Fox(temporaryRegistrationStore.get(request.identity._multiUser)) ?~>
+ "Challenge not found or expired" ~> BAD_REQUEST
+ _ = temporaryRegistrationStore.remove(request.identity._multiUser)
+ result <- {
+ try {
+ val response = PublicKeyCredential.parseRegistrationResponseJson(request.body.key)
+ val opts = FinishRegistrationOptions.builder().request(creationOpts).response(response).build()
+
+ for {
+ key <- Fox.future2Fox(Future {
+ Try(relyingParty.finishRegistration(opts)) match {
+ case scala.util.Success(k) => k
+ case scala.util.Failure(e) => {
+ logger.warn(s"WebAuthn registration failed: ${e.getMessage}")
+ throw e
+ }
+ }
+ })(blockingContext)
+
+ // Check if a passkey with this name already exists
+ existingKeys <- webAuthnCredentialDAO.listKeys(request.identity._multiUser)
+ _ <- Fox.bool2Fox(!existingKeys.exists(_.name == request.body.name)) ?~>
+ "A passkey with this name already exists" ~> BAD_REQUEST
+
+ credential = WebAuthnCredential(
+ ObjectId.generate,
+ request.identity._multiUser,
+ WebAuthnCredentialRepository.byteArrayToBytes(key.getKeyId.getId),
+ request.body.name,
+ key.getPublicKeyCose.getBytes,
+ key.getSignatureCount.toInt,
+ isDeleted = false
+ )
+ _ <- webAuthnCredentialDAO.insertOne(credential)
+ _ = logger.info(s"User ${request.identity._id} registered new passkey: ${request.body.name}")
+ } yield Ok(Json.obj("message" -> "Passkey registered successfully"))
+ } catch {
+ case e: RegistrationFailedException =>
+ logger.warn(s"WebAuthn registration failed: ${e.getMessage}")
+ Fox.failure("Failed to register key") ~> BAD_REQUEST
+ case e: Exception =>
+ logger.error(s"Error in WebAuthn registration: ${e.getMessage}", e)
+ Fox.failure("Registration failed: " + e.getMessage) ~> INTERNAL_SERVER_ERROR
+ }
+ }
+ } yield result📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def webauthnRegisterFinalize(): Action[WebAuthnRegistration] = | |
| sil.SecuredAction.async(validateJson[WebAuthnRegistration]) { implicit request => | |
| { | |
| val creationOpts = temporaryRegistrationStore.get(request.identity._multiUser) | |
| temporaryRegistrationStore.remove(request.identity._multiUser) | |
| creationOpts match { | |
| case Some(data) => { | |
| val response = PublicKeyCredential.parseRegistrationResponseJson(request.body.key); | |
| val opts = FinishRegistrationOptions.builder().request(data).response(response).build(); | |
| try { | |
| for { | |
| preKey <- Fox.future2Fox(Future { Try(relyingParty.finishRegistration(opts)) })(blockingContext); | |
| key <- preKey match { | |
| case scala.util.Success(key) => Fox.successful(key) | |
| case scala.util.Failure(e) => Fox.failure("Registration failed", Full(e)) | |
| }; | |
| result <- { | |
| val credential = WebAuthnCredential( | |
| ObjectId.generate, | |
| request.identity._multiUser, | |
| WebAuthnCredentialRepository.byteArrayToBytes(key.getKeyId.getId), | |
| request.body.name, | |
| key.getPublicKeyCose.getBytes, | |
| key.getSignatureCount.toInt, | |
| isDeleted = false, | |
| ) | |
| webAuthnCredentialDAO.insertOne(credential).map(_ => Ok("")) | |
| } | |
| } yield result; | |
| } catch { | |
| case e: RegistrationFailedException => Future.successful(BadRequest("Failed to register key")) | |
| } | |
| } | |
| case None => Future.successful(BadRequest("Challenge not found or expired")) | |
| } | |
| } | |
| } | |
| def webauthnRegisterFinalize(): Action[WebAuthnRegistration] = | |
| sil.SecuredAction.async(validateJson[WebAuthnRegistration]) { implicit request => | |
| for { | |
| creationOpts <- Fox.option2Fox(temporaryRegistrationStore.get(request.identity._multiUser)) ?~> | |
| "Challenge not found or expired" ~> BAD_REQUEST | |
| _ = temporaryRegistrationStore.remove(request.identity._multiUser) | |
| result <- { | |
| try { | |
| val response = PublicKeyCredential.parseRegistrationResponseJson(request.body.key) | |
| val opts = FinishRegistrationOptions.builder().request(creationOpts).response(response).build() | |
| for { | |
| key <- Fox.future2Fox(Future { | |
| Try(relyingParty.finishRegistration(opts)) match { | |
| case scala.util.Success(k) => k | |
| case scala.util.Failure(e) => | |
| logger.warn(s"WebAuthn registration failed: ${e.getMessage}") | |
| throw e | |
| } | |
| })(blockingContext) | |
| // Check if a passkey with this name already exists | |
| existingKeys <- webAuthnCredentialDAO.listKeys(request.identity._multiUser) | |
| _ <- Fox.bool2Fox(!existingKeys.exists(_.name == request.body.name)) ?~> | |
| "A passkey with this name already exists" ~> BAD_REQUEST | |
| credential = WebAuthnCredential( | |
| ObjectId.generate, | |
| request.identity._multiUser, | |
| WebAuthnCredentialRepository.byteArrayToBytes(key.getKeyId.getId), | |
| request.body.name, | |
| key.getPublicKeyCose.getBytes, | |
| key.getSignatureCount.toInt, | |
| isDeleted = false | |
| ) | |
| _ <- webAuthnCredentialDAO.insertOne(credential) | |
| _ = logger.info(s"User ${request.identity._id} registered new passkey: ${request.body.name}") | |
| } yield Ok(Json.obj("message" -> "Passkey registered successfully")) | |
| } catch { | |
| case e: RegistrationFailedException => | |
| logger.warn(s"WebAuthn registration failed: ${e.getMessage}") | |
| Fox.failure("Failed to register key") ~> BAD_REQUEST | |
| case e: Exception => | |
| logger.error(s"Error in WebAuthn registration: ${e.getMessage}", e) | |
| Fox.failure("Registration failed: " + e.getMessage) ~> INTERNAL_SERVER_ERROR | |
| } | |
| } | |
| } yield result | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
frontend/javascripts/admin/auth/change_password_view.tsx (4)
72-83: Unused variable in registerNewPasskey functionThe result from doWebAuthnRegistration is assigned but never used.
- const result = await doWebAuthnRegistration(newPasskeyName); + await doWebAuthnRegistration(newPasskeyName);Alternatively, if the result is needed for debugging:
- const result = await doWebAuthnRegistration(newPasskeyName); + const _result = await doWebAuthnRegistration(newPasskeyName);🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: This variable is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
Unsafe fix: If this is intentional, prepend result with an underscore.(lint/correctness/noUnusedVariables)
197-232: Improve UI for passkey listingThe passkey list items could benefit from improved styling and layout. The current implementation has the delete button directly next to the passkey name without proper spacing or alignment.
Consider using Ant Design's List component or improving the Row structure:
- {passkeys.map((passkey) => ( - <Row key={passkey.id}> - {passkey.name} - <Button - onClick={async () => { - await removeWebAuthnKey(passkey); - await fetchData(); - }} - > - Delete - </Button> - </Row> - ))} + {passkeys.map((passkey) => ( + <Row key={passkey.id} justify="space-between" align="middle" style={{ marginBottom: 8 }}> + <span>{passkey.name}</span> + <Button + danger + size="small" + onClick={async () => { + await removeWebAuthnKey(passkey); + await fetchPasskeys(); + }} + > + Delete + </Button> + </Row> + ))}
233-244: Consider adding validation for passkey nameThe modal allows submitting empty passkey names. Consider adding validation to prevent empty names or adding a disabled state to the OK button when the name is empty.
<Modal title="Enter a name for the new Passkey" open={isPasskeyNameModalOpen} onOk={registerNewPasskey} + okButtonProps={{ disabled: !newPasskeyName.trim() }} onCancel={() => setIsPasskeyNameModalOpen(false)} >
227-229: Consider adding state for empty passkeys listThe component doesn't show any special indication when the passkeys list is empty. Consider adding a message to guide users when they have no passkeys yet.
Add a conditional before the button:
+ {passkeys.length === 0 && ( + <Alert + type="info" + message="You don't have any passkeys registered yet. Click the button below to add your first passkey." + style={{ marginBottom: 16 }} + /> + )} <div style={{paddingTop: 10}}> <Button onClick={() => setIsPasskeyNameModalOpen(true)} type="primary"> Register new Passkey
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/admin/auth/change_password_view.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/javascripts/admin/auth/change_password_view.tsx
[error] 75-75: This variable is unused.
Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
Unsafe fix: If this is intentional, prepend result with an underscore.
(lint/correctness/noUnusedVariables)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (4)
frontend/javascripts/admin/auth/change_password_view.tsx (4)
11-17: Well-structured import organizationThe imports have been properly organized to include the necessary WebAuthn-related functions and React hooks needed for the new passkey functionality.
29-33: Good state management setupThe state variables are well-defined with appropriate types and naming conventions. Using the underscore prefix for
_isLoadingPasskeysproperly indicates it's currently not used in the render, which is a good practice.
35-37: Appropriate use of useEffect for initializationThe useEffect hook with an empty dependency array ensures that passkeys are fetched only once when the component mounts.
65-70: Well-implemented fetch functionThe fetchPasskeys function appropriately manages loading state and handles the async API call correctly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
frontend/javascripts/admin/auth/change_password_view.tsx (2)
32-32:⚠️ Potential issueMissing WebAuthnKeyDescriptor type import.
The code uses WebAuthnKeyDescriptor type but doesn't import it.
Add the import at the top of the file:
import { useEffect, useState } from "react"; + import { WebAuthnKeyDescriptor } from "types/api_flow_types"; import { doWebAuthnRegistration, listWebAuthnKeys, removeWebAuthnKey, revokeAuthToken, } from "admin/admin_rest_api";
219-219:⚠️ Potential issueBug: Undefined function call.
There's a call to an undefined
fetchData()function, which will cause a runtime error.- await fetchData(); + await fetchPasskeys();
🧹 Nitpick comments (5)
tools/proxy/proxy.js (1)
2-7: Update Node.js module imports with the node: protocolThe new imports for
fsandhttpsshould use the node: protocol prefix for better clarity and to explicitly indicate these are Node.js built-in modules.-const fs = require("fs"); +const fs = require("node:fs"); const httpProxy = require("http-proxy"); const { spawn, exec } = require("child_process"); -const path = require("path"); +const path = require("node:path"); const prefixLines = require("prefix-stream-lines"); -const https = require("https"); +const https = require("node:https");While we're at it, we should update the other Node.js built-in modules too.
const httpProxy = require("http-proxy"); -const { spawn, exec } = require("child_process"); +const { spawn, exec } = require("node:child_process");🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 4-4: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 5-5: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
[error] 7-7: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
frontend/javascripts/admin/auth/login_form.tsx (1)
154-157: Improve error message for better user experience.The current error message is generic and doesn't provide enough context to help users understand what went wrong.
- console.error("WebAuthn login failed", error); - Toast.errror("Login with Passkey failed"); + console.error("WebAuthn login failed", error); + Toast.error(`Login with Passkey failed: ${error.message || "Unknown error"}`);frontend/javascripts/admin/auth/change_password_view.tsx (3)
213-225: Improve passkey row layout and feedback.The current row layout for passkeys lacks proper spacing and alignment between the name and delete button.
- {passkeys.map((passkey) => ( - <Row key={passkey.id}> - {passkey.name} - <Button - onClick={async () => { - await removeWebAuthnKey(passkey); - await fetchData(); - }} - > - Delete - </Button> - </Row> + {passkeys.map((passkey) => ( + <Row key={passkey.id} justify="space-between" align="middle" style={{ marginBottom: 8 }}> + <Col>{passkey.name}</Col> + <Col> + <Button + danger + onClick={async () => { + try { + await removeWebAuthnKey(passkey); + await fetchPasskeys(); + Toast.success("Passkey deleted successfully"); + } catch (error) { + console.error("Failed to delete passkey", error); + Toast.error("Failed to delete passkey"); + } + }} + > + Delete + </Button> + </Col> + </Row>
65-70: Add loading state indicator for passkeys.The component tracks loading state but doesn't show any loading indicator to the user.
Consider adding a loading indicator when fetching passkeys:
async function fetchPasskeys(): Promise<void> { setIsLoadingPasskeys(true); const keys = await listWebAuthnKeys(); setPasskeys(keys); setIsLoadingPasskeys(false); }And in the JSX part where the passkeys are displayed:
<h3>Your Passkeys</h3> + {_isLoadingPasskeys && <div><Spin size="small" /> Loading passkeys...</div>} + {!_isLoadingPasskeys && passkeys.length === 0 && <div>No passkeys registered yet.</div>} {passkeys.map((passkey) => ( <Row key={passkey.id}> // ... rest of the codeDon't forget to add the Spin import:
- import { Alert, Button, Col, Form, Input, Modal, Row } from "antd"; + import { Alert, Button, Col, Form, Input, Modal, Row, Spin } from "antd";
237-243: Add validation for empty passkey name.The modal allows submitting an empty passkey name, which may cause issues or confusion.
<Modal title="Enter a name for the new Passkey" open={isPasskeyNameModalOpen} - onOk={registerNewPasskey} + onOk={() => { + if (!newPasskeyName.trim()) { + Toast.error("Please enter a name for your passkey"); + return; + } + registerNewPasskey(); + }} onCancel={() => setIsPasskeyNameModalOpen(false)} > <Input placeholder="Passkey name" value={newPasskeyName} onChange={(e) => setNewPasskeyName(e.target.value)} + onPressEnter={(e) => { + if (!newPasskeyName.trim()) { + Toast.error("Please enter a name for your passkey"); + return; + } + registerNewPasskey(); + }} /> </Modal>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/admin/auth/change_password_view.tsx(4 hunks)frontend/javascripts/admin/auth/login_form.tsx(2 hunks)tools/postgres/schema.sql(3 hunks)tools/proxy/proxy.js(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
tools/proxy/proxy.js
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 4-4: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 5-5: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 7-7: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (9)
tools/proxy/proxy.js (4)
9-9: Great use of a named constant for timeout valuesUsing a named constant for the timeout value improves code maintainability and prevents inconsistencies.
12-14: Consistent timeout settings using the constantGood refactoring to use the named constant for both timeout settings.
132-139: Ensure you understand the host header override implicationsYou're now explicitly setting the
hostheader to "localhost" when proxying to the Webpack dev server. This could affect how the server interprets or handles requests, especially if hostname-based routing logic exists.Verify that this change doesn't break any functionality that might depend on the original host header value, particularly in development environments with different hostnames.
11-14: Switch from var to const.Using
varcan lead to unintentional scope leakage. Preferconstwhen the reference never changes orletwhen it does. This aligns better with modern JS best practices.-var proxy = httpProxy.createProxyServer({ +const proxy = httpProxy.createProxyServer({ proxyTimeout: time5min, // 5 min timeout: time5min, // 5 min });frontend/javascripts/admin/auth/login_form.tsx (1)
142-163: WebAuthn login implementation looks good.The implementation correctly handles WebAuthn login with proper error handling and user flow. It dispatches appropriate actions on successful login.
tools/postgres/schema.sql (3)
421-430: Well-structured WebAuthn credentials table.The table definition is properly structured with appropriate data types, a primary key, foreign key constraint and a unique constraint to prevent duplicate credential keys for the same user.
23-23: Schema version update looks good.Schema version has been properly incremented from 125 to 126 to reflect the addition of the WebAuthn credentials table.
885-886: Foreign key constraint looks good.The foreign key constraint properly links the WebAuthn credentials to user accounts and includes appropriate cascade options.
frontend/javascripts/admin/auth/change_password_view.tsx (1)
72-83: Error handling in registerNewPasskey looks good.The function properly handles errors and provides appropriate feedback to the user.
| if (process.argv.includes("--tls")) { | ||
| console.log(loggingPrefix, "Using TLS") | ||
| https.createServer({ | ||
| key: fs.readFileSync("target/dev.key.pem"), | ||
| cert: fs.readFileSync("target/dev.cert.pem"), | ||
| }, app).listen(PORT) | ||
| } else { | ||
| app.listen(PORT); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for TLS certificate file reading
The implementation for TLS support is good, but it's missing error handling for the file reading operations. If the certificate files don't exist or are inaccessible, the application will crash without a helpful error message.
if (process.argv.includes("--tls")) {
console.log(loggingPrefix, "Using TLS")
- https.createServer({
- key: fs.readFileSync("target/dev.key.pem"),
- cert: fs.readFileSync("target/dev.cert.pem"),
- }, app).listen(PORT)
+ try {
+ const key = fs.readFileSync("target/dev.key.pem");
+ const cert = fs.readFileSync("target/dev.cert.pem");
+ https.createServer({ key, cert }, app).listen(PORT);
+ console.log(loggingPrefix, "HTTPS server started successfully");
+ } catch (error) {
+ console.error(loggingPrefix, "Failed to start HTTPS server:", error.message);
+ console.error(loggingPrefix, "Make sure you've generated SSL certificates using the gen-ssl-dev-certs.sh script");
+ process.exit(1);
+ }
} else {
app.listen(PORT);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (process.argv.includes("--tls")) { | |
| console.log(loggingPrefix, "Using TLS") | |
| https.createServer({ | |
| key: fs.readFileSync("target/dev.key.pem"), | |
| cert: fs.readFileSync("target/dev.cert.pem"), | |
| }, app).listen(PORT) | |
| } else { | |
| app.listen(PORT); | |
| } | |
| if (process.argv.includes("--tls")) { | |
| console.log(loggingPrefix, "Using TLS") | |
| try { | |
| const key = fs.readFileSync("target/dev.key.pem"); | |
| const cert = fs.readFileSync("target/dev.cert.pem"); | |
| https.createServer({ key, cert }, app).listen(PORT); | |
| console.log(loggingPrefix, "HTTPS server started successfully"); | |
| } catch (error) { | |
| console.error(loggingPrefix, "Failed to start HTTPS server:", error.message); | |
| console.error(loggingPrefix, "Make sure you've generated SSL certificates using the gen-ssl-dev-certs.sh script"); | |
| process.exit(1); | |
| } | |
| } else { | |
| app.listen(PORT); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/javascripts/admin/auth/change_password_view.tsx (6)
66-71: Add error handling to fetchPasskeys functionThe current implementation doesn't handle potential failures when fetching passkeys. If the API call fails, the loading state will remain true.
async function fetchPasskeys(): Promise<void> { setIsLoadingPasskeys(true); - const keys = await listWebAuthnKeys(); - setPasskeys(keys); - setIsLoadingPasskeys(false); + try { + const keys = await listWebAuthnKeys(); + setPasskeys(keys); + } catch (error) { + console.error("Failed to fetch passkeys:", error); + Toast.error("Failed to load passkeys"); + } finally { + setIsLoadingPasskeys(false); + } }
73-84: Add validation for empty passkey nameThe registerNewPasskey function doesn't validate if the passkey name is empty before attempting registration.
const registerNewPasskey = async () => { + if (!newPasskeyName.trim()) { + Toast.error("Passkey name cannot be empty"); + return; + } try { setIsPasskeyNameModalOpen(false); await doWebAuthnRegistration(newPasskeyName); Toast.success("Passkey registered successfully"); setNewPasskeyName(""); await fetchPasskeys(); } catch (e) { Toast.error(`Registering new Passkey '${newPasskeyName}' failed`); console.error("Could not register new Passkey", e); } };
214-226: Improve passkey list UI and add empty stateThe current UI for displaying passkeys lacks proper spacing and alignment. Also, there's no handling for when the passkeys list is empty.
-{passkeys.map((passkey) => ( - <Row key={passkey.id}> - {passkey.name} - <Button - onClick={async () => { - await removeWebAuthnKey(passkey); - await fetchPasskeys(); - }} - > - Delete - </Button> - </Row> -))} +{_isLoadingPasskeys ? ( + <div>Loading passkeys...</div> +) : passkeys.length === 0 ? ( + <div>No passkeys registered yet.</div> +) : ( + passkeys.map((passkey) => ( + <Row key={passkey.id} justify="space-between" align="middle" style={{ marginBottom: 8 }}> + <Col>{passkey.name}</Col> + <Col> + <Button + danger + onClick={async () => { + await removeWebAuthnKey(passkey); + await fetchPasskeys(); + }} + > + Delete + </Button> + </Col> + </Row> + )) +)}
240-244: Add keyboard support for passkey name inputThe input field for the passkey name should support submitting via the Enter key for better user experience.
<Input placeholder="Passkey name" value={newPasskeyName} onChange={(e) => setNewPasskeyName(e.target.value)} + onPressEnter={registerNewPasskey} />
207-212: Consider rewording the passkeys explanationThe explanation of passkeys could be slightly improved for clarity.
<p> - Passkeys are a new web authentication method that allows you to log in without a - password in a secured way. Microsoft Hello and Apple FaceID are examples of technologies - that can be used as passkeys to log in in WEBKNOSSOS. If you want to add a new passkey - to your account use the button below. + Passkeys are a secure web authentication method that allows you to log in without a + password. Technologies like Microsoft Windows Hello and Apple FaceID can be used as + passkeys to log in to WEBKNOSSOS. To add a new passkey to your account, use the + button below. </p>
34-34: Remove underscore prefix if the variable is usedThe
_isLoadingPasskeysvariable has an underscore prefix suggesting it might be unused, but it is actually used in the component.-const [_isLoadingPasskeys, setIsLoadingPasskeys] = useState(false); +const [isLoadingPasskeys, setIsLoadingPasskeys] = useState(false);And update all references to this variable accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/admin/auth/change_password_view.tsx(4 hunks)frontend/javascripts/admin/auth/login_form.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/admin/auth/login_form.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
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)