Repository navigation
Sisc1 209 OAuthSuccessHandler 수정 및 CustomOidcUserService 추가 - #111
Hidden character warning
Conversation
…m/SISC-IT/sisc-web into SISC1-209-BE-redirect-url-프론트로-전달
WalkthroughOAuth/OIDC 인증 흐름을 통합·정리하고 AuthProvider 매핑을 추가했으며, OIDC 전용 서비스와 DB 기반 OAuth 계정 조회를 도입하고, 성공 핸들러에서 토큰 전달을 URL에서 HttpOnly 쿠키로 변경했습니다. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant OAuthProvider as OAuth2/OIDC Provider
participant Backend
participant DB
participant Frontend
User->>Browser: 소셜 로그인 시작
Browser->>OAuthProvider: 인증/동의
OAuthProvider->>Browser: 코드/토큰
Browser->>Backend: 콜백 요청 (code, provider)
Note over Backend: CustomOAuth2UserService / CustomOidcUserService
Backend->>Backend: 사용자 속성 매핑 (provider, providerUid, email, name)
Backend->>DB: findByProviderAndProviderUid(provider, providerUid)
DB-->>Backend: UserOauthAccount + User
Note over Backend: OAuth2SuccessHandler
Backend->>Backend: 액세스/리프레시 토큰 생성
Backend->>Browser: HttpOnly 쿠키로 토큰 설정 + 302 리다이렉트
Browser->>Frontend: 리다이렉트 (쿠키 자동 전송)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60분
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOAuth2UserService.java (2)
56-75: 소셜 provider별 attribute 파싱 시 null/타입 가정이 강합니다
kakao분기에서kakao_account,profile을 바로 캐스팅/접근하고 있어 응답 스키마가 바뀌거나 권한이 부족한 경우NullPointerException가능성이 있습니다.github분기에서sub,name이 없을 때의 방어 로직이 없습니다.최소한 다음 정도는 고려해 보시면 좋겠습니다.
- 중간 단계에서
kakaoAccount == null/profile == null인 경우 의미 있는 예외 메시지로 실패 처리.- 예외 타입도
RuntimeException대신 도메인에 맞는 커스텀 예외나OAuth2AuthenticationException계열로 통일하면 디버깅이 수월합니다.
107-118: GrantedAuthority가 User.role과 분리돼 있어 향후 롤 확장 시 불일치 위험이 있습니다현재는 항상
new SimpleGrantedAuthority("ROLE_TEAM_MEMBER")로 권한을 고정하고 있는데, DB의User.role값과 불일치할 수 있습니다. 나중에ADMIN,LEADER등 롤이 늘어나면 버그로 이어질 가능성이 큽니다.예를 들어 다음처럼
User.role을 기반으로 권한을 구성하는 쪽이 더 안전해 보입니다(실제 enum/필드 이름에 맞게 조정 필요).- return new DefaultOAuth2User( - List.of(new SimpleGrantedAuthority("ROLE_TEAM_MEMBER")), - attributes, - "userId" - ); + var authority = new SimpleGrantedAuthority("ROLE_" + user.getRole().name()); + return new DefaultOAuth2User( + List.of(authority), + attributes, + "userId" + );backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOidcUserService.java (1)
36-55: OIDC attributes 확장 방식 자체는 괜찮지만, claims 사용 시 동작 차이를 인지해 두는 게 좋습니다현재 구현은
original = oidcUser.getAttributes()를 복사한 뒤,provider,providerUid,name을 추가하고,- 익명 서브클래스로
DefaultOidcUser를 생성하여getAttributes()만 override하고 있습니다.이렇게 하면
OAuth2SuccessHandler처럼getAttributes()를 사용하는 코드에서는 커스텀 필드를 정상적으로 볼 수 있지만,OidcUser#getClaims()나getUserInfo()를 직접 사용하는 코드에서는 여전히 원래 claims만 보이고provider정보는 보이지 않을 수 있습니다.당장은 문제가 아니지만, 이후 OIDC 관련 로직이 추가될 때
- attributes 기반으로만 읽는지,
- claims/userInfo를 직접 쓰는 코드가 생기지 않는지
를 맞춰 주면 혼동을 줄일 수 있습니다. 필요하다면
OidcUserInfo를 새로 만들어provider등을 포함시키는 방식도 고려해 볼 수 있습니다.backend/src/main/java/org/sejongisc/backend/auth/entity/AuthProvider.java (1)
8-15: AuthProvider.from 헬퍼 추가는 좋고, OAuth2SuccessHandler에서 재사용하면 중복을 줄일 수 있습니다
providerName.toLowerCase()를 기준으로"google","kakao","github"을 enum으로 매핑하는 구현은 명확하고 사용하기 좋습니다.- 현재
OAuth2SuccessHandler에서도 동일한 switch를 다시 작성하고 있으므로, 거기서는 단순히AuthProvider.from(providerStr)를 호출하도록 변경하면 중복을 없앨 수 있습니다.추가로, 외부 입력에서 null이 들어올 가능성이 있다면
providerName == null에 대한 방어 로직을 넣고 조금 더 친절한 예외 메시지를 주는 것도 고려해 볼 수 있습니다.backend/src/main/java/org/sejongisc/backend/common/auth/config/SecurityConfig.java (1)
71-99: 현재 보안 설정에서는/api/user/**만 인증이 필요하고, 나머지 요청은 모두 공개입니다이 블록을 보면:
- 여러 인증/회원 관련 엔드포인트 +
/login/**,/oauth2/**,/webjars/**는permitAll(),/api/user/**는authenticated(),- 그 외는
.anyRequest().permitAll()로 구성돼 있습니다.Spring Security의 매칭 순서 상
/api/user/**는 authenticated로 잘 보호되겠지만, 그 외 API (/api/**의 다른 경로 등)는 모두 공개 접근이 허용됩니다.서비스 보안 요구사항이
- “회원 정보만 보호하고 나머지는 공개 API”라면 괜찮지만,
- 더 많은 엔드포인트를 보호해야 한다면
.anyRequest().authenticated()또는 특정 prefix(/api/**등)에 대한 추가 rule이 필요할 수 있습니다.의도된 정책인지 한 번 더 확인해 보시는 것을 권장드립니다.
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java (3)
64-70: AuthProvider 매핑이 AuthProvider.from(String)과 중복됩니다현재:
AuthProvider provider = switch (providerStr) { case "kakao" -> AuthProvider.KAKAO; case "github" -> AuthProvider.GITHUB; case "google" -> AuthProvider.GOOGLE; default -> throw new IllegalStateException("Unknown OAuth provider: " + providerStr); };이미
AuthProvider.from(String providerName)이 존재하므로, 다음처럼 단순화하면 중복을 줄이고 한 곳에서만 매핑을 관리할 수 있습니다.- AuthProvider provider = - switch (providerStr) { - case "kakao" -> AuthProvider.KAKAO; - case "github" -> AuthProvider.GITHUB; - case "google" -> AuthProvider.GOOGLE; - default -> throw new IllegalStateException("Unknown OAuth provider: " + providerStr); - }; + AuthProvider provider = AuthProvider.from(providerStr);
98-112: 쿠키 보안 설정 및 리다이렉트 URL을 환경 설정으로 분리하는 것을 권장합니다현재:
secure(false)로 설정되어 있고 주석에 “로컬 개발”이라고 되어 있지만, 이 설정이 그대로 운영에도 적용될 수 있습니다.sameSite("Lax")로 고정되어 있어, 배포 환경(도메인/프론트 도메인 분리 등)에 따라 CORS/Cookie 이슈가 생길 수 있습니다.- 리다이렉트 URL이
"http://localhost:5173/oauth/success"로 하드코딩돼 있습니다.운영 환경까지 고려하면 다음을 추천드립니다.
secure,sameSite, 리다이렉트 URL(및 쿠키 도메인)을application-*.yml설정값으로 분리.- 프로파일(로컬/스테이징/실서버)에 따라
- 로컬:
secure=false,SameSite=Lax또는None(+ http),- 운영:
secure=true, HTTPS 도메인, 필요한 경우SameSite=None+ CORS 설정
로 다르게 가져가도록 구성.지금 단계에서는 로컬 개발을 우선하셨겠지만, 나중에 배포 시 이 부분을 꼭 챙겨 주셔야 할 것 같습니다.
120-127: 리다이렉트 URL 하드코딩 대신 설정 기반으로 전환을 고려해 보세요String redirectUrl = "http://localhost:5173/oauth/success";로그인 성공 후 프론트 주소가 바뀌거나, 환경(로컬/스테이징/운영)별로 다른 URL을 사용해야 할 때마다 코드를 수정/배포해야 합니다.
@Value또는 설정 프로퍼티(@ConfigurationProperties)로 redirect base URL을 주입받아 사용하면,
- 환경별 설정만 변경해도 되고,
- 테스트에서도 더 유연하게 핸들링할 수 있습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/src/main/java/org/sejongisc/backend/auth/entity/AuthProvider.java(1 hunks)backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOAuth2UserService.java(1 hunks)backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOidcUserService.java(1 hunks)backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java(4 hunks)backend/src/main/java/org/sejongisc/backend/common/auth/config/SecurityConfig.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOidcUserService.java (2)
backend/src/main/java/org/sejongisc/backend/common/auth/config/CustomOAuth2UserService.java (1)
Slf4j(25-121)backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java (1)
Slf4j(32-130)
🔇 Additional comments (2)
backend/src/main/java/org/sejongisc/backend/common/auth/config/SecurityConfig.java (1)
56-69: OAuth2/OIDC userInfoEndpoint 설정은 합리해 보이지만, Google이 실제로 OIDC로 들어오는지 확인이 필요합니다현재 설정:
.userInfoEndpoint(u -> { u.userService(customOAuth2UserService); // kakao, github u.oidcUserService(customOidcUserService); // google })
- Spring Security에서는 클라이언트 registration이 OIDC인 경우
oidcUserService가, 그 외에는userService가 사용됩니다.- 즉, Google registration이 OIDC로 설정되어 있어야
CustomOidcUserService가 실제로 사용되고, 그 안에서 세팅한provider,providerUid가OAuth2SuccessHandler까지 전달됩니다.구성 파일(yaml/properties)의 Google client registration이 OIDC로 설정되어 있는지 한 번만 확인해 두시면 좋겠습니다.
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java (1)
50-52: 검토 의견의 주요 전제가 Spring Security 클래스 계층 구조와 맞지 않습니다검토 의견에서는
DefaultOidcUser가DefaultOAuth2User를 상속하지 않는다고 주장하지만, Spring Security 공식 문서에 따르면DefaultOidcUser는 실제로DefaultOAuth2User를 확장(extends)합니다.실제 클래스 계층:
- DefaultOidcUser는 DefaultOAuth2User를 확장하고 OidcUser를 구현합니다
- 따라서
instanceof DefaultOAuth2User검사는 DefaultOidcUser 인스턴스에 대해 성공합니다- DefaultOidcUser는 OIDC 인증용 DefaultOAuth2User의 확장 구현입니다
검증된 코드:
CustomOidcUserService는DefaultOidcUser반환 (line 44-50)CustomOAuth2UserService는DefaultOAuth2User반환 (line 92-96)- 현재 instanceof 검사는 두 경우 모두 성공합니다
검토 의견의 핵심 주장인 "OIDC 로그인 시 항상
IllegalStateException이 발생할 가능성"은 실제 클래스 계층 구조에서 발생하지 않습니다. 현재 코드는 OIDC 로그인도 올바르게 처리합니다.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java (1)
106-114: CRITICAL: refreshCookie 설정이 중복되어 충돌하고 있습니다Lines 108-111에서
secure와sameSite속성이 두 번 설정되어 있습니다:
- Line 108-109:
secure(false),sameSite("Lax")- Line 110-111:
secure(true),sameSite("None")이는 명백한 버그입니다. 또한
secure=false와sameSite=None의 조합은 대부분의 브라우저에서 거부됩니다(SameSite=None은 Secure 쿠키여야 함).로컬 개발 환경이라면
secure(false)와sameSite("Lax")만 유지하고, 프로덕션에서는 환경 변수로secure(true)와sameSite("None")을 설정하도록 수정하세요.ResponseCookie refreshCookie = ResponseCookie.from("refresh", refreshToken) .httpOnly(true) .secure(false) .sameSite("Lax") - .secure(true) - .sameSite("None") .path("/") .maxAge(60L * 60 * 24 * 14) // 2 weeks .build();
♻️ Duplicate comments (1)
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java (1)
76-82: 예외 처리 방식을 개선하세요successHandler 내부에서
RuntimeException을 던지면failureHandler가 처리하지 못하고 500 에러가 발생하여 프론트엔드의/oauth/fail핸들링이 작동하지 않습니다. 이전 리뷰에서 이미 지적된 사항입니다.try-catch 블록으로 예외를 감싸고 에러 발생 시 명시적으로 리다이렉트하거나, OAuth2 인증 단계에서
OAuth2AuthenticationException을 던지도록 수정하세요.+try { UserOauthAccount account = userOauthAccountRepository .findByProviderAndProviderUid(provider, providerUid) .orElseThrow(() -> new RuntimeException("소셜 계정이 DB에 없습니다. (회원가입 필요)")); User user = userRepository.findById(account.getUser().getUserId()) .orElseThrow(() -> new RuntimeException("User not found")); + + // JWT 생성 및 쿠키 설정 로직... + +} catch (Exception e) { + log.error("[OAuth2SuccessHandler] Error during authentication success handling", e); + getRedirectStrategy().sendRedirect(request, response, + "http://localhost:5173/oauth/fail?error=account_not_found"); + return; +}
🧹 Nitpick comments (3)
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java (3)
17-17: 사용하지 않는 import를 제거하세요
OidcUser를 import했지만 코드에서 사용하지 않고 있습니다.-import org.springframework.security.oauth2.core.oidc.user.OidcUser;
122-122: 리다이렉트 URL을 설정 파일로 외부화하세요
http://localhost:5173/oauth/success가 하드코딩되어 있습니다. 프로덕션 환경에서는 다른 URL을 사용해야 하므로 환경 변수나 설정 파일로 관리하는 것이 좋습니다.@Value("${oauth.success.redirect-url}") private String oauthSuccessRedirectUrl; // ... String redirectUrl = oauthSuccessRedirectUrl;
48-48: 주석 처리된 로그를 정리하세요디버깅에 필요하다면 활성화하고, 불필요하다면 제거하는 것이 코드 가독성에 좋습니다.
Also applies to: 73-73, 127-127
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java(6 hunks)
🔇 Additional comments (1)
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java (1)
50-52: LGTM!패턴 매칭을 사용한 타입 체크가 올바르게 구현되어 있습니다.
| Map<String, Object> attrs = oauthUser.getAttributes(); | ||
|
|
||
| String providerStr = (String) attrs.get("provider"); | ||
| String providerUid = (String) attrs.get("providerUid"); |
There was a problem hiding this comment.
🧩 Analysis chain
CustomOAuth2UserService와 CustomOidcUserService가 필수 속성을 설정하는지 확인하세요
이 핸들러는 attributes에 provider와 providerUid가 존재한다고 가정합니다. 이 값들이 OAuth2/OIDC 사용자 서비스에서 올바르게 설정되는지 확인이 필요합니다.
다음 스크립트를 실행하여 사용자 서비스에서 이러한 속성을 설정하는지 확인하세요:
🏁 Script executed:
#!/bin/bash
# Description: Verify that CustomOAuth2UserService and CustomOidcUserService set provider and providerUid attributes
# Search for where provider and providerUid are added to attributes
rg -nP --type=java -C5 '(put\("provider"|put\("providerUid"|provider.*=|providerUid.*=)'
# Find CustomOAuth2UserService and CustomOidcUserService implementations
fd -t f "(CustomOAuth2UserService|CustomOidcUserService).java" -x cat {}Length of output: 24333
providerUid의 null 검증 누락
CustomOAuth2UserService와 CustomOidcUserService는 속성을 올바르게 설정하고 있습니다. 그러나 OAuth2SuccessHandler의 60-62줄에서 provider null 검증은 있지만, providerUid의 null 검증이 누락되어 있습니다.
77줄의 findByProviderAndProviderUid(provider, providerUid) 호출 전에 providerUid null 검증을 추가하세요.
if (providerStr == null) {
throw new IllegalStateException("OAuth provider attribute missing from attributes");
}
if (providerUid == null) {
throw new IllegalStateException("OAuth providerUid attribute missing from attributes");
}🤖 Prompt for AI Agents
In
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java
around lines 55-77, the code checks provider for null but omits a null check for
providerUid before calling findByProviderAndProviderUid; add a null validation
for providerUid (mirror the existing provider check) and throw an
IllegalStateException with a clear message like "OAuth providerUid attribute
missing from attributes" when providerUid is null so the subsequent
findByProviderAndProviderUid call is safe.
| String providerStr = (String) attrs.get("provider"); | ||
| String providerUid = (String) attrs.get("providerUid"); | ||
|
|
||
| if (providerStr == null) { | ||
| throw new IllegalStateException("OAuth provider attribute missing from attributes"); | ||
| } | ||
|
|
||
| AuthProvider provider = | ||
| switch (providerStr) { | ||
| case "kakao" -> AuthProvider.KAKAO; | ||
| case "github" -> AuthProvider.GITHUB; | ||
| case "google" -> AuthProvider.GOOGLE; | ||
| default -> throw new IllegalStateException("Unknown OAuth provider: " + providerStr); | ||
| }; |
There was a problem hiding this comment.
providerUid에 대한 null 체크를 추가하세요
providerStr은 null 체크를 하고 있지만, providerUid는 null 체크가 누락되어 있습니다. providerUid가 null인 경우 Line 77의 DB 조회에서 예기치 않은 동작이 발생할 수 있습니다.
String providerStr = (String) attrs.get("provider");
String providerUid = (String) attrs.get("providerUid");
if (providerStr == null) {
throw new IllegalStateException("OAuth provider attribute missing from attributes");
}
+if (providerUid == null) {
+ throw new IllegalStateException("OAuth providerUid attribute missing from attributes");
+}
AuthProvider provider =
switch (providerStr) {🤖 Prompt for AI Agents
In
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java
around lines 57 to 70, add a null-check for providerUid like the existing check
for providerStr and throw an IllegalStateException with a clear message if
providerUid is null (e.g., "OAuth provider UID attribute missing from
attributes"); ensure this check occurs before any DB lookup or further
processing so you fail-fast and avoid unexpected behavior when providerUid is
missing.
| ResponseCookie accessCookie = ResponseCookie.from("access", accessToken) | ||
| .httpOnly(true) | ||
| .secure(true) | ||
| .sameSite("None") | ||
| .secure(false) // 로컬 개발 | ||
| .sameSite("Lax") // 로컬에서는 None 비추천 | ||
| .path("/") | ||
| .maxAge(60L * 60) // 1 hour | ||
| .build(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
쿠키 보안 설정을 환경별로 구성 가능하도록 개선하세요
현재 secure(false)와 sameSite("Lax")가 하드코딩되어 있어 로컬 개발 환경에만 적합합니다. 프로덕션 배포 시 이 설정은 보안 취약점이 됩니다.
Spring 프로파일이나 환경 변수를 사용하여 환경별로 다른 설정을 적용하도록 수정하세요.
예시:
@Value("${cookie.secure:false}")
private boolean cookieSecure;
@Value("${cookie.sameSite:Lax}")
private String cookieSameSite;
// 쿠키 생성 시
ResponseCookie accessCookie = ResponseCookie.from("access", accessToken)
.httpOnly(true)
.secure(cookieSecure)
.sameSite(cookieSameSite)
.path("/")
.maxAge(60L * 60)
.build();🤖 Prompt for AI Agents
In
backend/src/main/java/org/sejongisc/backend/common/auth/config/OAuth2SuccessHandler.java
around lines 98-104, the cookie security options are hard-coded (secure(false),
sameSite("Lax")) which is unsafe for production; change to read these values
from configuration (e.g. @Value or Environment) with sensible defaults (secure
default true, sameSite default "Lax" or "None" per needs), replace the literal
calls with the injected variables when building the ResponseCookie, add
corresponding properties to application.properties/application-prod.properties
(and/or profiles) and validate/normalize the sameSite value before use.
2 participants
Summary by CodeRabbit
신기능
버그 수정