Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds Spring Security and WebAuthn: new security dependencies, SecurityFilterChain and JDBC repositories, WebAuthn JPA entities and migrations, login/signup controllers exposing CSRF, signup persisting users and creating an authenticated session; legacy AppUser and original V1 app_user creation were commented out. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Signup as SignupController
participant Repo as PublicKeyCredentialUserEntityRepository
participant DB as Database
participant SessionStore as HttpSessionSecurityContextRepository
Client->>Signup: POST /signup { username, displayName }
Signup->>Signup: validate request, check duplicates
Signup->>Repo: save new UserEntity(id, name, displayName)
Repo->>DB: INSERT app_user
DB-->>Repo: insert result
Signup->>SessionStore: create SecurityContext with UsernamePasswordAuthenticationToken
SessionStore-->>Client: session cookie set
Signup-->>Client: 200 OK (authenticated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/main/java/backendlab/team4you/webauthn/UserEntity.java (1)
19-21: Consider adding setters fornameanddisplayNamefor completeness.While JPA will use field access (based on
@Idplacement on the field), adding setters would allow programmatic updates to these fields if needed in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/webauthn/UserEntity.java` around lines 19 - 21, Add public setter methods setName(String name) and setDisplayName(String displayName) to the UserEntity class so the private fields name and displayName can be updated programmatically; implement them to assign the incoming parameter to the corresponding field (this.name = name, this.displayName = displayName) and keep existing getters and JPA field access unchanged.src/main/java/backendlab/team4you/webauthn/SignupController.java (1)
60-61: InjectSecurityContextRepositoryinstead of instantiating per request.Creating a new
HttpSessionSecurityContextRepositoryon each request is inefficient. Inject it as a dependency for better resource usage and testability.♻️ Proposed refactor
`@Controller` public class SignupController { private final PublicKeyCredentialUserEntityRepository users; + private final SecurityContextRepository securityContextRepository; private final SecureRandom random = new SecureRandom(); - public SignupController(PublicKeyCredentialUserEntityRepository users) { + public SignupController(PublicKeyCredentialUserEntityRepository users, + SecurityContextRepository securityContextRepository) { this.users = users; + this.securityContextRepository = securityContextRepository; } // ... `@PostMapping`("/signup") `@ResponseBody` public void signup(`@RequestBody` SignupRequest req, HttpServletRequest request, HttpServletResponse response) { // ... SecurityContext context = SecurityContextHolder.createEmptyContext(); context.setAuthentication(auth); - SecurityContextRepository repo = new HttpSessionSecurityContextRepository(); - repo.saveContext(context, request, response); + securityContextRepository.saveContext(context, request, response); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/webauthn/SignupController.java` around lines 60 - 61, The SignupController currently instantiates a new HttpSessionSecurityContextRepository on each request (lines using SecurityContextRepository repo = new HttpSessionSecurityContextRepository() and repo.saveContext(...)); change this to inject a SecurityContextRepository instance into SignupController (constructor or field injection, e.g., a private final SecurityContextRepository securityContextRepository) and replace the per-request instantiation with securityContextRepository.saveContext(context, request, response) so the repository is reused and testable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/backendlab/team4you/webauthn/LoginController.java`:
- Around line 8-15: The project lacks a SecurityFilterChain bean so Spring
Security blocks unauthenticated access to /login; add a configuration class
(e.g., SecurityConfig) annotated with `@Configuration` and `@EnableWebSecurity` that
defines a `@Bean` method (e.g., filterChain(HttpSecurity http)) which configures
HttpSecurity to permitAll for "/login" and "/signup", require authentication for
other requests, and set the form login page to "/login" (returning a built
SecurityFilterChain). This will allow LoginController.login and the login page
to be reachable without prior authentication.
In `@src/main/java/backendlab/team4you/webauthn/SignupController.java`:
- Around line 40-62: In signup(`@RequestBody` SignupRequest req,
HttpServletRequest request, HttpServletResponse response) validate and sanitize
req.username (trim and ensure non-null/non-empty) and req.displayName before
creating UserEntity, then check the users repository for an existing username
(e.g., users.findByUsername(req.username)) and if found return an appropriate
error response (400 for bad input or 409 for duplicate) instead of saving; only
create and save the new UserEntity and proceed with authentication when
validation passes. Ensure the repository has a findByUsername method, and
respond early on validation/duplicate failure with a clear status and message
rather than attempting to save.
In `@src/main/java/backendlab/team4you/webauthn/UserEntity.java`:
- Around line 11-13: Add a Flyway migration that creates the user_entities table
to match the JPA mapping in UserEntity (mapped to "user_entities") so schema
validation passes with spring.jpa.hibernate.ddl-auto=validate; update or add a
new migration (e.g., V2__create_user_entities.sql) to define columns and
constraints that correspond to fields in the UserEntity class and any referenced
PK/FK types so Hibernate's validation matches the actual DB schema.
In `@src/main/java/backendlab/team4you/webauthn/WebAuthnCredential.java`:
- Around line 106-140: The current primitive-returning getters in
WebAuthnCredential (getSignatureCount, isUvInitialized, isBackupEligible,
isBackupState) will NPE when their nullable fields (signatureCount: Long,
uvInitialized/backupEligible/backupState: Boolean) are null; update each getter
to return null-safe defaults instead of directly unboxing (e.g.,
getSignatureCount should return signatureCount != null ? signatureCount : 0L,
and the boolean getters should use Boolean.TRUE.equals(field) or field != null ?
field : false) so no NullPointerException occurs when DB fields are null.
- Around line 16-18: The WebAuthnCredential entity maps to table
user_credentials but no Flyway migration exists, so add a new Flyway migration
(e.g., V__ or next version like V2__create_user_credentials.sql) that creates
the user_credentials table with columns and types matching the
WebAuthnCredential entity fields (primary key, foreign keys, unique constraints,
timestamps, and binary/blobs where applicable for credential data), ensure the
migration filename follows your Flyway naming convention and is committed so it
runs at startup, and remove reliance on Hibernate to create/validate the table
(keep spring.jpa.hibernate.ddl-auto=validate).
- Around line 88-98: The code serializes transports with
AuthenticatorTransport.getValue() in setTransports but deserializes using
AuthenticatorTransport.valueOf(...) in getTransports, causing
IllegalArgumentException; fix by making serialization/deserialization
consistent: either change setTransports to store enum names via
AuthenticatorTransport.name(), or change getTransports to map each token to the
enum by matching token to AuthenticatorTransport.getValue() (e.g., a lookup
method on AuthenticatorTransport or stream/filter/findFirst) and handle unknown
values gracefully.
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/webauthn/SignupController.java`:
- Around line 60-61: The SignupController currently instantiates a new
HttpSessionSecurityContextRepository on each request (lines using
SecurityContextRepository repo = new HttpSessionSecurityContextRepository() and
repo.saveContext(...)); change this to inject a SecurityContextRepository
instance into SignupController (constructor or field injection, e.g., a private
final SecurityContextRepository securityContextRepository) and replace the
per-request instantiation with securityContextRepository.saveContext(context,
request, response) so the repository is reused and testable.
In `@src/main/java/backendlab/team4you/webauthn/UserEntity.java`:
- Around line 19-21: Add public setter methods setName(String name) and
setDisplayName(String displayName) to the UserEntity class so the private fields
name and displayName can be updated programmatically; implement them to assign
the incoming parameter to the corresponding field (this.name = name,
this.displayName = displayName) and keep existing getters and JPA field access
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7edf778-dd23-4ab1-a12a-1e0324c98952
📒 Files selected for processing (5)
pom.xmlsrc/main/java/backendlab/team4you/webauthn/LoginController.javasrc/main/java/backendlab/team4you/webauthn/SignupController.javasrc/main/java/backendlab/team4you/webauthn/UserEntity.javasrc/main/java/backendlab/team4you/webauthn/WebAuthnCredential.java
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/backendlab/team4you/config/SecurityConfig.java (2)
31-35: Externalize WebAuthn configuration for production readiness.The
rpIdandallowedOriginsare hardcoded tolocalhost, which will only work in development. These values should be externalized to application properties to support different environments.♻️ Suggested refactor using `@Value` injection
+import org.springframework.beans.factory.annotation.Value; + `@Configuration` public class SecurityConfig { + + `@Value`("${webauthn.rp-id}") + private String rpId; + + `@Value`("${webauthn.allowed-origins}") + private String allowedOrigins; `@Bean` SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { return http // ... .webAuthn(passkeys -> passkeys - .rpId("localhost") - .allowedOrigins("http://localhost:8080") + .rpId(rpId) + .allowedOrigins(allowedOrigins) .rpName("Passkey team4you") )Then add to
application.propertiesorapplication.yml:webauthn.rp-id=localhost webauthn.allowed-origins=http://localhost:8080🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/config/SecurityConfig.java` around lines 31 - 35, Externalize the hardcoded WebAuthn settings in SecurityConfig by adding properties (e.g. webauthn.rp-id and webauthn.allowed-origins) to application.properties/yml and injecting them into the SecurityConfig class (via `@Value` or a `@ConfigurationProperties` bean). Replace the literal strings used in the webAuthn(passkeys -> passkeys.rpId(...).allowedOrigins(...)) call with the injected fields (e.g. rpId and allowedOrigins) so the rpId and allowedOrigins values are read from configuration for different environments.
41-41: TODO: Passkey management view.This TODO indicates a pending passkey management page implementation. The typical flow requires a page where authenticated users can register additional passkeys.
Would you like me to help scaffold a Thymeleaf template for passkey management, or open an issue to track this task?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/config/SecurityConfig.java` at line 41, SecurityConfig.java contains a TODO for a "passkey management" view; implement this by adding a secured endpoint and Thymeleaf template: create a PasskeyController with a GET mapping (e.g., "/passkeys") that requires authentication and returns a new "passkeys.html" view, add the "passkeys.html" template under resources/templates with UI to list/register/remove passkeys and JS hooks for WebAuthn actions, update SecurityConfig (class SecurityConfig) to ensure "/passkeys" is accessible only to authenticated users, and add a navigation link to the template layout so authenticated users can reach the passkey management page.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/backendlab/team4you/config/SecurityConfig.java`:
- Around line 43-51: SecurityConfig registers
JdbcPublicKeyCredentialUserEntityRepository and JdbcUserCredentialRepository
which expect database tables that don't exist; update the Flyway migration
V1__init_schema.sql to create user_entities and user_credentials tables with the
schema Spring Security's WebAuthn JDBC repositories expect (add a user_entities
table keyed by username/user_handle and a user_credentials table containing
columns such as username (or user_handle), credential_id (binary), public_key
(binary or text), signature_count (integer), attestation/metadata fields,
transports, and any timestamps), ensuring column types match how
JdbcPublicKeyCredentialUserEntityRepository and JdbcUserCredentialRepository
read/write credential_id, public_key and sign count so the repositories in
SecurityConfig function without runtime table-not-found errors.
- Around line 53-60: The current userDetailsService in SecurityConfig returns a
User with an empty "{noop}" password which enables password-based login and
creates an auth bypass; fix by changing the userDetailsService method to not
expose a valid password-backed user — e.g., throw UsernameNotFoundException for
unknown usernames (or return a User with accountLocked/accountDisabled flags
set) so password form login cannot authenticate, and if you truly want
passkey-only authentication also remove/disable form login by disabling
HttpSecurity.formLogin() in your security configuration.
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/config/SecurityConfig.java`:
- Around line 31-35: Externalize the hardcoded WebAuthn settings in
SecurityConfig by adding properties (e.g. webauthn.rp-id and
webauthn.allowed-origins) to application.properties/yml and injecting them into
the SecurityConfig class (via `@Value` or a `@ConfigurationProperties` bean).
Replace the literal strings used in the webAuthn(passkeys ->
passkeys.rpId(...).allowedOrigins(...)) call with the injected fields (e.g. rpId
and allowedOrigins) so the rpId and allowedOrigins values are read from
configuration for different environments.
- Line 41: SecurityConfig.java contains a TODO for a "passkey management" view;
implement this by adding a secured endpoint and Thymeleaf template: create a
PasskeyController with a GET mapping (e.g., "/passkeys") that requires
authentication and returns a new "passkeys.html" view, add the "passkeys.html"
template under resources/templates with UI to list/register/remove passkeys and
JS hooks for WebAuthn actions, update SecurityConfig (class SecurityConfig) to
ensure "/passkeys" is accessible only to authenticated users, and add a
navigation link to the template layout so authenticated users can reach the
passkey management page.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 585ac495-d06c-4ace-9ba9-ecb1b747dcf4
📒 Files selected for processing (3)
src/main/java/backendlab/team4you/config/SecurityConfig.javasrc/main/java/backendlab/team4you/webauthn/SignupController.javasrc/main/java/backendlab/team4you/webauthn/WebAuthnCredential.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/backendlab/team4you/webauthn/SignupController.java
- src/main/java/backendlab/team4you/webauthn/WebAuthnCredential.java
…d WebAuthn V2 for enhanced authentication security
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/backendlab/team4you/user/AppUser.java (1)
1-57: Remove commented-out code instead of keeping it in the codebase.This entire file is commented out. Since
UserEntitynow fulfills the role ofAppUserand there are no references toAppUseranywhere in the codebase, this dead code should be deleted rather than left commented. Version control (git) preserves the history if you ever need to reference it later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/user/AppUser.java` around lines 1 - 57, The AppUser class is entirely commented out and redundant given UserEntity; delete the AppUser.java file to remove dead code, ensuring you also remove any import or build references if present and run a quick project compile to verify no remaining references to AppUser remain; if you want to preserve the old implementation, rely on git history instead of keeping commented code in AppUser.java.src/main/java/backendlab/team4you/webauthn/UserEntity.java (2)
35-40: Constructor silently accepts nullid, which is the primary key.The
@Idfield should not be null for a valid entity. While the current usage inSignupControlleralways provides a non-null ID, silently accepting null could mask bugs. Consider either removing the null check (fail-fast) or adding explicit validation.♻️ Optional: Add validation
public UserEntity(Bytes id, String name, String displayName) { + if (id == null) { + throw new IllegalArgumentException("id must not be null"); + } this.id = id != null ? id.toBase64UrlString() : null; this.name = name; this.displayName = displayName; this.createdAt = LocalDateTime.now(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/webauthn/UserEntity.java` around lines 35 - 40, The UserEntity constructor currently allows a null primary key by doing id != null ? id.toBase64UrlString() : null; change this to fail-fast: in UserEntity(Bytes id, String name, String displayName) validate that id is non-null and throw an IllegalArgumentException (or NullPointerException) with a clear message if it is null, then assign this.id = id.toBase64UrlString(); also ensure any callers (e.g., SignupController) provide a non-null Bytes and adjust tests if needed so the entity invariant is preserved.
29-30: Consider usingOffsetDateTimeorInstantfor timezone-aware timestamps.The
createdAtfield usesLocalDateTime, but the database columncreated_atis defined asTIMESTAMP WITH TIME ZONE.LocalDateTimedoesn't carry timezone information, which can lead to subtle bugs when the application runs across different timezones or during daylight saving time transitions. UsingOffsetDateTimeorInstantprovides better alignment with the database type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/webauthn/UserEntity.java` around lines 29 - 30, The createdAt field in UserEntity is declared as LocalDateTime but the DB column is TIMESTAMP WITH TIME ZONE; change the type to a timezone-aware type (OffsetDateTime or Instant) on the field named createdAt in class UserEntity, update the corresponding import and any getters/setters/constructors/usages to the new type, and ensure JPA mapping remains compatible (update any converters or annotations if you currently use a custom converter) so the entity stores and reads timezone-aware timestamps correctly.src/main/resources/db/migration/V2__cleanup_and_webauthn.sql (1)
9-25: Consider adding an index on the foreign key column for query performance.The
user_entity_user_idcolumn is a foreign key but doesn't have an explicit index. Queries that look up credentials by user (which is common in WebAuthn flows) would benefit from an index.🔧 Suggested addition
CREATE TABLE user_credentials ( credential_id VARCHAR(512) PRIMARY KEY, user_entity_user_id VARCHAR(255) NOT NULL, public_key BYTEA NOT NULL, attestation_object BYTEA, attestation_client_data_json BYTEA, signature_count BIGINT DEFAULT 0, uv_initialized BOOLEAN DEFAULT FALSE, backup_eligible BOOLEAN DEFAULT FALSE, backup_state BOOLEAN DEFAULT FALSE, created TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP, last_used TIMESTAMP WITH TIME ZONE, label VARCHAR(255), public_key_credential_type VARCHAR(50) DEFAULT 'public-key', authenticator_transports VARCHAR(255), CONSTRAINT fk_user_entity FOREIGN KEY (user_entity_user_id) REFERENCES app_user(id) ON DELETE CASCADE ); + +CREATE INDEX idx_user_credentials_user_id ON user_credentials(user_entity_user_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V2__cleanup_and_webauthn.sql` around lines 9 - 25, The user_credentials table's foreign key column user_entity_user_id lacks an index which can slow lookups; add an explicit index after the CREATE TABLE (or inline) to improve lookup performance—e.g., create an index named something like idx_user_credentials_user_entity_user_id on user_credentials(user_entity_user_id); update the V2__cleanup_and_webauthn.sql migration to include that CREATE INDEX statement (or ALTER TABLE ... ADD INDEX) so queries that reference user_entity_user_id use the index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/resources/db/migration/V2__cleanup_and_webauthn.sql`:
- Line 10: The credential_id column is too small: update the migration so the
credential_id VARCHAR(512) PRIMARY KEY can store Base64URL-encoded WebAuthn IDs
(expand to VARCHAR(1365) or use TEXT) to accommodate up to 1023 bytes encoded
(~1364 chars); change the definition in V2__cleanup_and_webauthn.sql for the
credential_id column and ensure any related schema constraints or PK/index
definitions referencing credential_id still apply after the size change, and
note WebAuthnCredential.setCredentialId() as the producer of these longer
values.
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/user/AppUser.java`:
- Around line 1-57: The AppUser class is entirely commented out and redundant
given UserEntity; delete the AppUser.java file to remove dead code, ensuring you
also remove any import or build references if present and run a quick project
compile to verify no remaining references to AppUser remain; if you want to
preserve the old implementation, rely on git history instead of keeping
commented code in AppUser.java.
In `@src/main/java/backendlab/team4you/webauthn/UserEntity.java`:
- Around line 35-40: The UserEntity constructor currently allows a null primary
key by doing id != null ? id.toBase64UrlString() : null; change this to
fail-fast: in UserEntity(Bytes id, String name, String displayName) validate
that id is non-null and throw an IllegalArgumentException (or
NullPointerException) with a clear message if it is null, then assign this.id =
id.toBase64UrlString(); also ensure any callers (e.g., SignupController) provide
a non-null Bytes and adjust tests if needed so the entity invariant is
preserved.
- Around line 29-30: The createdAt field in UserEntity is declared as
LocalDateTime but the DB column is TIMESTAMP WITH TIME ZONE; change the type to
a timezone-aware type (OffsetDateTime or Instant) on the field named createdAt
in class UserEntity, update the corresponding import and any
getters/setters/constructors/usages to the new type, and ensure JPA mapping
remains compatible (update any converters or annotations if you currently use a
custom converter) so the entity stores and reads timezone-aware timestamps
correctly.
In `@src/main/resources/db/migration/V2__cleanup_and_webauthn.sql`:
- Around line 9-25: The user_credentials table's foreign key column
user_entity_user_id lacks an index which can slow lookups; add an explicit index
after the CREATE TABLE (or inline) to improve lookup performance—e.g., create an
index named something like idx_user_credentials_user_entity_user_id on
user_credentials(user_entity_user_id); update the V2__cleanup_and_webauthn.sql
migration to include that CREATE INDEX statement (or ALTER TABLE ... ADD INDEX)
so queries that reference user_entity_user_id use the index.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72e3c521-37ae-40d0-8b3e-8bbf9cdcd2b3
📒 Files selected for processing (4)
src/main/java/backendlab/team4you/user/AppUser.javasrc/main/java/backendlab/team4you/webauthn/UserEntity.javasrc/main/resources/db/migration/V1__init_schema.sqlsrc/main/resources/db/migration/V2__cleanup_and_webauthn.sql
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/db/migration/V1__init_schema.sql
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Summary by CodeRabbit
New Features
Breaking Changes
Chores