Conversation
📝 WalkthroughWalkthroughAdds Apple OAuth support: new configuration binding, Apple OAuth client with ES256-signed client secret (uses nimbus-jose-jwt), token exchange and revocation, token/id_token models, bean registration, and an APPLE enum value. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AppleOAuthClient as Apple<br/>OAuth Client
participant AppleAuthServer as Apple<br/>Auth Server
participant JWTProcessor as JWT<br/>Signer/Parser
Client->>AppleOAuthClient: getMemberInfo(code)
AppleOAuthClient->>JWTProcessor: generateClientSecret() (ES256 sign using EC private key)
JWTProcessor-->>AppleOAuthClient: client_secret (JWT)
AppleOAuthClient->>AppleAuthServer: POST /auth/token (code, client_id, client_secret, grant_type)
AppleAuthServer-->>AppleOAuthClient: AppleTokenResponse (id_token, access_token, refresh_token)
AppleOAuthClient->>JWTProcessor: parseIdToken(id_token)
JWTProcessor-->>AppleOAuthClient: claims (sub, email)
AppleOAuthClient-->>Client: AppleMemberInfoResponse (sub, email)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
No actionable comments were generated in the recent review. 🎉 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 |
Summary of ChangesHello @Mindev27, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 애플 OAuth 로그인 기능을 시스템에 통합하여 사용자 인증을 위한 새로운 옵션을 제공합니다. 기존에 구현된 Google, Kakao, Naver와 같은 다른 OAuth 제공자들의 패턴을 따르면서, 사용자 경험을 확장하고 더 많은 로그인 방법을 지원하기 위함입니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Apple OAuth login functionality, adhering to existing patterns for other OAuth providers. However, critical security concerns were identified in the AppleOAuthClient implementation. The code lacks proper JWT signature and claim verification for the Apple id_token, and sensitive credentials (client secrets and tokens) are transmitted as query parameters in POST requests. Addressing these issues is essential to align with OpenID Connect security best practices and prevent potential credential leakage or account impersonation. Additionally, consider replacing magic numbers and strings with constants and improving exception handling for enhanced code clarity and robustness.
...n/mathrank-auth-domain/src/main/java/kr/co/mathrank/domain/auth/client/AppleOAuthClient.java
Show resolved
Hide resolved
...n/mathrank-auth-domain/src/main/java/kr/co/mathrank/domain/auth/client/AppleOAuthClient.java
Show resolved
Hide resolved
...n/mathrank-auth-domain/src/main/java/kr/co/mathrank/domain/auth/client/AppleOAuthClient.java
Show resolved
Hide resolved
...n/mathrank-auth-domain/src/main/java/kr/co/mathrank/domain/auth/client/AppleOAuthClient.java
Show resolved
Hide resolved
...n/mathrank-auth-domain/src/main/java/kr/co/mathrank/domain/auth/client/AppleOAuthClient.java
Show resolved
Hide resolved
...n/mathrank-auth-domain/src/main/java/kr/co/mathrank/domain/auth/client/AppleOAuthClient.java
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@domain/mathrank-auth-domain/build.gradle`:
- Line 7: The dependency declaration for com.nimbusds:nimbus-jose-jwt in
build.gradle is pinned to 9.47 which is vulnerable to CVE-2025-53864; update the
implementation coordinate 'com.nimbusds:nimbus-jose-jwt' in the build.gradle
(where implementation 'com.nimbusds:nimbus-jose-jwt:9.47' appears) to at least
10.0.2 (recommended 10.7+) and re-run dependency resolution/gradle build to
ensure the newer artifact is used and tests pass.
In
`@domain/mathrank-auth-domain/src/main/java/kr/co/mathrank/domain/auth/client/AppleOAuthClient.java`:
- Around line 134-149: The revoke(String) method currently uses retrieve() which
throws on 4xx/5xx so it never returns false; change revoke(String) to either (A)
perform a non-throwing exchange (use WebClient.exchangeToBodilessEntity or
exchangeToMono and inspect the ClientResponse status) and return
response.getStatusCode().is2xxSuccessful(), or (B) change the method signature
to void and let exceptions propagate; apply the chosen approach to the
revoke(...) method in AppleOAuthClient (reference: revoke(String) and
revokeClient.post()), and for getAccessToken(...) catch
RestClientResponseException thrown by WebClient.retrieve() and map it to your
InvalidOAuthLoginException (include response body/status in the exception) so
Apple errors produce the intended domain exception instead of raw
RestClientResponseException.
- Around line 67-80: The current parseIdToken method deserializes the JWT with
SignedJWT.parse but does not verify its signature or validate claims; update
parseIdToken to: parse the token (SignedJWT.parse), fetch Apple's JWKS from
https://appleid.apple.com/auth/keys, select the JWK by the token header's kid,
construct the RSA public key (or use RSAKey.toRSAPublicKey), create an
RSASSAVerifier and call signedJWT.verify(verifier) and throw
InvalidOAuthLoginException if verification fails; after successful verification,
validate claims.getIssuer() equals "https://appleid.apple.com",
claims.getAudience() contains your client_id, claims.getExpirationTime() is in
the future, and (if you use nonce) validate claims.getStringClaim("nonce"); only
then build and return the AppleMemberInfoResponse using claims.getSubject() and
claims.getStringClaim("email").
🧹 Nitpick comments (3)
domain/mathrank-auth-domain/src/main/java/kr/co/mathrank/domain/auth/client/AppleOAuthClient.java (3)
36-42: RestClient instances are inline-initializedfinalfields alongside a Lombok-injectedfinalfield.This works because Lombok's
@RequiredArgsConstructoronly includes uninitialized final fields. However, the twoRestClientfields being bothfinaland inline-initialized alongside the constructor-injectedappleConfigurationcould confuse future maintainers. Consider removingfinalfrom theRestClientfields or adding a brief comment clarifying the initialization semantics.
84-108: Consider caching the loaded private key rather than re-parsing PEM on every request.
generateClientSecret()is called on everygetMemberInfoandrevokeinvocation, and each call re-parses the PEM string and re-creates anECDSASigner. The private key is immutable configuration — parse it once (e.g., in a@PostConstructor lazy-init field) and reuse it.♻️ Sketch: cache the ECPrivateKey
+import jakarta.annotation.PostConstruct; + `@RequiredArgsConstructor` class AppleOAuthClient implements OAuthClientHandler { private final AppleConfiguration appleConfiguration; + private ECPrivateKey cachedPrivateKey; + + `@PostConstruct` + void init() { + this.cachedPrivateKey = loadPrivateKey(appleConfiguration.getPrivateKey()); + } private String generateClientSecret() { try { - final ECPrivateKey ecPrivateKey = loadPrivateKey(appleConfiguration.getPrivateKey()); final Instant now = Instant.now(); // ... unchanged ... - signedJWT.sign(new ECDSASigner(ecPrivateKey)); + signedJWT.sign(new ECDSASigner(cachedPrivateKey));
110-125: Broadcatch (Exception e)swallows all exceptions indiscriminately.
loadPrivateKeycatchesException, which could mask unexpected runtime errors (e.g.,NullPointerExceptionfrom anullconfig value). Narrow the catch to the expected checked exceptions (GeneralSecurityException,IllegalArgumentExceptionfor Base64 decode failures).♻️ Proposed narrower catch
- } catch (Exception e) { + } catch (java.security.GeneralSecurityException | IllegalArgumentException e) { throw new InvalidOAuthLoginException("애플 개인 키 로딩에 실패했습니다."); }
...n/mathrank-auth-domain/src/main/java/kr/co/mathrank/domain/auth/client/AppleOAuthClient.java
Show resolved
Hide resolved
...n/mathrank-auth-domain/src/main/java/kr/co/mathrank/domain/auth/client/AppleOAuthClient.java
Show resolved
Hide resolved
huhdy32
left a comment
huhdy32
left a comment
There was a problem hiding this comment.
이렇게 빨리 업데이트 해주실 줄 몰랐네요! 이렇게 빨리 코드 베아스를 파악하시다니 ㄷㄷ
감사합니다.
리뷰에서 예외와 별도로 로그를 추가해달라 부탁드렸는데요, 이유는,
예외에 담기는 메시지는 api 사용자에게 전달됨으로 보안상 상세한 정보를 담지 않는 반면, 로그는 개발자의 디버깅 과정을 위해 예외의 상세한 콘텍스트를 담아야할 필요가 있기 때문입니다. (물론 credential 한 정보는 빼고요 )
따라서, 예외 처리에 담기는 내용과 별도로 자세한 콘텍스트를 더 담아주시면, 만일에 발생할 예외 상황에 대해 더 자세히 파악할 수 있을것 같습니다.
귀찮게 하는 것 같아 죄송하네요 ㅎㅎ
감사합니다.
...n/mathrank-auth-domain/src/main/java/kr/co/mathrank/domain/auth/client/AppleOAuthClient.java
Show resolved
Hide resolved
| // Apple OAuth의 client_secret은 ES256으로 서명된 JWT이다. | ||
| // https://developer.apple.com/documentation/sign_in_with_apple/generate_and_validate_tokens |
| } catch (ParseException e) { | ||
| throw new InvalidOAuthLoginException("애플 서버로부터 사용할 수 없는 메시지를 받았습니다."); | ||
| } |
There was a problem hiding this comment.
포맷에 맞게 로그만 남겨주시면 감사하겠습니다!!
| } catch (JOSEException e) { | ||
| throw new InvalidOAuthLoginException("애플 client_secret 생성에 실패했습니다."); | ||
| } |
There was a problem hiding this comment.
포맷에 맞게 로그만 남겨주시면 감사하겠습니다!!
| } catch (Exception e) { | ||
| throw new InvalidOAuthLoginException("애플 개인 키 로딩에 실패했습니다."); | ||
| } |
There was a problem hiding this comment.
포맷에 맞게 로그만 남겨주시면 감사하겠습니다!!
2 participants
Summary by CodeRabbit