feature/change-user-entity-add-dev-user - #17
Merged
Conversation
…r repository and user service
…ng users via the 3-arg constructor
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces email-as-username with Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SignupController
participant UserService
participant UserRepository
participant Database
Client->>SignupController: POST /signup (username, displayName, email, firstName, lastName)
SignupController->>UserService: registerWebAuthnUser(username, displayName, email, firstName, lastName)
UserService->>UserRepository: findByName(trimmedUsername) / findByEmail(trimmedEmail)
alt name/email exists
UserRepository-->>UserService: Optional present
UserService-->>SignupController: throws ResponseStatusException (409)
SignupController-->>Client: HTTP error
else new user
UserService->>UserService: generate random id, build UserEntity, set role
UserService->>UserRepository: save(UserEntity)
UserRepository->>Database: INSERT user_entities...
UserRepository-->>UserService: saved UserEntity
UserService-->>SignupController: return UserEntity
SignupController->>Client: authenticate & redirect (UsernamePasswordAuthenticationToken)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/backendlab/team4you/controller/SignupController.java (1)
100-145:⚠️ Potential issue | 🟠 MajorThe new
With
usernameandfindByEmail(req.username). Existing usernames will bypass the 409 path and fail later whennameuniqueness is enforced. Validatereq.usernameviafindByName(req.username)and, if email must remain unique, checkreq.emailseparately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/controller/SignupController.java` around lines 100 - 145, The duplicate-user check is still calling findByEmail(req.username) after introducing SignupRequest.email; change the validation logic in the signup flow to call findByName(req.username) to verify username uniqueness, and if email must be unique also call findByEmail(req.email) to verify email uniqueness; update the 409 error branches to return the appropriate conflict when either findByName(...) or findByEmail(...) returns a match so existing usernames or emails trigger the correct response.
🤖 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/controller/SignupController.java`:
- Around line 78-80: The signup flow in SignupController currently derives admin
role from the username suffix (risking privilege escalation now that email is
separate); remove the implicit ADMIN assignment from the public signup path and
always set a safe default role (e.g., USER) on userEntity (related methods:
userEntity.setEmail, setFirstName, setLastName), and move any domain-based
elevation into a separate, secure flow that requires an independently verified
email claim or an admin-only endpoint; do not rely on username contents for role
assignment.
In `@src/main/java/backendlab/team4you/Team4youApplication.java`:
- Around line 20-39: The init ApplicationRunner currently hardcodes the dev
admin password ("123456") when creating a UserEntity; replace that by reading a
password from configuration or environment (e.g., System.getenv or Spring's
Environment property) and fall back to securely generating a random password if
none is provided, then call encoder.encode on that value instead of the literal;
ensure the change is applied in the init method that constructs UserEntity and
setPasswordHash, and log or output the generated password to a dev-only sink so
operators can access it without committing secrets.
In `@src/main/java/backendlab/team4you/user/UserEntity.java`:
- Around line 21-22: The new non-null, unique UserEntity.name field is not being
populated by the user creation flow in UserService, so update the UserService
code path that constructs new UserEntity (the place creating new UserEntity()
and setting firstName/lastName/email/phone/password) to also set a valid name
before saving; derive a login name (e.g., from email local-part or first+last),
ensure it meets uniqueness/format constraints and assign via user.setName(...),
or fallback/generate a unique suffix if collisions occur, so persistence and
authentication won't fail due to a missing name.
- Around line 27-28: The entity currently exposes the email field as a plain
column while the rest of the code (UserRepository.findByEmail and
UserService.registerUser) still treats email as unique; to fix, either restore a
DB-level uniqueness constraint (add a unique constraint on the email column in
UserEntity via `@Column`(unique = true) or `@Table`(uniqueConstraints = ...) and add
a corresponding DB migration that creates a unique index on the email column) or
change UserRepository.findByEmail and UserService.registerUser to handle
non-unique emails (e.g., return/list multiple users, change lookups to use a
true unique identifier), and ensure concurrent signup paths are addressed
consistently across the codebase.
In `@src/main/java/backendlab/team4you/user/UserService.java`:
- Around line 82-84: The new findByName(String name) relies on UserEntity.name
but registerUser(...) currently never sets that field; update the registerUser
method to populate UserEntity.name from the incoming username/DTO (or the same
parameter used to build email/password), persist it before returning, and add a
null/blank check to avoid creating users without a username; refer to
UserService.registerUser, UserService.findByName, and the UserEntity.name
property when making the change.
---
Outside diff comments:
In `@src/main/java/backendlab/team4you/controller/SignupController.java`:
- Around line 100-145: The duplicate-user check is still calling
findByEmail(req.username) after introducing SignupRequest.email; change the
validation logic in the signup flow to call findByName(req.username) to verify
username uniqueness, and if email must be unique also call
findByEmail(req.email) to verify email uniqueness; update the 409 error branches
to return the appropriate conflict when either findByName(...) or
findByEmail(...) returns a match so existing usernames or emails trigger the
correct response.
🪄 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: f86a8ae6-856b-4149-89f3-71f85f620423
📒 Files selected for processing (11)
src/main/java/backendlab/team4you/Team4youApplication.javasrc/main/java/backendlab/team4you/config/CustomAuthenticationSuccessHandler.javasrc/main/java/backendlab/team4you/config/SecurityConfig.javasrc/main/java/backendlab/team4you/controller/RegistrationController.javasrc/main/java/backendlab/team4you/controller/SignupController.javasrc/main/java/backendlab/team4you/repository/UserRepository.javasrc/main/java/backendlab/team4you/user/UserEntity.javasrc/main/java/backendlab/team4you/user/UserService.javasrc/main/java/backendlab/team4you/webauthn/WebAuthnCredential.javasrc/main/resources/db/migration/V7__add_column_email_and_firstname.sqlsrc/main/resources/templates/login.html
Comment on lines
+20
to
+39
| @Bean | ||
| @Profile("dev") | ||
| ApplicationRunner init(UserRepository repository, BCryptPasswordEncoder encoder) { | ||
| return args -> { | ||
| if (repository.count() == 0) { | ||
|
|
||
| UserEntity devUser = new UserEntity( | ||
| Bytes.fromBase64("01"), | ||
| "dev", // name (username) | ||
| "Developer" // displayName | ||
| ); | ||
|
|
||
| devUser.setPasswordHash(encoder.encode("123456")); | ||
| devUser.setRole("ADMIN"); | ||
| devUser.setEmail("dev@team4you.com"); | ||
|
|
||
| repository.save(devUser); | ||
| } | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid hardcoding a known admin password in source control.
@Profile("dev") helps, but this still creates a predictable ADMIN credential if the profile is enabled in the wrong environment or a shared dev stack. Read the password from environment/config, or generate it at startup instead of committing "123456".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/Team4youApplication.java` around lines 20 -
39, The init ApplicationRunner currently hardcodes the dev admin password
("123456") when creating a UserEntity; replace that by reading a password from
configuration or environment (e.g., System.getenv or Spring's Environment
property) and fall back to securely generating a random password if none is
provided, then call encoder.encode on that value instead of the literal; ensure
the change is applied in the init method that constructs UserEntity and
setPasswordHash, and log or output the generated password to a dev-only sink so
operators can access it without committing secrets.
Comment on lines
+27
to
+28
| @Column(name = "email") | ||
| private String email; |
Contributor
There was a problem hiding this comment.
Email is still treated as unique elsewhere, but this model no longer enforces it.
UserRepository.findByEmail(...) still assumes one user per email, and UserService.registerUser() still validates email uniqueness in application code. With this entity exposing email as a plain column and signup now writing it directly, duplicate emails can slip in through concurrent or alternate flows. Add a DB-level unique constraint/migration or stop using email as a unique identifier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/user/UserEntity.java` around lines 27 - 28,
The entity currently exposes the email field as a plain column while the rest of
the code (UserRepository.findByEmail and UserService.registerUser) still treats
email as unique; to fix, either restore a DB-level uniqueness constraint (add a
unique constraint on the email column in UserEntity via `@Column`(unique = true)
or `@Table`(uniqueConstraints = ...) and add a corresponding DB migration that
creates a unique index on the email column) or change UserRepository.findByEmail
and UserService.registerUser to handle non-unique emails (e.g., return/list
multiple users, change lookups to use a true unique identifier), and ensure
concurrent signup paths are addressed consistently across the codebase.
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/java/backendlab/team4you/controller/SignupController.java (1)
81-82:⚠️ Potential issue | 🔴 CriticalRemove implicit ADMIN elevation from public signup.
Line 81 still grants admin based on a self-declared email suffix. That is a privilege-escalation path and should not be in an open signup flow.
Safer role assignment
-String assignedRole = req.getEmail().endsWith("@team4you.com") ? "ADMIN" : "USER"; +String assignedRole = "USER"; userEntity.setRole(assignedRole);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/controller/SignupController.java` around lines 81 - 82, The signup flow in SignupController currently grants "ADMIN" if the email ends with "@team4you.com", which is an unsafe privilege escalation; remove that check and always set userEntity.setRole("USER") during public signup, delete the assignedRole conditional logic, and ensure any admin provisioning is moved to a separate, authenticated/admin-only path or an explicit onboarding process rather than inferred from the email suffix.
🤖 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/controller/SignupController.java`:
- Line 87: SignupController currently constructs the principal using
userEntity.getEmail() which conflicts with the authentication stack that looks
up principals by name; change the principal argument (the first parameter where
userEntity.getEmail() is used) to the username accessor (e.g.,
userEntity.getUsername() or userEntity.getName()) so the created principal
aligns with the rest of the auth flow (reference the construction site where
userEntity.getEmail() is passed along with List.of(new
SimpleGrantedAuthority("ROLE_USER"))).
In `@src/main/java/backendlab/team4you/user/UserService.java`:
- Line 63: In UserService.registerUser, add input validation and a uniqueness
pre-check before calling user.setName(dto.name()) and saving: first reject blank
or whitespace-only dto.name() (throw an IllegalArgumentException or your API's
ValidationException), then query the repository (e.g.,
UserRepository.existsByName or findByName) to detect an existing user with the
same name and throw a controlled DuplicateUserException / Conflict response
instead of letting the DB error bubble up; make these checks in the registerUser
method of UserService so the name assignment and persist happen only after
validation.
In `@src/main/resources/db/migration/V9__add_column_email_and_firstname.sql`:
- Around line 4-6: The migration adds columns email and first_name to
user_entities but lacks a DB-level uniqueness guard on email; modify the
migration V9__add_column_email_and_firstname.sql to add a unique constraint or
unique index on user_entities.email (e.g., ALTER TABLE user_entities ADD
CONSTRAINT ... UNIQUE (email) or CREATE UNIQUE INDEX ... ON
user_entities(email)) and ensure the statement uses IF NOT EXISTS semantics or a
safe conditional to avoid migration failures when rerunning; reference the table
name user_entities and the column name email when implementing this change.
---
Duplicate comments:
In `@src/main/java/backendlab/team4you/controller/SignupController.java`:
- Around line 81-82: The signup flow in SignupController currently grants
"ADMIN" if the email ends with "@team4you.com", which is an unsafe privilege
escalation; remove that check and always set userEntity.setRole("USER") during
public signup, delete the assignedRole conditional logic, and ensure any admin
provisioning is moved to a separate, authenticated/admin-only path or an
explicit onboarding process rather than inferred from the email suffix.
🪄 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: b6f02999-2e67-45d0-b115-b0d9907dbc50
📒 Files selected for processing (8)
src/main/java/backendlab/team4you/Team4youApplication.javasrc/main/java/backendlab/team4you/controller/RegistrationController.javasrc/main/java/backendlab/team4you/controller/SignupController.javasrc/main/java/backendlab/team4you/dto/UserRegistrationDTO.javasrc/main/java/backendlab/team4you/mapper/UserMapper.javasrc/main/java/backendlab/team4you/user/UserRepository.javasrc/main/java/backendlab/team4you/user/UserService.javasrc/main/resources/db/migration/V9__add_column_email_and_firstname.sql
✅ Files skipped from review due to trivial changes (1)
- src/main/java/backendlab/team4you/mapper/UserMapper.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/backendlab/team4you/controller/RegistrationController.java
- src/main/java/backendlab/team4you/Team4youApplication.java
…egisteruser in user service class
…ev-user' into feature/change-user-entity-add-dev-user
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/java/backendlab/team4you/controller/SignupController.java (1)
82-83:⚠️ Potential issue | 🔴 CriticalDon't grant
ADMINfrom a self-asserted email suffix.Anyone can post an
@team4you.comemail here and getADMINpersisted. Keep public signup on a safe default role and move elevation behind verified email ownership or an admin-only flow. This also leaves the saved role out of sync with the session authority on Line 88, which is still alwaysROLE_USER.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/controller/SignupController.java` around lines 82 - 83, Don't grant ADMIN based on a self-asserted email suffix: change the logic around assignedRole so that you always persist "USER" (remove the ternary that sets "ADMIN") and keep userEntity.setRole("USER") on signup; implement role elevation separately (e.g., a verifyEmailOwnership or adminGrantRole endpoint) rather than in SignupController, and ensure the persisted role and the session authority (the code around ROLE_USER on the session creation path) remain consistent after signup.
🤖 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/controller/SignupController.java`:
- Around line 78-85: The signup endpoint in SignupController currently bypasses
validation by setting fields on userEntity and calling users.save directly
(userEntity.setEmail, userEntity.setFirstName, userEntity.setLastName,
users.save), which skips the duplicate/email presence checks in
UserService.registerUser; instead, change the controller to call
UserService.registerUser(...) with the same request payload so validation and
conflict handling are reused, and remove the direct users.save path; if
registerUser has a different signature adapt the request/DTO to match or add an
overload in UserService that accepts the signup DTO, ensuring email null/blank
checks and duplicate username/email checks remain centralized in
UserService.registerUser.
In `@src/main/java/backendlab/team4you/user/UserService.java`:
- Around line 61-62: The pre-check using userRepository.findByName(...) is racy;
update UserService so you keep the pre-check but also wrap the persistence call
(the userRepository.save(...) invocation in the registration flow) in a
try/catch that handles database-unique-key/constraint exceptions (e.g.
DataIntegrityViolationException or ConstraintViolationException) and translate
those into the same IllegalArgumentException("Username already exists") (or the
same controlled response) so concurrent saves that hit the DB constraint produce
the identical error instead of bubbling a low-level exception.
- Around line 88-90: The findByName method currently queries with raw input;
update UserService.findByName to normalize the input exactly as registerUser
does (e.g., trim() and any other normalization like lowercasing) before calling
userRepository.findByName(name) so lookups match stored usernames; keep the
method signature and return behavior (orElse(null)) but pass the normalized
string into the repository call to ensure consistent matching with registerUser.
---
Duplicate comments:
In `@src/main/java/backendlab/team4you/controller/SignupController.java`:
- Around line 82-83: Don't grant ADMIN based on a self-asserted email suffix:
change the logic around assignedRole so that you always persist "USER" (remove
the ternary that sets "ADMIN") and keep userEntity.setRole("USER") on signup;
implement role elevation separately (e.g., a verifyEmailOwnership or
adminGrantRole endpoint) rather than in SignupController, and ensure the
persisted role and the session authority (the code around ROLE_USER on the
session creation path) remain consistent after signup.
🪄 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: e367bd08-3688-4b46-a31c-e92895ca6748
📒 Files selected for processing (4)
src/main/java/backendlab/team4you/controller/SignupController.javasrc/main/java/backendlab/team4you/user/UserEntity.javasrc/main/java/backendlab/team4you/user/UserService.javasrc/main/resources/db/migration/V9__add_column_email_and_firstname.sql
✅ Files skipped from review due to trivial changes (2)
- src/main/resources/db/migration/V9__add_column_email_and_firstname.sql
- src/main/java/backendlab/team4you/user/UserEntity.java
Comment on lines
+61
to
+62
| if (userRepository.findByName(dto.name().trim()).isPresent()) { | ||
| throw new IllegalArgumentException("Username already exists"); |
Contributor
There was a problem hiding this comment.
Username pre-check is still racy.
Two concurrent registrations can both pass this findByName(...).isPresent() check and still collide on the unique constraint when saving. Please also translate the save-time duplicate into the same controlled “username already exists” response instead of relying on the pre-check alone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/user/UserService.java` around lines 61 -
62, The pre-check using userRepository.findByName(...) is racy; update
UserService so you keep the pre-check but also wrap the persistence call (the
userRepository.save(...) invocation in the registration flow) in a try/catch
that handles database-unique-key/constraint exceptions (e.g.
DataIntegrityViolationException or ConstraintViolationException) and translate
those into the same IllegalArgumentException("Username already exists") (or the
same controlled response) so concurrent saves that hit the DB constraint produce
the identical error instead of bubbling a low-level exception.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/java/backendlab/team4you/user/UserService.java (1)
67-68:⚠️ Potential issue | 🟠 MajorPre-check-only uniqueness handling is still racy.
Line 67 and Line 94 do optimistic existence checks, but concurrent requests can still collide at save time (Line 87/Line 115) and leak low-level DB errors instead of your controlled conflict response.
Also applies to: 87-87, 94-97, 115-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/user/UserService.java` around lines 67 - 68, The pre-check using userRepository.findByName(...) is racy and can still let concurrent requests hit the DB unique constraint at userRepository.save(...); add a DB-level uniqueness constraint for the username and change the save paths to catch the persistence exception (e.g., DataIntegrityViolationException / ConstraintViolationException or your DB-specific DuplicateKeyException) around userRepository.save(...) in the UserService methods that perform the checks, then convert that exception to the same controlled conflict response (e.g., throw new IllegalArgumentException("Username already exists") or a ConflictException) so callers never see low-level DB errors; update the save logic that follows the findByName checks to implement this try-catch and keep the optimistic findByName check as pre-validation only.
🤖 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/user/UserService.java`:
- Around line 123-124: UserService.findByName currently calls name.trim()
unguarded which causes an NPE for null input; update the method to first check
if name is null or blank and return null (or Optional.empty) before calling
userRepository.findByName(name.trim()), i.e., guard the input in findByName and
only call trim() when non-null to avoid throwing a 500; refer to
UserService.findByName and the userRepository.findByName(...) call when applying
the change.
- Around line 112-113: Don't grant ADMIN role based solely on the
client-provided email variable; instead ensure role assignment is enforced
server-side: remove the email-based ternary that sets assignedRole and calling
userEntity.setRole("ADMIN") for matching ADMIN_EMAIL, and replace it with logic
that always assigns "USER" by default and only allows "ADMIN" when the operation
is performed by an already authenticated/authorized admin or via a secure
server-side provisioning path (e.g., ADMIN_EMAIL must be validated against a
secure, non-client-controllable source and the caller must have admin
privileges). Update the code around ADMIN_EMAIL, assignedRole, and
userEntity.setRole to enforce that admin role elevation cannot come from
untrusted client input.
- Around line 90-97: The registerWebAuthnUser method currently calls
username.trim() without validating inputs; add null/blank checks for username
and email at the start of registerWebAuthnUser (e.g., verify username and email
are non-null and not blank using String utilities) before calling trim() or
querying userRepository.findByName/findByEmail, and return/throw an appropriate
ResponseStatusException (e.g., BAD_REQUEST) for invalid input; after validation,
normalize using trim() into a local cleanName and proceed with the existing
conflict checks against userRepository.
---
Duplicate comments:
In `@src/main/java/backendlab/team4you/user/UserService.java`:
- Around line 67-68: The pre-check using userRepository.findByName(...) is racy
and can still let concurrent requests hit the DB unique constraint at
userRepository.save(...); add a DB-level uniqueness constraint for the username
and change the save paths to catch the persistence exception (e.g.,
DataIntegrityViolationException / ConstraintViolationException or your
DB-specific DuplicateKeyException) around userRepository.save(...) in the
UserService methods that perform the checks, then convert that exception to the
same controlled conflict response (e.g., throw new
IllegalArgumentException("Username already exists") or a ConflictException) so
callers never see low-level DB errors; update the save logic that follows the
findByName checks to implement this try-catch and keep the optimistic findByName
check as pre-validation only.
🪄 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: c4e5f33e-b0de-44dd-bdd2-02bce14de47e
📒 Files selected for processing (2)
src/main/java/backendlab/team4you/controller/SignupController.javasrc/main/java/backendlab/team4you/user/UserService.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/backendlab/team4you/controller/SignupController.java
Comment on lines
+123
to
+124
| public UserEntity findByName(String name){ | ||
| return userRepository.findByName(name.trim()).orElse(null); |
Contributor
There was a problem hiding this comment.
Guard against null in findByName.
Line 124 calls trim() directly; null input yields a 500/NPE instead of a controlled result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/user/UserService.java` around lines 123 -
124, UserService.findByName currently calls name.trim() unguarded which causes
an NPE for null input; update the method to first check if name is null or blank
and return null (or Optional.empty) before calling
userRepository.findByName(name.trim()), i.e., guard the input in findByName and
only call trim() when non-null to avoid throwing a 500; refer to
UserService.findByName and the userRepository.findByName(...) call when applying
the change.
…d admin assignment enables privilege escalation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/backendlab/team4you/user/UserService.java (1)
50-56:⚠️ Potential issue | 🟡 MinorWrong exception type and message in
deleteById.When the user is not found, the code throws
DuplicateEmailExceptionwith message "E-posten är redan taken" (email already taken). This is semantically incorrect—it should throwUserNotFoundExceptionsimilar to thedeleteUsermethod at line 138.🐛 Proposed fix
`@Transactional` public void deleteById(String id){ if(userRepository.findById(id).isEmpty()){ - throw new DuplicateEmailException("E-posten är redan taken"); + throw new UserNotFoundException("User not found with id: " + id); } userRepository.deleteById(id); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/user/UserService.java` around lines 50 - 56, The deleteById method is throwing DuplicateEmailException with an email-related message when a user isn't found; change this to throw UserNotFoundException instead and update the message to indicate the user with the given id was not found (use the same wording/exception pattern as the existing deleteUser method). Locate deleteById and replace the check that currently throws DuplicateEmailException upon userRepository.findById(id).isEmpty() so it throws new UserNotFoundException(...) with an appropriate id-not-found message before calling userRepository.deleteById(id).
🧹 Nitpick comments (1)
src/main/java/backendlab/team4you/user/UserService.java (1)
24-24: Remove unusedADMIN_EMAILconstant.This constant is no longer referenced after the email-based admin assignment was removed. Dead code should be cleaned up.
🧹 Proposed fix
- private static final String ADMIN_EMAIL = "admin@team4you.com";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/user/UserService.java` at line 24, Remove the unused ADMIN_EMAIL constant from the UserService class: delete the line declaring private static final String ADMIN_EMAIL = "admin@team4you.com" and any now-unnecessary references/imports related only to that constant so there is no dead-code left; ensure UserService compiles after removing ADMIN_EMAIL and run tests to verify nothing else relied on it.
🤖 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/user/UserService.java`:
- Around line 64-68: The service currently throws IllegalArgumentException from
UserService when dto.name() is null/blank or when
userRepository.findByName(...).isPresent(), but GlobalExceptionHandler only
handles DuplicateEmailException and UserNotFoundException; update the code so
these validation/errors are mapped to a controlled response: either replace the
thrown IllegalArgumentException with dedicated exceptions (e.g.,
UsernameInvalidException and UsernameAlreadyExistsException) and throw those
from UserService, or add an `@ExceptionHandler`(IllegalArgumentException.class)
method in GlobalExceptionHandler that converts IllegalArgumentException into a
400 Bad Request response with a clear message; ensure you reference UserService,
dto.name(), userRepository.findByName(...), and GlobalExceptionHandler when
making the change.
- Line 115: In UserService (the method that trims and validates emails), replace
the use of the raw email when persisting the entity—use the previously computed
cleanEmail instead of email (i.e., change the call site where
userEntity.setEmail(email) is used to set userEntity.setEmail(cleanEmail)) so
the stored email matches the duplicate check and has leading/trailing whitespace
removed.
- Around line 101-104: The pre-checks for existing username/email have a race
condition; wrap the repository save call (the userRepository.save(...)
invocation used by registerUser and the WebAuthn registration flow) in a
try-catch that catches org.springframework.dao.DataIntegrityViolationException
and rethrows a new ResponseStatusException(HttpStatus.CONFLICT, "<appropriate
message>") so DB unique-constraint violations are translated to 409 responses;
update both the registerUser and the WebAuthn registration method(s) to handle
this same save() path and ensure the catch uses a clear conflict message (e.g.,
"Användarnamnet eller e-posten är redan tagen").
---
Outside diff comments:
In `@src/main/java/backendlab/team4you/user/UserService.java`:
- Around line 50-56: The deleteById method is throwing DuplicateEmailException
with an email-related message when a user isn't found; change this to throw
UserNotFoundException instead and update the message to indicate the user with
the given id was not found (use the same wording/exception pattern as the
existing deleteUser method). Locate deleteById and replace the check that
currently throws DuplicateEmailException upon
userRepository.findById(id).isEmpty() so it throws new
UserNotFoundException(...) with an appropriate id-not-found message before
calling userRepository.deleteById(id).
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/user/UserService.java`:
- Line 24: Remove the unused ADMIN_EMAIL constant from the UserService class:
delete the line declaring private static final String ADMIN_EMAIL =
"admin@team4you.com" and any now-unnecessary references/imports related only to
that constant so there is no dead-code left; ensure UserService compiles after
removing ADMIN_EMAIL and run tests to verify nothing else relied on it.
🪄 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: 1f1bb961-f762-4175-ba03-62d31df15fea
📒 Files selected for processing (1)
src/main/java/backendlab/team4you/user/UserService.java
Comment on lines
+101
to
+104
| if(userRepository.findByName(cleanName).isPresent()) | ||
| throw new ResponseStatusException(HttpStatus.CONFLICT, "Användarnamnet är redan taget"); | ||
| if(userRepository.findByEmail(cleanEmail).isPresent()) | ||
| throw new ResponseStatusException(HttpStatus.CONFLICT, "E-posten är redan tagen"); |
Contributor
There was a problem hiding this comment.
WebAuthn registration has the same race condition as registerUser.
The pre-checks at lines 101-104 can pass for concurrent requests, then one will fail on the DB unique constraint with an uncontrolled exception. Wrap the save() at line 123 in a try-catch for DataIntegrityViolationException and translate to ResponseStatusException(HttpStatus.CONFLICT, ...).
🛡️ Proposed fix
+import org.springframework.dao.DataIntegrityViolationException;- return userRepository.save(userEntity);
+ try {
+ return userRepository.save(userEntity);
+ } catch (DataIntegrityViolationException e) {
+ throw new ResponseStatusException(HttpStatus.CONFLICT, "Username or email already taken");
+ }Also applies to: 123-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/user/UserService.java` around lines 101 -
104, The pre-checks for existing username/email have a race condition; wrap the
repository save call (the userRepository.save(...) invocation used by
registerUser and the WebAuthn registration flow) in a try-catch that catches
org.springframework.dao.DataIntegrityViolationException and rethrows a new
ResponseStatusException(HttpStatus.CONFLICT, "<appropriate message>") so DB
unique-constraint violations are translated to 409 responses; update both the
registerUser and the WebAuthn registration method(s) to handle this same save()
path and ensure the catch uses a clear conflict message (e.g., "Användarnamnet
eller e-posten är redan tagen").
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/backendlab/team4you/user/UserService.java (2)
134-136:⚠️ Potential issue | 🟡 MinorGuard against null input in
findByName.Line 135 calls
name.trim()directly; null input yields a 500/NPE instead of a controllednullresult consistent withfindByEmailandfindById.🛡️ Proposed fix
public UserEntity findByName(String name){ + if (name == null || name.isBlank()) { + return null; + } return userRepository.findByName(name.trim()).orElse(null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/user/UserService.java` around lines 134 - 136, Guard against null in UserService.findByName: check if the incoming name is null and return null before calling name.trim(), then proceed to call userRepository.findByName(name.trim()).orElse(null); this mirrors the null-safe behavior of findByEmail/findById and prevents an NPE when name is null.
64-72:⚠️ Potential issue | 🟠 MajorRace condition: wrap
save()in try-catch forDataIntegrityViolationException.The pre-checks at lines 67-68 and 71 can pass for concurrent requests, then one will fail on the DB unique constraint with an uncontrolled exception. Apply the same pattern used in
registerWebAuthnUser(lines 123-127).🛡️ Proposed fix
- userRepository.save(user); + try { + userRepository.save(user); + } catch (DataIntegrityViolationException e) { + throw new ResponseStatusException(HttpStatus.CONFLICT, "Username or email already taken"); + }Also applies to: 87-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/user/UserService.java` around lines 64 - 72, The pre-checks for duplicate name/email can race; wrap the call to userRepository.save(...) in a try-catch that catches DataIntegrityViolationException and translates it into the appropriate application exception (e.g., throw new DuplicateEmailException("E-posten är redan tagen") or ResponseStatusException for username) following the same pattern used in registerWebAuthnUser (lines around registerWebAuthnUser) so DB unique-constraint failures are handled gracefully; apply the same change to the other user-creation site referenced (the block around line 87) so both save paths catch DataIntegrityViolationException and map it to the existing DuplicateEmailException/ResponseStatusException as appropriate.
🤖 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/user/UserService.java`:
- Line 71: The duplicate-check and persistence in registerUser currently use raw
dto.email(), causing mismatch with registerWebAuthnUser which trims the email;
normalize the email in registerUser the same way as registerWebAuthnUser (trim
and any other normalization applied there) before calling
userRepository.findByEmail(...) and before constructing/saving the User entity
so duplicate detection and stored value are consistent with
registerWebAuthnUser.
---
Duplicate comments:
In `@src/main/java/backendlab/team4you/user/UserService.java`:
- Around line 134-136: Guard against null in UserService.findByName: check if
the incoming name is null and return null before calling name.trim(), then
proceed to call userRepository.findByName(name.trim()).orElse(null); this
mirrors the null-safe behavior of findByEmail/findById and prevents an NPE when
name is null.
- Around line 64-72: The pre-checks for duplicate name/email can race; wrap the
call to userRepository.save(...) in a try-catch that catches
DataIntegrityViolationException and translates it into the appropriate
application exception (e.g., throw new DuplicateEmailException("E-posten är
redan tagen") or ResponseStatusException for username) following the same
pattern used in registerWebAuthnUser (lines around registerWebAuthnUser) so DB
unique-constraint failures are handled gracefully; apply the same change to the
other user-creation site referenced (the block around line 87) so both save
paths catch DataIntegrityViolationException and map it to the existing
DuplicateEmailException/ResponseStatusException as appropriate.
🪄 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: 3b4a4a62-596b-4f82-92ee-2c9b2de806c1
📒 Files selected for processing (1)
src/main/java/backendlab/team4you/user/UserService.java
JohanHiths
approved these changes
Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Authentication
Behavior
Database