Feature/security#35
Conversation
# Conflicts: # src/main/java/org/example/visacasemanagementsystem/VisaCaseManagementSystemApplication.java
📝 WalkthroughWalkthroughReplaces SecurityUser with UserPrincipal across auth flow; adds persistent username and username-based lookups; introduces UserPrincipal and PasswordEncoder-backed authentication; enables role- and path-based security with form login; seeds users with encoded passwords and updates tests to populate username. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Browser as Browser
participant SecurityConfig as SecurityConfig
participant AuthProvider as AuthenticationProvider
participant UserDetailsService as UserDetailsService
participant UserRepository as UserRepository
participant UserPrincipal as UserPrincipal
participant Dashboard as Dashboard
User->>Browser: Navigate to /dashboard
Browser->>SecurityConfig: GET /dashboard
rect rgba(200,150,100,0.5)
note over SecurityConfig,AuthProvider: Authentication Required
SecurityConfig->>SecurityConfig: Is authenticated?
alt Not authenticated
SecurityConfig->>Browser: Redirect to /user/login
Browser->>User: Show login form
User->>Browser: Submit credentials
Browser->>SecurityConfig: POST /user/login
SecurityConfig->>AuthProvider: authenticate(username,password)
AuthProvider->>UserDetailsService: loadUserByUsername(username)
UserDetailsService->>UserRepository: findByUsername(username)
UserRepository-->>UserDetailsService: Optional<User>
UserDetailsService->>UserPrincipal: new UserPrincipal(user)
UserPrincipal-->>UserDetailsService: UserDetails
UserDetailsService-->>AuthProvider: UserDetails
AuthProvider->>AuthProvider: Verify password (PasswordEncoder)
alt Valid
AuthProvider-->>SecurityConfig: Auth success
SecurityConfig->>Browser: Redirect to /dashboard
else Invalid
AuthProvider-->>Browser: Login failed
end
else Already authenticated
end
end
rect rgba(100,150,200,0.5)
note over SecurityConfig,Dashboard: Authorization Check
SecurityConfig->>SecurityConfig: Check granted authorities
alt Has required ROLE
SecurityConfig->>Dashboard: Allow access
Dashboard-->>Browser: Render content
else Missing role
SecurityConfig->>Browser: 403 Forbidden
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
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 docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/example/visacasemanagementsystem/user/mapper/UserMapper.java (1)
25-44:⚠️ Potential issue | 🟠 MajorCoupling
usernametoTwo issues in this change:
updateEntityFromDTOnow setsusername = dto.email()whenever a profile is edited. SinceUserDetailsServiceImpl.loadUserByUsernameresolves users viafindByUsername, editing a user's email also changes their login identifier — invalidating any future logins with the old username and, depending on session strategy, potentially confusing already-authenticated sessions. If the intent is "email is the username", consider usingfindByEmailfor authentication) instead of denormalizing it into a separateusernamecolumn, or decoupleusernamefrom
toEntity(CreateUserDTO)now setsuser.setPassword(dto.password())in the mapper. Per prior learnings,UserService.createUser()is expected to encode viapasswordEncoder.encode(...). IfUserServicenow delegates full entity construction to this mapper and forgets to re-set the encoded password aftertoEntity(...), the plaintext set here will be what’s persisted. Safer pattern: omitpasswordfrom the mapper entirely and have the service performuser.setPassword(passwordEncoder.encode(dto.password()))as the single source of truth.🛡️ Proposed fix (option: don't map password in mapper)
public User toEntity(CreateUserDTO dto){ if (dto == null) return null; User user = new User(); user.setFullName(dto.fullName()); user.setEmail(dto.email()); user.setUsername(dto.email()); - user.setPassword(dto.password()); user.setUserAuthorization(dto.userAuthorization()); return user; } // Uppdatering (DTO --> Befintlig Entity) public void updateEntityFromDTO(UpdateUserDTO dto, User user) { if (dto == null || user == null) return; user.setFullName(dto.fullName()); user.setEmail(dto.email()); - user.setUsername(dto.email()); }And in
UserService.createUser(...)ensure:User user = userMapper.toEntity(dto); user.setPassword(passwordEncoder.encode(dto.password())); userRepository.save(user);Based on learnings from PR
#26thatUserService.createUser()is the intended place forpasswordEncoder.encode(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/mapper/UserMapper.java` around lines 25 - 44, The mapper currently couples username to email and maps plain-text passwords; change UserMapper so updateEntityFromDTO does NOT overwrite user.setUsername(dto.email()) (leave username unchanged or only change via an explicit username update flow) and remove setting password from toEntity(CreateUserDTO) so it does not write raw passwords. Then ensure UserService.createUser(...) calls userMapper.toEntity(dto) and explicitly sets user.setPassword(passwordEncoder.encode(dto.password())) before saving; if authentication should use email, adjust UserDetailsServiceImpl.loadUserByUsername to lookup by email (findByEmail) instead of findByUsername to avoid silently rotating login identity.src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java (1)
60-67:⚠️ Potential issue | 🟡 MinorSet
UserAuthorizationbeforesave(...), not after.
savedUser.setUserAuthorization(UserAuthorization.USER)runs afteruserRepository.save(user)and is never explicitly persisted — it only reaches the DB because the entity stays managed inside@Transactionaland gets flushed on commit. That is brittle (e.g., if someone later moves the write out of the transactional boundary, or the method is called from a non‑transactional context, the role assignment silently disappears).It also means whatever
userMapper.toEntity(dto)wrote for authorization is first persisted and then overwritten — if that's intentional defense‑in‑depth against privilege escalation via the DTO, do it explicitly before save.🐛 Proposed fix
- user.setPassword(passwordEncoder.encode(dto.password())); - try { - User savedUser = userRepository.save(user); - savedUser.setUserAuthorization(UserAuthorization.USER); - return userMapper.toDTO(savedUser); - } catch (DataIntegrityViolationException e) { + user.setPassword(passwordEncoder.encode(dto.password())); + // Force role server-side; never trust the DTO for privilege assignment on signup. + user.setUserAuthorization(UserAuthorization.USER); + try { + User savedUser = userRepository.save(user); + return userMapper.toDTO(savedUser); + } catch (DataIntegrityViolationException e) { throw new IllegalArgumentException("A user with this email already exists", e); }Based on learnings (PR
#26),UserService.createUseris the intended injection point for password/role hardening; making the role assignment explicit and pre‑save rounds that out.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java` around lines 60 - 67, In UserService.createUser, the UserAuthorization is set after calling userRepository.save(user), relying on transactional EntityManager flushing; move the hardening so the authorization is explicitly set on the entity before persisting: after converting DTO to entity (userMapper.toEntity(...)) and before calling userRepository.save(user) set user.setUserAuthorization(UserAuthorization.USER) (or override any incoming value), then call save and return userMapper.toDTO(savedUser); keep the existing DataIntegrityViolationException handling intact.
🧹 Nitpick comments (7)
src/main/java/org/example/visacasemanagementsystem/user/entity/User.java (1)
25-29: Consider DB-levelnullable = falseon the newusernameandpasswordcolumns.
@NotBlankis bean-validation only and runs during save lifecycle; it does not produce aNOT NULLDDL constraint. If validation is ever bypassed (native query, direct SQL, disabled validator), you could end up withnullusernames/passwords — andUserDetailsServiceImpl/ Spring Security will behave poorly with either. Mirror the existing style used onfullName:🛡️ Proposed fix
- `@NotBlank` `@Column`(unique = true) - private String username; - - `@NotBlank` `@Column` - private String password; + `@NotBlank` `@Column`(nullable = false, unique = true) + private String username; + + `@NotBlank` `@Column`(nullable = false) + private String password;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/entity/User.java` around lines 25 - 29, The new User entity fields username and password use `@NotBlank` (bean validation) but lack DB-level NOT NULL; update the User class to add nullable = false to the `@Column` annotations on the username and password fields (mirror the existing fullName style) so the database schema enforces non-null values and prevents null usernames/passwords from being persisted/bypassing validation.src/main/java/org/example/visacasemanagementsystem/user/UserAuthorization.java (1)
6-11: OverridingtoString()on the enum has wide-reaching side effects.Every caller that prints or serializes a
UserAuthorization— logs, Thymeleaf expressions like${user.userAuthorization}, JSON serialization (Jackson usestoString()for enums only whenWRITE_ENUMS_USING_TO_STRINGis enabled, but many customtoString()calls inUser.toString()and similar do use it), and any.toString()invoked on aUserAuthorizationvalue — will now emitROLE_USER/ROLE_ADMIN/ROLE_SYSADMINinstead ofUSER/ADMIN/SYSADMIN. This can silently break views, log output, and any other string-based consumers.
@Enumerated(EnumType.STRING)andEnum.valueOf(...)rely onname()(nottoString()), so persistence still works — but the broader behavioral change is easy to forget.Consider exposing a dedicated method instead and constructing the authority explicitly in
UserPrincipal:♻️ Proposed refactor
public enum UserAuthorization { USER, ADMIN, - SYSADMIN; - - `@Override` - public String toString() { - return "ROLE_" + this.name(); - } + SYSADMIN; + + public String asAuthority() { + return "ROLE_" + name(); + } }And in
UserPrincipal.getAuthorities():return Collections.singleton(new SimpleGrantedAuthority(user.getUserAuthorization().asAuthority()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/UserAuthorization.java` around lines 6 - 11, Overriding toString() on the UserAuthorization enum introduces wide-reaching breakage; remove the toString() override from UserAuthorization and add a dedicated accessor (e.g., asAuthority() or toRoleString()) that returns "ROLE_" + name(), then update the authority construction in UserPrincipal.getAuthorities() (use new SimpleGrantedAuthority(user.getUserAuthorization().asAuthority())) so all logging/serialization still sees the original enum names while security gets the ROLE_ prefixed string.src/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.java (1)
41-45:defaultSuccessUrl("/dashboard", true)forces the user back to/dashboardeven after they tried to access a deep link.Passing
trueas the second arg means "always redirect to this URL on success", so the saved-request redirect flow (return the user to the page they originally attempted) is disabled. For a dashboard-heavy app this may be intentional, but if deep-linking is ever desired, switch tofalse(or omit) and let Spring'sSavedRequestAwareAuthenticationSuccessHandlerdo the right thing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.java` around lines 41 - 45, The current formLogin configuration in SecurityConfig uses defaultSuccessUrl("/dashboard", true) which always redirects to /dashboard and prevents returning users to saved deep links; change the call in the SecurityConfig class (the formLogin(...) block where defaultSuccessUrl is configured) to use defaultSuccessUrl("/dashboard", false) or remove the boolean so SavedRequestAwareAuthenticationSuccessHandler can perform saved-request redirects instead.src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java (1)
104-141: Consolidate role‑check helpers and rename thevalidateSysAdminoverload.Two issues in this cluster:
validateSysAdmin(Long requesterId)(line 104) andvalidateSysAdmin(UserPrincipal principal)(line 123) share a name but have completely different semantics — one loads a user by id from the DB and checks the entity field, the other reads authorities off the authenticated principal. Overload resolution at the call site hides that divergence; give them distinct names (e.g.,requireSysAdminByIdvsrequireSysAdmin) so reviewers can tell which trust source is used.validateProfileAccess,validateSysAdmin(UserPrincipal), andvalidateAdminall repeat the sameprincipal.getAuthorities().stream().anyMatch(...)pattern with role strings. Extract a smallhasRole(UserPrincipal, String)helper (or a constants class forROLE_SYSADMIN/ROLE_ADMIN) to remove the duplication and keep role names in one place.♻️ Sketch
+ private static boolean hasRole(UserPrincipal principal, String role) { + return principal.getAuthorities().stream() + .anyMatch(a -> Objects.equals(a.getAuthority(), role)); + } + public void validateProfileAccess(UserPrincipal principal, Long userId) { boolean isOwnProfile = principal.getUserId().equals(userId); - boolean isSysAdmin = principal.getAuthorities().stream() - .anyMatch(a -> Objects.equals(a.getAuthority(), "ROLE_SYSADMIN")); - if (!isOwnProfile && !isSysAdmin) { + if (!isOwnProfile && !hasRole(principal, "ROLE_SYSADMIN")) { throw new UnauthorizedException("You do not have permission to edit this profile."); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java` around lines 104 - 141, Rename the DB-backed validateSysAdmin(Long requesterId) to requireSysAdminById and rename the principal-based validateSysAdmin(UserPrincipal principal) to requireSysAdmin so callers clearly indicate which trust source is used; then extract a single helper hasRole(UserPrincipal principal, String role) (and optionally central ROLE_SYSADMIN / ROLE_ADMIN constants) and refactor validateProfileAccess, requireSysAdmin, and validateAdmin to call hasRole(...) instead of repeating principal.getAuthorities().stream().anyMatch(...), updating all call sites accordingly.src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java (2)
42-60: Drop the redundantUserDetailsdefault overrides.
isAccountNonExpired,isAccountNonLocked,isCredentialsNonExpired, andisEnabledall just delegate toUserDetails.super.*. These overrides add noise and invite confusion about whether custom account‑state logic exists. Remove them until real behavior is needed (e.g., backed byenabled/lockedfields onUser).♻️ Proposed cleanup
- `@Override` - public boolean isAccountNonExpired() { - return UserDetails.super.isAccountNonExpired(); - } - - `@Override` - public boolean isAccountNonLocked() { - return UserDetails.super.isAccountNonLocked(); - } - - `@Override` - public boolean isCredentialsNonExpired() { - return UserDetails.super.isCredentialsNonExpired(); - } - - `@Override` - public boolean isEnabled() { - return UserDetails.super.isEnabled(); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java` around lines 42 - 60, The four redundant overrides in UserPrincipal—isAccountNonExpired, isAccountNonLocked, isCredentialsNonExpired, and isEnabled—simply delegate to UserDetails.super and should be removed to reduce noise; delete these methods from the UserPrincipal class (leaving other UserDetails methods intact) and only reintroduce custom implementations later if you implement real account-state fields (e.g., enabled/locked) on the User model.
20-24: Avoid deriving authority strings fromUserAuthorization.toString().Using
toString()as a business‑critical serialization format is fragile —toString()is conventionally for debugging/logging, and any future change (e.g., adding a display name, adjusting log formatting) will silently break authorization. Expose an explicit accessor onUserAuthorization(or build the string here) so the contract is intentional and searchable.♻️ Proposed refactor
- `@Override` - `@NullMarked` - public Collection<? extends GrantedAuthority> getAuthorities() { - return Collections.singleton(new SimpleGrantedAuthority(user.getUserAuthorization().toString())); - } + `@Override` + `@NullMarked` + public Collection<? extends GrantedAuthority> getAuthorities() { + return Collections.singleton( + new SimpleGrantedAuthority("ROLE_" + user.getUserAuthorization().name())); + }Then the custom
UserAuthorization#toString()override can be dropped, leavingtoString()to its default purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java` around lines 20 - 24, The getAuthorities() implementation in UserPrincipal should not rely on UserAuthorization.toString(); instead add an explicit accessor (e.g., UserAuthorization#getAuthority or `#getRoleName` or use an enum `#name`()) on the UserAuthorization type and call that here; change UserPrincipal.getAuthorities() to create the SimpleGrantedAuthority from that explicit accessor (e.g., new SimpleGrantedAuthority(user.getUserAuthorization().getAuthority())), and update/introduce the accessor in the UserAuthorization class so the authority string is a stable, documented contract (you can later remove any custom toString() override).src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java (1)
73-88:viewProfilere‑computes authorization flags already evaluated byvalidateProfileAccess.
validateProfileAccessat line 79 already performs theisOwnProfile || isSysAdmincheck (throwing on failure), and lines 81‑86 recompute essentially the same predicate to setcanEdit. That's two sources of truth for the same rule — if the policy ever changes (e.g., ADMIN can also edit), it'll have to be edited in both places.Consider either returning the decision from
UserService(e.g.,boolean canEditProfile(UserPrincipal, Long)) or replacing theanyMatchliteral with the samehasRolehelper suggested inUserService.java, so"ROLE_SYSADMIN"isn't hard‑coded in two files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java` around lines 73 - 88, The viewProfile method currently duplicates authorization logic from userService.validateProfileAccess; replace the local recomputation with a single call to the service so there is one source of truth: remove the isOwnProfile/isSysAdmin anyMatch logic and instead call a new or existing service method (e.g., userService.canEditProfile(principal, userId)) or use the UserService.hasRole helper to determine edit permission, then set model.addAttribute("canEdit", userService.canEditProfile(principal, userId)); keep validateProfileAccess for throwing on unauthorized access but derive the view flag from the service method to avoid duplicated hard‑coded "ROLE_SYSADMIN".
🤖 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/org/example/visacasemanagementsystem/config/SecurityConfig.java`:
- Around line 31-45: Replace the overly-broad requestMatchers in SecurityConfig
so they target the exact endpoints in UserViewController: change
requestMatchers("/**/admin").hasRole("ADMIN") to
requestMatchers("/dashboard/admin").hasRole("ADMIN") and
requestMatchers("/**/applicant").hasRole("USER") to
requestMatchers("/dashboard/applicant").hasRole("USER"); keep the other matchers
(.requestMatchers("/user/signup"), .requestMatchers("/user/login"),
.requestMatchers("/dashboard")) and retain .anyRequest().hasRole("SYSADMIN") if
you want all other routes restricted to SYSADMIN. Also remove or comment out
httpBasic(withDefaults()) (or conditionally enable it for API clients) to avoid
HTTP Basic prompts in browsers when using formLogin(...). Ensure these changes
are made in the SecurityConfig class around the authorizeHttpRequests(...) and
formLogin(...) configuration blocks.
In
`@src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java`:
- Around line 36-40: The User entity's username field is missing a NOT NULL
constraint which contradicts the `@NullMarked` contract on
UserPrincipal.getUsername(); update the User entity declaration for the username
field to include nullable = false and a validation annotation (e.g., `@NotBlank`)
and keep unique = true so it reads with both nullable = false and unique = true;
then run or create a DB migration to add the NOT NULL constraint and backfill
any existing NULL usernames in the users table so runtime authentication
(UserPrincipal.getUsername) cannot encounter nulls.
---
Outside diff comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/mapper/UserMapper.java`:
- Around line 25-44: The mapper currently couples username to email and maps
plain-text passwords; change UserMapper so updateEntityFromDTO does NOT
overwrite user.setUsername(dto.email()) (leave username unchanged or only change
via an explicit username update flow) and remove setting password from
toEntity(CreateUserDTO) so it does not write raw passwords. Then ensure
UserService.createUser(...) calls userMapper.toEntity(dto) and explicitly sets
user.setPassword(passwordEncoder.encode(dto.password())) before saving; if
authentication should use email, adjust
UserDetailsServiceImpl.loadUserByUsername to lookup by email (findByEmail)
instead of findByUsername to avoid silently rotating login identity.
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Around line 60-67: In UserService.createUser, the UserAuthorization is set
after calling userRepository.save(user), relying on transactional EntityManager
flushing; move the hardening so the authorization is explicitly set on the
entity before persisting: after converting DTO to entity
(userMapper.toEntity(...)) and before calling userRepository.save(user) set
user.setUserAuthorization(UserAuthorization.USER) (or override any incoming
value), then call save and return userMapper.toDTO(savedUser); keep the existing
DataIntegrityViolationException handling intact.
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.java`:
- Around line 41-45: The current formLogin configuration in SecurityConfig uses
defaultSuccessUrl("/dashboard", true) which always redirects to /dashboard and
prevents returning users to saved deep links; change the call in the
SecurityConfig class (the formLogin(...) block where defaultSuccessUrl is
configured) to use defaultSuccessUrl("/dashboard", false) or remove the boolean
so SavedRequestAwareAuthenticationSuccessHandler can perform saved-request
redirects instead.
In
`@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java`:
- Around line 73-88: The viewProfile method currently duplicates authorization
logic from userService.validateProfileAccess; replace the local recomputation
with a single call to the service so there is one source of truth: remove the
isOwnProfile/isSysAdmin anyMatch logic and instead call a new or existing
service method (e.g., userService.canEditProfile(principal, userId)) or use the
UserService.hasRole helper to determine edit permission, then set
model.addAttribute("canEdit", userService.canEditProfile(principal, userId));
keep validateProfileAccess for throwing on unauthorized access but derive the
view flag from the service method to avoid duplicated hard‑coded
"ROLE_SYSADMIN".
In `@src/main/java/org/example/visacasemanagementsystem/user/entity/User.java`:
- Around line 25-29: The new User entity fields username and password use
`@NotBlank` (bean validation) but lack DB-level NOT NULL; update the User class to
add nullable = false to the `@Column` annotations on the username and password
fields (mirror the existing fullName style) so the database schema enforces
non-null values and prevents null usernames/passwords from being
persisted/bypassing validation.
In
`@src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java`:
- Around line 42-60: The four redundant overrides in
UserPrincipal—isAccountNonExpired, isAccountNonLocked, isCredentialsNonExpired,
and isEnabled—simply delegate to UserDetails.super and should be removed to
reduce noise; delete these methods from the UserPrincipal class (leaving other
UserDetails methods intact) and only reintroduce custom implementations later if
you implement real account-state fields (e.g., enabled/locked) on the User
model.
- Around line 20-24: The getAuthorities() implementation in UserPrincipal should
not rely on UserAuthorization.toString(); instead add an explicit accessor
(e.g., UserAuthorization#getAuthority or `#getRoleName` or use an enum `#name`()) on
the UserAuthorization type and call that here; change
UserPrincipal.getAuthorities() to create the SimpleGrantedAuthority from that
explicit accessor (e.g., new
SimpleGrantedAuthority(user.getUserAuthorization().getAuthority())), and
update/introduce the accessor in the UserAuthorization class so the authority
string is a stable, documented contract (you can later remove any custom
toString() override).
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Around line 104-141: Rename the DB-backed validateSysAdmin(Long requesterId)
to requireSysAdminById and rename the principal-based
validateSysAdmin(UserPrincipal principal) to requireSysAdmin so callers clearly
indicate which trust source is used; then extract a single helper
hasRole(UserPrincipal principal, String role) (and optionally central
ROLE_SYSADMIN / ROLE_ADMIN constants) and refactor validateProfileAccess,
requireSysAdmin, and validateAdmin to call hasRole(...) instead of repeating
principal.getAuthorities().stream().anyMatch(...), updating all call sites
accordingly.
In
`@src/main/java/org/example/visacasemanagementsystem/user/UserAuthorization.java`:
- Around line 6-11: Overriding toString() on the UserAuthorization enum
introduces wide-reaching breakage; remove the toString() override from
UserAuthorization and add a dedicated accessor (e.g., asAuthority() or
toRoleString()) that returns "ROLE_" + name(), then update the authority
construction in UserPrincipal.getAuthorities() (use new
SimpleGrantedAuthority(user.getUserAuthorization().asAuthority())) so all
logging/serialization still sees the original enum names while security gets the
ROLE_ prefixed string.
🪄 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: cd2fd140-fed8-4916-9b6f-fd2dbd2678b0
📒 Files selected for processing (13)
src/main/java/org/example/visacasemanagementsystem/ApplicationViewController.javasrc/main/java/org/example/visacasemanagementsystem/VisaCaseManagementSystemApplication.javasrc/main/java/org/example/visacasemanagementsystem/config/DataInitializer.javasrc/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.javasrc/main/java/org/example/visacasemanagementsystem/user/UserAuthorization.javasrc/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.javasrc/main/java/org/example/visacasemanagementsystem/user/entity/User.javasrc/main/java/org/example/visacasemanagementsystem/user/mapper/UserMapper.javasrc/main/java/org/example/visacasemanagementsystem/user/repository/UserRepository.javasrc/main/java/org/example/visacasemanagementsystem/user/security/SecurityUser.javasrc/main/java/org/example/visacasemanagementsystem/user/security/UserDetailsServiceImpl.javasrc/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.javasrc/main/java/org/example/visacasemanagementsystem/user/service/UserService.java
💤 Files with no reviewable changes (1)
- src/main/java/org/example/visacasemanagementsystem/user/security/SecurityUser.java
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java (3)
31-34:@NullableongetPassword()contradicts the entity contract.
User.passwordis declared@NotBlank@column(nullable = false), sogetPassword()should never returnnullfor a persisted user. Marking it@Nullablehere will force callers (and any null-analysis tooling) to handle a case that cannot occur, and it's inconsistent with the@NullMarkedtreatment applied togetUsername()which has the same entity guarantees. Consider marking this@NullMarkedtoo (or simply leaving it unannotated if the class/package is already null-marked).🧹 Proposed change
- `@Override` - public `@Nullable` String getPassword() { - return user.getPassword(); - } + `@Override` + `@NullMarked` + public String getPassword() { + return user.getPassword(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java` around lines 31 - 34, Remove the incorrect `@Nullable` annotation on UserPrincipal.getPassword(), since User.getPassword() is annotated `@NotBlank/`@Column(nullable = false) and thus cannot be null; update the method in class UserPrincipal to either mark it as null-safe consistent with getUsername() (e.g., use `@NullMarked/`@NonNull as per the project's nullability convention) or leave it unannotated if the package/class is already null-marked so callers and static analysis treat getPassword() as non-null.
20-24: Guard against a nulluserAuthorization.
getAuthorities()dereferencesuser.getUserAuthorization()unconditionally. While the column is declared@NotNullon the entity, a partially-populatedUser(e.g., constructed in tests, or loaded before the field is set) will trigger an NPE inside Spring Security's authentication pipeline, which surfaces as an opaque 500 rather than a clear auth failure. Consider returning an empty authority collection (or throwing a descriptive exception) when the authorization is missing.🛡️ Proposed defensive check
- public Collection<? extends GrantedAuthority> getAuthorities() { - return Collections.singleton(new SimpleGrantedAuthority(user.getUserAuthorization().asAuthority())); - } + public Collection<? extends GrantedAuthority> getAuthorities() { + var authorization = user.getUserAuthorization(); + if (authorization == null) { + return Collections.emptyList(); + } + return Collections.singleton(new SimpleGrantedAuthority(authorization.asAuthority())); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java` around lines 20 - 24, getAuthorities() currently dereferences user.getUserAuthorization() and can NPE; update UserPrincipal.getAuthorities() to defensively check user.getUserAuthorization() for null and if null return Collections.emptyList() (or alternatively throw a descriptive IllegalStateException) instead of constructing a SimpleGrantedAuthority; ensure the check references the existing user field and the SimpleGrantedAuthority creation logic so you only create authorities when user.getUserAuthorization() is non-null.
1-45: Move@NullMarkedto the class level instead of annotating individual methods.The current pattern of sprinkling
@NullMarkedon individual methods is verbose and inconsistent—getAuthorities()andgetUsername()are annotated, butgetFullName()andgetUserId()are not, despite having identical nullness semantics. Per JSpecify guidance,@NullMarkedis intended for class or package declarations, with@Nullableused to opt out specific members (in this case,getPassword()). This approach eliminates per-method noise and ensures uniform null-safety contracts across the class.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java` around lines 1 - 45, Add `@NullMarked` to the UserPrincipal class declaration and remove the per-method `@NullMarked` annotations on getAuthorities() and getUsername(); keep the `@Nullable` on getPassword() to opt-out of the class-level nullness and remove annotations from other methods like getFullName() and getUserId() so the class-level nullness applies consistently for UserPrincipal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java`:
- Around line 31-34: Remove the incorrect `@Nullable` annotation on
UserPrincipal.getPassword(), since User.getPassword() is annotated
`@NotBlank/`@Column(nullable = false) and thus cannot be null; update the method
in class UserPrincipal to either mark it as null-safe consistent with
getUsername() (e.g., use `@NullMarked/`@NonNull as per the project's nullability
convention) or leave it unannotated if the package/class is already null-marked
so callers and static analysis treat getPassword() as non-null.
- Around line 20-24: getAuthorities() currently dereferences
user.getUserAuthorization() and can NPE; update UserPrincipal.getAuthorities()
to defensively check user.getUserAuthorization() for null and if null return
Collections.emptyList() (or alternatively throw a descriptive
IllegalStateException) instead of constructing a SimpleGrantedAuthority; ensure
the check references the existing user field and the SimpleGrantedAuthority
creation logic so you only create authorities when user.getUserAuthorization()
is non-null.
- Around line 1-45: Add `@NullMarked` to the UserPrincipal class declaration and
remove the per-method `@NullMarked` annotations on getAuthorities() and
getUsername(); keep the `@Nullable` on getPassword() to opt-out of the class-level
nullness and remove annotations from other methods like getFullName() and
getUserId() so the class-level nullness applies consistently for UserPrincipal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d637ba77-0393-417a-baf7-4778e937edd0
📒 Files selected for processing (6)
src/main/java/org/example/visacasemanagementsystem/user/UserAuthorization.javasrc/main/java/org/example/visacasemanagementsystem/user/entity/User.javasrc/main/java/org/example/visacasemanagementsystem/user/mapper/UserMapper.javasrc/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.javasrc/main/java/org/example/visacasemanagementsystem/user/service/UserService.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/org/example/visacasemanagementsystem/user/mapper/UserMapper.java
- src/main/java/org/example/visacasemanagementsystem/user/entity/User.java
- src/main/java/org/example/visacasemanagementsystem/user/UserAuthorization.java
- src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java
Martin-E-Karlsson
left a comment
Martin-E-Karlsson
left a comment
There was a problem hiding this comment.
Bra jobbat, jag tror inte något behöver göras angående den sista code rabbit issuen.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.java (1)
68-71:⚠️ Potential issue | 🟠 MajorAddress authorId security gap now that Spring Security is integrated.
Line 70 creates a
CreateCommentDTOwith a client-suppliedauthorId.CommentService.createComment()(lines 52–53) still trustsdto.authorId()directly instead of resolving the author from the authenticated principal, leaving comment-author impersonation possible. Spring Security is now integrated across the codebase (@AuthenticationPrincipalis in use in other controllers); updateCreateCommentDTOto removeauthorId, refactorCommentService.createComment()to resolve the author fromSecurityContextHolderor injectUserPrincipal, and update this test to use the authenticated principal pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.java` around lines 68 - 71, Tests and service still accept a client-supplied authorId which allows impersonation; remove authorId from CreateCommentDTO and update CommentService.createComment(...) to resolve the author from the authenticated principal (use SecurityContextHolder.getContext().getAuthentication() or inject the UserPrincipal used across the app) instead of trusting dto.authorId(), then update CommentServiceIntegrationTest to build CreateCommentDTO without authorId and set up the test's security principal (or mock SecurityContext) so the service resolves the expected author; adjust any callers/controllers accordingly to stop passing authorId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.java`:
- Around line 68-71: Tests and service still accept a client-supplied authorId
which allows impersonation; remove authorId from CreateCommentDTO and update
CommentService.createComment(...) to resolve the author from the authenticated
principal (use SecurityContextHolder.getContext().getAuthentication() or inject
the UserPrincipal used across the app) instead of trusting dto.authorId(), then
update CommentServiceIntegrationTest to build CreateCommentDTO without authorId
and set up the test's security principal (or mock SecurityContext) so the
service resolves the expected author; adjust any callers/controllers accordingly
to stop passing authorId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19941a8e-0710-4202-b951-263f537c0023
📒 Files selected for processing (1)
src/test/java/org/example/visacasemanagementsystem/comment/CommentServiceIntegrationTest.java
3 participants
Adds the basis for the security layer, enabling the creation of users and restricting endpoints. By default endpoints are restricted to the SYSADMIN role, unless otherwise specified.
Summary by CodeRabbit
New Features
Security Improvements
Bug Fixes
Tests