Conversation
# Conflicts: # src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java # src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.java
WalkthroughThis update removes the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OAuthController
participant SpringSecurity
participant AuthorizationRequestRedirectResolver
participant OAuth2Provider
Client->>OAuthController: GET /oauth/login/{registrationId}?redirect=...
OAuthController->>Client: Redirect to /api/v1/oauth2/authorization/{registrationId}?redirect=...
Client->>SpringSecurity: GET /api/v1/oauth2/authorization/{registrationId}?redirect=...
SpringSecurity->>AuthorizationRequestRedirectResolver: resolve(request)
AuthorizationRequestRedirectResolver->>SpringSecurity: OAuth2AuthorizationRequest with state (includes redirect)
SpringSecurity->>OAuth2Provider: Redirect to provider with OAuth2 request
OAuth2Provider->>SpringSecurity: Callback to /oauth2/callback/{registrationId}
SpringSecurity->>Client: OAuth2 login success/failure
Poem
✨ 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 primarily refactors the Spring Security configuration to enhance OAuth2 integration. It introduces a mechanism for dynamic post-login redirection and streamlines the security filter chain by removing previously used custom token filters. A new API endpoint is also added to facilitate this enhanced OAuth2 flow, and security access rules are temporarily relaxed for testing.
Highlights
- OAuth2 Custom Redirect Handling: I've implemented a custom
AuthorizationRequestRedirectResolverto allow dynamic redirection after OAuth2 login. This resolver captures aredirectquery parameter from the initial authorization request and embeds it into the OAuth2 state, enabling the application to redirect the user to a specific URL post-authentication. - Security Filter Chain Refactoring: The custom
AccessTokenFilterandTokenExceptionFilterhave been removed (commented out) from the Spring Security filter chain. This simplifies the authentication flow, indicating a potential shift in how token validation and exception handling are managed or that these filters are no longer required for the current scope. - New OAuth Login Endpoint: A new REST controller,
OAuthController, has been introduced with a/oauth/login/{registrationId}endpoint. This endpoint provides a custom entry point for initiating OAuth2 login, allowing clients to specify aredirectURL as a query parameter, which is then handled by the newAuthorizationRequestRedirectResolver. - Relaxed Security Authorization for Testing: For testing purposes, the
SecurityConfighas been modified to temporarily set all API endpoints topermitAll(), commenting out previous role-based access controls for/api/v1/profile/**and/api/v1/admin/**.
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
The pull request introduces API integration for OAuth2. It includes changes to the security configuration, OAuth2 failure handler, a redirect resolver, and a new OAuth controller. The changes aim to streamline the OAuth2 flow and improve error logging. Some adjustments to URI paths are suggested for better consistency.
| log.info("OAuth2 Error " + error.getErrorCode()); | ||
| log.info("OAuth2 Description " + error.getDescription()); |
There was a problem hiding this comment.
Consider using a single log statement with placeholders for better readability and performance. This avoids multiple string concatenations.
| log.info("OAuth2 Error " + error.getErrorCode()); | |
| log.info("OAuth2 Description " + error.getDescription()); | |
| log.info("OAuth2 Error: {}, Description: {}", error.getErrorCode(), error.getDescription()); |
| ) | ||
| .oauth2Login(oauth2 -> oauth2.redirectionEndpoint(redirection -> | ||
| redirection.baseUri("/api/v1/oauth2/callback/{registrationId}")) | ||
| redirection.baseUri("/oauth2/callback/{registrationId}")) |
There was a problem hiding this comment.
The base URI for redirection endpoint should start with /oauth2 instead of /api/v1/oauth2 to align with the controller mapping and overall OAuth2 configuration. This ensures consistency and avoids potential routing issues.
redirection.baseUri("/oauth2/callback/{registrationId}")| @RequestParam(REDIRECT_PARAM_KEY) String redirect | ||
| ) { | ||
| final String url = UriComponentsBuilder | ||
| .fromPath("/api/v1/oauth2/authorization/{registrationId}") |
There was a problem hiding this comment.
The path should be /oauth2/authorization/{registrationId} instead of /api/v1/oauth2/authorization/{registrationId} to align with the security configuration and avoid unnecessary API versioning in the OAuth2 flow. This ensures consistency and simplifies the routing.
fromPath("/oauth2/authorization/{registrationId}")
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/main/java/life/mosu/mosuserver/presentation/oauth/OAuthController.java (1)
26-30: Consider extracting the OAuth2 authorization base pathThe hardcoded path
/api/v1/oauth2/authorization/{registrationId}could be externalized to configuration for better maintainability.+@Value("${oauth2.authorization.base-path:/api/v1/oauth2/authorization}") +private String oauthAuthorizationBasePath; final String url = UriComponentsBuilder - .fromPath("/api/v1/oauth2/authorization/{registrationId}") + .fromPath(oauthAuthorizationBasePath + "/{registrationId}") .queryParam(REDIRECT_PARAM_KEY, redirect) .buildAndExpand(registrationId) .toUriString();src/main/java/life/mosu/mosuserver/global/resolver/AuthorizationRequestRedirectResolver.java (1)
60-70: Consider handling malformed existing stateThe
appendToStatemethod assumes the existing state is a valid query string format, but doesn't handle potential parsing errors.private String appendToState( String state, Map<String, String> additionalParams ) { UriComponentsBuilder uriBuilder = UriComponentsBuilder.newInstance(); if (state != null) { - uriBuilder.query(state); + try { + uriBuilder.query(state); + } catch (IllegalArgumentException e) { + log.warn("Malformed existing state parameter: {}", state); + // Continue with empty state + } } additionalParams.forEach(uriBuilder::queryParam); return uriBuilder.build().getQuery(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java(3 hunks)src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginFailureHandler.java(1 hunks)src/main/java/life/mosu/mosuserver/global/resolver/AuthorizationRequestRedirectResolver.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(1 hunks)src/main/java/life/mosu/mosuserver/presentation/oauth/TokenExceptionFilter.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/life/mosu/mosuserver/global/resolver/AuthorizationRequestRedirectResolver.java (2)
src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginFailureHandler.java (1)
Slf4j(14-29)src/main/java/life/mosu/mosuserver/presentation/oauth/OAuthController.java (1)
Slf4j(15-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_ci
🔇 Additional comments (6)
src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginFailureHandler.java (1)
23-25: LGTM: Improved logging formatThe logging improvements add better readability with proper spacing and additional error details through
toString().src/main/java/life/mosu/mosuserver/presentation/oauth/TokenExceptionFilter.java (1)
1-30: TokenExceptionFilter removal is safe – CustomRuntimeException is handled globallyGlobalExceptionHandler (src/main/java/life/mosu/mosuserver/global/exception/GlobalExceptionHandler.java) already defines
@ExceptionHandler(CustomRuntimeException.class) to translate all CustomRuntimeException instances into HTTP responses.
OAuth2AuthenticationException is handled by OAuth2LoginFailureHandler.No further action required.
src/main/java/life/mosu/mosuserver/global/resolver/AuthorizationRequestRedirectResolver.java (2)
38-58: LGTM: Well-implemented OAuth2 redirect handlingThe implementation correctly validates redirect URLs and safely appends them to the OAuth2 state parameter with proper null checks and logging.
54-57: Project uses Java 21 toolchain – Map.of is compatible
Thebuild.gradlespecifies a Java toolchain oflanguageVersion = JavaLanguageVersion.of(21), soMap.of()(Java 9+) is fully supported. No changes needed here.src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java (2)
97-97: LGTM! Logout configuration properly implementedThe logout configuration with redirect to root path ("/") is a good addition that allows users to properly terminate their sessions.
85-93: OAuth2 redirect URI and custom resolver correctly configuredAll changes have been verified and are wired as expected:
SecurityConfig.securityFilterChainuses redirectionEndpoint.baseUri("/oauth2/callback/{registrationId}") with no leftover references to the old/api/v1/oauth2/callbackpath.AuthorizationRequestRedirectResolveris annotated with@Componentand receives theDefaultOAuth2AuthorizationRequestResolverbean defined inOAuth2AuthorizationRequestResolverConfig.DefaultOAuth2AuthorizationRequestResolveris exposed via a@Configurationclass, ensuring proper injection.No further action required.
| //package life.mosu.mosuserver.presentation.oauth; | ||
| // | ||
| //import static org.springframework.http.HttpHeaders.AUTHORIZATION; | ||
| // | ||
| //import jakarta.servlet.FilterChain; | ||
| //import jakarta.servlet.ServletException; | ||
| //import jakarta.servlet.http.HttpServletRequest; | ||
| //import jakarta.servlet.http.HttpServletResponse; | ||
| //import java.io.IOException; | ||
| //import life.mosu.mosuserver.application.auth.AccessTokenService; | ||
| //import lombok.RequiredArgsConstructor; | ||
| //import org.springframework.beans.factory.annotation.Value; | ||
| //import org.springframework.security.core.Authentication; | ||
| //import org.springframework.security.core.context.SecurityContextHolder; | ||
| //import org.springframework.stereotype.Component; | ||
| //import org.springframework.util.ObjectUtils; | ||
| //import org.springframework.web.filter.OncePerRequestFilter; | ||
| // | ||
| //@Component | ||
| //@RequiredArgsConstructor | ||
| //public class AccessTokenFilter extends OncePerRequestFilter { | ||
| // | ||
| // private static final String TOKEN_PREFIX = "Bearer "; | ||
| // private final AccessTokenService accessTokenService; | ||
| // | ||
| // @Value("${endpoints.reissue}") | ||
| // private String reissueEndpoint; | ||
| // | ||
| // @Override | ||
| // protected void doFilterInternal( | ||
| // final HttpServletRequest request, | ||
| // final HttpServletResponse response, | ||
| // final FilterChain filterChain | ||
| // ) throws ServletException, IOException { | ||
| // if (request.getRequestURI().equals(reissueEndpoint)) { | ||
| // filterChain.doFilter(request, response); | ||
| // return; | ||
| // } | ||
| // | ||
| // if (request.getRequestURI().startsWith("/api/v1/oauth2")) { | ||
| // filterChain.doFilter(request, response); | ||
| // return; | ||
| // } | ||
| // | ||
| // final String accessToken = resolveToken(request); | ||
| // if (accessToken != null) { | ||
| // setAuthentication(accessToken); | ||
| // } | ||
| // filterChain.doFilter(request, response); | ||
| // } | ||
| // | ||
| // private void setAuthentication(final String accessToken) { | ||
| // final Authentication authentication = accessTokenService.getAuthentication(accessToken); | ||
| // SecurityContextHolder.getContext().setAuthentication(authentication); | ||
| // } | ||
| // | ||
| // private String resolveToken(final HttpServletRequest request) { | ||
| // final String token = request.getHeader(AUTHORIZATION); | ||
| // if (ObjectUtils.isEmpty(token) || !token.startsWith(TOKEN_PREFIX)) { | ||
| // return null; | ||
| // } | ||
| // return token.substring(TOKEN_PREFIX.length()); | ||
| // } | ||
| //} No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: Verify access token validation is maintained
The entire AccessTokenFilter has been commented out, which previously handled Bearer token validation and set authentication context. This is a critical security change that could leave API endpoints unprotected.
🏁 Script executed:
#!/bin/bash
# Description: Verify access token validation is handled elsewhere
# Expected: Find alternative token validation mechanisms
echo "Searching for access token validation..."
rg -A 5 -B 5 "Bearer\|Authorization\|accessToken" --type java
echo -e "\nSearching for security filter configurations..."
rg -A 10 -B 5 "addFilterBefore\|addFilterAfter\|SecurityFilterChain" --type java
echo -e "\nSearching for authentication mechanisms..."
rg -A 5 -B 5 "@PreAuthorize\|@Secured\|SecurityContext" --type javaLength of output: 519
Critical: AccessTokenFilter disabled with no fallback token validation
Our search across the codebase did not uncover any other Bearer-token validation, SecurityFilterChain setup, or use of SecurityContext, @PreAuthorize, etc. Commenting out AccessTokenFilter means API endpoints are effectively unprotected.
• File to fix:
– src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.java (lines 1–64)
• Action required:
– Re-enable this filter or implement an alternative security configuration (e.g. register a SecurityFilterChain with a JWT/Auth filter) to restore access-token validation.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.java
lines 1 to 64, the entire AccessTokenFilter class is commented out, disabling
bearer token validation and leaving API endpoints unprotected. To fix this,
uncomment the entire class including its annotations, imports, and methods to
re-enable the filter. Ensure it is properly registered as a Spring component so
that it intercepts requests and validates access tokens as intended.
| @GetMapping("/login/{registrationId}") | ||
| public RedirectView login( | ||
| @PathVariable String registrationId, | ||
| @RequestParam(REDIRECT_PARAM_KEY) String redirect | ||
| ) { | ||
| final String url = UriComponentsBuilder | ||
| .fromPath("/api/v1/oauth2/authorization/{registrationId}") | ||
| .queryParam(REDIRECT_PARAM_KEY, redirect) | ||
| .buildAndExpand(registrationId) | ||
| .toUriString(); | ||
| return new RedirectView(url); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and security logging
The controller lacks input validation and security logging, which could lead to security vulnerabilities and difficult debugging.
@GetMapping("/login/{registrationId}")
public RedirectView login(
@PathVariable String registrationId,
@RequestParam(REDIRECT_PARAM_KEY) String redirect
) {
+ log.info("OAuth login request for registrationId: {}, redirect: {}", registrationId, redirect);
+
+ // Validate registrationId to prevent path traversal
+ if (!registrationId.matches("^[a-zA-Z0-9_-]+$")) {
+ throw new IllegalArgumentException("Invalid registrationId format");
+ }
+
final String url = UriComponentsBuilder
.fromPath("/api/v1/oauth2/authorization/{registrationId}")
.queryParam(REDIRECT_PARAM_KEY, redirect)
.buildAndExpand(registrationId)
.toUriString();
+
+ log.debug("Redirecting to OAuth2 authorization URL: {}", url);
return new RedirectView(url);
}📝 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.
| @GetMapping("/login/{registrationId}") | |
| public RedirectView login( | |
| @PathVariable String registrationId, | |
| @RequestParam(REDIRECT_PARAM_KEY) String redirect | |
| ) { | |
| final String url = UriComponentsBuilder | |
| .fromPath("/api/v1/oauth2/authorization/{registrationId}") | |
| .queryParam(REDIRECT_PARAM_KEY, redirect) | |
| .buildAndExpand(registrationId) | |
| .toUriString(); | |
| return new RedirectView(url); | |
| } | |
| @GetMapping("/login/{registrationId}") | |
| public RedirectView login( | |
| @PathVariable String registrationId, | |
| @RequestParam(REDIRECT_PARAM_KEY) String redirect | |
| ) { | |
| log.info("OAuth login request for registrationId: {}, redirect: {}", registrationId, redirect); | |
| // Validate registrationId to prevent path traversal | |
| if (!registrationId.matches("^[a-zA-Z0-9_-]+$")) { | |
| throw new IllegalArgumentException("Invalid registrationId format"); | |
| } | |
| final String url = UriComponentsBuilder | |
| .fromPath("/api/v1/oauth2/authorization/{registrationId}") | |
| .queryParam(REDIRECT_PARAM_KEY, redirect) | |
| .buildAndExpand(registrationId) | |
| .toUriString(); | |
| log.debug("Redirecting to OAuth2 authorization URL: {}", url); | |
| return new RedirectView(url); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/oauth/OAuthController.java
around lines 21 to 32, add validation to ensure the registrationId and redirect
parameters conform to expected formats or allowed values to prevent injection or
misuse. Also, include security-related logging that records attempts to access
the login endpoint with these parameters, capturing both successful and
suspicious inputs for audit and debugging purposes.
|
|
||
| private final AuthorizationRequestRedirectResolver authorizationRequestRedirectResolver; |
There was a problem hiding this comment.
Critical Security Issue: Authentication filters removed
The removal of AccessTokenFilter and TokenExceptionFilter completely disables authentication and authorization checks. This allows unrestricted access to all API endpoints.
Even for testing purposes, consider using test-specific profiles or configurations instead of removing security in the main configuration.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java around
lines 46 to 47, the removal of AccessTokenFilter and TokenExceptionFilter
disables authentication and authorization, allowing unrestricted API access.
Reintroduce these filters into the security configuration to enforce proper
security checks. If testing without security is needed, implement separate test
profiles or configurations rather than removing these filters from the main
security setup.
| // .requestMatchers( | ||
| // "/api/v1/profile/**", | ||
| // "/api/v1/admin/**" | ||
| // ) | ||
| // .hasRole("ADMIN") | ||
| .anyRequest().permitAll() |
There was a problem hiding this comment.
Critical Security Issue: Admin endpoints exposed without authentication
Commenting out the authorization rules for /api/v1/profile/** and /api/v1/admin/** endpoints exposes sensitive administrative and user profile functionality without any authentication or authorization checks.
Combined with .anyRequest().permitAll(), this configuration allows unrestricted access to all endpoints in your application.
If this is intended for testing, consider:
-// .requestMatchers(
-// "/api/v1/profile/**",
-// "/api/v1/admin/**"
-// )
-// .hasRole("ADMIN")
- .anyRequest().permitAll()
+ .requestMatchers(
+ "/api/v1/profile/**",
+ "/api/v1/admin/**"
+ )
+ .hasRole("ADMIN")
+ .requestMatchers("/api/v1/test/**") // Only allow test endpoints
+ .permitAll()
+ .anyRequest().authenticated()📝 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.
| // .requestMatchers( | |
| // "/api/v1/profile/**", | |
| // "/api/v1/admin/**" | |
| // ) | |
| // .hasRole("ADMIN") | |
| .anyRequest().permitAll() | |
| .requestMatchers( | |
| "/api/v1/profile/**", | |
| "/api/v1/admin/**" | |
| ) | |
| .hasRole("ADMIN") | |
| .requestMatchers("/api/v1/test/**") // Only allow test endpoints | |
| .permitAll() | |
| .anyRequest().authenticated() |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java around
lines 77 to 82, the authorization rules for the /api/v1/profile/** and
/api/v1/admin/** endpoints are commented out, leaving these sensitive endpoints
exposed without authentication due to the .anyRequest().permitAll() setting. To
fix this, uncomment and restore the requestMatchers for these paths and enforce
.hasRole("ADMIN") or appropriate roles to restrict access, ensuring that only
authorized users can access these endpoints.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements
Changes
Removals