[feat] #9 - 구글 소셜 로그인 기능 구현#14
Conversation
…to feat/#9-google-oauth2-login
WalkthroughGoogle OAuth2 소셜 로그인과 JWT 기반 인증 시스템이 추가되었다. User 엔티티/저장소, CustomOAuth2UserService, JwtTokenProvider, RefreshTokenService, AuthCodeService, 인증 성공/실패 핸들러, JwtAuthenticationFilter, RedisConfig, SecurityConfig 갱신, 토큰 교환용 AuthController가 포함된다. ChangesOAuth2 + JWT 인증 시스템
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant CustomOAuth2UserService
participant OAuthSuccessHandler
participant RefreshTokenService
participant AuthCodeService
participant AuthController
participant JwtTokenProvider
Browser->>CustomOAuth2UserService: OAuth2 사용자 정보 로드
CustomOAuth2UserService->>OAuthSuccessHandler: CustomUserDetails 반환
OAuthSuccessHandler->>JwtTokenProvider: generateRefreshToken(userId)
OAuthSuccessHandler->>RefreshTokenService: save(userId, refreshToken)
OAuthSuccessHandler->>AuthCodeService: generateAndSave(userId)
OAuthSuccessHandler-->>Browser: redirectUri?code=...
Browser->>AuthController: GET /api/auth/token?code=...
AuthController->>AuthCodeService: getAndDelete(code)
AuthController->>JwtTokenProvider: generateAccessToken(userId)
AuthController-->>Browser: accessToken JSON
sequenceDiagram
participant Client
participant JwtAuthenticationFilter
participant JwtTokenProvider
participant UserRepository
participant SecurityContextHolder
Client->>JwtAuthenticationFilter: Authorization: Bearer <token>
JwtAuthenticationFilter->>JwtTokenProvider: validateAccessToken(token)
JwtTokenProvider-->>JwtAuthenticationFilter: true/false
JwtAuthenticationFilter->>JwtTokenProvider: getUserId(token)
JwtAuthenticationFilter->>UserRepository: findById(userId)
UserRepository-->>JwtAuthenticationFilter: User(optional)
JwtAuthenticationFilter->>SecurityContextHolder: setAuthentication(CustomUserDetails)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…-Timo/Timo-Server into feat/#9-google-oauth2-login
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
src/main/java/com/Timo/Timo/global/config/SecurityConfig.java (1)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCORS Origin을 프로퍼티로 분리해 주세요.
Line 65는 credentials CORS에서 localhost Origin을 코드에 고정하고 있습니다. 배포 도메인 반영을 TODO에 의존하면 환경별 로그인 호출이 쉽게 깨지므로
application-*.yml또는@ConfigurationProperties로 분리하는 편이 좋습니다. 필요하면 설정 클래스까지 포함한 패치를 만들어 드릴 수 있습니다. As per path instructions,src/main/java/**/global/**/*.java:Spring Boot 설정이 과도하게 복잡하거나 전역 상태에 의존하지 않는지 확인해 주세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/Timo/Timo/global/config/SecurityConfig.java` at line 65, The CORS origins in SecurityConfig are hardcoded to localhost, which should be moved out of the code and into configuration. Update the CORS setup in SecurityConfig to read allowed origins from application-*.yml or a dedicated `@ConfigurationProperties` class, and wire that property into the existing corsConfigurationSource/CorsConfiguration usage instead of calling setAllowedOrigins with fixed values. Keep the security configuration simple and environment-driven so login CORS works correctly across local and deployed environments.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/Timo/Timo/global/auth/handler/OAuthSuccessHandler.java`:
- Around line 55-57: The OAuth success flow in OAuthSuccessHandler currently
appends accessToken to the redirect URL as a query parameter, which can expose
sensitive data. Update the redirect handling to avoid passing the token in the
URL, preferably by setting it in an HttpOnly Secure cookie or switching to a
one-time authorization code exchange. Keep the fix localized around the
redirectUrl construction in OAuthSuccessHandler and ensure no token is written
to browser history, referrers, or logs.
- Line 50: The refresh-token cookie lifetime in OAuthSuccessHandler is hardcoded
to 7 days, which can drift from the Redis/JWT expiry settings. Update the cookie
maxAge to use the same refresh token expiry source as
JwtTokenProvider#getRefreshTokenExpiry() so the browser cookie, Redis TTL, and
JWT refresh token all stay aligned when the expiration changes.
- Line 48: refresh token cookie의 Secure 설정이 OAuthSuccessHandler에서 하드코딩되어 있어 배포
환경에서 실수로 비활성화될 수 있습니다. OAuthSuccessHandler의 쿠키 생성 로직에서 secure(false)를 제거하고, 기본값은
true로 두되 로컬/개발 프로필에서만 끌 수 있도록 설정값이나 환경변수로 분리해 주세요. 관련해서 refresh token 쿠키를 만드는
메서드와 secure 옵션을 적용하는 부분을 함께 수정해 설정이 안전하게 주입되도록 정리해 주세요.
- Line 3: Use the configured redirect URI value instead of the OAuth2
REDIRECT_URI constant when building the post-login redirect in
OAuthSuccessHandler. Update the success flow that appends the access token so it
reads the actual configured redirectUri and redirects to that target rather than
constructing a redirect_uri?accessToken=... URL from the constant.
In
`@src/main/java/com/Timo/Timo/global/auth/service/CustomOAuth2UserService.java`:
- Around line 26-31: In CustomOAuth2UserService, validate that the OAuth2User
attributes contain a non-null email before calling
userRepository.findByEmail(email). If the email is missing, convert the failure
into an OAuth2AuthenticationException with a clear error so the OAuth failure
handler can respond consistently; keep the check close to the attribute
extraction for email, name, and picture in the same loadUser flow.
- Around line 22-35: The existing-user profile update path in
CustomOAuth2UserService.loadUser is modifying the entity returned from
userRepository.findByEmail() without a transaction boundary, so the change may
not be persisted. Add a transactional boundary to loadUser or explicitly persist
the updated User after existing.update(name, picture), and keep the fix anchored
around loadUser, userRepository.findByEmail, and existing.update.
In `@src/main/java/com/Timo/Timo/global/auth/service/RefreshTokenService.java`:
- Around line 18-24: The RefreshTokenService.save method is storing the Redis
entry with the wrong TTL unit, causing the key to expire much earlier than the
JWT refresh token. Update the redisTemplate.opsForValue().set call to use the
same time unit expected by JwtTokenProvider.getRefreshTokenExpiry, and align the
Redis expiration with the token’s actual lifetime so the refresh token remains
available until JWT expiry.
- Around line 35-36: The refresh token validation in RefreshTokenService.isValid
currently calls equals on refreshToken directly, which can throw a
NullPointerException when the token is missing. Update this method to use a
null-safe comparison, such as comparing refreshToken against get(email) with
Objects.equals, so missing cookie/parameter cases return false instead of
causing a 500. Keep the fix localized to isValid and ensure null handling stays
safe.
In `@src/main/java/com/Timo/Timo/global/config/SecurityConfig.java`:
- Around line 41-48: The security matcher list in SecurityConfig does not
explicitly allow /login-success, so OAuth callback flow may still be blocked.
Update the permitAll requestMatchers in SecurityConfig to include the
/login-success endpoint alongside the existing /login/** and /oauth2/** entries,
using the same matcher setup so the success redirect can be accessed without
authentication.
- Around line 49-56: The current SecurityConfig.oauth2Login setup is causing
`/api/**` authentication failures to be handled like browser login redirects
instead of API 401 responses. Update the security configuration in
SecurityConfig to distinguish API requests from OAuth2 login flow, and add an
AuthenticationEntryPoint for API paths so missing or expired JWTs return a 401
JSON response. Keep the OAuth2 login success/failure handlers for the web login
flow, but route API authentication failures through the new entry point.
In `@src/main/java/com/Timo/Timo/global/jwt/filter/JwtAuthenticationFilter.java`:
- Around line 36-37: AccessToken 전용 검증이 없어서 JwtAuthenticationFilter의 인증 분기에서
RefreshToken까지 통과할 수 있습니다. JwtTokenProvider에 token type claim을 구분하는
validateAccessToken(...)을 추가하고, JwtAuthenticationFilter의 doFilterInternal()에서는
validateToken() 대신 access 전용 검증만 호출하도록 바꾸세요. 또한 getEmail(token) 전에 토큰이 access인지
먼저 확인해 인증 컨텍스트가 RefreshToken으로 설정되지 않게 해 주세요.
In `@src/main/java/com/Timo/Timo/global/jwt/provider/JwtTokenProvider.java`:
- Around line 39-67: `JwtTokenProvider`에서 리프레시 토큰이 인증에 그대로 사용되는 문제를 막아야 합니다.
`validateToken()`과 `getEmail()`이 단순 서명 검증만 하지 말고 토큰의 `type` 클레임을 확인하도록 수정하고,
`JwtAuthenticationFilter`는 인증 시 `"access"`만 허용하도록 호출 흐름을 맞추세요. 재발급용 경로에서는
`"refresh"`를 기대하도록 분리해 `generateRefreshToken()`, `getClaims()`,
`validateToken()`, `getEmail()`의 역할이 명확히 구분되게 하세요.
---
Nitpick comments:
In `@src/main/java/com/Timo/Timo/global/config/SecurityConfig.java`:
- Line 65: The CORS origins in SecurityConfig are hardcoded to localhost, which
should be moved out of the code and into configuration. Update the CORS setup in
SecurityConfig to read allowed origins from application-*.yml or a dedicated
`@ConfigurationProperties` class, and wire that property into the existing
corsConfigurationSource/CorsConfiguration usage instead of calling
setAllowedOrigins with fixed values. Keep the security configuration simple and
environment-driven so login CORS works correctly across local and deployed
environments.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 465d4783-1636-40ab-ab14-a346d674e55f
📒 Files selected for processing (13)
.coderabbit.yamlbuild.gradlesrc/main/java/com/Timo/Timo/domain/user/entity/User.javasrc/main/java/com/Timo/Timo/domain/user/repository/UserRepository.javasrc/main/java/com/Timo/Timo/global/auth/dto/CustomUserDetails.javasrc/main/java/com/Timo/Timo/global/auth/handler/OAuthFailureHandler.javasrc/main/java/com/Timo/Timo/global/auth/handler/OAuthSuccessHandler.javasrc/main/java/com/Timo/Timo/global/auth/service/CustomOAuth2UserService.javasrc/main/java/com/Timo/Timo/global/auth/service/RefreshTokenService.javasrc/main/java/com/Timo/Timo/global/config/RedisConfig.javasrc/main/java/com/Timo/Timo/global/config/SecurityConfig.javasrc/main/java/com/Timo/Timo/global/jwt/filter/JwtAuthenticationFilter.javasrc/main/java/com/Timo/Timo/global/jwt/provider/JwtTokenProvider.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/Timo/Timo/global/auth/handler/OAuthSuccessHandler.java`:
- Line 50: The accessToken cookie is using the refresh token expiry, so update
OAuthSuccessHandler to set its maxAge from the access token lifetime instead.
Add a dedicated access token expiry getter to JwtTokenProvider, then replace the
existing refreshTokenExpiry usage in the cookie builder with that new
access-token-specific value so the cookie expires together with the access
token.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d92c60b5-e247-403c-8f89-8e8f4b110fa4
📒 Files selected for processing (3)
src/main/java/com/Timo/Timo/global/auth/handler/JwtAuthenticationEntryPoint.javasrc/main/java/com/Timo/Timo/global/auth/handler/OAuthSuccessHandler.javasrc/main/java/com/Timo/Timo/global/config/SecurityConfig.java
laura-jung
left a comment
laura-jung
left a comment
There was a problem hiding this comment.
현재 스크린샷으로 보았을 때는 accessToken이 url로 들어오는 것 같아보이는데 맞을까요...?
accessToken도 일종의 개인정보라 응답 body로 내려주는게 좋을 것 같다고 생각합니다! body로도 오고 있는 것 같아서 redirect 경로만 수정하면 될 것 같아요! 이외에도 코멘트 달아두었으니 확인 부탁드립니다.
oauth 로그인 쉽지 않은데 너무 잘 해준 것 같아서 복복복해주고 싶습니다. (실은 전 예전에 실패했었어요) 파일링도 깔끔하고 아주그냥 굳입니다
| private String email; | ||
|
|
||
| private String name; | ||
| private String picture; |
There was a problem hiding this comment.
p3) picture이 아마도 이미지 사진일 것 같은데 이미지의 경우에는 imageUrl과 같은 네임으로 많이 사용합니다! 아마 oauth에서는 필드명으로 picture를 사용하는 것으로 알고 있는데 도메인 엔티티에서는 수정해주는게 좋을 것 같습니다
There was a problem hiding this comment.
아하 OAuth의 필드명을 그대로 사용했었는데, 말씀해주신 것처럼 도메인 엔티티에서는 imageUrl이 의미도 더 명확하고 직관적인 것 같네요! imageUrl로 변경해서 반영했습니다. 감사합니다!
| @Getter | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| @Builder | ||
| @AllArgsConstructor |
There was a problem hiding this comment.
p3) @AllArgsConstructor의 경우 외부에서 생성자를 만들 위험이 있기 때문에 조심해서 사용을 해야하는데요 저희 서비스의 경우 기본적으로는 구글의 소셜로그인을 통해 사용자가 생성되기 때문에 임의로 사용자가 만들어질 수 있는 해당 어노테이션을 사용하는건 위험하다고 생각합니다! 실제 생성은 @builder가 붙은 private 생성자로 제한해도 좋을 것 같아요. 그러면 User가 어떤 값으로 생성되어야 하는지 더 명확해지고, 잘못된 상태의 엔티티 생성을 줄일 수 있을 것 같습니다.
@Builder
private User(String email, String name, String profileImageUrl, Provider provider) {
this.email = email;
this.name = name;
this.ImageUrl = ImageUrl;
this.provider = provider;
}
이런식으로요!
There was a problem hiding this comment.
옹 예시 코드까지 감사합니다:) @AllArgsConstructor를 사용하면 외부에서 임의의 상태로 User를 생성할 수 있다는 점을 놓쳤던 것 같습니다. 말씀해주신 방향대로 @AllArgsConstructor 대신 @Builder가 적용된 private 생성자를 통해서만 생성할 수 있도록 수정였습니다-!!
| this.picture = picture; | ||
| } | ||
|
|
||
| public enum Provider { GOOGLE } |
There was a problem hiding this comment.
p4) 이 enum의 경우 따로 파일을 분리하는게 어떨까요?
There was a problem hiding this comment.
안 그래도 고민했던 부분이었는데, 현재는 User 엔티티 내에서만 한 번 사용되어 내부 enum으로 두었습니다. 다만 위의 다른 리뷰에서 말씀해주신 것처럼 추후 Provider가 추가되거나 다른 곳에서 재사용될 가능성도 고려해 별도 enum 파일로 분리하여 반영했습니다. 감사합니다!
| @Override | ||
| public String getUsername(){ | ||
| return user.getEmail(); | ||
| } |
There was a problem hiding this comment.
getUsername과 getName모두 getEmail을 가져오는 것 같은데 두개가 뭐가 다른건가요?
There was a problem hiding this comment.
저도 이 부분 정말 헷갈렸는데요,, 둘 다 사용자를 식별하는 용도이긴 하지만, 역할이 조금 다르다고 합니다!
현재 해당 클래스는 구글 로그인으로 받아온 사용자를 Spring Security가 이해할 수 있는 로그인 객체로 변환해 주는 어댑터 역할을 하며, OAuth2User와 UserDetails인터페이스를 모두 구현하고 있습니다.
getName()->OAuth2User인터페이스 구현 (OAuth2 로그인할 때 제공자(구글)이 사용하는 사용자 식별 = '이 OAuth2 사용자의 식별자가 뭐야?')getUsername()->UserDetails인터페이스 구현 (Spring Security가 내부적으로 사용자를 식별할 때 사용하는 값 = '현재 로그인한 사람이 누구야?')
이었는데 아래 코드리뷰와 관련해서 식별자 값을 email->userId로 변경 했으니 참고해주시면 감사하겠습니다:)
There was a problem hiding this comment.
완전 이해되었습니다 꼼꼼한 설명 감사합니다.
There was a problem hiding this comment.
p3) 지금 보았을때 위의 jwtAuthenticationEntryPoint와 oauthFailureHandler의 코드가 겹치는 부분이 많은 것 같아요! 따로 writer로 분리해서 작성하면 중복코드 사용을 줄일 수 있지 않을까하는 생각입니다!
There was a problem hiding this comment.
오홍 인지하지 못하고 있었는데 ErrorDto 생성 → status 설정 → contentType 설정 → write 하는 로직이 동일하게 반복되고 있었네요! AuthErrorResponseWriter로 분리하여 중복을 제거하고 이후 에러 응답 형식이 바뀌더라도 한 곳만 수정하면 되도록 개선해야겠습니다-!!
| String accessToken = jwtTokenProvider.generateAccessToken(email); | ||
| String refreshToken = jwtTokenProvider.generateRefreshToken(email); |
There was a problem hiding this comment.
사용자 식별용으로 email을 사용하신 이유가 있나요? 현재는 구글 로그인 밖에 없지만 후에 다른 소셜 로그인이 추가된다면 email이 중복될 수도 있을 것 같아서요!
p2)저의 경우에는 userId를 많이 사용하는 것 같습니다.
email을 사용하신 특별한 이유가 있다면 수정하지 않으셔도 됩니다
There was a problem hiding this comment.
구글만 사용할 때로 한정해서 생각했던 것 같습니다. 현재는 구글 로그인만 있어서 email 중복 문제가 없었는데 말씀 주신대로 추후 다른 소셜 로그인 추가 시 같은 email로 가입된 경우 충돌이 생길 수 있을 것 같아 userId로 수정하도록 하겠습니다!!
| .email(email) | ||
| .name(name) | ||
| .picture(picture) | ||
| .provider(User.Provider.GOOGLE) |
There was a problem hiding this comment.
p3) 여기서도 google로 박아두기 보다는 후에 확장성을 위해 받아올 수 있게 하는게 좋지 않을까요?
There was a problem hiding this comment.
넵 좋습니다! registrationId를 통해 동적으로 받아오도록 변경했습니다! 확장성을 많이 고려하지 못했던 것 같습니다😓
| @Bean | ||
| public CorsConfigurationSource corsConfigurationSource() { | ||
| CorsConfiguration config = new CorsConfiguration(); | ||
| config.setAllowedOrigins(List.of("http://localhost:3000", "http://localhost:5173")); // TODO: 프론트 배포 시 도메인으로 변경 |
| ResponseCookie refreshCookie = ResponseCookie.from("refreshToken", refreshToken) | ||
| .httpOnly(true) | ||
| .secure(cookieSecure) | ||
| .path("/") | ||
| .maxAge(Duration.ofSeconds(jwtTokenProvider.getRefreshTokenExpiry())) | ||
| .sameSite("Strict") | ||
| .build(); |
There was a problem hiding this comment.
p2) refreshCookie를 생성하고 있지만 응답 헤더에 추가하는 코드가 없어 실제 클라이언트로 쿠키가 내려가지 않을 것 같습니다.
response.addHeader(HttpHeaders.SET_COOKIE, refreshCookie.toString()) 처리가 필요한 것 같습니다. 현재 상태라면 refresh token가 redis에는 저장되지만 브라우저 쿠키에는 저장되지 않아 재발급이 실패할 수 있을 것 같습니다.
There was a problem hiding this comment.
헉 맞습니다! 마지막에 accessToken 전달 방식을 쿠키로 잘못 전달하고 있었어서 Body 응답으로 변경하는 과정에서 refreshToken 쿠키 헤더 추가 코드가 누락되었던 것 같습니다.
꼼꼼하게 봐주셔서 감사합니다! 바로 수정하겠습니다~
| private final ObjectMapper objectMapper; | ||
|
|
||
| @Value("${app.oauth2.redirect-uri}") | ||
| private String redirectUri; |
There was a problem hiding this comment.
이부분은 아직 구현되지 않아서 따로 사용되지 않는거겠죠...?
There was a problem hiding this comment.
redirectUri 선언만 해두고 실제로 사용하는 코드가 누락되어 있었네요.. 테스트용 코드 작성하느라 수정하면서 실수로 지운 것 같습니다.. getRedirectStrategy().sendRedirect()로 리다이렉트 처리 다시 추가해놓겠습니다!
|
테스트 결과는 이전에 제가 수정하다보니 body 방식의 문제점 알게 되어서 이거를 해결하기 위해 1회성 authorization code 방식으로 변경했습니다!
로그인 성공 시 30초 TTL의 1회성 code를 생성하여 URL 파라미터로 전달하고, |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/com/Timo/Timo/global/auth/handler/OAuthSuccessHandler.java (1)
31-32: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
cookieSecure기본값을true로 변경해 주세요.설정으로 분리한 점은 좋으나, 기본값이
false라 배포 시app.auth.cookie-secure값을 명시적으로 설정하지 않으면 refreshToken 쿠키가 평문 HTTP로도 전송될 수 있습니다. 과거 리뷰에서도 기본값을true로 제안했던 부분입니다.🛡️ 제안 수정
- `@Value`("${app.auth.cookie-secure:false}") + `@Value`("${app.auth.cookie-secure:true}") private boolean cookieSecure;As per past review comments,
@src/main/java/com/Timo/Timo/global/auth/handler/OAuthSuccessHandler.java의 secure 값은 "기본값true인 설정값으로 분리해 주세요"라는 제안이 있었습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/Timo/Timo/global/auth/handler/OAuthSuccessHandler.java` around lines 31 - 32, The OAuthSuccessHandler cookieSecure setting is still defaulting to false, which leaves refreshToken cookies non-secure unless explicitly configured. Update the `@Value` default on cookieSecure in OAuthSuccessHandler to true so the secure-cookie behavior is enabled by default, and keep the property override support intact for app.auth.cookie-secure.
🧹 Nitpick comments (1)
src/main/java/com/Timo/Timo/domain/user/controller/AuthController.java (1)
22-30: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win토큰 교환 엔드포인트를 POST로 바꾸세요.
RFC 6749는 token endpoint에POST+application/x-www-form-urlencoded를 요구합니다. 지금처럼GET으로 1회용code를 소비하면 규격을 벗어나고, 쿼리 문자열이 접근 로그에 남을 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/Timo/Timo/domain/user/controller/AuthController.java` around lines 22 - 30, The AuthController token exchange endpoint currently uses GET with a query parameter, which should be changed to the OAuth2 token-style POST flow. Update the token(...) method in AuthController to accept POST requests and read the code from form-encoded request data rather than `@RequestParam`, keeping the existing authCodeService.getAndDelete(code) and unauthorized handling intact. Make sure the endpoint still returns the same ResponseEntity<Map<String, String>> response while using POST and application/x-www-form-urlencoded semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/Timo/Timo/domain/user/controller/AuthController.java`:
- Around line 28-30: AuthController의 인증 실패 응답이 다른 흐름과 달리 빈 401 바디를 반환하고 있습니다.
userId null 처리 분기에서 단순히 status만 내리지 말고, OAuthFailureHandler와
JwtAuthenticationEntryPoint가 사용하는 AuthErrorResponseWriter를 재사용해 표준 ErrorDto 형식의
401 응답을 반환하도록 맞춰 주세요. AuthController의 해당 엔드포인트 응답 생성 로직을 공통 인증 실패 응답 경로로 통일하면
됩니다.
In `@src/main/java/com/Timo/Timo/global/auth/service/AuthCodeService.java`:
- Around line 29-36: The AuthCodeService.getAndDelete method currently does a
separate get and delete on the Redis key, which can allow the same code to be
consumed more than once under concurrent requests. Update this method to perform
the lookup and removal atomically by using Redis
ValueOperations.getAndDelete(key) if available, or by executing a Lua script
against redisTemplate so the KEY_PREFIX-based key is read and deleted in one
operation.
---
Duplicate comments:
In `@src/main/java/com/Timo/Timo/global/auth/handler/OAuthSuccessHandler.java`:
- Around line 31-32: The OAuthSuccessHandler cookieSecure setting is still
defaulting to false, which leaves refreshToken cookies non-secure unless
explicitly configured. Update the `@Value` default on cookieSecure in
OAuthSuccessHandler to true so the secure-cookie behavior is enabled by default,
and keep the property override support intact for app.auth.cookie-secure.
---
Nitpick comments:
In `@src/main/java/com/Timo/Timo/domain/user/controller/AuthController.java`:
- Around line 22-30: The AuthController token exchange endpoint currently uses
GET with a query parameter, which should be changed to the OAuth2 token-style
POST flow. Update the token(...) method in AuthController to accept POST
requests and read the code from form-encoded request data rather than
`@RequestParam`, keeping the existing authCodeService.getAndDelete(code) and
unauthorized handling intact. Make sure the endpoint still returns the same
ResponseEntity<Map<String, String>> response while using POST and
application/x-www-form-urlencoded semantics.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 84ce08d6-fb55-4fce-a2ec-c5bceaecf005
📒 Files selected for processing (14)
src/main/java/com/Timo/Timo/domain/user/controller/AuthController.javasrc/main/java/com/Timo/Timo/domain/user/entity/User.javasrc/main/java/com/Timo/Timo/domain/user/enums/Provider.javasrc/main/java/com/Timo/Timo/global/auth/handler/AuthErrorResponseWriter.javasrc/main/java/com/Timo/Timo/global/auth/handler/JwtAuthenticationEntryPoint.javasrc/main/java/com/Timo/Timo/global/auth/handler/OAuthFailureHandler.javasrc/main/java/com/Timo/Timo/global/auth/handler/OAuthSuccessHandler.javasrc/main/java/com/Timo/Timo/global/auth/service/AuthCodeService.javasrc/main/java/com/Timo/Timo/global/auth/service/CustomOAuth2UserService.javasrc/main/java/com/Timo/Timo/global/auth/service/RefreshTokenService.javasrc/main/java/com/Timo/Timo/global/config/SecurityConfig.javasrc/main/java/com/Timo/Timo/global/exception/code/ErrorCode.javasrc/main/java/com/Timo/Timo/global/jwt/filter/JwtAuthenticationFilter.javasrc/main/java/com/Timo/Timo/global/jwt/provider/JwtTokenProvider.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/com/Timo/Timo/global/auth/service/RefreshTokenService.java
- src/main/java/com/Timo/Timo/global/exception/code/ErrorCode.java
- src/main/java/com/Timo/Timo/global/config/SecurityConfig.java
- src/main/java/com/Timo/Timo/global/auth/service/CustomOAuth2UserService.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/Timo/Timo/global/auth/controller/AuthController.java`:
- Around line 14-16: The token exchange endpoint in AuthController is currently
exposed as a GET, which can leak the authorization code and accessToken via
history/cache/prefetch/logs; change the exchange handler to use POST instead of
GET, and update the response handling to set Cache-Control: no-store (and
Pragma: no-cache if needed). Locate the endpoint mapped in AuthController and
adjust the request mapping/response building so the token exchange path no
longer returns sensitive data through a cacheable GET response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2878ccf4-4141-4d91-9559-46a8a1aac2fb
📒 Files selected for processing (3)
src/main/java/com/Timo/Timo/global/auth/controller/AuthController.javasrc/main/java/com/Timo/Timo/global/auth/service/AuthCodeService.javasrc/main/java/com/Timo/Timo/global/exception/code/ErrorCode.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/Timo/Timo/global/exception/code/ErrorCode.java
- src/main/java/com/Timo/Timo/global/auth/service/AuthCodeService.java
| import org.springframework.web.bind.annotation.GetMapping; | ||
| import org.springframework.web.bind.annotation.RequestMapping; | ||
| import org.springframework.web.bind.annotation.RequestParam; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify token endpoint callers and security matcher coverage before switching GET to POST.
rg -nP -C3 '(/api/auth/token|requestMatchers\s*\(|csrf|cors)' src/main/javaRepository: Team-Timo/Timo-Server
Length of output: 4087
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== AuthController outline ==\n'
ast-grep outline src/main/java/com/Timo/Timo/global/auth/controller/AuthController.java --view expanded || true
printf '\n== SecurityConfig outline ==\n'
ast-grep outline src/main/java/com/Timo/Timo/global/config/SecurityConfig.java --view expanded || true
printf '\n== AuthController relevant lines ==\n'
sed -n '1,220p' src/main/java/com/Timo/Timo/global/auth/controller/AuthController.java | cat -n
printf '\n== SecurityConfig relevant lines ==\n'
sed -n '1,220p' src/main/java/com/Timo/Timo/global/config/SecurityConfig.java | cat -n
printf '\n== Token endpoint callers / references ==\n'
rg -n --hidden --glob '!**/target/**' --glob '!**/build/**' --glob '!**/dist/**' '/api/auth/token|getAndDelete\(|accessToken|Cache-Control|Pragma' .Repository: Team-Timo/Timo-Server
Length of output: 7970
토큰 교환 엔드포인트는 POST로 바꾸고 응답 캐시를 막아 주세요.
현재 GET으로 인증 코드를 소모한 뒤 accessToken을 내려서, 브라우저 히스토리/캐시/프리페치/접근 로그에 값이 남을 수 있습니다. 응답에는 Cache-Control: no-store(필요하면 Pragma: no-cache도) 를 추가해 주세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/Timo/Timo/global/auth/controller/AuthController.java`
around lines 14 - 16, The token exchange endpoint in AuthController is currently
exposed as a GET, which can leak the authorization code and accessToken via
history/cache/prefetch/logs; change the exchange handler to use POST instead of
GET, and update the response handling to set Cache-Control: no-store (and
Pragma: no-cache if needed). Locate the endpoint mapped in AuthController and
adjust the request mapping/response building so the token exchange path no
longer returns sensitive data through a cacheable GET response.
Source: Path instructions
There was a problem hiding this comment.
p2) accessToken을 발급하는 엔드포인트라서 코드래빗 리뷰처럼 post 메소드가 더 어울릴 것 같네요.
get은 히스토리가 남아서 보안상 위험할 수도 있을 것 같아요!!
그리고 swagger 처리 해주셔야 합니다!!
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for existing Swagger/OpenAPI usage =="
rg -nP -C2 '`@Tag`\(|`@Operation`\(|springdoc|SpringDoc|swagger' src/main/java --iglob '*.java' || true
echo "== build.gradle swagger deps =="
rg -n 'springdoc|swagger' build.gradle || true
echo "== AuthController full content =="
cat -n src/main/java/com/Timo/Timo/global/auth/controller/AuthController.java
echo "== SecurityConfig permitAll rules =="
rg -n 'permitAll|requestMatchers' src/main/java/com/Timo/Timo/global/config/SecurityConfig.java -A1 -B1
echo "== Other controllers for style reference =="
fd -e java -i 'controller' src/main/javaLength of output: 4615
@laura-jung 맞습니다! 아래처럼 반영하는 걸 제안드립니다.
GET→POST로 변경 (히스토리/캐시 노출 방지)- 응답에
Cache-Control: no-store헤더 추가 springdoc-openapi가 이미 의존성에 포함되어 있으니 (build.gradle)@Tag,@Operation,@Parameter애노테이션으로 Swagger 문서화 추가
SecurityConfig의 requestMatchers("/api/auth/token")은 경로 기준이라 메서드를 바꿔도 별도 설정 변경은 필요 없습니다.
Task: AuthController /api/auth/token을 POST로 변경 + 캐시 방지 헤더 + Swagger 문서화
The requested changes have been implemented and a pull request has been created: View PR
aneykrap
left a comment
aneykrap
left a comment
There was a problem hiding this comment.
자윤님 완전 고생하셨습니당 잘 작성해주셔서 공부하는 느낌으로 다시 읽어보았습니당
| ResponseCookie refreshCookie = ResponseCookie.from("refreshToken", refreshToken) | ||
| .httpOnly(true) | ||
| .secure(cookieSecure) | ||
| .path("/") |
There was a problem hiding this comment.
p4) Refresh Token 쿠키가 path("/")로 설정되어 모든 API 요청에 함께 전송되는것 같아요. 실제 사용되는 재발급·로그아웃 API로 경로를 제한하면 토큰의 불필요한 전송과 노출 범위를 줄일 수 있을 것 같습니다. /api/auth 정도로 제한하는 것은 어떨까요?
|
|
||
| public void save(String userId, String refreshToken){ | ||
| redisTemplate.opsForValue().set( | ||
| KEY_PREFIX + userId, |
There was a problem hiding this comment.
p3) Redis key가 refresh:{userId} 하나라서 같은 사용자가 다른 기기에서 로그인하면 기존 Refresh Token이 덮어써지는것 같아요. (=> 다중기기 지원 X) 기존 기기는 Access Token 만료 후 재발급에 실패하게 되는데 세션 식별자를 key에 포함하는 건 어떨까요?
저도 리다이렉트하면서 동시에 body 전달 불가 문제로 소셜 로그인에 실패한 경험이 있습니다...... |
| import org.springframework.web.bind.annotation.GetMapping; | ||
| import org.springframework.web.bind.annotation.RequestMapping; | ||
| import org.springframework.web.bind.annotation.RequestParam; |
There was a problem hiding this comment.
p2) accessToken을 발급하는 엔드포인트라서 코드래빗 리뷰처럼 post 메소드가 더 어울릴 것 같네요.
get은 히스토리가 남아서 보안상 위험할 수도 있을 것 같아요!!
그리고 swagger 처리 해주셔야 합니다!!
| public String get(String email){ | ||
| return redisTemplate.opsForValue().get(KEY_PREFIX + email); | ||
| } |
There was a problem hiding this comment.
p4) 현재 refresh token key는 userId 기준으로 저장하는 것 같은데, get(String email) 파라미터명이 이전 의미로 남아 있는 것 같아요! 동작에는 영향이 없지만 get(String userId)로 맞추면 의미가 일관돼서 더 좋을 것 같습니다
3 participants
관련 이슈 🛠
작업 내용 요약 ✏️
Google OAuth2 소셜 로그인 기반 인증/인가 시스템 구축
JWT(AccessToken + RefreshToken)와 Redis를 활용한 Stateless 인증 구현
주요 변경 사항 🛠️
트러블 슈팅 ⚽️
/login-success경로가 permitAll에 누락되어 인증 필터에 막히는 문제 수정테스트 결과 📄
http://localhost:8080/oauth2/authorization/google접속 후 Google 계정 선택스크린샷 📷
http://localhost:8080/oauth2/authorization/google접속 시 로그인>구글 계정 선택 화면로그인 성공 시 테스트용
/login-success페이지로 리다이렉트 확인로그인 성공 시 개발자 도구 > 애플리케이션 탭에서 쿠키 내 refreshToken 들어간 것 확인
로그인 성공 시 accesToken 발급을 위한 1회용 코드 생성 확인
1회용 코드로 accessToken 발급 확인
리뷰 요구사항 📢
📎 참고 자료 (선택)
Summary by CodeRabbit
New Features
Bug Fixes