Conversation
WalkthroughThis update introduces a new encryption utility along with several modifications to improve Apple authentication and user withdrawal handling. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AppleLoginHandler
participant AppleTokenVerifier
participant EncryptUtil
participant AppleUserTokenRepository
Client->>AppleLoginHandler: Login request with Apple token
AppleLoginHandler->>AppleTokenVerifier: getAccessTokenFromAuthCode(authCode, codeVerifier)
AppleTokenVerifier-->>AppleLoginHandler: Return TokenPair (access, refresh)
AppleLoginHandler->>EncryptUtil: encrypt(refreshToken)
EncryptUtil-->>AppleLoginHandler: Return encryptedRefreshToken
AppleLoginHandler->>AppleUserTokenRepository: Save encrypted refresh token
AppleLoginHandler-->>Client: Return successful login response
sequenceDiagram
participant User
participant WithdrawService
participant AppleUserTokenRepository
participant EncryptUtil
participant AppleTokenVerifier
User->>WithdrawService: Initiate withdrawal
WithdrawService->>AppleUserTokenRepository: Fetch AppleUserToken for user
AppleUserTokenRepository-->>WithdrawService: Return encrypted token
WithdrawService->>EncryptUtil: getDecryptedToken(encrypted token)
EncryptUtil-->>WithdrawService: Return decrypted token
WithdrawService->>AppleTokenVerifier: revoke(decrypted token)
AppleTokenVerifier-->>WithdrawService: Acknowledge revocation
WithdrawService->>AppleUserTokenRepository: Delete AppleUserToken record
WithdrawService-->>User: Confirm withdrawal completion
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
src/main/java/org/runimo/runimo/user/repository/AppleUserTokenRepository.java (1)
7-10: Consider adding explicit deleteByUserId method for withdrawal flowThe repository implementation looks good and follows Spring Data JPA conventions. Since this PR focuses on user withdrawal functionality, consider adding an explicit
deleteByUserId(Long userId)method to make the withdrawal flow more readable and intentional in the service layer.void deleteByUserId(Long userId);This would better document the withdrawal use case in the repository interface itself.
src/main/java/org/runimo/runimo/user/domain/AppleUserToken.java (1)
14-15: Include NotNull constraints for required fieldsConsider adding validation constraints to ensure data integrity:
@NoArgsConstructor(access = AccessLevel.PROTECTED) public class AppleUserToken extends BaseEntity { @NotNull @Column(name = "user_id", unique = true) private Long userId; @NotNull @Column(name = "refresh_token") private String refreshToken; // Constructor remains the same }This ensures that both fields are always populated, preventing null values in the database.
src/main/java/org/runimo/runimo/auth/service/EncryptUtil.java (1)
1-44: Add more specific exception handling for encryption operations.The current implementation throws a generic Exception, which doesn't provide enough information for troubleshooting specific encryption/decryption failures.
Consider wrapping specific exceptions with a custom EncryptionException:
public class EncryptionException extends RuntimeException { public EncryptionException(String message, Throwable cause) { super(message, cause); } }Then update the methods to catch and wrap specific exceptions:
-public String encrypt(String plainText) throws Exception { +public String encrypt(String plainText) { + try { // Encryption code + } catch (NoSuchAlgorithmException | NoSuchPaddingException e) { + throw new EncryptionException("Encryption algorithm not available", e); + } catch (InvalidKeyException | InvalidAlgorithmParameterException e) { + throw new EncryptionException("Invalid encryption key or parameters", e); + } catch (IllegalBlockSizeException | BadPaddingException e) { + throw new EncryptionException("Encryption failed", e); + } }🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 22-22: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(CIPHER_TRANS)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA(desede-is-deprecated-java)
[warning] 33-33: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(CIPHER_TRANS)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA(desede-is-deprecated-java)
[warning] 22-22: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(CIPHER_TRANS)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html(use-of-aes-ecb-java)
[warning] 33-33: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(CIPHER_TRANS)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html(use-of-aes-ecb-java)
src/main/java/org/runimo/runimo/auth/service/apple/AppleTokenVerifier.java (1)
124-147: Consider improving error handling in the revoke method.The revoke method throws the same generic RuntimeException for different error cases, which limits troubleshooting capabilities.
Consider implementing more specific exception handling to differentiate between different failure scenarios:
public void revoke(final String appleRefreshToken) { String clientSecret = generateAppleClientSecret(); // JWT 생성 MultiValueMap<String, String> params = new LinkedMultiValueMap<>(); params.add("client_id", clientId); params.add("client_secret", clientSecret); params.add("token", appleRefreshToken); params.add("token_type_hint", "refresh_token"); HttpHeaders headers = new HttpHeaders(); headers.setContentType(MediaType.APPLICATION_FORM_URLENCODED); HttpEntity<MultiValueMap<String, String>> entity = new HttpEntity<>(params, headers); try { ResponseEntity<String> response = restTemplate.postForEntity(REVOKE_URL, entity, String.class); if (!response.getStatusCode().is2xxSuccessful()) { - throw new RuntimeException("Failed to revoke Apple refresh token"); + log.error("Failed to revoke Apple refresh token. Status: {}, Body: {}", + response.getStatusCode(), response.getBody()); + throw new AppleApiException("Failed to revoke Apple refresh token", + response.getStatusCode()); } } catch (RestClientException e) { - log.error("Failed to revoke Apple refresh token", e); - throw new RuntimeException("Failed to revoke Apple refresh token", e); + log.error("Network error while revoking Apple refresh token", e); + throw new AppleApiException("Network error while revoking Apple refresh token", e); + } catch (Exception e) { + log.error("Unexpected error while revoking Apple refresh token", e); + throw new AppleApiException("Unexpected error while revoking Apple refresh token", e); } }src/main/java/org/runimo/runimo/user/service/WithdrawService.java (1)
48-54: Consider narrowing the exception catch blockCatching
Exceptionis broad. For better maintainability, catch a more specific exception type or create a custom exception to handle decryption issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/org/runimo/runimo/auth/service/EncryptUtil.java(1 hunks)src/main/java/org/runimo/runimo/auth/service/apple/AppleLoginHandler.java(3 hunks)src/main/java/org/runimo/runimo/auth/service/apple/AppleTokenVerifier.java(4 hunks)src/main/java/org/runimo/runimo/user/domain/AppleUserToken.java(1 hunks)src/main/java/org/runimo/runimo/user/repository/AppleUserTokenRepository.java(1 hunks)src/main/java/org/runimo/runimo/user/service/WithdrawService.java(2 hunks)src/main/resources/application.yml(1 hunks)src/main/resources/sql/schema.sql(2 hunks)src/test/resources/application.yml(1 hunks)src/test/resources/sql/schema.sql(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/main/java/org/runimo/runimo/user/domain/AppleUserToken.java (1)
src/main/java/org/runimo/runimo/common/GlobalConsts.java (1)
NoArgsConstructor(8-21)
src/main/java/org/runimo/runimo/auth/service/EncryptUtil.java (1)
src/main/java/org/runimo/runimo/auth/service/apple/AppleLoginHandler.java (1)
Component(23-79)
src/main/java/org/runimo/runimo/auth/service/apple/AppleTokenVerifier.java (1)
src/main/java/org/runimo/runimo/auth/exceptions/UserJwtException.java (1)
UserJwtException(6-19)
🪛 ast-grep (0.31.1)
src/main/java/org/runimo/runimo/auth/service/EncryptUtil.java
[warning] 22-22: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(CIPHER_TRANS)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 33-33: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(CIPHER_TRANS)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 22-22: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(CIPHER_TRANS)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 33-33: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(CIPHER_TRANS)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
🔇 Additional comments (18)
src/main/resources/sql/schema.sql (2)
3-3: LGTM - Table drop statement in correct order.The drop statement for the new table is properly placed before other drops to maintain dependency order.
45-54: LGTM - Properly structured table creation.The
apple_user_tokentable is properly defined with appropriate columns, constraints, and foreign key relationships.src/test/resources/sql/schema.sql (2)
3-3: LGTM - Consistent test schema table drop.The test schema's drop statement mirrors the production schema, which is good practice.
68-77: LGTM - Test schema table creation matches production.The table definition in the test schema correctly matches the production schema, ensuring consistent behavior across environments.
src/main/java/org/runimo/runimo/auth/service/apple/AppleTokenVerifier.java (4)
25-25: LGTM - Adding TokenPair import for enhanced token management.Good addition to support returning both access and refresh tokens together.
64-65: LGTM - Well-defined constant for Apple revocation endpoint.Using a constant for the revocation URL improves maintainability.
98-98: LGTM - Updated method signature to return TokenPair.Good change to return both tokens needed for the user withdrawal feature.
117-117: LGTM - Properly creating TokenPair with both tokens.Correctly extracts both id_token and refresh_token from Apple's response.
src/main/java/org/runimo/runimo/user/service/WithdrawService.java (3)
5-11: Imports appear correct and consistentThese newly introduced imports appropriately reference Apple token logic and encryption utilities.
23-25: Good injection of additional dependenciesThe added final fields for
AppleTokenVerifier,AppleUserTokenRepository, andEncryptUtilcohesively integrate Apple token revocation and encryption logic into the withdrawal process.
32-34: Conditional withdrawal for Apple users looks correctChecking the provider before invoking
withdrawAppleUserensures that Apple-specific logic is only applied to Apple users.src/main/java/org/runimo/runimo/auth/service/apple/AppleLoginHandler.java (7)
10-18: Imports correctly align with the new encryption and Apple token repositoryThese imports facilitate storing the encrypted refresh token and managing Apple user tokens.
30-31: Dependency injection for Apple-related services is well-structuredInjecting
AppleUserTokenRepositoryandEncryptUtilin the constructor supports persisting and securing the refresh token.
33-33: Switching from read-only transaction is appropriateSince token data is now saved or updated in this flow, removing
readOnly = trueis a valid change.
35-38: Access token retrieval and decodingObtaining the token pair and decoding the access token using
JWT.decodeis reasonable, with proper exception handling for invalid tokens.
43-44: Saving the Apple refresh token immediately after user lookupThis maintains up-to-date token records in the database for Apple users.
49-60: Conditional registration logic is clearly structuredThrowing an
UnRegisteredUserExceptionand generating a temporal token for new users is a sound approach.
70-78: Refresh token encryption logic is soundReturning a new
AppleUserTokenwith an encrypted refresh token aligns with best practices for sensitive data storage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/org/runimo/runimo/user/service/WithdrawService.java (1)
48-54: Parameter name mismatch with its actual usageThe parameter name
encryptedIdTokensuggests it's an ID token, but it's actually used for a refresh token. This could cause confusion for future developers.-private String getDecryptedToken(String encryptedIdToken) { +private String getDecryptedToken(String encryptedRefreshToken) { try { - return encryptUtil.decrypt(encryptedIdToken); + return encryptUtil.decrypt(encryptedRefreshToken); } catch (Exception e) { - throw new RuntimeException("Failed to decrypt ID token", e); + throw new RuntimeException("Failed to decrypt refresh token", e); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/runimo/runimo/auth/service/apple/AppleLoginHandler.java(3 hunks)src/main/java/org/runimo/runimo/user/service/WithdrawService.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/runimo/runimo/auth/service/apple/AppleLoginHandler.java
🔇 Additional comments (6)
src/main/java/org/runimo/runimo/user/service/WithdrawService.java (6)
5-9: Good organization of importsThe imports are well-organized and properly include all necessary classes for Apple user withdrawal functionality.
23-25: Good dependency injection setupThe new dependencies are properly declared as private final fields, which will be automatically injected through the constructor thanks to the
@RequiredArgsConstructorannotation.
32-34: Good separation of concerns for Apple user withdrawalThe implementation correctly checks for Apple provider and delegates to a specialized method, maintaining clean code organization.
39-46: Well-structured Apple user withdrawal implementationThe method follows a clear sequence: fetch the token, decrypt it, revoke it, and then delete it from the database. This aligns with the PR objectives for secure user withdrawal.
43-44: Correct variable namingGood use of a descriptive variable name that accurately reflects that this is a decrypted refresh token.
27-37: Robust transaction handlingThe
@Transactionalannotation ensures that the entire withdrawal process, including the Apple-specific operations, is executed atomically. This prevents partial withdrawals that could leave the system in an inconsistent state.
ekgns33
force-pushed
the
feat/apple-user-withdraw
branch
from
e7ee464 to
8d2d465
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/org/runimo/runimo/user/service/WithdrawService.java (2)
52-53: Error message doesn't match the token type being decrypted.The error message incorrectly references "ID token" when the method is actually decrypting a refresh token.
- throw new RuntimeException("Failed to decrypt ID token", e); + throw new RuntimeException("Failed to decrypt refresh token", e);
48-54: Consider using a more specific exception type.The method currently throws a generic
RuntimeException. Consider creating a domain-specific exception class for token-related errors to provide better error handling and clarity.- private String getDecryptedToken(String encryptedRefreshToken) { - try { - return encryptUtil.decrypt(encryptedRefreshToken); - } catch (Exception e) { - throw new RuntimeException("Failed to decrypt refresh token", e); - } - } + private String getDecryptedToken(String encryptedRefreshToken) { + try { + return encryptUtil.decrypt(encryptedRefreshToken); + } catch (Exception e) { + throw new TokenDecryptionException("Failed to decrypt refresh token", e); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/runimo/runimo/auth/service/apple/AppleLoginHandler.java(3 hunks)src/main/java/org/runimo/runimo/user/service/WithdrawService.java(2 hunks)
🔇 Additional comments (11)
src/main/java/org/runimo/runimo/user/service/WithdrawService.java (4)
5-7: Appropriate imports added for new functionality.The import statements are correctly added to support the new Apple user withdrawal functionality, including
EncryptUtil,AppleTokenVerifier, and related domain classes.Also applies to: 9-9, 11-11
23-25: Dependencies properly configured with constructor injection.The new dependencies are properly defined as private final fields which will be automatically injected via the
@RequiredArgsConstructorannotation. This follows Spring best practices for dependency injection.
32-34: Well-structured conditional for handling Apple users.The conditional logic for identifying and handling Apple users is cleanly implemented, separating the Apple-specific withdrawal logic into a dedicated method.
39-46: Apple user withdrawal implementation is comprehensive.The
withdrawAppleUsermethod properly handles all necessary steps: retrieving the token, decrypting it, revoking it via Apple, and cleaning up the database record.src/main/java/org/runimo/runimo/auth/service/apple/AppleLoginHandler.java (7)
10-10: Required imports added for token handling functionality.The imports for
EncryptUtil,AppleUserToken, andAppleUserTokenRepositoryare correctly added to support the new functionality.Also applies to: 14-14, 18-18
30-31: Dependencies properly configured with constructor injection.The new dependencies are properly defined as private final fields which will be automatically injected via the
@RequiredArgsConstructorannotation.
33-33: Appropriate transaction annotation modification.The
@Transactionalannotation has been appropriately modified to allow write operations, which is necessary for saving the refresh token.
35-35: TokenPair integration enhances the authentication process.The method now correctly uses
TokenPairto handle both access and refresh tokens, improving the structure and clarity of the authentication process.Also applies to: 43-46
49-60: Refactoring improves code organization.The extraction of user retrieval logic into a separate method improves code organization and readability.
Consider translating the Korean comment to English to maintain consistency with the rest of the codebase:
- // 사용자 정보가 있으면 반환하고 없다면, 회원가입을 위한 토큰을 생성하여 예외를 던진다. + // Returns user information if it exists, otherwise throws an exception with a registration token
62-68: Correct implementation of token storage.The method correctly saves or updates Apple refresh tokens, and the user ID is properly referenced as noted in previous review comments.
Consider translating the Korean comment to English for consistency:
- // 애플에서 발급한 refresh token을 DB에 저장한다. + // Saves the refresh token issued by Apple to the database
70-78: Token encryption is properly implemented.The token encryption is properly implemented with appropriate error handling for encryption failures.
Consider using a more specific exception type instead of a generic RuntimeException:
- throw new RuntimeException("Failed to encrypt refresh token", e); + throw new TokenEncryptionException("Failed to encrypt refresh token", e);
jeeheaG
left a comment
jeeheaG
left a comment
There was a problem hiding this comment.
고생하셨습니다! 애플로그인이 신경쓸 게 많군요..
2 participants
작업내역
revoke해야합니다.ref)
1. generate-validate-token은 #27에서 애플로그인을 구현하면서 적용했습니다.
id_token만을 이용했지만,refresh_token저장 필요에 따라TokenPair을 만들어 서비스로 반환하도록src/main/java/org/runimo/runimo/auth/service/apple/AppleTokenVerifier.java의getAccessTokenFromAuthCode를 변경했습니다.2. revoke를 위해서는
refresh_token이 필요합니다.이는 Runimo에서 배포한 것이 아닌, 애플에서 유저마다 영구적인 값으로 발급됩니다. 민감한 정보이기에 암호화를 하여 데이터베이스에 저장하는 로직을 작성했습니다.
또한 해당 정보는
revoke이후에는 폐기되야 하므로 HARD-DELETE합니다.Summary by CodeRabbit
New Features
Chores