Merged
Conversation
…결정되도록 구현 - CustomFailureHandler 코드 리팩토링 및 파라미터를 통해 동적 리다이렉트 - CustomSuccessHandler 코드 리팩토링 및 파라미터를 통해 동적 리다이렉트 - OAuth2DTO Redis에 저장할 유저 정보 DTO와 OAuth2 파라미터를 전달할 DTO를 추가 - OAuthParams 파라미터에서 OAuth2Params 객체를 만들때 유효성 검사를 하며 데이터를 조회할 때 Enum을 통해 꺼내도록 함으로써 오타 방지와 유지보수 용이 - OAuth2HandlerProperties 리다이렉트 URL 뿐만아니라 지정된 도메인을 화이트리스트 방식으로 가져오도록 변경 - OAuth2UserService 파라미터에 따라 Seller 로직, Customer 로직 (미구현) 분기 - OAuth2ClientValidationFilter OAuth2 로그인 URL이 유효하지 검증하도록 책임 확장 - CustomOAuth2AuthorizationRequestResolver OAuth2 로그인 시 파라미터를 추출
1 task
WalkthroughOAuth2 상태(state) 인코딩/디코딩과 프로파일 기반 도메인 리다이렉트, Redis 임시 코드 저장, DTO 통합(OAuth2InfoRedisDTO → OAuth2DTO)이 도입되었고 이를 위한 상태 파서·커스텀 리졸버·검증 필터·핸들러·보안 설정 확장이 추가되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant User as 사용자
participant Client as 클라이언트
participant Filter as OAuth2ClientValidationFilter
participant Resolver as CustomOAuth2AuthorizationRequestResolver
participant AuthServer as OAuth2_인증서버
participant Success as CustomSuccessHandler
participant Redis as Redis
participant Props as OAuth2HandlerProperties
User->>Client: OAuth2 로그인 요청 (?user=...&profile=...)
Client->>Filter: /oauth2/authorization/{provider} 요청
Filter->>Filter: profile 기본값 주입, 파라미터·state 검증 (OAuth2StateParser)
Filter->>Resolver: 권한요청 생성(래핑된 request)
Resolver->>AuthServer: redirect(state에 user/profile 결합)
AuthServer->>Success: 콜백 (/login/oauth2/code/{provider}?state=...)
Success->>Success: state 파싱 → OAuthParams 생성
Success->>Redis: 임시 코드(UUID) 저장 (TTL)
Success->>Props: getSuccessUrl(UUID, OAuthParams)
Success->>Client: 도메인 기반 성공 URL로 리다이렉트 (?code=UUID)
sequenceDiagram
participant Client as 클라이언트
participant Filter as OAuth2ClientValidationFilter
participant StateParser as OAuth2StateParser
participant Failure as CustomFailureHandler
participant Props as OAuth2HandlerProperties
Client->>Filter: 잘못된/누락된 OAuth2 요청
Filter->>StateParser: state 파싱 시도
StateParser-->>Filter: OAuth2Exception(INVALID_OAUTH_PARAMS)
Filter->>Failure: 인증 실패 처리 전달
Failure->>StateParser: state 파싱(로그/알림용)
StateParser-->>Failure: 예외/파싱 실패
Failure->>Props: getErrorUrlWithReferer(INVALID_OAUTH_PARAMS, referer)
Failure->>Client: 에러 도메인으로 리다이렉트
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
[refactor] Seller 연락처/출고지 Store 이관에 따른 createOrderDelivery 수정
- OAuth2DTO의 SRP를 지키기위해 클래스로 기능을 묶음 - OAuth2DTO와 HttpServletRequest와의 결합력 낮춤 - FailureHandler에서 예외처리 도중 문제 발생 시 서버 내의 HTML 파일 출력 - OAuth2ClientValidationFilter에서 OAuth URL 검증 시 profile 파라미터의 디폴트 값을 prod로 설정 - OAuth2UserService에서 OCP를 위해 Enum Factory 방식으로 리팩토링
…결정되도록 구현 - CustomFailureHandler 코드 리팩토링 및 파라미터를 통해 동적 리다이렉트 - CustomSuccessHandler 코드 리팩토링 및 파라미터를 통해 동적 리다이렉트 - OAuth2DTO Redis에 저장할 유저 정보 DTO와 OAuth2 파라미터를 전달할 DTO를 추가 - OAuthParams 파라미터에서 OAuth2Params 객체를 만들때 유효성 검사를 하며 데이터를 조회할 때 Enum을 통해 꺼내도록 함으로써 오타 방지와 유지보수 용이 - OAuth2HandlerProperties 리다이렉트 URL 뿐만아니라 지정된 도메인을 화이트리스트 방식으로 가져오도록 변경 - OAuth2UserService 파라미터에 따라 Seller 로직, Customer 로직 (미구현) 분기 - OAuth2ClientValidationFilter OAuth2 로그인 URL이 유효하지 검증하도록 책임 확장 - CustomOAuth2AuthorizationRequestResolver OAuth2 로그인 시 파라미터를 추출
- OAuth2DTO의 SRP를 지키기위해 클래스로 기능을 묶음 - OAuth2DTO와 HttpServletRequest와의 결합력 낮춤 - FailureHandler에서 예외처리 도중 문제 발생 시 서버 내의 HTML 파일 출력 - OAuth2ClientValidationFilter에서 OAuth URL 검증 시 profile 파라미터의 디폴트 값을 prod로 설정 - OAuth2UserService에서 OCP를 위해 Enum Factory 방식으로 리팩토링
…nto feature/oauth2-login-redirect
- 디버깅용 log 삭제
mono0801
requested review from
CUCU7103,
exjuu,
kimbro97,
kimjongsoo97 and
kmindev
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/com/bbangle/bbangle/auth/seller/service/OAuthSellerServiceUnitTest.java (1)
79-85:⚠️ Potential issue | 🟡 Minor테스트 mock 설정에서 잘못된 클래스 타입 사용
실패 테스트에서
OAuth2DTO.class를 사용하고 있지만, 실제 프로덕션 코드(OAuthSellerService.getSellerInfoFromRedis)는OAuth2DTO.InfoDTO.class를 사용합니다. 이로 인해 mock이 실제 메서드 호출과 일치하지 않아 테스트가 의도한 대로 동작하지 않습니다.🐛 수정 제안
given( redisRepository.getDTOAndDelete( OAuthSellerService.OAUTH_CODE_NAMESPACE, "code", - OAuth2DTO.class + OAuth2DTO.InfoDTO.class ) ).willReturn(null);
🤖 Fix all issues with AI agents
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java`:
- Around line 53-60: The getDomain(ProfileType profile, UserType user) method
can throw NPE when profile or user is null; add explicit null-checks at the
start of getDomain to validate both profile and user (e.g., if (profile == null)
throw new IllegalArgumentException("profile must not be null"); and similarly
for user) and then keep the existing switch logic; ensure the exception messages
name the offending parameter so callers get a clear failure reason rather than
an NPE.
In
`@src/test/java/com/bbangle/bbangle/config/security/auth/CustomFailureHandlerUnitTest.java`:
- Around line 103-114: The test uses Mockito's argument matcher any() as a stub
return value for stateParser.getParams in CustomFailureHandlerUnitTest; replace
willReturn(any()) with a concrete return value matching getParams' type (e.g.,
Collections.emptyMap(), Optional.empty(), null, or a small Map of expected
params) so the stub returns a real value; update the line with
given(stateParser.getParams(any())).willReturn(...) to supply that concrete
object and keep the existing verifications for
customFailureHandler.onAuthenticationFailure, slackAdaptor.sendAlert, and
response.sendRedirect.
🧹 Nitpick comments (7)
src/main/resources/static/oauth.html (1)
1-60: 로고/파비콘 URL 하드코딩은 환경 의존성이 있습니다
현재 개발용 S3 도메인에 고정되어 있어 운영 환경에서 깨질 수 있습니다. 가능하면 상대 경로 또는 환경별 설정으로 분리하는 방향을 고려해 주세요.src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java (1)
124-133: 설정 계약 문서화 또는 절대 URL 처리 분기 추가 필요
oauth2HandlerProperties.redirect().error()설정값의 형식이 명시되지 않아 문제가 발생할 수 있습니다. 현재 코드는 경로만 처리 가능하지만, 절대 URL로 설정될 경우replacePath()조합이 예상대로 동작하지 않습니다.다음 중 하나를 선택하세요:
- 설정값이 경로-only임을 문서화하고 config validation 추가
- 절대 URL도 지원하도록 분기 처리 추가 (아래 예시 참고)
절대 URL 지원 구현
- return ServletUriComponentsBuilder.fromRequestUri(request) - .replacePath(oauth2HandlerProperties.redirect().error()) // path 교체 - .replaceQuery(null) // 기존 쿼리 제거 - .queryParam("error", code) - .queryParam("code", code.getCode()) - .build() - .toUriString(); + String errorBase = oauth2HandlerProperties.redirect().error(); + UriComponentsBuilder builder = errorBase.startsWith("http") + ? UriComponentsBuilder.fromUriString(errorBase) + : ServletUriComponentsBuilder.fromRequestUri(request).replacePath(errorBase); + + return builder + .replaceQuery(null) + .queryParam("error", code) + .queryParam("code", code.getCode()) + .build() + .toUriString();src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java (2)
52-61:RequestContextHolder사용 시 주의 필요
RequestContextHolder는 동기 서블릿 환경에서는 잘 동작하지만, 비동기 처리나 다른 스레드에서 호출될 경우null을 반환하거나 예외가 발생할 수 있습니다. 현재 OAuth2 플로우에서는 문제없지만, 향후 비동기 처리 도입 시 주의가 필요합니다.
90-94: TODO 확인: Customer 로그인 로직 미구현현재 Customer 로그인 시
_NOT_SUPPORTED_YET예외를 던지도록 되어 있습니다. 이슈#596과연관된 후속 작업으로 보이며, Customer 로그인이 필요한 시점에 구현이 필요합니다.이 기능을 구현하거나 추적 이슈를 생성하는 것을 도와드릴까요?
src/main/java/com/bbangle/bbangle/config/security/OAuth2SecurityConfig.java (1)
70-75: Bean 메서드 이름 컨벤션
OAuth2Resolver메서드 이름이 대문자로 시작합니다. Java 메서드 이름 컨벤션에 따르면 소문자로 시작하는 것이 일반적입니다(oAuth2Resolver또는oauth2Resolver).♻️ 수정 제안
`@Bean` - public CustomOAuth2AuthorizationRequestResolver OAuth2Resolver( + public CustomOAuth2AuthorizationRequestResolver oAuth2Resolver( ClientRegistrationRepository clientRegistrationRepository ) { return new CustomOAuth2AuthorizationRequestResolver(clientRegistrationRepository); }src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java (1)
15-27: 쿼리 문자열 결합 방식 때문에 기존 쿼리가 있는 URL이 깨질 수 있습니다.success/error 값에 이미 쿼리가 포함되어 있으면 현재 로직은
?가 중복되어 잘못된 URL이 됩니다. 구분자를 계산해 추가하는 방식이 더 안전합니다.🛠️ 개선 제안(diff)
public record RedirectUrl( String success, String error ) { private String createSuccessUrl(UUID uuid) { - return success + "?generateToken=" + uuid; + String separator = success.contains("?") ? "&" : "?"; + return success + separator + "generateToken=" + uuid; } private String createErrorUrl(BbangleErrorCode code) { - StringBuilder url = new StringBuilder(error); - String error = code != null ? code.toString() : "UNKNOWN_ERROR"; - - url.append("?error=").append(error); - if (code != null) url.append("&code=").append(code.getCode()); - - return url.toString(); + String errorCode = code != null ? code.toString() : "UNKNOWN_ERROR"; + String separator = error.contains("?") ? "&" : "?"; + StringBuilder url = new StringBuilder(error) + .append(separator) + .append("error=") + .append(errorCode); + if (code != null) url.append("&code=").append(code.getCode()); + return url.toString(); } }Also applies to: 33-44
src/main/java/com/bbangle/bbangle/config/security/auth/CustomFailureHandler.java (1)
87-97: params가 null일 때 code도 null이면 NPE 가능성이 있습니다.현재 경로에서는 발생 가능성이 낮지만, 방어적으로 기본 에러 값을 두면 안전합니다.
🛠️ 방어 로직 제안(diff)
private void redirectToErrorPage( HttpServletRequest request, HttpServletResponse response, OAuthParams params, BbangleErrorCode code ) throws IOException { if (params == null) { - getRedirectStrategy().sendRedirect(request, response, DEFAULT_OAUTH_PAGE + "?error=" + code.name()); + String errorCode = (code != null) ? code.name() : "UNKNOWN_ERROR"; + getRedirectStrategy().sendRedirect(request, response, DEFAULT_OAUTH_PAGE + "?error=" + errorCode); } else { getRedirectStrategy().sendRedirect(request, response, oauth2HandlerProperties.getErrorUrl(code, params)); } }
src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java
Show resolved
Hide resolved
src/test/java/com/bbangle/bbangle/config/security/auth/CustomFailureHandlerUnitTest.java
Outdated
Show resolved
Hide resolved
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)
src/main/java/com/bbangle/bbangle/config/security/auth/CustomSuccessHandler.java (1)
32-44:⚠️ Potential issue | 🟠 MajorCustomSuccessHandler에서 state 파싱 실패 시 예외 처리 추가 필요
stateParser.getParams()(39행)에서OAuth2Exception이 발생할 수 있으나 예외 처리가 없습니다. CustomFailureHandler는 동일한 작업에 대해 try-catch로 명시적으로 처리하고 있으므로(38-45행), SuccessHandler에서도 일관된 예외 처리를 추가해야 합니다. SuccessHandler의 예외는 FailureHandler로 자동 위임되지 않으며, Spring Security의 기본 exception handling으로 처리되어 사용자 경험이 일관되지 않을 수 있습니다.
🤖 Fix all issues with AI agents
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java`:
- Around line 124-133: createRedirectUrl currently builds a redirect URL from
the incoming request host (using ServletUriComponentsBuilder) which causes
redirects to the backend host; change it to use the
oauth2HandlerProperties.getErrorUrl(...) logic used by
CustomFailureHandler/CustomSuccessHandler so the frontend domain is chosen based
on profile/user. In practice, inside createRedirectUrl(HttpServletRequest
request, BbangleErrorCode code) gather the same profile and user params already
extracted earlier (from the wrapped request / request parameters) and call
oauth2HandlerProperties.getErrorUrl(code, params) (or the equivalent getErrorUrl
signature) to produce the redirect URL instead of constructing it from the
request. Ensure createRedirectUrl returns that getErrorUrl result.
In
`@src/test/java/com/bbangle/bbangle/auth/seller/service/OAuthSellerServiceIntegrationTest.java`:
- Around line 70-71: The test is retrieving a Redis entry stored as
OAuth2DTO.InfoDTO but calls redisRepository.getDTOAndDelete with
OAuth2DTO.class, causing deserialization errors; update the call to use the
correct type class (OAuth2DTO.InfoDTO.class) when calling getDTOAndDelete for
the OAuthSellerService.OAUTH_CODE_NAMESPACE and "code" key so the stored InfoDTO
is deserialized correctly.
In
`@src/test/java/com/bbangle/bbangle/config/security/auth/OAuth2UserServiceUnitTest.java`:
- Around line 253-268: The test OAuth2UserServiceUnitTest is missing a stub for
stateParser.getParams which can return null and cause an NPE instead of the
expected OAuth2Exception; update the test that calls
oAuth2UserService.loadUser(request) to stub stateParser.getParams(...) to return
a non-null params map (e.g., a Map with required keys) before invoking loadUser
so the code path reaches the missing-name handling; locate the test method
containing given(request.getClientRegistration())... and add a
doReturn(paramsMap).when(stateParser).getParams(any()) (or the equivalent
stubbing) so verify(oAuth2SellerFacade, never()).login(any()) assertion remains
valid.
🧹 Nitpick comments (4)
src/main/java/com/bbangle/bbangle/auth/oauth/client/dto/OAuth2DTO.java (1)
8-9:@Slf4j어노테이션이 사용되지 않습니다.이 클래스에서 로깅 기능을 사용하지 않으므로
@Slf4j어노테이션을 제거하는 것이 좋습니다.♻️ 제안된 수정
-import lombok.extern.slf4j.Slf4j; - -@Slf4j public class OAuth2DTO {src/main/java/com/bbangle/bbangle/auth/oauth/client/OAuth2StateParser.java (1)
37-46:Exception대신 더 구체적인 예외 타입 사용 고려
catch (Exception e)대신IllegalArgumentException과NullPointerException을 명시적으로 처리하면 의도가 더 명확해집니다. 다만 현재 구현도 안전하게 동작합니다.♻️ 제안된 수정 (선택사항)
public OAuthParams getParams(String user, String profile) { try { return OAuthParams.builder() .user(UserType.valueOf(user.toUpperCase())) .profile(ProfileType.valueOf(profile.toUpperCase())) .build(); - } catch (Exception e) { + } catch (IllegalArgumentException | NullPointerException e) { throw new OAuth2Exception(BbangleErrorCode.INVALID_OAUTH_PARAMS, e); } }src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java (2)
15-20: 변수명이 메서드명과 동일하여 혼란을 줄 수 있습니다.Line 16에서 지역 변수
domain이 record 컴포넌트 접근자domain()과 동일한 이름을 사용하고 있습니다. 현재는 동작하지만, 코드 가독성을 위해 변수명을 구분하는 것이 좋습니다.♻️ 제안된 수정
public String getSuccessUrl(UUID uuid, OAuthParams params) { - String domain = domain().getDomain(params.profile(), params.user()); + String targetDomain = domain().getDomain(params.profile(), params.user()); String uri = redirect().createSuccessUrl(uuid); - return domain + uri; + return targetDomain + uri; }
22-27:getErrorUrl에서도 동일한 변수명 shadowing 이슈가 있습니다.
getSuccessUrl과 마찬가지로 지역 변수명을 변경하는 것이 좋습니다.♻️ 제안된 수정
public String getErrorUrl(BbangleErrorCode code, OAuthParams params) { - String domain = domain().getDomain(params.profile(), params.user()); + String targetDomain = domain().getDomain(params.profile(), params.user()); String uri = redirect().createErrorUrl(code); - return domain + uri; + return targetDomain + uri; }
src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java
Outdated
Show resolved
Hide resolved
src/test/java/com/bbangle/bbangle/auth/seller/service/OAuthSellerServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/bbangle/bbangle/config/security/auth/OAuth2UserServiceUnitTest.java
Show resolved
Hide resolved
- OAuth2ClientValidationFilter에서 잘못된 URL을 입력했을 시 프론트로 돌려보내지 못하는 문제 수정
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java`:
- Around line 123-125: createRedirectUrl currently passes
request.getHeader("Referer") raw into oauth2HandlerProperties.getErrorUrl which
can be null or a full URL; update createRedirectUrl to null-check the Referer,
parse it to extract the host (e.g., via URI#getHost) or fall back to
request.getServerName(), then pass that host/domain string to
oauth2HandlerProperties.getErrorUrl; also update
OAuth2HandlerProperties.Domain.getDomain(String referer) to perform robust
matching using contains() or startsWith() against the incoming host instead of
strict equality so domain resolution no longer fails when Referer is a full URL
or contains subpaths.
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java`:
- Around line 67-72: The getDomain(String referer) method is null-unsafe and
uses exact equals checks that fail for full Referer URLs; update getDomain to
first guard against null/empty referer, then parse or normalize the referer
(e.g., create a URI and compare host, or use startsWith/or contains against
local, customer, seller base URLs) to determine which domain constant (local,
customer, seller) to return; ensure you reference the existing method getDomain
and the domain fields local, customer, seller and return a sensible default
(local) when referer is null or doesn’t match.
In `@src/main/resources/application-local.yml`:
- Around line 76-79: application-local.yml의 domain.local, domain.seller,
domain.customer에 환경변수가 없을 경우 플레이스홀더 해석 실패로 애플리케이션이 기동하지 않을 수 있으니 domain.local /
domain.seller / domain.customer 설정에서 기본값을 제공하거나(.env 및 로컬 실행 문서에 필수 항목으로 추가하는
대신) 환경변수가 누락되었을 때 안전하게 동작하도록 구성하세요; 구체적으로 domain.local, domain.seller,
domain.customer 항목을 찾아 각 플레이스홀더에 기본값을 지정하거나 로컬 실행 문서(.env 예시 포함)에 해당 세 변수를 필수로
명시해 누락 시 앱이 실패하지 않도록 만드세요.
In `@src/main/resources/static/oauth.html`:
- Around line 8-55: The page currently hardcodes the logo URL in the <link
href="...logo.svg"> and <img src="...logo.svg"> tags which points to a dev S3
domain; replace those hardcoded external URLs with a local static asset or an
environment-configurable path. Add the logo file to your app's static resources
and update the <link> and <img> references (the logo <link href> and the <img
src> in oauth.html) to use a relative path (or a template variable / config
property) so the asset is served from your app in all environments rather than
relying on the dev S3 domain.
🧹 Nitpick comments (1)
src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java (1)
92-95: Customer 로그인 TODO 추적 권장
현재는 _NOT_SUPPORTED_YET로 처리되고 있어 추후 일정이 잡히면 이슈로 관리하는 편이 좋겠습니다. 원하시면 기본 구현안/스텁 정리 도와드릴게요.
src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java (1)
92-96: Customer 미지원 처리 명시 OK. TODO 추적이 필요하면 말씀해주세요.필요하시다면 Customer 로그인 로직 초안/이슈 템플릿을 만들어 드릴까요?
src/main/java/com/bbangle/bbangle/config/security/auth/CustomFailureHandler.java (1)
61-76: 로깅 전략 검토 필요
code == null인 경우(알 수 없는 예외)와 5xx 에러 모두 Slack 알림이 전송됩니다. 4xx 에러는 경고 로그만 남기고 알림을 보내지 않는 현재 로직이 의도된 것인지 확인해 주세요.src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java (1)
31-31: 상수 선언 개선
PARAM_PROFILE은 인스턴스 필드가 아닌 클래스 상수로 선언해야 합니다.♻️ 수정 제안
- private final String PARAM_PROFILE = "profile"; + private static final String PARAM_PROFILE = "profile";
There was a problem hiding this comment.
🧹 Nitpick comments (11)
src/test/java/com/bbangle/bbangle/order/seller/service/SellerOrderServiceTest.java (1)
383-389: 기존 배송 업데이트 테스트에서 Store 주소 필드 미설정이 테스트에서는
Store에phoneNumberVO,originAddressLine,originAddressDetail를 설정하지 않고 있습니다. 현재는 기존OrderDelivery가 있어createOrderDelivery가 호출되지 않으므로 문제가 되지 않지만, 다른 성공 케이스 테스트(Line 304-309, 458-463)와 일관성을 위해 설정해두는 것이 좋을 수 있습니다. 향후 테스트 로직 변경 시 잠재적 NPE를 방지할 수 있습니다.src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java (1)
41-49:error변수명이 레코드 필드를 섀도잉합니다.Line 43의 지역 변수
error가RedirectUrl레코드의 컴포넌트error를 섀도잉하고 있습니다. Line 42에서 먼저error필드를 사용한 후 Line 43에서 같은 이름의 지역 변수를 선언하므로, 현재 동작에는 문제가 없지만 향후 유지보수 시 혼동을 유발할 수 있습니다.♻️ 변수명 변경 제안
private String createErrorUrl(BbangleErrorCode code) { StringBuilder url = new StringBuilder(error); - String error = code != null ? code.toString() : "UNKNOWN_ERROR"; + String errorName = code != null ? code.toString() : "UNKNOWN_ERROR"; - url.append("?error=").append(error); + url.append("?error=").append(errorName); if (code != null) url.append("&code=").append(code.getCode()); return url.toString(); }src/main/java/com/bbangle/bbangle/auth/oauth/client/dto/OAuth2DTO.java (1)
8-9:@Slf4j어노테이션이 사용되지 않고 있습니다.현재 클래스 내에서
log를 사용하는 곳이 없습니다. 불필요한 어노테이션 제거를 고려해주세요.♻️ 제거 제안
-@Slf4j public class OAuth2DTO {src/main/java/com/bbangle/bbangle/auth/oauth/client/OAuth2StateParser.java (1)
37-46:getParams(String user, String profile)에서 null 입력 시 NPE가catch(Exception)으로 처리됩니다.
OAuth2ClientValidationFilter에서request.getParameter("user")가null일 경우,user.toUpperCase()에서 NPE가 발생하고 이것이 포괄적catch(Exception)으로 잡혀INVALID_OAUTH_PARAMS로 변환됩니다. 동작은 올바르지만, 의도를 명확히 하기 위해 null 검증을 선행하는 것을 고려해볼 수 있습니다.♻️ 명시적 null 검증 제안
public OAuthParams getParams(String user, String profile) { + if (user == null || profile == null) { + throw new OAuth2Exception(BbangleErrorCode.INVALID_OAUTH_PARAMS); + } try { return OAuthParams.builder() .user(UserType.valueOf(user.toUpperCase())) .profile(ProfileType.valueOf(profile.toUpperCase())) .build(); } catch (Exception e) { throw new OAuth2Exception(BbangleErrorCode.INVALID_OAUTH_PARAMS, e); } }src/main/java/com/bbangle/bbangle/config/security/auth/CustomSuccessHandler.java (1)
22-22:@Slf4j어노테이션이 선언되었지만log가 사용되지 않습니다.디버깅 용도로 추가한 것이라면, 현재 사용되지 않으므로 제거하거나 의미 있는 로깅을 추가하는 것을 고려해 주세요.
src/main/java/com/bbangle/bbangle/config/security/auth/CustomFailureHandler.java (1)
36-47: 상태 파싱 실패 시OAuth2Exception외의 예외 처리 누락 가능성
stateParser.getParams()가OAuth2Exception만 던지도록 구현되어 있지만,NullPointerException등 예상치 못한 런타임 예외가 발생하면 catch되지 않고 상위로 전파되어 Spring Security의 기본 에러 처리로 빠집니다. 방어적으로Exception을 함께 처리하거나, 현재 구조가 의도된 것이라면 무시해도 됩니다.🛡️ 방어적 예외 처리 제안
try { oauthParams = stateParser.getParams(request.getParameter("state")); } catch (OAuth2Exception e) { handleStateParsingFailure(request, response, e); return; + } catch (Exception e) { + handleStateParsingFailure(request, response, + new OAuth2Exception(BbangleErrorCode.INVALID_OAUTH_PARAMS, e)); + return; }src/test/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilterUnitTest.java (1)
38-48:@InjectMocks와 수동 생성이 혼재되어 있어 유지보수 시 혼란 가능
filter가@InjectMocks로 선언되어 있지만,redirect_client_not_found와redirect_invalid_oauth_params테스트에서는mockProp()과 함께 수동으로 재생성합니다.@InjectMocks로 주입된 인스턴스에는oauth2HandlerProperties가null이므로, 이후 happy path 로직 변경 시 예상치 못한 NPE가 발생할 수 있습니다. 모든 테스트에서 일관되게 수동 생성하거나,@Mock으로OAuth2HandlerProperties를 주입하는 방식을 권장합니다.src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java (1)
105-121:stateParser.getParams()결과가 사용되지 않고 버려집니다.
validateOAuth2Params에서stateParser.getParams(user, profile)의 반환값을 사용하지 않고 유효성 검증 목적으로만 호출합니다. 현재 동작에는 문제가 없지만, 검증 결과를 버리는 것이 의도적임을 명시하는 것이 좋습니다.🔧 의도를 명확히 하는 개선안
try { - stateParser.getParams(user, profile); + stateParser.getParams(user, profile); // 유효성 검증만 수행, 결과는 이후 state에서 재파싱 return true; } catch (OAuth2Exception e) {src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java (2)
54-63:RequestContextHolder의존은 동작하지만 취약한 패턴입니다.
DefaultOAuth2UserService.loadUser()는 HTTP 요청 스레드에서 호출되므로 현재는 동작하지만,RequestContextHolder에 대한 암묵적 의존은 비동기 처리나 다른 컨텍스트에서 재사용 시 문제가 될 수 있습니다.CustomOAuth2AuthorizationRequestResolver에서additionalParameters에 state 정보를 넣고OAuth2UserRequest.getAdditionalParameters()로 가져오는 방식이 더 안전할 수 있습니다.현재 구조에서 테스트를 위해
protected로 열어놓은 점은 적절합니다.
92-96: 미사용 변수member가 선언되어 있습니다.TODO 플레이스홀더임을 감안하더라도, 미사용 변수는 제거하는 것이 깔끔합니다.
🧹 미사용 변수 제거
private CustomUserDetails loginCustomer(OAuth2Response response) { - Member member; // TODO : 추후에 결정되면 Customer 로그인 로직 추가 throw new OAuth2Exception(BbangleErrorCode._NOT_SUPPORTED_YET); }src/test/java/com/bbangle/bbangle/config/security/auth/OAuth2UserServiceUnitTest.java (1)
272-304: CUSTOMER 로그인 경로에 대한 테스트 추가를 권장합니다.현재 모든 테스트가
SELLER경로만 검증합니다.loginCustomer가_NOT_SUPPORTED_YET예외를 던지는 것을 검증하는 테스트를 추가하면, 추후 Customer 로그인 구현 시 기존 동작 변경을 감지할 수 있습니다.🧪 CUSTOMER 경로 테스트 예시
`@Test` `@DisplayName`("Customer 로그인 시도 시 NOT_SUPPORTED_YET 예외를 던진다.") void fail_login_customer_not_supported() { // given OAuth2User oAuth2User = kakaoOAuth2User(); OAuthParams customerParams = OAuthParams.builder() .user(UserType.CUSTOMER) .profile(ProfileType.PROD) .build(); given(request.getClientRegistration()).willReturn(clientRegistration); given(clientRegistration.getRegistrationId()).willReturn("kakao"); given(stateParser.getParams(any())).willReturn(customerParams); doReturn(oAuth2User).when(oAuth2UserService).loadOAuth2User(any()); // when & then assertThatThrownBy(() -> oAuth2UserService.loadUser(request)) .isInstanceOf(OAuth2Exception.class) .satisfies(ex -> { OAuth2Exception oAuth2Ex = (OAuth2Exception) ex; assertThat(oAuth2Ex.getCode()) .isEqualTo(BbangleErrorCode._NOT_SUPPORTED_YET); }); }
- test 코드 수정
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/test/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilterUnitTest.java (5)
62-89: Referer 헤더가 없는 경우에 대한 테스트가 누락되어 있습니다.
redirect_client_not_found와redirect_invalid_oauth_params테스트 모두Referer헤더가 항상 "test"로 설정되어 있습니다. 프로덕션 환경에서 Referer 헤더가 없는 요청이 들어올 경우 (null),getErrorUrl에null이 전달되며 redirect URL 생성 시 NPE나 잘못된 URL이 만들어질 수 있습니다. Referer가 없는 경우의 fallback 동작을 검증하는 테스트 추가를 권장합니다.
100-103:stateParser.getParams()모킹 시 매처(matcher) 사용이 일관되지 않습니다.Line 102에서는
any(), anyString()을, Line 137/160에서는anyString(), anyString()을 사용하고 있습니다.any()는null도 매칭하지만anyString()은null을 매칭하지 않으므로, 실제 필터가null을 전달하는 경우 의도치 않게 모킹이 적용되지 않을 수 있습니다. 일관성을 위해 동일한 매처를 사용하는 것을 권장합니다.
125-146:pass_client_exists와oauth2_profileParam_default_wrapping테스트의 setup이 거의 동일합니다.두 테스트 모두 동일한 URI, user 파라미터, mock 설정을 사용하며, 검증 대상만 다릅니다(
pass_client_exists는 filterChain 호출 여부,oauth2_profileParam_default_wrapping은 wrapped request의 profile 파라미터). 현재 상태에서도 가독성은 충분하지만, profile 파라미터가 명시적으로 제공된 경우(예:"local")에 대한 테스트가 없습니다. 기본값이 아닌 profile이 전달될 때 올바르게 전파되는지 확인하는 테스트 추가를 고려해 주세요.Also applies to: 148-173
175-189: Non-OAuth2 요청 테스트가 깔끔합니다.
verifyNoInteractions(clientRegistrationRepository)검증으로 불필요한 repository 조회가 없음을 확인하는 점이 좋습니다. 추가로stateParser와oauth2HandlerProperties에 대해서도verifyNoInteractions를 적용하면 비-OAuth2 경로에서의 side-effect가 전혀 없음을 더 명확하게 보장할 수 있습니다.♻️ 제안 코드
verify(filterChain).doFilter(request, response); verifyNoInteractions(clientRegistrationRepository); + verifyNoInteractions(stateParser); + verifyNoInteractions(oauth2HandlerProperties); assertThat(response.getRedirectedUrl()).isNull();
71-76:.willAnswer()내부에서 URL 조합 로직을 재현하고 있습니다.두 테스트(
redirect_client_not_found,redirect_invalid_oauth_params)에서getErrorUrl모킹 시.willAnswer()로 실제 URL 구성 로직(referer + "/login?error=" + code + "&code=" + code.getCode())을 재현하고 있습니다. 이 방식은getErrorUrl의 내부 구현이 변경되면 테스트 모킹도 함께 수정해야 하는 커플링이 발생합니다.
.willReturn()으로 고정된 예상 URL을 반환하면 테스트가 더 단순해지고, 필터가getErrorUrl의 반환값을 그대로 redirect에 사용하는지만 검증하는 데 집중할 수 있습니다.♻️ 예시: redirect_client_not_found 테스트 단순화
- given(oauth2HandlerProperties.getErrorUrl(BbangleErrorCode.NOT_SUPPORTED_SERVER, "test")) - .willAnswer(invocation -> { - BbangleErrorCode code = invocation.getArgument(0); - String referer = invocation.getArgument(1); - return referer + "/login?error=" + code + "&code=" + code.getCode(); - }); + String expectedUrl = "test/login?error=NOT_SUPPORTED_SERVER&code=" + BbangleErrorCode.NOT_SUPPORTED_SERVER.getCode(); + given(oauth2HandlerProperties.getErrorUrl(BbangleErrorCode.NOT_SUPPORTED_SERVER, "test")) + .willReturn(expectedUrl);이후 assertion도 해당
expectedUrl과 직접 비교하면 됩니다.Also applies to: 104-109
- 프론트로 리다이렉팅이 힘든 경우 서버내의 정적 페이지를 출력
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/test/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilterUnitTest.java (1)
148-173:oauth2_profileParam_default_wrapping테스트에서profile파라미터를 명시적으로 설정한 경우도 검증하면 좋겠습니다.현재는
profile이 없는 경우만 테스트하고 있습니다.profile=local을 명시적으로 설정했을 때 래핑된 요청에서"local"이 반환되는지 확인하는 테스트를 추가하면,wrapRequestDefaultParams의 동작을 더 완전히 검증할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilterUnitTest.java` around lines 148 - 173, Add a complementary unit test (e.g., oauth2_profileParam_explicit_preserved) that mirrors oauth2_profileParam_default_wrapping but sets request.setParameter("profile", "local") before calling filter.doFilter(request, response, filterChain); then capture the wrapped request via ArgumentCaptor<HttpServletRequest> from verify(filterChain).doFilter(...), and assert that wrappedRequest.getParameter("profile") equals "local" and response.getRedirectedUrl() is null to ensure explicit profile values are preserved by the wrapping logic.src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java (1)
50-59:createErrorUri에서 지역 변수error가 레코드 컴포넌트error를 섀도잉합니다.Line 51의
new StringBuilder(error)는 레코드 필드를 사용하고, Line 53의String error는 동일한 이름의 지역 변수를 선언합니다. 동작에는 문제없지만 가독성이 떨어집니다.♻️ 변수 이름 변경 제안
private String createErrorUri(BbangleErrorCode code) { StringBuilder url = new StringBuilder(error); - String error = code != null ? code.toString() : "UNKNOWN_ERROR"; - url.append("?error=").append(error); + String errorName = code != null ? code.toString() : "UNKNOWN_ERROR"; + url.append("?error=").append(errorName); if (code != null) url.append("&code=").append(code.getCode()); return url.toString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java` around lines 50 - 59, The method createErrorUri(BbangleErrorCode code) shadows the record/component field error with a local variable named error; rename the local variable (for example to errorName or errorLabel) and update its usages in createErrorUri so new StringBuilder(...) and the "?error=" append reference the renamed local variable, keeping the conditional code appending code.getCode() unchanged; this eliminates the shadowing while preserving behavior.src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java (1)
105-121:stateParser.getParams()를 검증 목적으로만 호출하고 결과를 버리고 있습니다.
validateOAuth2Params에서stateParser.getParams(user, profile)의 반환값을 사용하지 않고, 예외 발생 여부만 확인합니다. 현재 동작에는 문제없지만, 불필요한 객체 생성입니다. 사소한 사항이므로 현재 상태로도 괜찮습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java` around lines 105 - 121, The validateOAuth2Params method currently calls stateParser.getParams(user, profile) only to trigger validation and discards its return value, causing unnecessary object creation; change this to call a validation-only method on the parser (e.g., add and call stateParser.validateParams(user, profile) which throws OAuth2Exception on failure and avoids building the params object) or, if adding a method is not possible, refactor getParams to support a lightweight validation path and update validateOAuth2Params to use that instead (references: validateOAuth2Params and stateParser.getParams).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java`:
- Around line 123-126: createRedirectUrl currently forwards the raw Referer
header into oauth2HandlerProperties.getErrorUrlWithReferer which can be missing
or manipulated; update createRedirectUrl to validate and normalize the Referer
by parsing its host (e.g., with URI), compare against an allowlist of trusted
domains (or extract only the hostname), and only pass a trusted domain to
getErrorUrlWithReferer; if parsing fails, host is not in the allowlist, or
Referer is null/empty, call getErrorUrlWithReferer with null so it falls back to
DEFAULT_OAUTH_PAGE; refer to createRedirectUrl and
oauth2HandlerProperties.getErrorUrlWithReferer when making the change.
- Around line 77-83: getDomain(String referer) uses String.contains() which
makes domain matching order-dependent and can pick shorter substrings (e.g.,
"local" inside "seller"); instead, extract the host from the referer (e.g., via
URI parsing) and perform exact comparisons (equalsIgnoreCase or equals) against
your known domain identifiers, with null checks and safe URI handling; update
getDomain(String referer) to parse referer -> host, then compare host to domain
constants (or use endsWith with a leading dot for subdomain allowance) rather
than contains(), and keep the existing null-safety logic.
---
Nitpick comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java`:
- Around line 105-121: The validateOAuth2Params method currently calls
stateParser.getParams(user, profile) only to trigger validation and discards its
return value, causing unnecessary object creation; change this to call a
validation-only method on the parser (e.g., add and call
stateParser.validateParams(user, profile) which throws OAuth2Exception on
failure and avoids building the params object) or, if adding a method is not
possible, refactor getParams to support a lightweight validation path and update
validateOAuth2Params to use that instead (references: validateOAuth2Params and
stateParser.getParams).
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java`:
- Around line 50-59: The method createErrorUri(BbangleErrorCode code) shadows
the record/component field error with a local variable named error; rename the
local variable (for example to errorName or errorLabel) and update its usages in
createErrorUri so new StringBuilder(...) and the "?error=" append reference the
renamed local variable, keeping the conditional code appending code.getCode()
unchanged; this eliminates the shadowing while preserving behavior.
In
`@src/test/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilterUnitTest.java`:
- Around line 148-173: Add a complementary unit test (e.g.,
oauth2_profileParam_explicit_preserved) that mirrors
oauth2_profileParam_default_wrapping but sets request.setParameter("profile",
"local") before calling filter.doFilter(request, response, filterChain); then
capture the wrapped request via ArgumentCaptor<HttpServletRequest> from
verify(filterChain).doFilter(...), and assert that
wrappedRequest.getParameter("profile") equals "local" and
response.getRedirectedUrl() is null to ensure explicit profile values are
preserved by the wrapping logic.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java (1)
49-50: Base64 URL 인코더에withoutPadding()추가 권장
Base64.getUrlEncoder()는 기본적으로=패딩을 포함합니다. OAuth2 state 파라미터는 URL query string을 통해 전달되며, RFC 4648에 따르면 base64url 인코딩 시 패딩은 생략 가능합니다.OAuth2StateParser의Base64.getUrlDecoder()는 패딩 없는 문자열을 자동으로 처리하므로 호환성에 문제없습니다. 일부 프록시나 프레임워크에서 패딩 문자가 예기치 않게 처리될 가능성을 제거하기 위해withoutPadding()을 사용하는 것이 권장됩니다.♻️ 패딩 제거 제안
- String encodedUser = Base64.getUrlEncoder().encodeToString(user.getBytes(StandardCharsets.UTF_8)); - String encodedProfile = Base64.getUrlEncoder().encodeToString(profile.getBytes(StandardCharsets.UTF_8)); + String encodedUser = Base64.getUrlEncoder().withoutPadding().encodeToString(user.getBytes(StandardCharsets.UTF_8)); + String encodedProfile = Base64.getUrlEncoder().withoutPadding().encodeToString(profile.getBytes(StandardCharsets.UTF_8));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java` around lines 49 - 50, The Base64 URL encoder usage in CustomOAuth2AuthorizationRequestResolver currently uses Base64.getUrlEncoder() when creating encodedUser and encodedProfile which includes '=' padding; update these to use Base64.getUrlEncoder().withoutPadding() so the OAuth2 state parameter is base64url encoded without padding (use the same change for both encodedUser and encodedProfile), ensuring compatibility with OAuth2 state parsing and avoiding proxy/framework issues with '=' characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java`:
- Around line 49-50: The Base64 URL encoder usage in
CustomOAuth2AuthorizationRequestResolver currently uses Base64.getUrlEncoder()
when creating encodedUser and encodedProfile which includes '=' padding; update
these to use Base64.getUrlEncoder().withoutPadding() so the OAuth2 state
parameter is base64url encoded without padding (use the same change for both
encodedUser and encodedProfile), ensuring compatibility with OAuth2 state
parsing and avoiding proxy/framework issues with '=' characters.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java (2)
44-46:profile이null인 경우 state 인코딩을 건너뛰는 동작이OAuth2ClientValidationFilter와 불일치할 수 있습니다.
OAuth2ClientValidationFilter.wrapRequestDefaultParams에서profile이 null/빈값일 때"prod"로 정규화하고 있습니다. 필터가 이 리졸버보다 먼저 실행된다면, 여기서profile == null은 도달 불가능한 분기가 됩니다. 반대로 필터 없이 이 리졸버가 호출될 경우 state에 파라미터가 누락되어 downstream에서 파싱 실패가 발생할 수 있습니다.필터 순서에 대한 암묵적 의존 대신, 이 메서드 내에서도
OAuth2DTO.defaultProfile(profile)을 적용하여 방어적으로 처리하는 것을 고려해 주세요.♻️ 제안
String user = request.getParameter("user"); String profile = request.getParameter("profile"); - if (user == null || profile == null) return authorizationRequest; + if (user == null) return authorizationRequest; + profile = OAuth2DTO.defaultProfile(profile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java` around lines 44 - 46, The code in CustomOAuth2AuthorizationRequestResolver that returns early when profile is null should defensively normalize profile using OAuth2DTO.defaultProfile(profile) before deciding to skip state encoding; update the logic in the method (the resolver handling getParameter("user") / getParameter("profile") inside CustomOAuth2AuthorizationRequestResolver) to call OAuth2DTO.defaultProfile(profile) and use that normalized value when constructing the state (instead of returning when profile == null), ensuring the state always contains the normalized profile consistent with OAuth2ClientValidationFilter.wrapRequestDefaultParams.
13-13:@Slf4j어노테이션이 사용되지 않습니다.
log필드가 클래스 내에서 한 번도 사용되지 않으므로 제거하는 것이 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java` at line 13, The `@Slf4j` annotation on CustomOAuth2AuthorizationRequestResolver is unused (no log calls in the class); remove the annotation (and its import) to avoid generating an unused log field, or alternatively add meaningful logging using the generated log field where appropriate—preferably remove `@Slf4j` and its import from the class to fix the unused logger.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java`:
- Around line 48-52: Change the state splitting in OAuth2StateParser.getParams
to limit the split to three parts so embedded '|' in the original CSRF state
cannot produce extra array elements; replace the current stateParam.split("\\|")
call with stateParam.split("\\|", 3) (keep the existing parts.length < 3 check
and the same indexing of parts[1] and parts[2]) so user/profile extraction
remains stable even if the CSRF state contains '|' characters.
---
Nitpick comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java`:
- Around line 44-46: The code in CustomOAuth2AuthorizationRequestResolver that
returns early when profile is null should defensively normalize profile using
OAuth2DTO.defaultProfile(profile) before deciding to skip state encoding; update
the logic in the method (the resolver handling getParameter("user") /
getParameter("profile") inside CustomOAuth2AuthorizationRequestResolver) to call
OAuth2DTO.defaultProfile(profile) and use that normalized value when
constructing the state (instead of returning when profile == null), ensuring the
state always contains the normalized profile consistent with
OAuth2ClientValidationFilter.wrapRequestDefaultParams.
- Line 13: The `@Slf4j` annotation on CustomOAuth2AuthorizationRequestResolver is
unused (no log calls in the class); remove the annotation (and its import) to
avoid generating an unused log field, or alternatively add meaningful logging
using the generated log field where appropriate—preferably remove `@Slf4j` and its
import from the class to fix the unused logger.
.../java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java
Outdated
Show resolved
Hide resolved
- 문자열대신 상수로 state 파라미터 추출 - 에러 fallback URL 수정 - State 파라미터 구분자를 | 문자에서 URL-safe 문자인 . 으로 변경
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/com/bbangle/bbangle/config/security/auth/CustomSuccessHandler.java (1)
49-66: Redis 저장 실패 시 예외 처리가 적절합니다.모든 예외를 catch하여
OAuth2Exception으로 래핑하는 방식이 일관된 오류 처리를 보장합니다. 다만, 로깅이 없어서 디버깅 시 원인 파악이 어려울 수 있습니다.🔧 (선택) 예외 발생 시 로깅 추가 제안
} catch (Exception e) { + log.error("Failed to store OAuth2 code in Redis", e); throw new OAuth2Exception(BbangleErrorCode.INTERNAL_SERVER_ERROR, e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/CustomSuccessHandler.java` around lines 49 - 66, The catch block in CustomSuccessHandler.generateCode wraps all exceptions from redisRepository.setFromDTO into an OAuth2Exception but does not log the original error; add a logging statement that records the exception and relevant context (uuid, oAuth2User.id(), OAUTH_CODE_NAMESPACE, TEMP_CODE_TTL) before throwing the OAuth2Exception so the root cause can be diagnosed; use the class logger (e.g., LOGGER or log) to log an error with a clear message and the exception, then rethrow the OAuth2Exception as currently implemented.src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java (1)
93-97: Customer 로그인 미구현 상태입니다.TODO 주석으로 표시된 미구현 기능이 있습니다. 현재 Customer 타입으로 요청 시
_NOT_SUPPORTED_YET예외가 발생합니다.이 기능 구현을 추적하기 위한 이슈를 생성해 드릴까요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java` around lines 93 - 97, The loginCustomer method in OAuth2UserService currently throws OAuth2Exception with BbangleErrorCode._NOT_SUPPORTED_YET and lacks Customer login logic; implement Customer login handling: accept the OAuth2Response, resolve or create a Member for Customer-type responses (using existing Member creation/lookup utilities used by loginSocial/loginPartner), map it to CustomUserDetails and return it, and remove the TODO/exception; if you prefer to postpone, at minimum replace the raw exception with a clear UnsupportedOperationException and create a tracking issue referenced in a comment. Ensure you update method references: loginCustomer, OAuth2Response, CustomUserDetails, and BbangleErrorCode accordingly.src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java (1)
48-52: State 파싱 시.구분자로 인한 잠재적 취약점 - 방어적 코드 개선 권장
OAuth2StateParser.getParams()는split("\\.")로 state를 분할하고parts[1],parts[2]를 사용합니다. 현재 Spring Security의 Base64URL 인코딩된 state에는.이 없어 문제없지만, 원본 state에.이 포함되면parts배열이 3개 이상이 되어 인덱싱 오류가 발생할 수 있습니다.더 안전한 구현을 위해 뒤에서부터 파싱하는 방식을 권장합니다.
♻️ 제안: OAuth2StateParser의 안전한 파싱 방식
public OAuthParams getParams(String stateParam) { if (stateParam == null || stateParam.isBlank()) throw new OAuth2Exception(BbangleErrorCode.INVALID_OAUTH_PARAMS); int lastDot = stateParam.lastIndexOf('.'); if (lastDot == -1) throw new OAuth2Exception(BbangleErrorCode.INVALID_OAUTH_PARAMS); int secondLastDot = stateParam.lastIndexOf('.', lastDot - 1); if (secondLastDot == -1) throw new OAuth2Exception(BbangleErrorCode.INVALID_OAUTH_PARAMS); String user = stateParam.substring(secondLastDot + 1, lastDot); String profile = stateParam.substring(lastDot + 1); return getParams(user, profile); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java` around lines 48 - 52, The state concatenation in CustomOAuth2AuthorizationRequestResolver builds state as authorizationRequest.getState() + "." + user + "." + profile which can break OAuth2StateParser.getParams()’s split("\\.") logic; update OAuth2StateParser.getParams(String) to defensively parse from the end (use lastIndexOf('.') and second-to-last lastIndexOf('.') to extract user and profile), validate null/blank input and presence of two dots, and throw BbangleErrorCode.INVALID_OAUTH_PARAMS on failure so parsing is robust even if the original state contains '.' characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java`:
- Around line 55-64: The method getParams() currently throws
IllegalStateException when RequestAttributes is not a ServletRequestAttributes,
which breaks Spring Security's OAuth2 failure flow; change this to throw an
OAuth2AuthenticationException (constructed with an OAuth2Error describing the
invalid or missing HTTP request context) so the error is handled by Spring
Security. Update the exception throw in getParams() (and any related
null/invalid-state paths around RequestContextHolder/ServletRequestAttributes
and stateParser.getParams) to create and throw an OAuth2AuthenticationException
with a clear OAuth2Error code and message.
---
Nitpick comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java`:
- Around line 48-52: The state concatenation in
CustomOAuth2AuthorizationRequestResolver builds state as
authorizationRequest.getState() + "." + user + "." + profile which can break
OAuth2StateParser.getParams()’s split("\\.") logic; update
OAuth2StateParser.getParams(String) to defensively parse from the end (use
lastIndexOf('.') and second-to-last lastIndexOf('.') to extract user and
profile), validate null/blank input and presence of two dots, and throw
BbangleErrorCode.INVALID_OAUTH_PARAMS on failure so parsing is robust even if
the original state contains '.' characters.
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/CustomSuccessHandler.java`:
- Around line 49-66: The catch block in CustomSuccessHandler.generateCode wraps
all exceptions from redisRepository.setFromDTO into an OAuth2Exception but does
not log the original error; add a logging statement that records the exception
and relevant context (uuid, oAuth2User.id(), OAUTH_CODE_NAMESPACE,
TEMP_CODE_TTL) before throwing the OAuth2Exception so the root cause can be
diagnosed; use the class logger (e.g., LOGGER or log) to log an error with a
clear message and the exception, then rethrow the OAuth2Exception as currently
implemented.
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java`:
- Around line 93-97: The loginCustomer method in OAuth2UserService currently
throws OAuth2Exception with BbangleErrorCode._NOT_SUPPORTED_YET and lacks
Customer login logic; implement Customer login handling: accept the
OAuth2Response, resolve or create a Member for Customer-type responses (using
existing Member creation/lookup utilities used by loginSocial/loginPartner), map
it to CustomUserDetails and return it, and remove the TODO/exception; if you
prefer to postpone, at minimum replace the raw exception with a clear
UnsupportedOperationException and create a tracking issue referenced in a
comment. Ensure you update method references: loginCustomer, OAuth2Response,
CustomUserDetails, and BbangleErrorCode accordingly.
src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
src/main/resources/application-dev.yml (1)
73-76: 신규 환경 변수LOCAL_DOMAIN,SELLER_DOMAIN,CUSTOMER_DOMAIN을.env템플릿 및 배포 문서에 추가하세요세 가지 환경 변수가 새로 도입되었으나,
.env.example또는 배포 런북에 기록되지 않으면 신규 환경 셋업 시 변수 누락으로 인한 바인딩 실패(또는 빈 값으로 인한 잘못된 리다이렉트)가 발생할 수 있습니다.#!/bin/bash # Description: .env 예시 파일에 신규 환경 변수가 문서화되어 있는지 확인 fd -e properties -e env -e example --exec grep -l "DOMAIN" {} fd "\.env" --exec grep -n "DOMAIN" {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/application-dev.yml` around lines 73 - 76, 새로 추가된 환경 변수 LOCAL_DOMAIN, SELLER_DOMAIN, CUSTOMER_DOMAIN이 application-dev.yml의 domain.local / domain.seller / domain.customer에 바인딩되므로 .env 템플릿(.env.example)과 배포 런북(런북 또는 배포 문서)에 동일한 변수명을 문서화하고 기본값/설명(example values 및 용도)을 추가하여 배포 시 누락되지 않도록 업데이트하세요; 즉 .env.example에 LOCAL_DOMAIN=..., SELLER_DOMAIN=..., CUSTOMER_DOMAIN=... 항목과 간단한 설명을 추가하고 배포 문서의 환경 변수 목록 및 설정 절차에 이 변수들을 기재해 주세요.src/test/java/com/bbangle/bbangle/config/security/auth/OAuth2UserServiceUnitTest.java (1)
275-307:UserType.CUSTOMER미지원 경로에 대한 테스트 케이스 미존재PR 설명에 따르면 Customer 로그인은 아직 구현되지 않았으며,
OAuth2UserService에 미지원 타입 처리 로직이 포함될 가능성이 높습니다.ExceptionTest에UserType.CUSTOMER를 파라미터로 전달했을 때 예상된 예외가 발생하는지 검증하는 테스트 케이스 추가를 권장합니다.💡 추가 테스트 케이스 예시
`@Test` `@DisplayName`("Customer 로그인 미지원 시 예외를 던진다.") void fail_login_customer_not_supported() { // given OAuth2User oAuth2User = kakaoOAuth2User(); OAuthParams customerParams = OAuthParams.builder() .user(UserType.CUSTOMER) .profile(ProfileType.PROD) .build(); given(request.getClientRegistration()).willReturn(clientRegistration); given(clientRegistration.getRegistrationId()).willReturn("kakao"); given(stateParser.getParams(DUMMY_STATE)).willReturn(customerParams); doReturn(oAuth2User).when(oAuth2UserService).loadOAuth2User(any()); // when & then assertThatThrownBy(() -> oAuth2UserService.loadUser(request)) .isInstanceOf(OAuth2Exception.class) .satisfies(ex -> { OAuth2Exception oAuth2Ex = (OAuth2Exception) ex; // 프로젝트에서 정의된 미지원 에러 코드로 교체 assertThat(oAuth2Ex.getCode()).isEqualTo(BbangleErrorCode.INVALID_OAUTH_PARAMS); }); verify(oAuth2SellerFacade, never()).login(any()); }Customer 로그인 미지원 케이스에 대한 테스트를 생성해 드릴 수 있습니다. 새 이슈를 열어드릴까요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/bbangle/bbangle/config/security/auth/OAuth2UserServiceUnitTest.java` around lines 275 - 307, Add a new test in the ExceptionTest nested class to verify that UserType.CUSTOMER is rejected: create OAuth2User via kakaoOAuth2User(), build OAuthParams customerParams with UserType.CUSTOMER and ProfileType.PROD, mock request.getClientRegistration() -> clientRegistration and clientRegistration.getRegistrationId() -> "kakao", mock stateParser.getParams(DUMMY_STATE) to return customerParams, stub oAuth2UserService.loadOAuth2User(any()) to return the OAuth2User, then assert that calling oAuth2UserService.loadUser(request) throws OAuth2Exception whose getCode() equals the project’s unsupported param error (e.g. BbangleErrorCode.INVALID_OAUTH_PARAMS), and verify oAuth2SellerFacade.login(...) is never invoked; use the existing helper methods and mocks (loadUser, oAuth2SellerFacade, stateParser, clientRegistration, kakaoOAuth2User) to implement this test.src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java (1)
93-97:loginCustomer에서 사용되지 않는Member member변수Line 94의
Member member선언은 즉시throw로 넘어가므로 사용되지 않습니다. 현재는 TODO 플레이스홀더이므로, 변수 선언을 제거하거나 주석으로 남기는 것이 깔끔합니다.♻️ 정리 제안
private CustomUserDetails loginCustomer(OAuth2Response response) { - Member member; // TODO : 추후에 결정되면 Customer 로그인 로직 추가 throw new OAuth2Exception(BbangleErrorCode._NOT_SUPPORTED_YET); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java` around lines 93 - 97, The local variable Member member declared in the method loginCustomer(OAuth2Response response) is unused because the method immediately throws an OAuth2Exception; remove the unused declaration (or convert it to a commented TODO if you want to keep a placeholder) to clean up the method body and avoid dead code in OAuth2UserService.loginCustomer.src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java (3)
29-40:getErrorUrlWithReferer에서getDefaultErrorUrl호출로 중복 제거 가능Lines 33-34의 fallback URL 생성 로직이
getDefaultErrorUrl(Line 38-40)과 완전히 동일합니다.♻️ 리팩터링 제안
public String getErrorUrlWithReferer(BbangleErrorCode code, String referer) { String domain = domain().getDomain(referer); String uri = redirect().createErrorUri(code); if (domain == null) - return DEFAULT_OAUTH_PAGE + "?error=" + code.toString() + "&code=" + code.getCode(); + return getDefaultErrorUrl(code); return domain + uri; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java` around lines 29 - 40, The fallback construction in getErrorUrlWithReferer duplicates the logic in getDefaultErrorUrl; change the null-domain branch in getErrorUrlWithReferer to call and return getDefaultErrorUrl(code) instead of rebuilding the URL string, leaving the existing domain + uri return path intact and keeping usage of domain() and redirect().createErrorUri(code).
50-59:createErrorUri에서 로컬 변수error가 레코드 필드error를 섀도잉합니다Line 51의
new StringBuilder(error)는 레코드 필드error(리다이렉트 경로)를 참조하고, Line 53의String error는 에러 코드 문자열을 담는 로컬 변수입니다. 동작은 정확하지만, 동일한 이름이 서로 다른 의미로 사용되어 유지보수 시 혼동을 줄 수 있습니다.♻️ 변수명 변경 제안
private String createErrorUri(BbangleErrorCode code) { StringBuilder url = new StringBuilder(error); - String error = code != null ? code.toString() : "UNKNOWN_ERROR"; - url.append("?error=").append(error); + String errorName = code != null ? code.toString() : "UNKNOWN_ERROR"; + url.append("?error=").append(errorName); if (code != null) url.append("&code=").append(code.getCode()); return url.toString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java` around lines 50 - 59, The method createErrorUri(BbangleErrorCode code) shadows the record field error with a local String error variable; rename the local variable (e.g., errorName or errorString) to avoid shadowing and clarify intent, update the StringBuilder initialization to still use the record field (this.error or the field name) for the base URL and append the renamed local variable when adding the ?error= query, and ensure the conditional append of &code uses code.getCode() unchanged; adjust references in createErrorUri accordingly.
77-83:getDomain(String referer)—contains()기반 매칭의 실제 문제점HTTP Referer 헤더(전체 URL 형태)에 대해
contains()를 사용하므로, 설정된 도메인이 다른 도메인의 부분 문자열인 경우 오매칭이 발생합니다. 예를 들어customer = "bbangle.com",seller = "seller.bbangle.com"일 때 seller의 referer(https://seller.bbangle.com/path)는 customer로 잘못 매칭됩니다 (line 80에서 "seller.bbangle.com"이 "bbangle.com"을 포함하므로).더 안전한 매칭 방식으로 개선하세요:
- 더 구체적인 도메인을 먼저 검사 (seller → customer → local 순서)
startsWith()사용으로 URL 스킴 이후 도메인만 정확히 비교- 또는 URL 파싱을 통한 도메인 추출 (java.net.URL 활용)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java` around lines 77 - 83, getDomain(String referer) wrongly uses contains() order causing substring mismatches (e.g., "bbangle.com" matching before "seller.bbangle.com"); update getDomain to extract the host from the referer (use java.net.URL to parse and call getHost()), then match against the configured domains using exact host comparisons and/or startsWith on the host, and ensure more specific domains (seller) are checked before less specific ones (customer, local) when performing the comparisons; reference the getDomain method and the fields local, customer, seller when making the change.src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java (1)
113-115:"user"리터럴을 상수로 추출 권장
PARAM_PROFILE상수를 사용하는 것과 일관성을 위해"user"문자열 리터럴도 클래스 레벨 상수로 추출하는 것이 좋습니다.♻️ 제안 리팩터
private static final String PARAM_PROFILE = "profile"; +private static final String PARAM_USER = "user";- String user = request.getParameter("user"); + String user = request.getParameter(PARAM_USER);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java` around lines 113 - 115, Extract the "user" string literal into a class-level constant (e.g., PARAM_USER) in OAuth2ClientValidationFilter and replace request.getParameter("user") with request.getParameter(PARAM_USER); ensure the new constant follows the existing naming/visibility pattern used by PARAM_PROFILE and update any other occurrences of the "user" literal in this class to use PARAM_USER for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/CustomSuccessHandler.java`:
- Around line 42-47: Wrap the call to stateParser.getParams(...) in
CustomSuccessHandler inside a try-catch that catches OAuth2Exception; on catch,
invoke the same error handling used by CustomFailureHandler (e.g., call
handleStateParsingFailure(request, response) or perform the equivalent redirect
to the default error page) and return early so generateCode(...) and
createRedirectUrl(...) are not executed with a bad dto; keep existing flow
(generateCode, createRedirectUrl, sendRedirect) in the try path.
In `@src/main/resources/application-production.yml`:
- Around line 73-76: Missing environment variable fallbacks for LOCAL_DOMAIN,
SELLER_DOMAIN, and CUSTOMER_DOMAIN will cause startup failure if any are absent;
add sensible default fallbacks (e.g., empty string or local defaults) in
application-production.yml by changing the values to
${LOCAL_DOMAIN:defaultValue}, ${SELLER_DOMAIN:defaultValue},
${CUSTOMER_DOMAIN:defaultValue} (use appropriate defaults for each environment),
and mirror the same fallback strategy in application-dev.yml and
application-local.yml; also update .env.example, deployment/infrastructure
manifests (IaC), and any onboarding/developer docs to declare these three
variables so all environments provide values.
- Line 74: 프로덕션 환경에서 LOCAL_DOMAIN이 실제로 사용되지 않아 필수 환경변수로 비어 있으면 컨텍스트가 실패하는 문제를
고치려면 application-production.yml의 local 항목을 제거하거나 기본값 빈 문자열을 추가해 프로퍼티를 optional로
만드세요; 구체적으로 OAuth2DTO.defaultProfile(...)와 ProfileType.LOCAL,
OAuth2HandlerProperties.getDomain(...) 호출 경로를 참고해 production 프로파일에서
LOCAL_DOMAIN, SELLER_DOMAIN, CUSTOMER_DOMAIN이 없어도 동작하도록
application-production.yml에서 "local: ${LOCAL_DOMAIN:}" (또는 동일하게 seller/customer
항목에 ":")로 변경하거나 아예 local 항목을 삭제하고 필요시 문서화해 두세요.
In
`@src/test/java/com/bbangle/bbangle/config/security/auth/OAuth2UserServiceUnitTest.java`:
- Around line 279-295: The test method fail_login_seller_BbangleException in
OAuth2UserServiceUnitTest is misnamed because it currently throws a
RuntimeException from oAuth2SellerFacade.login(...) while the name implies a
BbangleException; either rename the test to reflect a generic unexpected
exception (e.g., fail_login_seller_unexpected_exception or
fail_login_seller_exception_wrapping) or change the stub to throw new
BbangleException(BbangleErrorCode.INTERNAL_SERVER_ERROR) so the test intent
matches the behavior of OAuth2UserService.loginSeller() and the mocked call to
oAuth2SellerFacade.login(...).
---
Duplicate comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java`:
- Around line 37-53:
CustomOAuth2AuthorizationRequestResolver.saveParamsAndReturn appends user and
profile using "." as a delimiter which can break parsing if
authorizationRequest.getState() contains dots; update
OAuth2StateParser.getParams to parse the combined state using split("\\.", 3)
(i.e. limit=3) so the original state (parts[0]) may contain dots while parts[1]
and parts[2] reliably map to user and profile; ensure
OAuth2StateParser.getParams uses this split and handles missing parts
gracefully.
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java`:
- Around line 128-131: The current createRedirectUrl should defensively handle a
null or full-URL Referer: check request.getHeader("Referer") for null and, if
non-null, normalize it to a domain (or the expected fragment) before calling
oauth2HandlerProperties.getErrorUrlWithReferer; either extract the host via
URL-parsing or call the existing getDomain helper and pass that normalized
value, or alternatively update getErrorUrlWithReferer to explicitly accept and
safely handle null and full URLs. Ensure createRedirectUrl uses
createRedirectUrl, getErrorUrlWithReferer, and getDomain (or URL parsing)
consistently so null referer and full "https://..." inputs are handled
predictably.
In
`@src/test/java/com/bbangle/bbangle/config/security/auth/OAuth2UserServiceUnitTest.java`:
- Around line 256-258: The test fail_login_name_givenName_null previously risked
an NPE; ensure the stateParser.getParams(DUMMY_STATE) stub is present (as added)
returning sellerParams() in that test, and apply the same fix to any other tests
that invoke OAuth2UserService with DUMMY_STATE—i.e., stub
stateParser.getParams(DUMMY_STATE) to return a non-null sellerParams() to
prevent NPE in OAuth2UserServiceUnitTest.
---
Nitpick comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2ClientValidationFilter.java`:
- Around line 113-115: Extract the "user" string literal into a class-level
constant (e.g., PARAM_USER) in OAuth2ClientValidationFilter and replace
request.getParameter("user") with request.getParameter(PARAM_USER); ensure the
new constant follows the existing naming/visibility pattern used by
PARAM_PROFILE and update any other occurrences of the "user" literal in this
class to use PARAM_USER for consistency.
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java`:
- Around line 29-40: The fallback construction in getErrorUrlWithReferer
duplicates the logic in getDefaultErrorUrl; change the null-domain branch in
getErrorUrlWithReferer to call and return getDefaultErrorUrl(code) instead of
rebuilding the URL string, leaving the existing domain + uri return path intact
and keeping usage of domain() and redirect().createErrorUri(code).
- Around line 50-59: The method createErrorUri(BbangleErrorCode code) shadows
the record field error with a local String error variable; rename the local
variable (e.g., errorName or errorString) to avoid shadowing and clarify intent,
update the StringBuilder initialization to still use the record field
(this.error or the field name) for the base URL and append the renamed local
variable when adding the ?error= query, and ensure the conditional append of
&code uses code.getCode() unchanged; adjust references in createErrorUri
accordingly.
- Around line 77-83: getDomain(String referer) wrongly uses contains() order
causing substring mismatches (e.g., "bbangle.com" matching before
"seller.bbangle.com"); update getDomain to extract the host from the referer
(use java.net.URL to parse and call getHost()), then match against the
configured domains using exact host comparisons and/or startsWith on the host,
and ensure more specific domains (seller) are checked before less specific ones
(customer, local) when performing the comparisons; reference the getDomain
method and the fields local, customer, seller when making the change.
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java`:
- Around line 93-97: The local variable Member member declared in the method
loginCustomer(OAuth2Response response) is unused because the method immediately
throws an OAuth2Exception; remove the unused declaration (or convert it to a
commented TODO if you want to keep a placeholder) to clean up the method body
and avoid dead code in OAuth2UserService.loginCustomer.
In `@src/main/resources/application-dev.yml`:
- Around line 73-76: 새로 추가된 환경 변수 LOCAL_DOMAIN, SELLER_DOMAIN, CUSTOMER_DOMAIN이
application-dev.yml의 domain.local / domain.seller / domain.customer에 바인딩되므로 .env
템플릿(.env.example)과 배포 런북(런북 또는 배포 문서)에 동일한 변수명을 문서화하고 기본값/설명(example values 및
용도)을 추가하여 배포 시 누락되지 않도록 업데이트하세요; 즉 .env.example에 LOCAL_DOMAIN=...,
SELLER_DOMAIN=..., CUSTOMER_DOMAIN=... 항목과 간단한 설명을 추가하고 배포 문서의 환경 변수 목록 및 설정 절차에
이 변수들을 기재해 주세요.
In
`@src/test/java/com/bbangle/bbangle/config/security/auth/OAuth2UserServiceUnitTest.java`:
- Around line 275-307: Add a new test in the ExceptionTest nested class to
verify that UserType.CUSTOMER is rejected: create OAuth2User via
kakaoOAuth2User(), build OAuthParams customerParams with UserType.CUSTOMER and
ProfileType.PROD, mock request.getClientRegistration() -> clientRegistration and
clientRegistration.getRegistrationId() -> "kakao", mock
stateParser.getParams(DUMMY_STATE) to return customerParams, stub
oAuth2UserService.loadOAuth2User(any()) to return the OAuth2User, then assert
that calling oAuth2UserService.loadUser(request) throws OAuth2Exception whose
getCode() equals the project’s unsupported param error (e.g.
BbangleErrorCode.INVALID_OAUTH_PARAMS), and verify oAuth2SellerFacade.login(...)
is never invoked; use the existing helper methods and mocks (loadUser,
oAuth2SellerFacade, stateParser, clientRegistration, kakaoOAuth2User) to
implement this test.
src/main/java/com/bbangle/bbangle/config/security/auth/CustomSuccessHandler.java
Show resolved
Hide resolved
src/test/java/com/bbangle/bbangle/config/security/auth/OAuth2UserServiceUnitTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/bbangle/bbangle/config/security/auth/CustomSuccessHandler.java (1)
54-71:⚠️ Potential issue | 🟡 Minor
generateCode에서 Redis 실패 시OAuth2Exception이 성공 핸들러 내에서 전파됩니다.성공 핸들러 내에서 발생하는
OAuth2Exception은CustomFailureHandler로 라우팅되지 않고 unhandled 예외로 처리됩니다.onAuthenticationSuccess메서드에서 이 예외도 catch하여 에러 페이지로 리다이렉트하는 것이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/CustomSuccessHandler.java` around lines 54 - 71, The generateCode method can throw OAuth2Exception which bubbles out of the success path; update CustomSuccessHandler.onAuthenticationSuccess to wrap the call(s) to generateCode (and any downstream Redis interactions) in a try-catch that catches OAuth2Exception (and general Exception), logs the error, and performs a safe redirect to your error page or delegates to CustomFailureHandler (e.g., response.sendRedirect("/oauth2/error") or invoking the failure handler) so the exception is handled instead of becoming unhandled in the success handler.
🧹 Nitpick comments (5)
src/main/java/com/bbangle/bbangle/auth/oauth/client/dto/OAuth2DTO.java (1)
9-10:@Slf4j어노테이션이 선언되어 있지만 클래스 내에서 로그를 사용하지 않습니다.불필요한 어노테이션을 제거하면 코드가 더 깔끔해집니다.
🧹 제거 제안
-@Slf4j public class OAuth2DTO {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/auth/oauth/client/dto/OAuth2DTO.java` around lines 9 - 10, The class OAuth2DTO is annotated with Lombok's `@Slf4j` but doesn't use the generated log field; remove the unused annotation to clean up the class. Edit the OAuth2DTO declaration to delete `@Slf4j` and also remove the corresponding import (if present) for lombok.extern.slf4j.Slf4j so there are no unused imports or compiler warnings.src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java (1)
86-100:loginSeller의 broad catch가 모든 예외를INTERNAL_SERVER_ERROR로 감싸고 있습니다.
OAuth2SellerFacade.login에서 이미 비즈니스 예외(예:DataIntegrityViolationException)를 처리하고 있으므로, 여기서catch (Exception e)로 모든 예외를 감싸면 디버깅 시 원인 파악이 어려울 수 있습니다. 현재로서는 방어적 코드로 합리적이나, 추후 세분화를 고려해주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java` around lines 86 - 100, The loginSeller method currently catches all Exceptions and wraps them as OAuth2Exception(INTERNAL_SERVER_ERROR); instead, narrow the catch to only unexpected/runtime exceptions and let known business exceptions from oAuth2SellerFacade.login propagate (or rethrow them unchanged). Specifically, in loginSeller, remove the broad catch(Exception e) or replace it with targeted catches for unexpected exceptions (e.g., RuntimeException or a generic PersistenceException) and only wrap those in new OAuth2Exception(BbangleErrorCode.INTERNAL_SERVER_ERROR, e), while allowing domain/business exceptions thrown by OAuth2SellerFacade.login (such as DataIntegrityViolationException or custom service exceptions) to bubble up so their original types/message are preserved.src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java (2)
20-23: TODO: OAuth 로그인 URL이SellerApiPath.PREFIX로 하드코딩되어 있습니다.Customer 로그인 지원 시 이 경로를 변경해야 합니다. Customer 로그인 구현 시 이 TODO를 함께 해결해야 하므로 이슈로 추적하는 것을 권장합니다.
이 TODO를 추적할 이슈를 생성해 드릴까요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java` around lines 20 - 23, The authorization URL is hardcoded to SellerApiPath.PREFIX when creating DefaultOAuth2AuthorizationRequestResolver in CustomOAuth2AuthorizationRequestResolver; make the base authorization path configurable by injecting a dedicated config/property or constructor parameter (e.g., authorizationBasePath) instead of using SellerApiPath.PREFIX, pass that value into the DefaultOAuth2AuthorizationRequestResolver initializer, and add a short TODO/issue reference to track switching to the Customer login path later; update the constructor signature of CustomOAuth2AuthorizationRequestResolver and any call sites to supply the new parameter so the resolver can support both seller and customer login flows.
46-54:user파라미터가 검증 없이 state에 직접 결합됩니다.
user값에 구분자.가 포함된 경우(예:user=SELLER.LOCAL), state 파싱 시 인덱스가 어긋나 profile 값이 조작될 수 있습니다. 현재OAuth2ClientValidationFilter의 enum 검증으로 차단되지만, 방어적 프로그래밍 관점에서 resolver에서도 검증하는 것이 안전합니다.🛡️ user 값 검증 추가 제안
// 1. OAuth2 파라미터 값 추출 String user = request.getParameter("user"); String profile = request.getParameter("profile"); if (user == null) return authorizationRequest; + if (user.contains(".")) return authorizationRequest; // 2. profile 파라미터 값 2차 검증 profile = OAuth2DTO.defaultProfile(profile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java` around lines 46 - 54, The code currently appends the raw request parameter user into the OAuth2 state in CustomOAuth2AuthorizationRequestResolver, which allows a user value containing the '.' delimiter to break state parsing; validate and sanitize user before concatenation by rejecting or normalizing values that contain '.' (or other unsafe chars) and/or by enforcing the same allowed enum/pattern used by OAuth2ClientValidationFilter (or a strict regex like alphanumeric/underscore), e.g., obtain user = request.getParameter("user"); if null fallback/return as before, otherwise validate via the same enum lookup or pattern and only then build the state as authorizationRequest.getState() + "." + safeUser + "." + profile; if validation fails, handle by falling back to a safe default or aborting the request so no unsafe user is embedded in the state.src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java (1)
56-66:error로컬 변수가 record 필드error를 섀도잉합니다.Line 58에서
new StringBuilder(error)는 record 필드(에러 URI 경로)를 사용하고, Line 60에서String error = ...는 동일한 이름의 로컬 변수를 선언합니다. 동작은 정확하지만 가독성을 위해 로컬 변수명을 변경하는 것을 권장합니다.♻️ 변수명 변경 제안
private String createErrorUri(BbangleErrorCode code) { StringBuilder url = new StringBuilder(error); - String error = code != null ? code.toString() : "UNKNOWN_ERROR"; - url.append("?error=").append(error); + String errorName = code != null ? code.toString() : "UNKNOWN_ERROR"; + url.append("?error=").append(errorName); if (code != null) url.append("&code=").append(code.getCode()); return url.toString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java` around lines 56 - 66, The createErrorUri method shadows the record field named error by declaring a local variable String error; rename the local variable (e.g., errorName or errorCodeString) so it no longer hides the record field, update its use in the url.append("?error=") call to use the new local name, and keep the conditional append of code via code.getCode() intact; this change removes the shadowing while preserving behavior in createErrorUri(BbangleErrorCode code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java`:
- Around line 87-93: The getDomain method has reversed comments for the
customer/seller checks and a potential substring-matching bug: because customer
may be a substring of seller (or vice versa), checking customer before seller
can return the wrong domain; update getDomain to check the more specific/longer
domain (seller) before customer, correct the inline comments for each check, and
consider replacing referer.contains(...) with a more robust check (e.g., exact
host comparison or parsing the referer URL) to avoid partial matches — adjust
the checks in getDomain and the comments for local, seller, and customer
accordingly.
---
Outside diff comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/CustomSuccessHandler.java`:
- Around line 54-71: The generateCode method can throw OAuth2Exception which
bubbles out of the success path; update
CustomSuccessHandler.onAuthenticationSuccess to wrap the call(s) to generateCode
(and any downstream Redis interactions) in a try-catch that catches
OAuth2Exception (and general Exception), logs the error, and performs a safe
redirect to your error page or delegates to CustomFailureHandler (e.g.,
response.sendRedirect("/oauth2/error") or invoking the failure handler) so the
exception is handled instead of becoming unhandled in the success handler.
---
Duplicate comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/CustomSuccessHandler.java`:
- Around line 42-51: Wrap the call to stateParser.getParams(...) in
CustomSuccessHandler in a try-catch that catches OAuth2Exception (and optionally
RuntimeException), mirror CustomFailureHandler behavior by invoking the same
handleStateParsingFailure fallback when parsing fails, and return/stop further
processing so you do not call generateCode(...), createRedirectUrl(...), or
getRedirectStrategy().sendRedirect(...) on a bad dto; this ensures state parsing
errors are handled consistently and do not result in a 500 being returned to the
user.
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java`:
- Around line 62-71: Replace the IllegalStateException in getParams with an
OAuth2AuthenticationException so the error flows through Spring Security's
OAuth2 failure handling; specifically, in OAuth2UserService.getParams() (and any
callers like loadUser) throw new OAuth2AuthenticationException(new
OAuth2Error(...)) with a clear error code/message (e.g. "invalid_request" /
"Must be called in HTTP request context") instead of IllegalStateException so
CustomFailureHandler receives and handles the error.
---
Nitpick comments:
In `@src/main/java/com/bbangle/bbangle/auth/oauth/client/dto/OAuth2DTO.java`:
- Around line 9-10: The class OAuth2DTO is annotated with Lombok's `@Slf4j` but
doesn't use the generated log field; remove the unused annotation to clean up
the class. Edit the OAuth2DTO declaration to delete `@Slf4j` and also remove the
corresponding import (if present) for lombok.extern.slf4j.Slf4j so there are no
unused imports or compiler warnings.
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java`:
- Around line 20-23: The authorization URL is hardcoded to SellerApiPath.PREFIX
when creating DefaultOAuth2AuthorizationRequestResolver in
CustomOAuth2AuthorizationRequestResolver; make the base authorization path
configurable by injecting a dedicated config/property or constructor parameter
(e.g., authorizationBasePath) instead of using SellerApiPath.PREFIX, pass that
value into the DefaultOAuth2AuthorizationRequestResolver initializer, and add a
short TODO/issue reference to track switching to the Customer login path later;
update the constructor signature of CustomOAuth2AuthorizationRequestResolver and
any call sites to supply the new parameter so the resolver can support both
seller and customer login flows.
- Around line 46-54: The code currently appends the raw request parameter user
into the OAuth2 state in CustomOAuth2AuthorizationRequestResolver, which allows
a user value containing the '.' delimiter to break state parsing; validate and
sanitize user before concatenation by rejecting or normalizing values that
contain '.' (or other unsafe chars) and/or by enforcing the same allowed
enum/pattern used by OAuth2ClientValidationFilter (or a strict regex like
alphanumeric/underscore), e.g., obtain user = request.getParameter("user"); if
null fallback/return as before, otherwise validate via the same enum lookup or
pattern and only then build the state as authorizationRequest.getState() + "." +
safeUser + "." + profile; if validation fails, handle by falling back to a safe
default or aborting the request so no unsafe user is embedded in the state.
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java`:
- Around line 56-66: The createErrorUri method shadows the record field named
error by declaring a local variable String error; rename the local variable
(e.g., errorName or errorCodeString) so it no longer hides the record field,
update its use in the url.append("?error=") call to use the new local name, and
keep the conditional append of code via code.getCode() intact; this change
removes the shadowing while preserving behavior in
createErrorUri(BbangleErrorCode code).
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2UserService.java`:
- Around line 86-100: The loginSeller method currently catches all Exceptions
and wraps them as OAuth2Exception(INTERNAL_SERVER_ERROR); instead, narrow the
catch to only unexpected/runtime exceptions and let known business exceptions
from oAuth2SellerFacade.login propagate (or rethrow them unchanged).
Specifically, in loginSeller, remove the broad catch(Exception e) or replace it
with targeted catches for unexpected exceptions (e.g., RuntimeException or a
generic PersistenceException) and only wrap those in new
OAuth2Exception(BbangleErrorCode.INTERNAL_SERVER_ERROR, e), while allowing
domain/business exceptions thrown by OAuth2SellerFacade.login (such as
DataIntegrityViolationException or custom service exceptions) to bubble up so
their original types/message are preserved.
src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java (1)
56-66:createErrorUri내 지역 변수error가 레코드 컴포넌트error를 섀도잉Line 58에서 레코드의
error필드(리다이렉트 경로 URL)를 사용한 뒤, Line 60에서 같은 이름의 지역 변수error(에러 코드명 문자열)를 선언합니다. 기능적으로는 올바르지만, 유지보수 시 혼란을 줄 수 있습니다. 지역 변수명을errorCode또는errorName등으로 변경하는 것을 권장합니다.또한 동일한 맥락에서
getSuccessUrl(Line 19)과getErrorUrl(Line 26)의 지역 변수domain도 레코드 컴포넌트domain()과 동일한 이름을 사용하고 있어,resolvedDomain등의 명확한 이름으로 변경하는 것이 가독성에 도움이 됩니다.♻️ 리팩터링 제안
- String domain = domain().getDomain(params.profile(), params.user()); + String resolvedDomain = domain().getDomain(params.profile(), params.user()); String uri = redirect().createSuccessUri(uuid); - return domain + uri; + return resolvedDomain + uri;private String createErrorUri(BbangleErrorCode code) { StringBuilder url = new StringBuilder(error); - String error = code != null ? code.toString() : "UNKNOWN_ERROR"; - url.append("?error=").append(error); + String errorCode = code != null ? code.toString() : "UNKNOWN_ERROR"; + url.append("?error=").append(errorCode); if (code != null) url.append("&code=").append(code.getCode()); return url.toString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java` around lines 56 - 66, The local variable in createErrorUri shadows the record component error; rename the local String error to a clearer name (e.g., errorCode or errorName) and update its use in url.append calls to avoid shadowing; likewise, in getSuccessUrl and getErrorUrl rename local domain variables to resolvedDomain (or similar) to avoid shadowing the record component domain(), update all references accordingly, and run a quick compile to ensure no remaining shadowing or name conflicts in OAuth2HandlerProperties and its methods.src/main/java/com/bbangle/bbangle/auth/seller/controller/SellerOauthController.java (1)
45-49: 구현체의userType,profileType파라미터에@RequestParam어노테이션 명시 권장
userType과profileType파라미터에@RequestParam어노테이션이 없고, 인터페이스인SellerOauthApi에 선언된 어노테이션에 의존하고 있습니다. Spring 5.1부터 MVC 컨트롤러가 인터페이스를 구현하면, 인터페이스에 정의된 웹 요청 관련 기능을 포함한 모든 것을 상속합니다. 따라서 런타임 동작에는 문제가 없습니다.다만 이 엔드포인트는 Spring Security의
OAuth2AuthorizationRequestRedirectFilter가 먼저 가로채므로 컨트롤러 메서드는 실제로 호출되지 않으며, 파라미터는 순수하게 Swagger 문서화 목적으로만 존재합니다. 코드 가독성을 위해 구현체에도 어노테이션을 명시적으로 추가하는 것을 권장합니다.♻️ 어노테이션 명시 제안
+import org.springframework.web.bind.annotation.RequestParam; ... `@Override` `@GetMapping`("/authorization/{oauthServerType}") public void sellerLogin( `@PathVariable` OauthServerType oauthServerType, - UserType userType, - ProfileType profileType + `@RequestParam`("user") UserType userType, + `@RequestParam`(value = "profile", required = false, defaultValue = "prod") ProfileType profileType ) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/auth/seller/controller/SellerOauthController.java` around lines 45 - 49, The sellerLogin method in SellerOauthController is missing explicit `@RequestParam` annotations on the UserType and ProfileType parameters (they currently rely on SellerOauthApi's interface annotations); add `@RequestParam` to the userType and profileType parameters on the sellerLogin implementation so the parameters are explicitly documented and visible in the controller (keep the existing OauthServerType `@PathVariable` as-is) to improve readability and Swagger docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/bbangle/bbangle/auth/seller/controller/swagger/SellerOauthApi.java`:
- Around line 79-80: The error code list shows a leading underscore only on
_NOT_SUPPORTED_YET which may be inconsistent; inspect the BbangleErrorCode enum
(reference BbangleErrorCode._NOT_SUPPORTED_YET and its toString()) to confirm
whether toString() returns the leading underscore string, then either (a) remove
the underscore from the enum constant/name or override toString()/getCode() so
it returns "NOT_SUPPORTED_YET" to match other codes, or (b) if the underscore is
intentionally part of the public code, add a clear comment in SellerOauthApi
(near the error table) explaining the intentional underscore for client
consumers and documenting the expected string returned by
BbangleErrorCode._NOT_SUPPORTED_YET.toString(). Ensure the chosen change is
applied consistently across enum usage and API docs.
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java`:
- Around line 41-44: getDefaultErrorUrl may NPE when the BbangleErrorCode
parameter is null; add a null check similar to createErrorUri so you never call
code.toString() or code.getCode() on null. Inside getDefaultErrorUrl, compute
safeErrorName and safeErrorCode (e.g., fallback to a constant like
"UNKNOWN_ERROR" and 0 or empty string) when code is null, then build the URL
using DEFAULT_OAUTH_PAGE + "?error=" + safeErrorName + "&code=" + safeErrorCode;
ensure the method signature and return type remain unchanged and reference
BbangleErrorCode, getDefaultErrorUrl and DEFAULT_OAUTH_PAGE.
---
Duplicate comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java`:
- Around line 87-93: The getDomain(String referer) method can return the wrong
domain when one domain string (customer) is a substring of another (seller); fix
by testing more specific/longer domain values before shorter ones—adjust the
contains checks (e.g., check seller before customer or sort by length) so that
seller is evaluated prior to customer, keeping the existing local check and null
handling intact; update the condition order inside getDomain to ensure the
longest/more specific domain match wins (use the seller and customer variables
as referenced).
---
Nitpick comments:
In
`@src/main/java/com/bbangle/bbangle/auth/seller/controller/SellerOauthController.java`:
- Around line 45-49: The sellerLogin method in SellerOauthController is missing
explicit `@RequestParam` annotations on the UserType and ProfileType parameters
(they currently rely on SellerOauthApi's interface annotations); add
`@RequestParam` to the userType and profileType parameters on the sellerLogin
implementation so the parameters are explicitly documented and visible in the
controller (keep the existing OauthServerType `@PathVariable` as-is) to improve
readability and Swagger docs.
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java`:
- Around line 56-66: The local variable in createErrorUri shadows the record
component error; rename the local String error to a clearer name (e.g.,
errorCode or errorName) and update its use in url.append calls to avoid
shadowing; likewise, in getSuccessUrl and getErrorUrl rename local domain
variables to resolvedDomain (or similar) to avoid shadowing the record component
domain(), update all references accordingly, and run a quick compile to ensure
no remaining shadowing or name conflicts in OAuth2HandlerProperties and its
methods.
src/main/java/com/bbangle/bbangle/auth/seller/controller/swagger/SellerOauthApi.java
Show resolved
Hide resolved
src/main/java/com/bbangle/bbangle/config/security/auth/OAuth2HandlerProperties.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java (3)
21-22: TODO 미해결 — OAuth 로그인 URL 변경 작업 추적 필요현재 authorization base URI가
SellerApiPath.PREFIX + "/oauth2/authorization"(즉/api/v1/seller/oauth2/authorization)로 고정되어 있으며, TODO 주석이 이 URL을 변경해야 한다고 명시하고 있습니다. PR이 병합된 후에도 이 작업이 백로그에 남아 있을 가능성이 있습니다.별도 이슈로 추적을 원하신다면 이슈 생성을 도와드릴 수 있습니다. 필요하신가요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java` around lines 21 - 22, The authorization base URI is hardcoded to SellerApiPath.PREFIX + "/oauth2/authorization" in CustomOAuth2AuthorizationRequestResolver; remove the TODO by making the base URI configurable (e.g., via an injected property or constructor parameter) and default to the current value, update the constructor call that passes clientRegistrationRepository to accept the configurable baseUri, and replace the literal SellerApiPath.PREFIX + "/oauth2/authorization" with the new configurable value so the OAuth login URL can be changed without code edits.
6-6:@Slf4j선언만 되어 있고 실제 사용 없음 — null-user 조기 반환에 로그 누락
log필드가 한 곳도 사용되지 않습니다. 특히 Line 48에서user == null로 조기 반환될 때 아무런 흔적이 남지 않습니다.OAuth2ClientValidationFilter가 먼저 user를 검증하지만, 필터가 우회되거나 resolver가 프로그래밍 방식으로 호출될 경우 파라미터 없는 원본 state가 그대로 전달되고 이후OAuth2StateParser에서INVALID_OAUTH_PARAMS예외가 발생하는데, 어느 요청이 원인인지 추적할 수단이 없습니다.
@Slf4j를 제거하거나, 유지한다면 Line 48에 warn 로그를 추가하세요.♻️ Proposed refactor
if (user == null) { + log.warn("[OAuth2Resolver] 'user' parameter is missing in request URI: {}; returning unmodified authorizationRequest", request.getRequestURI()); return authorizationRequest; }Also applies to: 12-12, 43-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java` at line 6, The `@Slf4j` annotation is present but unused; when user == null in CustomOAuth2AuthorizationRequestResolver (the early return around the user-null check), add a log.warn that records the request context (e.g., incoming request id/state/parameters or principal info) so you can trace which request caused the early return; specifically, inside the method that checks for user == null (in CustomOAuth2AuthorizationRequestResolver) log a warning with enough identifying details before returning, or if you prefer not to log, remove the `@Slf4j` annotation to avoid dead code.
54-54:user파라미터가 정규화 없이 state에 삽입됨Line 51에서
profile은OAuth2DTO.defaultProfile()을 통해 소문자로 정규화되지만,user는 요청에서 읽은 원본 값 그대로 삽입됩니다 (예:"customer","CUSTOMER","Customer"모두 가능).OAuth2StateParser.getParams가 파싱 시.toUpperCase()를 적용하므로 동작 오류는 없지만, state 값이 호출자에 따라 달라져 일관성이 없고 디버깅 시 혼란을 줄 수 있습니다.
profile과 동일하게user도 삽입 전 정규화하는 것을 권장합니다.♻️ Proposed refactor
- String state = authorizationRequest.getState() + "." + user + "." + profile; + String normalizedUser = user.toLowerCase(); + String state = authorizationRequest.getState() + "." + normalizedUser + "." + profile;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java` at line 54, The state string appends the raw request 'user' value without normalization causing inconsistent state variants; in CustomOAuth2AuthorizationRequestResolver normalize the 'user' the same way as 'profile' (e.g., via OAuth2DTO.defaultProfile(user) or the existing normalization utility) before composing String state = authorizationRequest.getState() + "." + user + "." + profile so state values are consistent and compatible with OAuth2StateParser.getParams; update the code that reads/sets 'user' and adjust any related unit tests if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/bbangle/bbangle/auth/seller/controller/SellerOauthController.java`:
- Around line 46-49: SellerOauthController의 메서드 파라미터가 SellerOauthApi 인터페이스와 쿼리
파라미터 이름/기본값이 불일치하여 Swagger와 실제 바인딩이 달라집니다; SellerOauthController의 해당 엔드포인트 서명에서
enum 파라미터 userType과 profileType에 대해 각각 `@RequestParam`("user") UserType userType 및
`@RequestParam`(value = "profile", required = false, defaultValue = "prod")
ProfileType profileType 어노테이션을 추가해 쿼리 이름과 기본값을 인터페이스와 일치시키고 OauthServerType
파라미터(OauthServerType oauthServerType)는 그대로 유지하세요.
---
Nitpick comments:
In
`@src/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java`:
- Around line 21-22: The authorization base URI is hardcoded to
SellerApiPath.PREFIX + "/oauth2/authorization" in
CustomOAuth2AuthorizationRequestResolver; remove the TODO by making the base URI
configurable (e.g., via an injected property or constructor parameter) and
default to the current value, update the constructor call that passes
clientRegistrationRepository to accept the configurable baseUri, and replace the
literal SellerApiPath.PREFIX + "/oauth2/authorization" with the new configurable
value so the OAuth login URL can be changed without code edits.
- Line 6: The `@Slf4j` annotation is present but unused; when user == null in
CustomOAuth2AuthorizationRequestResolver (the early return around the user-null
check), add a log.warn that records the request context (e.g., incoming request
id/state/parameters or principal info) so you can trace which request caused the
early return; specifically, inside the method that checks for user == null (in
CustomOAuth2AuthorizationRequestResolver) log a warning with enough identifying
details before returning, or if you prefer not to log, remove the `@Slf4j`
annotation to avoid dead code.
- Line 54: The state string appends the raw request 'user' value without
normalization causing inconsistent state variants; in
CustomOAuth2AuthorizationRequestResolver normalize the 'user' the same way as
'profile' (e.g., via OAuth2DTO.defaultProfile(user) or the existing
normalization utility) before composing String state =
authorizationRequest.getState() + "." + user + "." + profile so state values are
consistent and compatible with OAuth2StateParser.getParams; update the code that
reads/sets 'user' and adjust any related unit tests if needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/bbangle/bbangle/auth/seller/controller/SellerOauthController.javasrc/main/java/com/bbangle/bbangle/auth/seller/controller/swagger/SellerOauthApi.javasrc/main/java/com/bbangle/bbangle/config/security/auth/CustomOAuth2AuthorizationRequestResolver.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/bbangle/bbangle/auth/seller/controller/swagger/SellerOauthApi.java
Comment on lines
+46
to
49
| @PathVariable OauthServerType oauthServerType, | ||
| UserType userType, | ||
| ProfileType profileType | ||
| ) {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the SellerOauthController.java file and check its current state
find . -name "SellerOauthController.java" -type fRepository: eco-dessert-platform/backend
Length of output: 157
🏁 Script executed:
# Check Spring Boot/Framework version in pom.xml or gradle files
find . -name "pom.xml" -o -name "build.gradle" | head -5Repository: eco-dessert-platform/backend
Length of output: 86
🏁 Script executed:
# Also check the project structure
ls -la | head -20Repository: eco-dessert-platform/backend
Length of output: 1084
🏁 Script executed:
# Check Spring version in build.gradle
cat build.gradle | grep -A 5 -B 5 "spring"Repository: eco-dessert-platform/backend
Length of output: 3291
🏁 Script executed:
# Read the SellerOauthController.java file
cat -n ./src/main/java/com/bbangle/bbangle/auth/seller/controller/SellerOauthController.javaRepository: eco-dessert-platform/backend
Length of output: 5370
🏁 Script executed:
# Find the SellerOauthApi interface
find . -name "SellerOauthApi*" -type fRepository: eco-dessert-platform/backend
Length of output: 158
🏁 Script executed:
# Check the content of SellerOauthApi
cat -n ./src/main/java/com/bbangle/bbangle/auth/seller/controller/swagger/SellerOauthApi.javaRepository: eco-dessert-platform/backend
Length of output: 6626
🌐 Web query:
Spring Framework 6 Spring Boot 3 enum request parameter binding without @RequestParam annotation
💡 Result:
In Spring Framework 6 / Spring Boot 3 (Spring MVC), you can bind an enum from the query string without @RequestParam because Spring enables default resolution for “simple value types”: if a controller argument is a simple type (and no other resolver matches), Spring treats it as if it were annotated with @RequestParam, using the method parameter name as the request parameter name. (docs.spring.io)
`@GetMapping`("/orders")
List<OrderDto> list(Status status) { // binds from ?status=NEW
...
}Key points
- Enums are supported by default via Spring’s built-in type conversion (String → Enum), as long as the incoming value matches the enum constant name (typically case-sensitive). (docs.enterprise.spring.io)
- Because the request parameter name is derived from the Java parameter name, this usually requires compiling with
-parameters(otherwise the name may bearg0, and binding won’t match what you expect). (docs.spring.io) - If you need a different query parameter name, optional/default handling, or custom mapping (e.g.,
?status=new), then use@RequestParam(...)and/or a customConverter/Formatter. (docs.spring.io)
Citations:
- 1: https://docs.spring.io/spring-framework/reference/web/webflux/controller/ann-methods/requestparam.html?utm_source=openai
- 2: https://docs.enterprise.spring.io/spring-framework/reference/web/webmvc/mvc-controller/ann-methods/typeconversion.html?utm_source=openai
- 3: https://docs.spring.io/spring-framework/docs/4.1.6.RELEASE_to_4.2.0.RC1/Spring%20Framework%204.1.6.RELEASE/org/springframework/web/method/annotation/RequestParamMethodArgumentResolver.html?utm_source=openai
- 4: https://docs.spring.io/spring-framework/reference/web/webflux/controller/ann-methods/requestparam.html?utm_source=openai
🏁 Script executed:
# Check if there's a compiler configuration for -parameters flag
grep -r "parameters" build.gradleRepository: eco-dessert-platform/backend
Length of output: 54
🏁 Script executed:
# Check the build.gradle for compileJava options
grep -A 10 "compileJava" build.gradleRepository: eco-dessert-platform/backend
Length of output: 54
🏁 Script executed:
# Read the full build.gradle to check for compiler configuration
cat build.gradleRepository: eco-dessert-platform/backend
Length of output: 6612
🏁 Script executed:
# Let's verify the parameter name resolution by checking if there's any custom configuration
# and confirm the expected behavior difference
grep -i "parameters\|compiler" build.gradleRepository: eco-dessert-platform/backend
Length of output: 54
인터페이스 계약 위반: 쿼리 파라미터 이름 불일치
SellerOauthApi 인터페이스에서는 @RequestParam("user")와 @RequestParam(value = "profile", required = false, defaultValue = "prod")로 정의되어 있으나, 구현부인 SellerOauthController에서는 이 어노테이션이 없습니다.
Spring Framework 6은 어노테이션 없는 enum 파라미터를 기본적으로 바인딩할 수 있지만, 메서드 파라미터 이름을 쿼리 파라미터 이름으로 사용합니다. 따라서 현재 구현은 ?userType=...과 ?profileType=...을 기대하지만, 인터페이스와 Swagger 문서는 ?user=...과 ?profile=...을 명시하고 있습니다.
또한 profileType의 기본값(prod) 적용도 작동하지 않습니다. 반드시 다음과 같이 수정하세요:
제안 수정
`@Override`
`@GetMapping`("/authorization/{oauthServerType}")
public void sellerLogin(
`@PathVariable` OauthServerType oauthServerType,
- UserType userType,
- ProfileType profileType
+ `@RequestParam`("user") UserType userType,
+ `@RequestParam`(value = "profile", required = false, defaultValue = "prod") ProfileType profileType
) {}📝 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.
Suggested change
| @PathVariable OauthServerType oauthServerType, | |
| UserType userType, | |
| ProfileType profileType | |
| ) {} | |
| `@PathVariable` OauthServerType oauthServerType, | |
| `@RequestParam`("user") UserType userType, | |
| `@RequestParam`(value = "profile", required = false, defaultValue = "prod") ProfileType profileType | |
| ) {} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/bbangle/bbangle/auth/seller/controller/SellerOauthController.java`
around lines 46 - 49, SellerOauthController의 메서드 파라미터가 SellerOauthApi 인터페이스와 쿼리
파라미터 이름/기본값이 불일치하여 Swagger와 실제 바인딩이 달라집니다; SellerOauthController의 해당 엔드포인트 서명에서
enum 파라미터 userType과 profileType에 대해 각각 `@RequestParam`("user") UserType userType 및
`@RequestParam`(value = "profile", required = false, defaultValue = "prod")
ProfileType profileType 어노테이션을 추가해 쿼리 이름과 기본값을 인터페이스와 일치시키고 OauthServerType
파라미터(OauthServerType oauthServerType)는 그대로 유지하세요.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reviewer
1팀
History
🚀 Major Changes & Explanations
추가된 기능
user파라미터 추가profile파라미터 추가코드 설명
OAuth2ClientValidationFilter
CustomOAuth2AuthorizationRequestResolver
OAuth2DTO
OAuth2UserService
SuccessHandler / FailureHandler
파라미터 설명
📷 Test Image
💡 ETC
State 파라미터에 OAuth2 파라미터를 저장한 이유
FailureHandler에서 예외를 캐치하여 처리하다 예외가 발생할 경우 (파라미터 값 추출 불가) 서버 내의 HTML로 리다이렉트 하도록 했습니다.
Summary by CodeRabbit
릴리스 노트