Conversation
WalkthroughThe changes introduce enhanced handling of user profile registration status during authentication and OAuth flows. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthController
participant AuthService
participant ProfileRepo
Client->>AuthController: POST /login (LoginRequest)
AuthController->>AuthService: login(request)
AuthService->>ProfileRepo: existsByUserId(userId)
AuthService-->>AuthController: LoginResponse(token, isProfileRegistered)
AuthController-->>Client: Set-Cookie: accessToken, JSON(LoginResponse)
sequenceDiagram
participant Client
participant OAuthController
participant OAuth2SuccessHandler
participant OAuthUserService
participant ProfileRepo
Client->>OAuthController: GET /login/{registrationId}?redirect=...
OAuthController-->>Client: Redirect to /api/v1/oauth2/authorization/{registrationId}?redirect=...
Client->>OAuth2SuccessHandler: OAuth2 login callback
OAuth2SuccessHandler->>OAuthUserService: loadUser()
OAuthUserService->>ProfileRepo: existsByUserId(userId)
OAuthUserService-->>OAuth2SuccessHandler: OAuthUser(isProfileRegistered)
OAuth2SuccessHandler-->>Client: Set-Cookie: accessToken, JSON(isProfileRegistered)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🪛 ast-grep (0.38.6)src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java[warning] 37-37: The application does not appear to verify inbound requests which can lead to a Cross-site request forgery (CSRF) vulnerability. If the application uses cookie-based authentication, an attacker can trick users into sending authenticated HTTP requests without their knowledge from any arbitrary domain they visit. To prevent this vulnerability start by identifying if the framework or library leveraged has built-in features or offers plugins for CSRF protection. CSRF tokens should be unique and securely random. The (cookie-missing-samesite-java) 🔇 Additional comments (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @wlgns12370, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly refines the authentication and authorization flow, particularly for OAuth. It shifts token delivery to secure HTTP-only cookies and enriches login responses with profile registration status, improving both security and user experience. Additionally, it streamlines the OAuth initiation process and enforces best practices for utility classes.
Highlights
- OAuth Flow Refinement: OAuth login tokens are now returned via HTTP-only cookies instead of URL query parameters, enhancing security and streamlining the client-side handling of tokens.
- Profile Registration Status: Both regular and OAuth login responses now include an
isProfileRegisteredboolean field, providing immediate feedback on whether a user has completed their profile setup. - Utility Class Instantiation Prevention: The
EncodeUtilclass has been updated with a private constructor to prevent direct instantiation, enforcing its role as a static utility class. - OAuth Initiation Endpoint Simplification: The
/oauth/login/{registrationId}endpoint now directly constructs and redirects to the Spring Security OAuth2 authorization URL, simplifying the OAuth initiation process for clients by offloading the URL construction to the backend. - OAuthUser Refactoring: The
OAuthUserclass no longer implementsUserDetails, simplifying its contract and focusing solely on OAuth-specific user details, while still providing necessary user information.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the OAuth login process to return the access token in a cookie and adds a flag to indicate if a user's profile is registered. I found a critical security issue where an authentication check was removed from a profile update endpoint. I've also pointed out several areas to improve code correctness and maintainability, such as fixing a bug in JSON serialization and improving exception handling.
|
|
||
| @PutMapping | ||
| @PreAuthorize("isAuthenticated()") | ||
| // @PreAuthorize("isAuthenticated()") |
There was a problem hiding this comment.
The @PreAuthorize("isAuthenticated()") annotation on the update method has been commented out. This disables authentication for the profile update endpoint, allowing any unauthenticated user to modify any user's profile by simply providing a userId. This is a critical security vulnerability.
Please reinstate this annotation to ensure that only authenticated users can modify profiles.
| // @PreAuthorize("isAuthenticated()") | |
| @PreAuthorize("isAuthenticated()") |
| } catch (final Exception e) { | ||
| throw new CustomRuntimeException(ErrorCode.INCORRECT_ID_OR_PASSWORD); | ||
| } |
There was a problem hiding this comment.
The catch (final Exception e) block is too broad. It catches all exceptions, including potential RuntimeExceptions, and masks them as an INCORRECT_ID_OR_PASSWORD error. This can make debugging difficult as it hides the true cause of an error.
It's better to catch the more specific AuthenticationException that is thrown by authenticate() on login failure. Other unexpected exceptions should be allowed to propagate to be handled by a global exception handler.
| } catch (final Exception e) { | |
| throw new CustomRuntimeException(ErrorCode.INCORRECT_ID_OR_PASSWORD); | |
| } | |
| } catch (final org.springframework.security.core.AuthenticationException e) { | |
| throw new CustomRuntimeException(ErrorCode.INCORRECT_ID_OR_PASSWORD); | |
| } |
| final UserJpaEntity oAuthUser = updateOrWrite(userInfo); | ||
|
|
||
| return new OAuthUser(oAuthUser, oAuth2UserAttributes, userNameAttributeName); | ||
| Boolean isProfileRegistered = profileRepository.existsByUserName((user.getName())); |
There was a problem hiding this comment.
The check for profile registration uses profileRepository.existsByUserName((user.getName())). This is inconsistent with AuthService, which uses existsByUserId. Using the user's name is brittle, as names are not guaranteed to be unique and can sometimes be changed by the user. It's much more robust to use the user's unique ID from the UserJpaEntity (oAuthUser).
| Boolean isProfileRegistered = profileRepository.existsByUserName((user.getName())); | |
| Boolean isProfileRegistered = profileRepository.existsByUserId(oAuthUser.getId()); |
| String jsonResponse = | ||
| "{\"isProfileRegistered\": \"" + oAuthUser.getIsProfileRegistered() + "\"}"; |
There was a problem hiding this comment.
Manually constructing JSON strings is error-prone. The boolean value isProfileRegistered is being serialized as a string (e.g., "true") instead of a boolean literal (true) because it's wrapped in quotes. This will likely cause parsing issues on the client-side.
While the best long-term solution is to use a JSON serialization library like Jackson (e.g., via ObjectMapper) to serialize a DTO or a Map, a quick fix is to remove the quotes around the boolean value.
| String jsonResponse = | |
| "{\"isProfileRegistered\": \"" + oAuthUser.getIsProfileRegistered() + "\"}"; | |
| String jsonResponse = | |
| "{\"isProfileRegistered\": " + oAuthUser.getIsProfileRegistered() + "}"; |
| public static LoginResponse of(final Token token, final Boolean isNewUser) { | ||
| return new LoginResponse(token, isNewUser); | ||
| } |
There was a problem hiding this comment.
The parameter name isNewUser in the of factory method is inconsistent with the record's field name, isProfileRegistered. This can be confusing. For better clarity and consistency, the parameter name should match the field it's intended for.
| public static LoginResponse of(final Token token, final Boolean isNewUser) { | |
| return new LoginResponse(token, isNewUser); | |
| } | |
| public static LoginResponse of(final Token token, final Boolean isProfileRegistered) { | |
| return new LoginResponse(token, isProfileRegistered); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.java (1)
1-65: Re-enable and register the AccessTokenFilter for JWT authenticationThe
AccessTokenFilteris fully commented out and isn’t registered in yourSecurityFilterChain, leaving all endpoints effectively unprotected.Locations to address:
src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.javais entirely commented out.src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java(around line 67) has no.addFilterBefore(...)or similar registration forAccessTokenFilter.Suggested fix:
- Uncomment and restore the
AccessTokenFilterclass.- In
SecurityConfig.securityFilterChain(...), register it before the standard authentication filter. For example:@Bean public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { http // ... existing config ... .addFilterBefore(new AccessTokenFilter(accessTokenService), UsernamePasswordAuthenticationFilter.class); return http.build(); }Please re-enable the filter or document the intended replacement to ensure JWT tokens are correctly validated.
🧹 Nitpick comments (1)
src/main/java/life/mosu/mosuserver/application/oauth/OAuthUser.java (1)
8-8: Verify the necessity of the @slf4j annotation.The
@Slf4jannotation was added but no logging statements are visible in the current code. Consider removing it if logging is not used, or add appropriate logging for OAuth operations.Also applies to: 13-13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/life/mosu/mosuserver/application/auth/AuthService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/oauth/OAuthUser.java(1 hunks)src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java(3 hunks)src/main/java/life/mosu/mosuserver/domain/profile/ProfileJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java(2 hunks)src/main/java/life/mosu/mosuserver/global/util/EncodeUtil.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java(3 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/AuthControllerDocs.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/oauth/OAuthController.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/profile/ProfileController.java(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.38.6)
src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java
[warning] 36-36: A cookie was detected without setting the 'HttpOnly' flag. The 'HttpOnly' flag for cookies instructs the browser to forbid client-side scripts from reading the cookie. Set the 'HttpOnly' flag by calling 'cookie.setHttpOnly(true);
Context: response.addCookie(cookie);
Note: [CWE-1004] Sensitive Cookie Without 'HttpOnly' Flag. [REFERENCES]
- https://owasp.org/www-community/HttpOnly
(cookie-missing-httponly-java)
[warning] 36-36: The application does not appear to verify inbound requests which can lead to a Cross-site request forgery (CSRF) vulnerability. If the application uses cookie-based authentication, an attacker can trick users into sending authenticated HTTP requests without their knowledge from any arbitrary domain they visit. To prevent this vulnerability start by identifying if the framework or library leveraged has built-in features or offers plugins for CSRF protection. CSRF tokens should be unique and securely random. The Synchronizer Token or Double Submit Cookie patterns with defense-in-depth mechanisms such as the sameSite cookie flag can help prevent CSRF. For more information, see: [Cross-site request forgery prevention](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Req\ uest_Forgery_Prevention_Cheat_Sheet.html).
Context: response.addCookie(cookie);
Note: [CWE-352] Cross-Site Request Forgery (CSRF). [REFERENCES]
- https://stackoverflow.com/questions/42717210/samesite-cookie-in-java-application
(cookie-missing-samesite-java)
[warning] 36-36: A cookie was detected without setting the 'secure' flag. The 'secure' flag for cookies prevents the client from transmitting the cookie over insecure channels such as HTTP. Set the 'secure' flag by calling '.setSecure(true);'.
Context: response.addCookie(cookie);
Note: [CWE-614] Sensitive Cookie in HTTPS Session Without 'Secure' Attribute. [REFERENCES]
- https://owasp.org/www-community/controls/SecureCookieAttribute
(cookie-missing-secure-flag-java)
🔇 Additional comments (21)
src/main/java/life/mosu/mosuserver/domain/profile/ProfileJpaRepository.java (1)
12-12: LGTM: New method for profile existence check by username.The
existsByUserNamemethod follows Spring Data JPA conventions and complements the existingexistsByUserIdmethod. This appears to support the OAuth flow changes where profile existence needs to be checked by username.src/main/java/life/mosu/mosuserver/application/auth/AuthService.java (3)
4-4: LGTM: ProfileJpaRepository dependency added for profile existence check.The new dependency is appropriately used to check profile registration status in the login flow.
27-27: LGTM: Login method return type updated to LoginResponse.The return type change from
TokentoLoginResponseprovides richer information to clients about both authentication and profile registration status.
37-39: LGTM: Profile registration status check and response construction.The logic correctly checks if the user has a registered profile and includes this information in the response. The use of
existsByUserIdis appropriate since we have the authenticated user's ID.src/main/java/life/mosu/mosuserver/presentation/auth/AuthControllerDocs.java (2)
9-9: LGTM: Import addition for LoginResponse DTOThe import aligns with the updated return type in the login method.
17-19: LGTM: Method signature and documentation updatesThe changes correctly reflect the new response structure that includes profile registration status. The operation description appropriately mentions the transition plan for cookie-based authentication and frontend coordination.
src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java (4)
9-9: LGTM: Import addition for LoginResponse DTOThe import aligns with the updated return type and response structure.
36-38: LGTM: Method signature and response handling updatesThe method signature change to return
LoginResponseand the variable renaming fromtokentoresponsecorrectly reflect the new response structure that includes profile registration status.
40-46: LGTM: Cookie configuration with proper security flagsThe cookie creation correctly extracts the access token from the nested token structure and applies appropriate security settings (httpOnly, secure, path, maxAge).
50-50: LGTM: Response body updated to use LoginResponseThe response body correctly wraps the
LoginResponseobject, maintaining consistency with the updated return type.src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java (3)
6-6: LGTM: Import addition for ProfileJpaRepositoryThe import is necessary for the profile registration check functionality.
23-23: LGTM: ProfileJpaRepository dependency additionThe dependency is properly injected for checking profile registration status.
43-46: LGTM: Profile registration check and OAuthUser constructor updateThe profile registration check is performed at the appropriate time after user retrieval/creation, and the result is correctly passed to the OAuthUser constructor. This aligns with the PR objective of tracking profile registration status.
src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java (3)
3-3: LGTM: Import addition for CookieThe import is necessary for the new cookie-based token delivery mechanism.
33-34: LGTM: JSON response constructionThe JSON response correctly includes the profile registration status from the OAuthUser object.
39-42: LGTM: Response handling change from redirect to JSONThe change from redirect-based to direct JSON response aligns with the PR objective of improving OAuth handling. The response includes proper content type and encoding settings.
src/main/java/life/mosu/mosuserver/presentation/oauth/OAuthController.java (4)
3-3: LGTM: Import addition for redirect parameter keyThe import provides access to the constant used for redirect parameter consistency.
8-8: LGTM: Import addition for PathVariableThe import is necessary for the new path variable parameter in the login method.
12-12: LGTM: Import addition for RedirectViewThe import is necessary for the new return type that simplifies redirect handling.
21-31: LGTM: OAuth endpoint refactoringThe refactoring improves the OAuth flow by:
- Using a cleaner URL structure with path variables
- Simplifying URL construction with UriComponentsBuilder
- Using RedirectView for better Spring MVC integration
The endpoint now provides a more intuitive entry point for OAuth login flows.
src/main/java/life/mosu/mosuserver/application/oauth/OAuthUser.java (1)
14-14: Verify removal of UserDetails interface from OAuthUserAutomated search found no remaining calls or casts to UserDetails APIs on OAuthUser. Since dropping UserDetails is a breaking change in our authentication layer, please manually confirm that:
- No
UserDetailsService,AuthenticationProvider, or custom security filters still expectOAuthUserto implement UserDetails- All authorization logic now relies on
OAuth2Usermethods/attributes instead ofUserDetails- Any uses of
getUsername(),getPassword(),isAccountNonExpired(), etc., have been updated accordingly
| public EncodeUtil() { | ||
| throw new UnsupportedOperationException(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the constructor private and provide a more descriptive error message.
The approach to prevent instantiation is correct, but it can be improved following Java best practices for utility classes.
- public EncodeUtil() {
- throw new UnsupportedOperationException();
- }
+ private EncodeUtil() {
+ throw new UnsupportedOperationException("This is a utility class and cannot be instantiated");
+ }Making the constructor private prevents external instantiation attempts and provides a clearer intent, while the descriptive error message helps developers understand why the exception occurs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public EncodeUtil() { | |
| throw new UnsupportedOperationException(); | |
| } | |
| private EncodeUtil() { | |
| throw new UnsupportedOperationException("This is a utility class and cannot be instantiated"); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/util/EncodeUtil.java around lines 7
to 9, change the constructor from public to private to prevent external
instantiation properly. Also, update the exception message in the constructor to
a more descriptive one that clearly explains why instantiation is not allowed,
following Java best practices for utility classes.
|
|
||
| @PutMapping | ||
| @PreAuthorize("isAuthenticated()") | ||
| // @PreAuthorize("isAuthenticated()") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: Authentication check removed from profile update endpoint.
The @PreAuthorize("isAuthenticated()") annotation was commented out, which removes authentication requirements from the profile update endpoint. This could allow unauthenticated users to modify profiles.
Please verify this change is intentional and that alternative authentication mechanisms are in place.
🏁 Script executed:
#!/bin/bash
# Verify if there are alternative authentication mechanisms for this endpoint
ast-grep --pattern 'class ProfileController {
$$$
@PutMapping
$$$
public ResponseEntity<$_> update($_) {
$$$
}
$$$
}'Length of output: 135
🏁 Script executed:
#!/bin/bash
# Locate and inspect ProfileController for authentication annotations on the update endpoint
FILE="src/main/java/life/mosu/mosuserver/presentation/profile/ProfileController.java"
if [[ -f "$FILE" ]]; then
echo "----- Begin ProfileController.java (lines 1–200) -----"
sed -n '1,200p' "$FILE"
echo "----- End excerpt -----"
echo
echo "----- Security annotations in ProfileController.java -----"
rg -n "@PreAuthorize|@Secured|@RolesAllowed" "$FILE" || echo "(none found)"
echo
echo "----- @PutMapping occurrences in ProfileController.java -----"
rg -n "@PutMapping" "$FILE" || echo "(none found)"
else
echo "Error: File not found: $FILE"
exit 1
fiLength of output: 3303
Critical: Profile update endpoint is unprotected and accepts arbitrary user IDs
Observations:
- In
ProfileController.java(line 40), the@PreAuthorize("isAuthenticated()")annotation on theupdatemethod is commented out. - The
updatemethod still takes@RequestParam Long userId, allowing any caller to specify and modify another user’s profile. - No other security annotations (e.g.
@Secured,@RolesAllowed) or global config protect this endpoint.
Recommended fixes:
- Reinstate the authentication check on the
updatemethod (e.g. restore@PreAuthorize("isAuthenticated()")). - Replace
@RequestParam Long userIdwith@UserId Long userIdso the user ID is derived from the authenticated principal. - Review your Spring Security configuration to ensure all
/profileendpoints require authentication.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/profile/ProfileController.java
at line 40, the update method is unprotected and accepts a userId parameter from
the request, allowing unauthorized profile modifications. To fix this, uncomment
and restore the @PreAuthorize("isAuthenticated()") annotation on the update
method to enforce authentication. Replace the @RequestParam Long userId
parameter with @UserId Long userId to derive the user ID from the authenticated
principal instead of the request. Additionally, verify the Spring Security
configuration to ensure all /profile endpoints require authentication.
src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java
Show resolved
Hide resolved
| final String attributeKey, | ||
| final Boolean isProfileRegistered | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation and maintain consistency with field naming.
If the field naming is updated as suggested, ensure the constructor parameter name is also updated for consistency. Consider adding validation for the new parameter.
public OAuthUser(
final UserJpaEntity user,
final Map<String, Object> attributes,
- final String attributeKey,
- final Boolean isProfileRegistered
+ final String attributeKey,
+ final boolean profileRegistered
) {
this.user = user;
this.attributes = attributes;
this.attributeKey = attributeKey;
- this.isProfileRegistered = isProfileRegistered;
+ this.profileRegistered = profileRegistered;
}Also applies to: 32-32
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/oauth/OAuthUser.java around
lines 26 to 28 and line 32, update the constructor parameter name to match the
new field naming for consistency. Additionally, add validation logic for the new
parameter to ensure it meets expected criteria before assignment, such as
checking for null or invalid values, to maintain robustness.
| @Getter | ||
| Boolean isProfileRegistered; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix field naming convention and consider type choice.
The field has several issues:
- Naming:
isProfileRegistereddoesn't follow Java conventions. Boolean fields should not use "is" prefix as it creates awkward getter names (getIsProfileRegistered()). ConsiderprofileRegisteredinstead. - Type: Using
Booleaninstead ofbooleanintroduces potential null values. If nullability is intentional, document it; otherwise, use primitiveboolean. - Modifiers: The field should be
privateandfinalfor better encapsulation and immutability.
- @Getter
- Boolean isProfileRegistered;
+ @Getter
+ private final boolean profileRegistered;Then update the constructor parameter accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Getter | |
| Boolean isProfileRegistered; | |
| @Getter | |
| private final boolean profileRegistered; |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/oauth/OAuthUser.java around
lines 20 to 21, rename the field from isProfileRegistered to profileRegistered
to follow Java boolean naming conventions and avoid awkward getter names. Change
its type from Boolean to primitive boolean unless null values are required, in
which case document the nullability. Also, make the field private and final to
improve encapsulation and immutability. Finally, update the constructor
parameter to match the new field name and type.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.java (1)
1-65: Security filter disabled—restore or replace authentication enforcementThe
AccessTokenFilterclass is fully commented out, and no other request filter or Spring Security configuration was found. This leaves all protected endpoints unguarded:
src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.javais entirely commented out- No other classes extend
OncePerRequestFilterfor authentication- No
@EnableWebSecurityorSecurityFilterChain/WebSecurityConfigurerAdapterconfiguration present- Controller methods still show commented-out
@PreAuthorizeannotationsPlease re-enable the token filter or introduce a new Spring Security configuration (for example, a
SecurityFilterChainbean or a subclass ofWebSecurityConfigurerAdapter) and ensure method-level security (@EnableMethodSecurityor equivalent) is active so that all protected routes require valid tokens.
♻️ Duplicate comments (5)
src/main/java/life/mosu/mosuserver/presentation/profile/ProfileController.java (1)
40-40: Critical security vulnerability: Authentication check is disabled.The
@PreAuthorize("isAuthenticated()")annotation has been commented out, allowing unauthenticated users to modify any profile by providing auserIdparameter. This is a critical security vulnerability that must be addressed immediately.src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginResponse.java (1)
8-10: Fix parameter name inconsistency in factory method.The parameter name
isNewUserin the factory method doesn't match the record field nameisProfileRegistered. This inconsistency is confusing and could lead to misunderstandings.- public static LoginResponse of(final Token token, final Boolean isNewUser) { - return new LoginResponse(token, isNewUser); + public static LoginResponse of(final Token token, final Boolean isProfileRegistered) { + return new LoginResponse(token, isProfileRegistered);src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java (1)
43-43: Use userId instead of userName for profile existence check.The profile registration check uses
existsByUserName(user.getName())which is inconsistent withAuthServicethat usesexistsByUserId. Using username is brittle as names may not be unique and can change.- Boolean isProfileRegistered = profileRepository.existsByUserName((user.getName())); + Boolean isProfileRegistered = profileRepository.existsByUserId(oAuthUser.getId());This approach is more robust and consistent with the rest of the application.
src/main/java/life/mosu/mosuserver/application/auth/AuthService.java (1)
40-42: Address the overly broad exception handling as noted in previous review.The broad
catch (final Exception e)block masks all exceptions asINCORRECT_ID_OR_PASSWORDerrors, making debugging difficult. This issue was previously flagged but remains unresolved.src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java (1)
33-34: Fix the JSON boolean serialization issue as noted in previous review.The boolean value
isProfileRegisteredis being serialized as a string instead of a boolean literal due to the surrounding quotes. This will cause parsing issues on the client-side.
🧹 Nitpick comments (1)
src/main/java/life/mosu/mosuserver/application/oauth/OAuthUser.java (1)
8-8: Remove unused @slf4j annotation and import.The
@Slf4jannotation and import are not being used in this class. Consider removing them to keep the code clean and avoid unnecessary dependencies.-import lombok.extern.slf4j.Slf4j; -@Slf4j public class OAuthUser implements OAuth2User {Also applies to: 13-13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/life/mosu/mosuserver/application/auth/AuthService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/oauth/OAuthUser.java(1 hunks)src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java(3 hunks)src/main/java/life/mosu/mosuserver/domain/profile/ProfileJpaRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java(2 hunks)src/main/java/life/mosu/mosuserver/global/util/EncodeUtil.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java(3 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/AuthControllerDocs.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/oauth/OAuthController.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/profile/ProfileController.java(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.38.6)
src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java
[warning] 36-36: The application does not appear to verify inbound requests which can lead to a Cross-site request forgery (CSRF) vulnerability. If the application uses cookie-based authentication, an attacker can trick users into sending authenticated HTTP requests without their knowledge from any arbitrary domain they visit. To prevent this vulnerability start by identifying if the framework or library leveraged has built-in features or offers plugins for CSRF protection. CSRF tokens should be unique and securely random. The Synchronizer Token or Double Submit Cookie patterns with defense-in-depth mechanisms such as the sameSite cookie flag can help prevent CSRF. For more information, see: [Cross-site request forgery prevention](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Req\ uest_Forgery_Prevention_Cheat_Sheet.html).
Context: response.addCookie(cookie);
Note: [CWE-352] Cross-Site Request Forgery (CSRF). [REFERENCES]
- https://stackoverflow.com/questions/42717210/samesite-cookie-in-java-application
(cookie-missing-samesite-java)
[warning] 36-36: A cookie was detected without setting the 'HttpOnly' flag. The 'HttpOnly' flag for cookies instructs the browser to forbid client-side scripts from reading the cookie. Set the 'HttpOnly' flag by calling 'cookie.setHttpOnly(true);
Context: response.addCookie(cookie);
Note: [CWE-1004] Sensitive Cookie Without 'HttpOnly' Flag. [REFERENCES]
- https://owasp.org/www-community/HttpOnly
(cookie-missing-httponly-java)
[warning] 36-36: A cookie was detected without setting the 'secure' flag. The 'secure' flag for cookies prevents the client from transmitting the cookie over insecure channels such as HTTP. Set the 'secure' flag by calling '.setSecure(true);'.
Context: response.addCookie(cookie);
Note: [CWE-614] Sensitive Cookie in HTTPS Session Without 'Secure' Attribute. [REFERENCES]
- https://owasp.org/www-community/controls/SecureCookieAttribute
(cookie-missing-secure-flag-java)
🔇 Additional comments (10)
src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java (1)
45-46: Constructor call properly updated to include profile registration status.The OAuthUser constructor call correctly passes the profile registration status, aligning with the enhanced authentication flow requirements.
src/main/java/life/mosu/mosuserver/presentation/auth/AuthControllerDocs.java (1)
9-9: LGTM! Clean interface update for new response type.The changes properly update the interface to use
LoginResponseinstead ofToken, and the expanded operation description provides helpful context about the transition plan with the frontend team.Also applies to: 17-19
src/main/java/life/mosu/mosuserver/application/auth/AuthService.java (1)
24-24: LGTM! Profile registration check is properly implemented.The addition of
ProfileJpaRepositorydependency and the profile registration check logic are correctly implemented. TheLoginResponse.of()method call properly combines the token and profile status.Also applies to: 37-39
src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java (1)
36-51: LGTM! Controller properly updated for new response structure.The changes correctly update the controller to handle
LoginResponseinstead ofToken. The cookie creation properly extracts the access token from the nested structure (response.token().accessToken()), and the response body is appropriately updated.src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java (1)
39-42: LGTM! Improved approach with JSON response instead of redirect.The change from redirect-based token delivery to cookie-based with JSON response is a better security practice and provides cleaner client-side handling.
src/main/java/life/mosu/mosuserver/presentation/oauth/OAuthController.java (1)
21-32: LGTM! Cleaner OAuth endpoint implementation.The refactoring improves the OAuth flow with:
- More RESTful path variable approach for
registrationId- Safer URL construction using
UriComponentsBuilder- Appropriate use of
RedirectViewinstead of manual redirect headers- Clean static import usage for the redirect parameter key
src/main/java/life/mosu/mosuserver/application/oauth/OAuthUser.java (4)
14-14: Good architectural simplification.Removing the
UserDetailsinterface implementation simplifies the class responsibility to focus solely on OAuth2 user representation, which aligns with the single responsibility principle.
26-27: Constructor parameter type should match field type recommendation.The constructor parameter type is consistent with the field declaration, but if the field type is changed to
booleanprimitive as suggested above, this parameter should also be changed accordingly.
32-32: Correct field assignment implementation.The field assignment follows standard constructor initialization patterns and correctly assigns the parameter value to the instance field.
14-52: Well-structured refactoring with clear purpose.The overall refactoring successfully:
- Simplifies class responsibility by removing
UserDetailsinterface- Adds profile registration status tracking capability
- Maintains clean object-oriented design
- Integrates well with the broader OAuth handling improvements
The changes align with the PR objectives to enhance OAuth processing procedures.
src/main/java/life/mosu/mosuserver/domain/profile/ProfileJpaRepository.java
Outdated
Show resolved
Hide resolved
| Cookie cookie = new Cookie("accessToken", accessToken); | ||
| response.addCookie(cookie); |
There was a problem hiding this comment.
Add security flags to the access token cookie.
The cookie is missing critical security flags that could lead to vulnerabilities:
HttpOnlyflag to prevent client-side script accessSecureflag to ensure transmission only over HTTPSSameSiteflag to prevent CSRF attacks
Apply this diff to add the security flags:
- Cookie cookie = new Cookie("accessToken", accessToken);
- response.addCookie(cookie);
+ Cookie cookie = new Cookie("accessToken", accessToken);
+ cookie.setHttpOnly(true);
+ cookie.setSecure(true);
+ cookie.setPath("/");
+ response.addCookie(cookie);Note: For SameSite attribute, consider using ResponseCookie instead of Cookie as it provides better control over modern cookie attributes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Cookie cookie = new Cookie("accessToken", accessToken); | |
| response.addCookie(cookie); | |
| Cookie cookie = new Cookie("accessToken", accessToken); | |
| cookie.setHttpOnly(true); | |
| cookie.setSecure(true); | |
| cookie.setPath("/"); | |
| response.addCookie(cookie); |
🧰 Tools
🪛 ast-grep (0.38.6)
[warning] 36-36: The application does not appear to verify inbound requests which can lead to a Cross-site request forgery (CSRF) vulnerability. If the application uses cookie-based authentication, an attacker can trick users into sending authenticated HTTP requests without their knowledge from any arbitrary domain they visit. To prevent this vulnerability start by identifying if the framework or library leveraged has built-in features or offers plugins for CSRF protection. CSRF tokens should be unique and securely random. The Synchronizer Token or Double Submit Cookie patterns with defense-in-depth mechanisms such as the sameSite cookie flag can help prevent CSRF. For more information, see: [Cross-site request forgery prevention](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Req\ uest_Forgery_Prevention_Cheat_Sheet.html).
Context: response.addCookie(cookie);
Note: [CWE-352] Cross-Site Request Forgery (CSRF). [REFERENCES]
- https://stackoverflow.com/questions/42717210/samesite-cookie-in-java-application
(cookie-missing-samesite-java)
[warning] 36-36: A cookie was detected without setting the 'HttpOnly' flag. The 'HttpOnly' flag for cookies instructs the browser to forbid client-side scripts from reading the cookie. Set the 'HttpOnly' flag by calling 'cookie.setHttpOnly(true);
Context: response.addCookie(cookie);
Note: [CWE-1004] Sensitive Cookie Without 'HttpOnly' Flag. [REFERENCES]
- https://owasp.org/www-community/HttpOnly
(cookie-missing-httponly-java)
[warning] 36-36: A cookie was detected without setting the 'secure' flag. The 'secure' flag for cookies prevents the client from transmitting the cookie over insecure channels such as HTTP. Set the 'secure' flag by calling '.setSecure(true);'.
Context: response.addCookie(cookie);
Note: [CWE-614] Sensitive Cookie in HTTPS Session Without 'Secure' Attribute. [REFERENCES]
- https://owasp.org/www-community/controls/SecureCookieAttribute
(cookie-missing-secure-flag-java)
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java
around lines 36 to 37, the access token cookie lacks essential security flags.
Replace the use of javax.servlet.http.Cookie with
org.springframework.http.ResponseCookie to set the cookie, and configure it with
HttpOnly, Secure, and SameSite attributes. Then add the cookie to the response
header instead of using response.addCookie. This ensures the cookie is protected
against client-side script access, only sent over HTTPS, and mitigates CSRF
risks.
| @Getter | ||
| Boolean isProfileRegistered; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using primitive boolean type for better null safety.
Using Boolean wrapper type allows null values which could lead to null pointer exceptions. If the profile registration status should always have a definite true/false value, consider using boolean primitive type instead.
@Getter
-Boolean isProfileRegistered;
+boolean isProfileRegistered;If null values are intentionally allowed for this field, please add documentation explaining when and why this field would be null.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Getter | |
| Boolean isProfileRegistered; | |
| @Getter | |
| boolean isProfileRegistered; |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/oauth/OAuthUser.java at lines
20-21, the field isProfileRegistered is declared as a Boolean wrapper type which
allows null values and may cause null pointer exceptions. Change the type from
Boolean to the primitive boolean to ensure it always holds a definite true or
false value. If null is intentionally allowed, add clear documentation
explaining the circumstances under which this field can be null.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Refactor