Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces a new passkey registration and login feature using WebAuthn. It adds a Yubico WebAuthn server dependency to the Maven configuration. The backend includes a new REST controller for passkey registration and status queries, a service managing passkey credentials with session-based challenges, and a JPA entity representing passkey credentials with auditing. An MVC controller serves a new HTML template implementing client-side passkey registration via JavaScript and WebAuthn APIs. A database migration creates a table for storing passkey credentials. The main index page is enhanced with a login section supporting Google OAuth2 and passkey login flows. Minor formatting cleanup is applied to properties and exception handler files. A repository interface for passkey credentials is also added. Additionally, a WebAuthn login REST controller and a DTO for login data were introduced. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant PasskeyDemoPage
participant PasskeyController
participant PasskeyService
User->>Browser: Open /passkey/demo
Browser->>PasskeyDemoPage: Render passkey-demo.html
User->>Browser: Click "Create Passkey"
Browser->>PasskeyDemoPage: JS fetches registration options from /passkey/begin-registration
PasskeyDemoPage->>PasskeyController: Request registration challenge
PasskeyController->>PasskeyService: startRegistration(userEmail)
PasskeyService-->>PasskeyController: Return challenge
PasskeyController-->>PasskeyDemoPage: Send challenge options
PasskeyDemoPage->>Browser: navigator.credentials.create() with challenge
Browser->>PasskeyDemoPage: Credential created
PasskeyDemoPage->>PasskeyController: POST /passkey/register with credential JSON
PasskeyController->>PasskeyService: saveCredential(credential JSON, username)
PasskeyService-->>PasskeyController: Store credential in DB
PasskeyController-->>PasskeyDemoPage: 200 OK + confirmation
PasskeyDemoPage->>Browser: Display registration result
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
src/main/java/org/example/springboot25/entities/PasskeyCredential.java (1)
10-42: Missing important object methods and documentation.While getters and setters are properly implemented, consider adding
equals(),hashCode(), andtoString()methods for proper object comparison and debugging. Also, add JavaDoc comments to explain the purpose of each field.+/** + * Entity representing a WebAuthn credential (passkey). + * Stores the necessary information for authenticating a user with a passkey. + */ @Entity public class PasskeyCredential { + /** + * The username associated with this credential + */ private String userName; + /** + * The unique identifier for this credential, base64url encoded + */ @Id private String credentialId; + /** + * The public key associated with this credential, in PEM format + */ @Column(columnDefinition = "TEXT") private String publicKey; + /** + * The signature count to prevent replay attacks + */ private long signatureCount; // Getters and setters... + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + PasskeyCredential that = (PasskeyCredential) o; + return credentialId.equals(that.credentialId); + } + + @Override + public int hashCode() { + return credentialId.hashCode(); + } + + @Override + public String toString() { + return "PasskeyCredential{" + + "userName='" + userName + '\'' + + ", credentialId='" + credentialId + '\'' + + ", signatureCount=" + signatureCount + + '}'; + }src/main/java/org/example/springboot25/controller/PasskeyController.java (1)
23-27: Improved error handling needed for status endpoint.The status endpoint should handle potential exceptions and provide appropriate HTTP status codes.
@GetMapping("/status/{id}") public ResponseEntity<String> checkCredential(@PathVariable String id) { + if (id == null || id.isBlank()) { + return ResponseEntity.badRequest().body("Credential ID cannot be empty"); + } + + try { boolean exists = passkeyService.credentialExists(id); - return ResponseEntity.ok(exists ? "Credential found" : "Not found"); + if (exists) { + return ResponseEntity.ok("Credential found"); + } else { + return ResponseEntity.status(HttpStatus.NOT_FOUND).body("Credential not found"); + } + } catch (Exception e) { + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) + .body("Error checking credential: " + e.getMessage()); + } }src/main/java/org/example/springboot25/service/PasskeyService.java (3)
18-19: In-memory storage should be replaced with persistent storage for productionThe comment indicates this is a simple in-memory implementation. For a production environment, this should be replaced with a database-backed solution to persist credentials across application restarts.
- // Enkel in-memory storage – byt till databas om ni vill - private final ConcurrentHashMap<String, PasskeyCredential> credentialStore = new ConcurrentHashMap<>(); + // TODO: Replace with database implementation before deploying to production + private final ConcurrentHashMap<String, PasskeyCredential> credentialStore = new ConcurrentHashMap<>();
28-29: The rawId field is not being usedThe
rawIdis extracted but never used. Either use it or remove it to avoid confusion.- String rawId = json.get("rawId").asText(); String publicKey = json.get("response").toString();
46-48: Add null handling to getCredentialById methodThe method should handle the case when a credential ID doesn't exist in the store.
public PasskeyCredential getCredentialById(String id) { - return credentialStore.get(id); + PasskeyCredential credential = credentialStore.get(id); + if (credential == null) { + logger.debug("No credential found for ID: {}", id); + } + return credential; }src/main/resources/templates/passkey/passkey-demo.html (2)
33-44: Improve error handling and user feedbackThe current implementation lacks detailed error messages and user feedback during the registration process.
try { + const statusEl = document.getElementById("statusMessage"); + statusEl.innerText = "Registering passkey..."; const credential = await navigator.credentials.create({ publicKey: publicKeyOptions }); const json = JSON.stringify(credential, replacer); const res = await fetch("/passkey/register", { method: "POST", headers: { "Content-Type": "application/json" }, body: json }); - document.getElementById("statusMessage").innerText = await res.text(); + if (!res.ok) { + throw new Error("Server error: " + await res.text()); + } + + const result = await res.text(); + statusEl.innerHTML = `✅ Success! <br>${result}`;
51-57: Add helper function for ArrayBuffer conversionTo complement the
replacerfunction, add a helper function to convert base64 strings back to ArrayBuffers.Add this function to support bidirectional conversion:
// Convert base64 to ArrayBuffer for WebAuthn function base64ToArrayBuffer(base64String) { const binaryString = atob(base64String); const len = binaryString.length; const bytes = new Uint8Array(len); for (let i = 0; i < len; i++) { bytes[i] = binaryString.charCodeAt(i); } return bytes.buffer; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pom.xml(1 hunks)src/main/java/org/example/springboot25/controller/PasskeyController.java(1 hunks)src/main/java/org/example/springboot25/controller/PasskeyViewController.java(1 hunks)src/main/java/org/example/springboot25/entities/PasskeyCredential.java(1 hunks)src/main/java/org/example/springboot25/service/PasskeyService.java(1 hunks)src/main/resources/templates/passkey/passkey-demo.html(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/org/example/springboot25/service/PasskeyService.java (1)
src/main/java/org/example/springboot25/entities/PasskeyCredential.java (1)
PasskeyCredential(3-43)
🔇 Additional comments (2)
pom.xml (1)
53-57:Details
✅ Verification successful
Added webauthn-server-core dependency for passkey functionality.
The Yubico WebAuthn library is a good choice for implementing passkey/WebAuthn functionality. This will provide the necessary server-side components for handling WebAuthn registration and authentication flows.
🏁 Script executed:
#!/bin/bash # Check for newer versions of the webauthn-server-core library echo "Checking for the latest version of webauthn-server-core..." curl -s "https://search.maven.org/solrsearch/select?q=g:com.yubico+AND+a:webauthn-server-core&rows=20&wt=json" | jq -r '.response.docs[] | "\(.v) (\(.timestamp)"' | sort -V echo "Checking for known vulnerabilities..." curl -s "https://ossindex.sonatype.org/api/v3/component-report/pkg:maven/com.yubico/webauthn-server-core@2.5.0" | jqLength of output: 809
#!/bin/bash echo "Determining the latest available version of com.yubico:web-authn-server-core..." curl -s "https://search.maven.org/solrsearch/select?q=g:com.yubico+AND+a:webauthn-server-core&rows=20&wt=json" \ | jq -r '.response.docs[].latestVersion' \ | sort -V \ | tail -n1 \ | xargs -I{} echo "Latest version: {}"
Approve:
webauthn-server-coredependency is secure and up‑to‑dateFile:
pom.xml(lines 53–57)<dependency> <groupId>com.yubico</groupId> <artifactId>webauthn-server-core</artifactId> <version>2.5.0</version> </dependency>• Vulnerability scan (OSS Index) shows no known issues for version 2.5.0
• 2.5.0 appears to be the latest stable release available on Maven Central
• This addition provides the necessary server‑side components for WebAuthn/passkey flowssrc/main/java/org/example/springboot25/controller/PasskeyViewController.java (1)
7-14:Details
❌ Incorrect review comment
Controller setup follows Spring conventions but lacks security controls.
The
PasskeyViewControllercorrectly implements a Spring MVC controller serving a view template. The endpoint naming and return value are appropriate for serving a Thymeleaf template.Consider adding security constraints to this controller, as passkey registration should typically be a protected operation:
@Controller @RequestMapping("/passkey") +@PreAuthorize("isAuthenticated()") // Or another appropriate security expression public class PasskeyViewController { // ... }
🏁 Script executed:
#!/bin/bash # Check if Spring Security is properly configured in the application echo "Checking for WebSecurityConfigurerAdapter or SecurityFilterChain configuration..." rg -A 10 "SecurityFilterChain|WebSecurityConfigurerAdapter" --type java echo "\nChecking security configuration for /passkey endpoints..." rg -A 15 "antMatchers\(.*passkey.*\)|requestMatchers\(.*passkey.*\)" --type java echo "\nChecking for any PreAuthorize annotations on controllers..." rg -A 3 "@PreAuthorize" --type javaLength of output: 25408
SecurityConfig already enforces authentication for
/passkeyendpointsYour
SecurityConfigis annotated with@EnableMethodSecurityand configures:.authorizeHttpRequests(auth -> auth .requestMatchers("/", "/about", "/register", "/css/**", "/js/**", "/images/**", "/error/**").permitAll() .anyRequest().authenticated() )Since
/passkey/demoisn’t in thepermitAll()list, it already requires an authenticated user. Adding@PreAuthorize("isAuthenticated()")to the controller would be functionally redundant. If you later need finer‑grained role checks, method‑level annotations are available.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/org/example/springboot25/entities/PasskeyCredential.java (1)
3-15: Entity looks properly structured with JPA annotations.The entity has been properly annotated with
@Entity,@Idfor the primary key (credentialId), and@Column(columnDefinition = "TEXT")for the publicKey to accommodate potentially large text values.
🧹 Nitpick comments (5)
src/main/java/org/example/springboot25/entities/PasskeyCredential.java (5)
7-15: Consider adding validation and a default constructor.While the JPA annotations are in place, consider adding:
- Field validations (e.g.,
@NotNullfor required fields)- An explicit default constructor for JPA compliance
- A
@Tableannotation to explicitly define the table name@Entity +@Table(name = "passkey_credentials") public class PasskeyCredential { + // Default constructor required by JPA + public PasskeyCredential() {} + + @NotNull private String userName; @Id + @NotNull private String credentialId; @Column(columnDefinition = "TEXT") + @NotNull private String publicKey; private long signatureCount;
8-33: Add equals, hashCode, and toString methods.For proper entity behavior, especially when used in collections or debugging, implement these standard methods.
@Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; PasskeyCredential that = (PasskeyCredential) o; return Objects.equals(credentialId, that.credentialId); } @Override public int hashCode() { return Objects.hash(credentialId); } @Override public String toString() { return "PasskeyCredential{" + "userName='" + userName + '\'' + ", credentialId='" + credentialId + '\'' + ", signatureCount=" + signatureCount + '}'; }Don't forget to add the import:
import java.util.Objects;
8-16: Consider adding audit fields and indexes.For security and operational purposes, tracking when credentials were created or modified is valuable. Also, consider adding an index on userName if you'll be querying by it.
@Entity @Table(name = "passkey_credentials") +@EntityListeners(AuditingEntityListener.class) public class PasskeyCredential { @NotNull + @Index private String userName; @Id @NotNull private String credentialId; @Column(columnDefinition = "TEXT") @NotNull private String publicKey; private long signatureCount; + + @CreatedDate + private Instant createdAt; + + @LastModifiedDate + private Instant updatedAt;Don't forget to add the imports:
import org.springframework.data.annotation.CreatedDate; import org.springframework.data.annotation.LastModifiedDate; import org.springframework.data.jpa.domain.support.AuditingEntityListener; import java.time.Instant; import org.hibernate.annotations.Index;You'll also need to enable JPA auditing in your configuration:
@EnableJpaAuditing
1-50: Add documentation to explain the entity's purpose.Since this is security-related code, it's important to document the entity and its fields with Javadoc comments.
package org.example.springboot25.entities; import jakarta.persistence.Entity; import jakarta.persistence.Id; import jakarta.persistence.Column; +/** + * Entity representing a WebAuthn passkey credential stored in the system. + * This entity maps to a user's registered authenticator and contains + * the necessary information to verify future authentications. + */ @Entity public class PasskeyCredential { + /** + * The username associated with this credential. + */ private String userName; + + /** + * The unique identifier for this credential, serves as the primary key. + * This is the base64url-encoded credential ID from the authenticator. + */ @Id private String credentialId; + + /** + * The public key used to verify signatures from the authenticator. + * Stored as TEXT to accommodate potentially large key data. + */ @Column(columnDefinition = "TEXT") private String publicKey; + + /** + * A counter maintained by the authenticator that increases with each use. + * Used to detect cloned authenticators and prevent replay attacks. + */ private long signatureCount;
1-50: Consider implementing Serializable.JPA entities often implement Serializable to support various persistence and caching mechanisms.
package org.example.springboot25.entities; import jakarta.persistence.Entity; import jakarta.persistence.Id; import jakarta.persistence.Column; +import java.io.Serializable; @Entity -public class PasskeyCredential { +public class PasskeyCredential implements Serializable { + + private static final long serialVersionUID = 1L;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/example/springboot25/controller/PasskeyController.java(1 hunks)src/main/java/org/example/springboot25/entities/PasskeyCredential.java(1 hunks)src/main/java/org/example/springboot25/service/PasskeyService.java(1 hunks)src/main/resources/templates/passkey/passkey-demo.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/resources/templates/passkey/passkey-demo.html
- src/main/java/org/example/springboot25/controller/PasskeyController.java
- src/main/java/org/example/springboot25/service/PasskeyService.java
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/main/java/org/example/springboot25/entities/PasskeyCredential.java (3)
61-63: Consider adding a constructor with required fields.While the no-arg constructor is required for JPA, adding a constructor that accepts required fields (userName, credentialId, publicKey) would improve usability when creating new instances programmatically.
// Default constructor required by JPA public PasskeyCredential() { } +/** + * Creates a new PasskeyCredential with required fields. + * + * @param userName the username associated with this credential + * @param credentialId the unique identifier for this credential + * @param publicKey the public key used to verify signatures + * @param signatureCount the initial signature count + */ +public PasskeyCredential(String userName, String credentialId, String publicKey, long signatureCount) { + this.userName = userName; + this.credentialId = credentialId; + this.publicKey = publicKey; + this.signatureCount = signatureCount; +}
44-46: Consider security aspects of storing public key data.While storing the public key as TEXT is appropriate, ensure proper handling of this potentially sensitive cryptographic material in memory and in logs. You've correctly excluded it from toString() method.
107-111: Minor: Use conventional parameter naming in equals method.Consider using a more conventional parameter name in the equals method. While
thatworks,otherorpasskeyCredentialwould be more aligned with common Java conventions.@Override public boolean equals(Object o) { if (this == o) return true; - if (!(o instanceof PasskeyCredential that)) return false; - return Objects.equals(credentialId, that.credentialId); + if (!(o instanceof PasskeyCredential other)) return false; + return Objects.equals(credentialId, other.credentialId); }src/main/resources/templates/index.html (2)
56-59: Remove development artifacts and improve comment clarity.The Swedish comment
<!-- Eva testar en grej -->and the class namelogin-test-buttonssuggest this was a quick experiment. Please remove or replace with a descriptive comment, and rename the container class to something likelogin-buttons-container.
70-100: Refactor inline script: externalize, defer loading, and clean up.The inline
<script>for passkey login should be moved to a dedicated JS file (e.g.,passkey-login.js) and loaded withdefer. This improves maintainability and keeps the template clean. Also:
- Use
th:inline="javascript"to inject endpoint URLs and CSRF tokens.- Add UI feedback (disable button and show spinner during requests).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/org/example/springboot25/controller/PasskeyController.java(1 hunks)src/main/java/org/example/springboot25/entities/PasskeyCredential.java(1 hunks)src/main/resources/application.properties(0 hunks)src/main/resources/db/migration/V7__create_passkey_credentials.sql(1 hunks)src/main/resources/templates/index.html(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/resources/application.properties
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/db/migration/V7__create_passkey_credentials.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/springboot25/controller/PasskeyController.java
🔇 Additional comments (1)
src/main/java/org/example/springboot25/entities/PasskeyCredential.java (1)
1-126: Well-structured entity class with proper persistence annotations.This is a well-designed entity class that follows JPA best practices with appropriate annotations, documentation, and implementation of required methods. Great job addressing the previous review comment about persistence annotations.
| <div class="login-test-buttons" | ||
| style="margin-top: 20px; display: flex; flex-direction: column; gap: 10px; align-items: center;"> | ||
|
|
||
| <button class="login-button google-login" | ||
| onclick="location.href='/oauth2/authorization/google'"> | ||
| Login with Google | ||
| </button> | ||
|
|
||
| <button id="passkey-login-btn" class="login-button passkey-login"> | ||
| Login with Passkey | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure login buttons are only shown to unauthenticated users and use Thymeleaf i18n & URL resolution.
The login buttons should be wrapped in sec:authorize="!isAuthenticated()" (or th:if) to avoid showing them to signed‑in users. Also, avoid hardcoded paths and inline styles:
- Wrap the
<div>insec:authorize="!isAuthenticated()". - Replace inline CSS with a class in
main.css. - Use Thymeleaf for URLs: e.g.
<button type="button" th:onclick="'location.href=' + @{/oauth2/authorization/google}">…</button>. - Externalize button text into
messages.propertiesand useth:text="#{login.with.google}"/th:text="#{login.with.passkey}".
| const startResp = await fetch("/webauthn/login/start", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" } | ||
| }); | ||
|
|
||
| const options = await startResp.json(); | ||
| const credential = await navigator.credentials.get({ | ||
| publicKey: options.publicKeyCredentialRequestOptions | ||
| }); | ||
|
|
||
| const finishResp = await fetch("/webauthn/login/finish", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(credential) | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Handle WebAuthn response serialization & CSRF protection.
JSON.stringify(credential)on aPublicKeyCredentialwill fail because it contains ArrayBuffers. You need to Base64‑encodecredential.rawId,authenticatorData, andclientDataJSONbefore sending.- Spring Security’s CSRF protection is enabled on POST by default. Include the CSRF token in your fetch headers (e.g., read from a
<meta name="_csrf" …>tag).
To verify the CSRF meta tags exist in your templates:
Please address these to avoid runtime errors and secure the login flow.
🏁 Script executed:
#!/bin/bash
rg '<meta name="_csrf"' -R src/main/resources/templatesLength of output: 462
I’ll re-run the CSRF tag check with the correct ripgrep syntax:
🏁 Script executed:
#!/bin/bash
# Search recursively for CSRF meta tags in templates
rg '<meta name="_csrf"' -n src/main/resources/templatesLength of output: 57
Ensure WebAuthn credential serialization & CSRF token inclusion
I ran rg '<meta name="_csrf"' -n src/main/resources/templates and found no CSRF meta tags in your HTML. Please:
- Serialize and Base64‑encode all ArrayBuffer fields in the
PublicKeyCredentialbefore callingJSON.stringify. - Add Thymeleaf CSRF meta tags (
_csrfand_csrf_header) to your<head>and read them in JavaScript. - Include the CSRF header in your fetch requests to avoid Spring Security blocking POSTs.
Locations to update in src/main/resources/templates/index.html:
- In the
<head>:
<head>
+ <!-- CSRF tokens for AJAX -->
+ <meta name="_csrf" th:content="${_csrf.token}"/>
+ <meta name="_csrf_header" th:content="${_csrf.headerName}"/>
</head>- In your login script:
- const finishResp = await fetch("/webauthn/login/finish", {
- method: "POST",
- headers: { "Content-Type": "application/json" },
- body: JSON.stringify(credential)
- });
+ // Read CSRF token/header from meta tags
+ const csrfToken = document.querySelector('meta[name="_csrf"]').getAttribute('content');
+ const csrfHeader = document.querySelector('meta[name="_csrf_header"]').getAttribute('content');
+
+ // Helper to Base64‑encode ArrayBuffers
+ function serializePublicKeyCredential(cred) {
+ const encode = buf => btoa(String.fromCharCode(...new Uint8Array(buf)));
+ return {
+ id: cred.id,
+ type: cred.type,
+ rawId: encode(cred.rawId),
+ response: {
+ clientDataJSON: encode(cred.response.clientDataJSON),
+ authenticatorData: encode(cred.response.authenticatorData),
+ signature: encode(cred.response.signature),
+ userHandle: cred.response.userHandle
+ ? encode(cred.response.userHandle)
+ : null
+ }
+ };
+ }
+ const credentialJSON = serializePublicKeyCredential(credential);
+ const finishResp = await fetch("/webauthn/login/finish", {
+ method: "POST",
+ headers: {
+ "Content-Type": "application/json",
+ [csrfHeader]: csrfToken
+ },
+ body: JSON.stringify(credentialJSON)
+ });These changes will prevent runtime serialization errors and ensure your POSTs aren’t rejected by CSRF protection.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/org/example/springboot25/service/PasskeyService.java (2)
13-18: Remove unused import(s) to keep the class clean
java.util.UUIDis imported but never used. Unused imports trigger compiler warnings and can fail CI if-Werroris enabled.-import java.util.UUID;
41-47: Persist and invalidate the registration challenge properly
startRegistrationstores the raw byte[] under the"register_challenge"key but never clears it. If the same session subsequently performs another registration the old challenge remains and may be accepted accidentally by whoever consumes it in the controller.Consider removing the attribute once it is consumed (e.g. inside
finishRegistration) or usingsession.removeAttribute("register_challenge")right after successful validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/example/springboot25/entities/PasskeyCredential.java(1 hunks)src/main/java/org/example/springboot25/repository/PasskeyCredentialRepository.java(1 hunks)src/main/java/org/example/springboot25/service/PasskeyService.java(1 hunks)src/main/resources/application.properties(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/example/springboot25/repository/PasskeyCredentialRepository.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/application.properties
- src/main/java/org/example/springboot25/entities/PasskeyCredential.java
🧰 Additional context used
🪛 GitHub Actions: Java CI with Maven
src/main/java/org/example/springboot25/service/PasskeyService.java
[error] 60-60: Compilation error: cannot find symbol method setUser(User) in PasskeyCredential.
[error] 90-90: Compilation error: cannot find symbol method getUser() in PasskeyCredential.
[error] 93-93: Compilation error: cannot find symbol method getAuthorities() in User.
🔇 Additional comments (1)
src/main/java/org/example/springboot25/service/PasskeyService.java (1)
71-75: Repository query likely uses the wrong parameter type
credentialRepo.findAllByUser(user.getUserName())passes aString, yet method naming implies it expects aUser(entity) parameter. Mismatch will break at runtime or even compile depending on the repository signature.Ensure the repository is declared consistently:
List<PasskeyCredential> findAllByUser(User user);and call it with the entity:
-List<PasskeyCredential> credentials = credentialRepo.findAllByUser(user.getUserName()); +List<PasskeyCredential> credentials = credentialRepo.findAllByUser(user);
| public void finishLogin(String credentialId, byte[] clientDataHash, byte[] authenticatorData, byte[] signature) { | ||
| Optional<PasskeyCredential> optionalCredential = credentialRepo.findByCredentialId(credentialId); | ||
| if (optionalCredential.isEmpty()) { | ||
| throw new RuntimeException("Credential not found"); | ||
| } | ||
| PasskeyCredential credential = optionalCredential.get(); | ||
| User user = credential.getUser(); | ||
|
|
||
| UsernamePasswordAuthenticationToken auth = new UsernamePasswordAuthenticationToken( | ||
| user, null, user.getAuthorities() | ||
| ); | ||
| SecurityContextHolder.getContext().setAuthentication(auth); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Compilation failure & security gap in finishLogin
PasskeyCredential#getUser()is missing (see previous comment).User#getAuthorities()does not exist – the entity is not a Spring‐SecurityUserDetails.
Proposed quick fix (after adding the relation):
-User user = credential.getUser();
-
-UsernamePasswordAuthenticationToken auth = new UsernamePasswordAuthenticationToken(
- user, null, user.getAuthorities()
-);
+User user = credential.getUser();
+
+// Build authorities manually (assumes a roles collection on User)
+List<GrantedAuthority> authorities = user.getRoles().stream()
+ .map(role -> new SimpleGrantedAuthority(role.getName()))
+ .collect(Collectors.toList());
+
+UsernamePasswordAuthenticationToken auth =
+ new UsernamePasswordAuthenticationToken(user.getUserName(), null, authorities);- Critical: The method logs the user in without validating
clientDataHash,authenticatorData,signature, or the stored challenge. This defeats WebAuthn’s entire security model.
At minimum:
• Retrieve and remove "login_challenge" from the session.
• Use a WebAuthn server library (webauthn-server-core) to verify the signature & counters before authenticating.
📝 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.
| public void finishLogin(String credentialId, byte[] clientDataHash, byte[] authenticatorData, byte[] signature) { | |
| Optional<PasskeyCredential> optionalCredential = credentialRepo.findByCredentialId(credentialId); | |
| if (optionalCredential.isEmpty()) { | |
| throw new RuntimeException("Credential not found"); | |
| } | |
| PasskeyCredential credential = optionalCredential.get(); | |
| User user = credential.getUser(); | |
| UsernamePasswordAuthenticationToken auth = new UsernamePasswordAuthenticationToken( | |
| user, null, user.getAuthorities() | |
| ); | |
| SecurityContextHolder.getContext().setAuthentication(auth); | |
| } | |
| public void finishLogin(String credentialId, byte[] clientDataHash, byte[] authenticatorData, byte[] signature) { | |
| Optional<PasskeyCredential> optionalCredential = credentialRepo.findByCredentialId(credentialId); | |
| if (optionalCredential.isEmpty()) { | |
| throw new RuntimeException("Credential not found"); | |
| } | |
| PasskeyCredential credential = optionalCredential.get(); | |
| User user = credential.getUser(); | |
| // Build authorities manually (assumes a roles collection on User) | |
| List<GrantedAuthority> authorities = user.getRoles().stream() | |
| .map(role -> new SimpleGrantedAuthority(role.getName())) | |
| .collect(Collectors.toList()); | |
| UsernamePasswordAuthenticationToken auth = | |
| new UsernamePasswordAuthenticationToken(user.getUserName(), null, authorities); | |
| SecurityContextHolder.getContext().setAuthentication(auth); | |
| } |
🧰 Tools
🪛 GitHub Actions: Java CI with Maven
[error] 90-90: Compilation error: cannot find symbol method getUser() in PasskeyCredential.
[error] 93-93: Compilation error: cannot find symbol method getAuthorities() in User.
| PasskeyCredential credential = new PasskeyCredential(); | ||
| credential.setCredentialId(credentialId); | ||
| credential.setPublicKey(publicKey); | ||
| credential.setUserHandle(userHandle); | ||
| credential.setUser(user); | ||
| credential.setSignatureCount(0); | ||
| credentialRepo.save(credential); | ||
| } |
There was a problem hiding this comment.
Compilation failure – PasskeyCredential lacks a user association
CI reports cannot find symbol method setUser(User) at line 60. Either the entity does not expose such setter/getter or the field is named differently.
If the intention is to persist a Many‑To‑One relation to User, add the mapping to PasskeyCredential:
+@ManyToOne(fetch = FetchType.LAZY)
+@JoinColumn(name = "user_id", nullable = false)
+private User user;
+public User getUser() {
+ return user;
+}
+
+public void setUser(User user) {
+ this.user = user;
+}Afterwards the call in finishRegistration will compile:
-credential.setUser(user);
+credential.setUser(user);🧰 Tools
🪛 GitHub Actions: Java CI with Maven
[error] 60-60: Compilation error: cannot find symbol method setUser(User) in PasskeyCredential.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/main/java/org/example/springboot25/entities/PasskeyCredential.java (5)
133-143: Consider including userHandle in equals/hashCode.While using only the credentialId for equals/hashCode is valid since it's the primary key, the userHandle byte array is not properly handled. If userHandle is important for logical equality, consider including it using Arrays.equals() and Arrays.hashCode().
@Override public boolean equals(Object o) { if (this == o) return true; if (!(o instanceof PasskeyCredential that)) return false; - return Objects.equals(credentialId, that.credentialId); + return Objects.equals(credentialId, that.credentialId) && + Arrays.equals(userHandle, that.userHandle); } @Override public int hashCode() { - return Objects.hash(credentialId); + int result = Objects.hash(credentialId); + result = 31 * result + Arrays.hashCode(userHandle); + return result; }
145-152: Consider enhancing toString() method.The current toString() method excludes several important fields like userHandle, publicKey, and the User relationship. While this might be intentional to avoid verbose output, consider at least adding a reference to the associated user for better debugging capabilities.
@Override public String toString() { return "PasskeyCredential{" + "userName='" + userName + '\'' + ", credentialId='" + credentialId + '\'' + ", signatureCount=" + signatureCount + + ", userId=" + (user != null ? user.getId() : "null") + '}'; }
43-45: Add maximum length validation for userName field.While the field is correctly marked as @NotNull, consider adding a maximum length constraint to prevent potential database issues with overly long values.
@NotNull @Column(nullable = false) +@Size(max = 255) private String userName;
51-53: Add maximum length validation for credentialId field.As the primary key, it's especially important to validate the length of this field to prevent issues with overly long values.
@Id @NotNull +@Size(max = 255) private String credentialId;
69-71: Consider adding an index on the user_id foreign key.Since this is a many-to-one relationship with User, you'll likely query PasskeyCredential entities by user. Adding an index can improve query performance.
@ManyToOne(fetch = FetchType.LAZY) -@JoinColumn(name = "user_id", nullable = false) +@JoinColumn(name = "user_id", nullable = false) +@Index(name = "idx_passkey_user") private User user;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/example/springboot25/entities/PasskeyCredential.java(1 hunks)src/main/java/org/example/springboot25/service/PasskeyService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/springboot25/service/PasskeyService.java
🔇 Additional comments (2)
src/main/java/org/example/springboot25/entities/PasskeyCredential.java (2)
1-153: Well-implemented JPA entity with good structure and documentation.The PasskeyCredential entity is well-structured with appropriate JPA annotations, relationship mapping, and field documentation. The inclusion of auditing support, proper Serializable implementation, and a clean toString method are all positive aspects.
1-153: Impressive implementation of WebAuthn credential storage entityThe PasskeyCredential entity is very well-designed for WebAuthn integration. The proper use of JPA annotations, validation constraints, relationship mapping, and documentation demonstrates a good understanding of both JPA and WebAuthn concepts. The inclusion of the signatureCount field for security against replay attacks and the proper handling of the binary userHandle with @lob are particularly noteworthy.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/org/example/springboot25/controller/WebAuthnLoginController.java (1)
1-43: Consider additional security enhancements.While the basic passkey implementation is in place, consider these security improvements:
- Validate the email format in the startLogin method
- Add rate limiting to prevent brute force attacks
- Add CSRF protection for these endpoints
- Consider returning authentication tokens instead of just success messages
- Use more specific types for ResponseEntity instead of wildcards
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/example/springboot25/controller/WebAuthnLoginController.java(1 hunks)src/main/java/org/example/springboot25/dto/PasskeyLoginDTO.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/example/springboot25/dto/PasskeyLoginDTO.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/org/example/springboot25/controller/WebAuthnLoginController.java (1)
src/main/java/org/example/springboot25/dto/PasskeyLoginDTO.java (1)
PasskeyLoginDTO(3-40)
🔇 Additional comments (1)
src/main/java/org/example/springboot25/controller/WebAuthnLoginController.java (1)
10-19: Good controller structure with proper dependency injection.The controller is well-structured with appropriate annotations and follows best practices for dependency injection using constructor injection with a final field.
| @PostMapping("/finish") | ||
| public ResponseEntity<?> finishLogin(@RequestBody PasskeyLoginDTO loginDTO) { | ||
| passkeyService.finishLogin( | ||
| loginDTO.getCredentialId(), | ||
| loginDTO.getClientDataHash(), | ||
| loginDTO.getAuthenticatorData(), | ||
| loginDTO.getSignature() | ||
| ); | ||
| return ResponseEntity.ok("Login successful"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper error handling and structured response.
The finishLogin method lacks error handling and returns a simple string response. Consider handling potential exceptions (like credential validation failures) and return a structured JSON response with authentication details.
@PostMapping("/finish")
- public ResponseEntity<?> finishLogin(@RequestBody PasskeyLoginDTO loginDTO) {
+ public ResponseEntity<Map<String, Object>> finishLogin(@RequestBody PasskeyLoginDTO loginDTO) {
+ try {
passkeyService.finishLogin(
loginDTO.getCredentialId(),
loginDTO.getClientDataHash(),
loginDTO.getAuthenticatorData(),
loginDTO.getSignature()
);
- return ResponseEntity.ok("Login successful");
+ return ResponseEntity.ok(Map.of(
+ "status", "success",
+ "message", "Login successful"
+ ));
+ } catch (Exception e) {
+ return ResponseEntity.badRequest().body(Map.of(
+ "status", "error",
+ "message", e.getMessage()
+ ));
+ }
}📝 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.
| @PostMapping("/finish") | |
| public ResponseEntity<?> finishLogin(@RequestBody PasskeyLoginDTO loginDTO) { | |
| passkeyService.finishLogin( | |
| loginDTO.getCredentialId(), | |
| loginDTO.getClientDataHash(), | |
| loginDTO.getAuthenticatorData(), | |
| loginDTO.getSignature() | |
| ); | |
| return ResponseEntity.ok("Login successful"); | |
| } | |
| @PostMapping("/finish") | |
| public ResponseEntity<Map<String, Object>> finishLogin(@RequestBody PasskeyLoginDTO loginDTO) { | |
| try { | |
| passkeyService.finishLogin( | |
| loginDTO.getCredentialId(), | |
| loginDTO.getClientDataHash(), | |
| loginDTO.getAuthenticatorData(), | |
| loginDTO.getSignature() | |
| ); | |
| return ResponseEntity.ok(Map.of( | |
| "status", "success", | |
| "message", "Login successful" | |
| )); | |
| } catch (Exception e) { | |
| return ResponseEntity.badRequest().body(Map.of( | |
| "status", "error", | |
| "message", e.getMessage() | |
| )); | |
| } | |
| } |
| @PostMapping("/start") | ||
| public ResponseEntity<?> startLogin(@RequestBody String email) { | ||
| String challenge = passkeyService.startLogin(email); | ||
|
|
||
| return ResponseEntity.ok( | ||
| Map.of("publicKeyCredentialRequestOptions", Map.of( | ||
| "challenge", challenge, | ||
| "rpId", "localhost", | ||
| "userVerification", "preferred" | ||
| )) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoded "localhost" will break in production environments.
The Relying Party ID is hardcoded to "localhost", which will only work in development environments. This value should be configurable through application properties to support different deployment environments.
+ @Value("${webauthn.rpId}")
+ private String rpId;
@PostMapping("/start")
public ResponseEntity<?> startLogin(@RequestBody String email) {
String challenge = passkeyService.startLogin(email);
return ResponseEntity.ok(
Map.of("publicKeyCredentialRequestOptions", Map.of(
"challenge", challenge,
- "rpId", "localhost",
+ "rpId", rpId,
"userVerification", "preferred"
))
);
}Also consider adding validation for the email input and specific error handling.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/main/java/org/example/springboot25/controller/PasskeyController.java (3)
61-65: Remove testing comments and translate non-English comments.The code contains testing-related comments that should be removed before production deployment. Additionally, comments should be in English for better maintainability and consistency.
- // --- Eva testar passkey login flow --- - @GetMapping("/webauthn/login/start") -@PreAuthorize("permitAll()") // 👈 Viktigt: alla ska kunna initiera login +@PreAuthorize("permitAll()") // Important: this endpoint must be accessible to unauthenticated users for login initiation
77-80: Consider adding expiration for login challenges.The current implementation stores the challenge in the session but doesn't set an expiration. WebAuthn challenges should have a short lifetime to prevent replay attacks.
byte[] challenge = new byte[32]; new SecureRandom().nextBytes(challenge); session.setAttribute("login_challenge", challenge); +session.setAttribute("login_challenge_timestamp", System.currentTimeMillis());Then in the finish method:
// In finishPasskeyLogin method Long challengeTimestamp = (Long) session.getAttribute("login_challenge_timestamp"); if (challengeTimestamp == null || (System.currentTimeMillis() - challengeTimestamp) > 300000) { // 5 minutes expiration throw new BadRequestException("Login challenge expired or invalid"); }
121-122: Remove testing comments.Remove the test-related comments at the end of the file.
- // --- Slut på Eva testar passkey login flow --- }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/example/springboot25/controller/PasskeyController.java(1 hunks)src/main/resources/templates/index.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/templates/index.html
| @GetMapping("/webauthn/login/start") | ||
| @PreAuthorize("permitAll()") // 👈 Viktigt: alla ska kunna initiera login | ||
| public ResponseEntity<Map<String, Object>> startPasskeyLogin(@RequestParam String userEmail) { | ||
| Optional<User> userOpt = userRepo.findByUserEmail(userEmail); | ||
| if (userOpt.isEmpty()) { | ||
| return ResponseEntity.status(HttpStatus.NOT_FOUND).body(Map.of("error", "User not found")); | ||
| } | ||
|
|
||
| User user = userOpt.get(); | ||
| List<PasskeyCredential> creds = credentialRepo.findAllByUser(user); | ||
| if (creds.isEmpty()) { | ||
| return ResponseEntity.status(HttpStatus.NOT_FOUND).body(Map.of("error", "No credentials found")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add rate limiting for login endpoints.
The login endpoints don't have any rate limiting, which could make them vulnerable to brute force attacks. Consider implementing rate limiting for these public endpoints.
You could implement rate limiting using Spring's @RateLimiter annotation or a custom solution:
// Example using bucket4j with Spring Boot
@GetMapping("/webauthn/login/start")
@PreAuthorize("permitAll()")
public ResponseEntity<Map<String, Object>> startPasskeyLogin(@RequestParam String userEmail) {
// Check rate limit first
String ipAddress = request.getRemoteAddr();
if (!rateLimiterService.tryConsume(ipAddress)) {
return ResponseEntity.status(HttpStatus.TOO_MANY_REQUESTS)
.body(Map.of("error", "Too many login attempts. Please try again later."));
}
// Existing login flow code...
}This would require implementing a RateLimiterService using a library like bucket4j, or Spring's built-in rate limiting capabilities.
| "rpId", "localhost", // eller din domän i deployment | ||
| "allowCredentials", allowCredentials, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make rpId configurable for different environments.
The "rpId" parameter is hardcoded to "localhost" with a comment in Swedish. This should be made configurable through application properties to support different environments.
- "rpId", "localhost", // eller din domän i deployment
+ "rpId", passkeyService.getRelyingPartyId(), // Use configured domain for the current environmentAdd a method to retrieve this from configuration in the PasskeyService:
// In PasskeyService.java
@Value("${webauthn.rpId:localhost}")
private String rpId;
public String getRelyingPartyId() {
return rpId;
}| // Enligt WebAuthn ska clientDataHash = SHA256(clientDataJSON) | ||
| byte[] clientDataHash = java.security.MessageDigest.getInstance("SHA-256").digest(clientDataJSON); | ||
|
|
||
| passkeyService.finishLogin(credentialId, clientDataHash, authenticatorData, signature); | ||
| return ResponseEntity.ok("Login successful"); |
There was a problem hiding this comment.
Verify session challenge during login.
The code calculates a client data hash but doesn't verify if the challenge in the session exists or matches the challenge from the client. This is a critical security step in the WebAuthn authentication flow.
// Enligt WebAuthn ska clientDataHash = SHA256(clientDataJSON)
byte[] clientDataHash = java.security.MessageDigest.getInstance("SHA-256").digest(clientDataJSON);
+ // Retrieve and validate the challenge from the session
+ byte[] sessionChallenge = (byte[]) session.getAttribute("login_challenge");
+ if (sessionChallenge == null) {
+ throw new BadRequestException("No active login challenge found");
+ }
+ // Clear the challenge from the session to prevent replay attacks
+ session.removeAttribute("login_challenge");
passkeyService.finishLogin(credentialId, clientDataHash, authenticatorData, signature);📝 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.
| // Enligt WebAuthn ska clientDataHash = SHA256(clientDataJSON) | |
| byte[] clientDataHash = java.security.MessageDigest.getInstance("SHA-256").digest(clientDataJSON); | |
| passkeyService.finishLogin(credentialId, clientDataHash, authenticatorData, signature); | |
| return ResponseEntity.ok("Login successful"); | |
| // Enligt WebAuthn ska clientDataHash = SHA256(clientDataJSON) | |
| byte[] clientDataHash = java.security.MessageDigest | |
| .getInstance("SHA-256") | |
| .digest(clientDataJSON); | |
| // Retrieve and validate the challenge from the session | |
| byte[] sessionChallenge = (byte[]) session.getAttribute("login_challenge"); | |
| if (sessionChallenge == null) { | |
| throw new BadRequestException("No active login challenge found"); | |
| } | |
| // Clear the challenge from the session to prevent replay attacks | |
| session.removeAttribute("login_challenge"); | |
| passkeyService.finishLogin( | |
| credentialId, | |
| clientDataHash, | |
| authenticatorData, | |
| signature | |
| ); | |
| return ResponseEntity.ok("Login successful"); |
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Login failed: " + e.getMessage()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling for production code.
Using e.printStackTrace() is not ideal for production code as it writes to standard error output. Consider using a logger instead for better observability and control over log levels.
+ import org.slf4j.Logger;
+ import org.slf4j.LoggerFactory;
public class PasskeyController {
+ private static final Logger logger = LoggerFactory.getLogger(PasskeyController.class);
// ... existing code
@PostMapping("/webauthn/login/finish")
@PreAuthorize("permitAll()")
public ResponseEntity<String> finishPasskeyLogin(@RequestBody Map<String, Object> credentialJson) {
try {
// ... existing code
} catch (Exception e) {
- e.printStackTrace();
+ logger.error("WebAuthn login failed", e);
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Login failed: " + e.getMessage());
}
}📝 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.
| } catch (Exception e) { | |
| e.printStackTrace(); | |
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body("Login failed: " + e.getMessage()); | |
| // add at top of the file alongside your other imports | |
| import org.slf4j.Logger; | |
| import org.slf4j.LoggerFactory; | |
| @RestController | |
| @RequestMapping("/passkey") | |
| public class PasskeyController { | |
| // add logger declaration | |
| private static final Logger logger = LoggerFactory.getLogger(PasskeyController.class); | |
| // ... existing code ... | |
| @PostMapping("/webauthn/login/finish") | |
| @PreAuthorize("permitAll()") | |
| public ResponseEntity<String> finishPasskeyLogin(@RequestBody Map<String, Object> credentialJson) { | |
| try { | |
| // ... existing code ... | |
| } catch (Exception e) { | |
| - e.printStackTrace(); | |
| + logger.error("WebAuthn login failed", e); | |
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED) | |
| .body("Login failed: " + e.getMessage()); | |
| } | |
| } | |
| } |
# Conflicts: # src/main/java/org/example/springboot25/exceptions/GlobalExceptionHandler.java
Summary by CodeRabbit