Updated security för UserService, View and Graph#52
Conversation
WalkthroughThis update strengthens security and access control in user management controllers. In Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UserGraphQLController
participant SecurityContext
participant UserService
Client->>UserGraphQLController: updateUser(userId, input)
UserGraphQLController->>SecurityContext: Get current authenticated user
UserGraphQLController->>UserService: checkIfOwnerOrAdmin(userId, currentUser)
UserService-->>UserGraphQLController: Access allowed or exception thrown
UserGraphQLController->>UserService: Perform update
UserService-->>UserGraphQLController: Updated user data
UserGraphQLController-->>Client: Return updated user
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/org/example/springboot25/controller/UserGraphQLController.java (2)
28-31: Consider adding pagination togetAllUsersReturning the full user list in one GraphQL response can quickly degrade performance and expose excessive data as the table grows. Expose
page/size(or cursor‑based) arguments and delegate to a paginated service method.
34-40: InjectAuthenticationinstead of queryingSecurityContextHolderrepeatedlyEvery resolver re‑fetches the authentication and current user:
Authentication auth = SecurityContextHolder.getContext().getAuthentication(); User currentUser = userService.findUserByUserName(auth.getName());
- This is duplicated across three methods.
SecurityContextHolder.getContext()may returnnullin async execution unless@Asyncsecurity context propagation is enabled.- Spring will happily inject
Authentication(orPrincipal) directly.Suggested shape:
-@PreAuthorize("isAuthenticated()") -public UserOutputDTO getUserById(@Argument Long userId) { - Authentication auth = SecurityContextHolder.getContext().getAuthentication(); - User currentUser = userService.findUserByUserName(auth.getName()); +@PreAuthorize("isAuthenticated()") +public UserOutputDTO getUserById(@Argument Long userId, Authentication auth) { + User currentUser = userService.findUserByUserName(auth.getName());Extract the
currentUserlookup to a private helper or a dedicated@Componentto DRY the code.Also consider handling
NotFoundExceptionfromfindUserByUserNameand converting it to a GraphQL‑friendly error.Also applies to: 42-50, 51-59
src/main/java/org/example/springboot25/controller/UserViewController.java (1)
186-188: Deduplicate owner/admin verification & harden against missing authenticationThe same three‑line block:
User current = userService.findUserByUserName(SecurityContextHolder.getContext().getAuthentication().getName()); userService.checkIfOwnerOrAdmin(userId, current);is repeated four times. Besides breaching DRY, it assumes
getAuthentication()is non‑null. A signed‑out user hitting these endpoints would causeNullPointerExceptionbefore yourAccessDeniedHandlergets a chance.Refactor:
- Accept
Authentication authas a method parameter – Spring MVC will inject it.- Create a private method:
private User resolveAndAuthorize(Long targetId, Authentication auth) { User current = userService.findUserByUserName(auth.getName()); userService.checkIfOwnerOrAdmin(targetId, current); return current; }
- Reuse it in all mutating handlers.
This removes duplication and provides a single place to null‑check
auth.Also applies to: 208-210, 251-253, 269-271
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/example/springboot25/controller/UserGraphQLController.java(2 hunks)src/main/java/org/example/springboot25/controller/UserViewController.java(13 hunks)src/main/java/org/example/springboot25/service/UserService.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/org/example/springboot25/controller/UserGraphQLController.java (1)
src/main/java/org/example/springboot25/controller/AdminController.java (1)
PreAuthorize(19-158)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/main/java/org/example/springboot25/controller/UserGraphQLController.java (1)
65-65: Always returning true might hide errors.The
deleteUsermethod always returns true regardless of the actual outcome of the deletion operation. Consider returning the actual result of the deletion operation or using exceptions consistently for error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/example/springboot25/controller/UserGraphQLController.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/org/example/springboot25/controller/UserGraphQLController.java (1)
src/main/java/org/example/springboot25/controller/AdminController.java (1)
PreAuthorize(19-158)
🪛 GitHub Actions: Java CI with Maven
src/main/java/org/example/springboot25/controller/UserGraphQLController.java
[error] 38-38: NullPointerException: Cannot invoke "org.springframework.security.core.Authentication.getName()" because "auth" is null in method getUserById.
[error] 62-62: NullPointerException: Cannot invoke "org.springframework.security.core.Authentication.getName()" because "auth" is null in method deleteUser.
[error] 53-53: NullPointerException: Cannot invoke "org.springframework.security.core.Authentication.getName()" because "auth" is null in method updateUser.
[error] 62-62: NullPointerException: Cannot invoke "org.springframework.security.core.Authentication.getName()" because "auth" is null in method deleteUser (error case).
🔇 Additional comments (4)
src/main/java/org/example/springboot25/controller/UserGraphQLController.java (4)
7-7: Appropriate security imports added.The addition of necessary imports for User entity, PreAuthorize annotations, and SecurityContextHolder shows good preparation for implementing security controls throughout the controller.
Also applies to: 12-14
29-29: Good restriction of admin-only functionality.Restricting the
getAllUsers()operation to administrators only is an appropriate security measure as this prevents regular users from accessing the complete user database.
69-72:Details
✅ Verification successful
Reconsider authentication requirement for createUser.
Requiring authentication to create a user might prevent new users from registering. Consider whether this endpoint should be public for registration purposes, or if there's a separate registration endpoint for new users.
🏁 Script executed:
#!/bin/bash # Check if there's a separate registration endpoint without authentication echo "Checking for public registration endpoints..." rg -i "register|signup|sign.up|registration" --type javaLength of output: 1595
Clarify access for GraphQL createUser mutation
We’ve verified that a public registration endpoint already exists for UI sign‑ups:
- In
SecurityConfig.java,requestMatchers("/", "/about", "/register", …).permitAll()opens/register.UserViewController.javahandles GET/POST for/register.- Your GraphQL mutation
createUserremains secured by@PreAuthorize("isAuthenticated()").If your intent is to let end‑users self‑register via GraphQL, remove or relax the
@PreAuthorizeoncreateUser. Otherwise, it’s correct to keep this mutation authenticated and rely on the existing UI endpoint for new user sign‑up.
37-39:Details
✅ Verification successful
Verify consistent ownership and admin checks.
The newly added owner/admin checks are good for security but need to be implemented consistently across endpoints. Please verify that the
checkIfOwnerOrAdminmethod in the UserService handles all edge cases properly, such as when checking ownership of a non-existent user.Also applies to: 52-54, 61-63
🏁 Script executed:
#!/bin/bash # Look for the implementation of checkIfOwnerOrAdmin to review its behavior echo "Checking implementation of checkIfOwnerOrAdmin method..." rg -A 15 "checkIfOwnerOrAdmin" --type javaLength of output: 13277
Ownership/Admin Guards Verified
- The
checkIfOwnerOrAdminguard is consistently applied on the GraphQL endpoints
–getUserById(L37–39)
–updateUser(L52–54)
–deleteUser(L61–63)checkIfOwnerOrAdminproperly throwsAccessDeniedExceptionfor null or unauthorized inputs- Actual existence of the target user is validated downstream in the service (admins receive
NotFoundExceptionfor missing users; non‑owners/non‑admins always getAccessDeniedExceptionto avoid leaking existence)This behavior aligns with our security requirements.
🧰 Tools
🪛 GitHub Actions: Java CI with Maven
[error] 38-38: NullPointerException: Cannot invoke "org.springframework.security.core.Authentication.getName()" because "auth" is null in method getUserById.
| @PreAuthorize("isAuthenticated()") | ||
| public UserOutputDTO getUserByUserName(@Argument String userName) { | ||
| return userService.getUserDtoByUserName(userName); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Authentication requirement for user lookup.
Good addition of the authentication requirement, but consider whether the same ownership check that exists in getUserById() should also be applied here to prevent users from looking up arbitrary usernames.
- Fix Authentication null pointer issue - Fix Authentication null pointer in deleteUser - Fix Authentication null pointer in updateUser.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores