[BE-Feat] 토큰 발급 시 만료기한 추가 응답 #202
Conversation
WalkthroughThe changes update token expiration handling in the authentication flow. In the AuthService, a new configuration-injected Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthService
Client->>AuthService: login(credentials)
activate AuthService
AuthService-->>AuthService: Load 'expired' duration from configuration
AuthService->>AuthService: Calculate expiredAt = now() + expired
AuthService-->>Client: Return OAuthResponse(token, expiredAt)
deactivate AuthService
sequenceDiagram
participant AppConfig
participant JwtProvider
AppConfig->>JwtProvider: Inject property ${jwt.expired}
JwtProvider-->>JwtProvider: Set validityInMs accordingly
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🔭 Outside diff range comments (1)
backend/src/test/java/endolphin/backend/global/security/JwtProviderTest.java (1)
23-43: 🛠️ Refactor suggestionAdd test coverage for token expiration.
While the basic token validation is tested, there's no test verifying that tokens actually expire after the configured validityInMs period (10 seconds).
Consider adding a test like this:
@DisplayName("토큰 만료 테스트") @Test void tokenShouldExpireAfterValidity() throws InterruptedException { // given Long userId = 123L; String email = "[email protected]"; String token = jwtProvider.createToken(userId, email); // when Thread.sleep(11000); // Wait for token to expire (10 seconds + 1 second buffer) // then assertThatThrownBy(() -> jwtProvider.validateToken(token)) .isInstanceOf(RuntimeException.class) .hasMessageContaining("expired JWT token"); }
🧹 Nitpick comments (3)
backend/src/main/java/endolphin/backend/domain/auth/AuthService.java (1)
30-31: Consider documenting the time unit in the property name.The property name
jwt.expiredcould be more explicit about the time unit being milliseconds.Consider renaming to
jwt.expired.millisor adding a comment in the configuration file.backend/src/test/java/endolphin/backend/domain/auth/AuthServiceTest.java (1)
45-48: Consider using a constant for the test expiration time.The hardcoded value
10000could be moved to a constant or loaded from test configuration to maintain consistency.+ private static final long TEST_TOKEN_EXPIRATION_MS = 10000; + @BeforeEach public void setUp() { - ReflectionTestUtils.setField(authService, "expired", 10000); + ReflectionTestUtils.setField(authService, "expired", TEST_TOKEN_EXPIRATION_MS); }backend/src/test/resources/application.yml (1)
47-50: JWT Expiration Parameter ConfigurationA new configuration key
expired: 1000has been added under thejwtsection. Please confirm that:
- Units: The value is intended to be in milliseconds.
- Appropriateness: The value
1000(i.e., 1 second) matches your test and/or production scenarios. In test environments, this might be acceptable; however, ensure it aligns with the application's overall token validity expectations.Optionally, for improved clarity, consider renaming the parameter to something like
expirationMillisto immediately convey its unit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/main/java/endolphin/backend/domain/auth/AuthService.java(3 hunks)backend/src/main/java/endolphin/backend/domain/auth/dto/OAuthResponse.java(1 hunks)backend/src/main/java/endolphin/backend/global/security/JwtProvider.java(1 hunks)backend/src/test/java/endolphin/backend/domain/auth/AuthServiceTest.java(3 hunks)backend/src/test/java/endolphin/backend/global/security/JwtProviderTest.java(2 hunks)backend/src/test/resources/application.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Send PR Notification to Reviewers
🔇 Additional comments (5)
backend/src/test/java/endolphin/backend/global/security/JwtProviderTest.java (2)
8-8: LGTM on ReflectionTestUtils usage!The test setup correctly initializes the validityInMs field using ReflectionTestUtils.
Also applies to: 20-20
20-20:Details
❓ Verification inconclusive
Verify token validity duration in production configuration.
The test uses a 10-second validity period, which is suitable for testing but would be too short for production use.
Let's check the production configuration:
🏁 Script executed:
#!/bin/bash # Search for JWT validity configuration in application properties/yaml files echo "Searching for JWT validity configuration..." fd -e yml -e yaml -e properties | xargs rg -i "validity|jwt|token.*expir"Length of output: 220
Ensure production JWT token validity is correctly configured.
The test sets the token validity to 10 seconds (10000 ms), which is appropriate only for testing. However, the shell script output only found configuration in the test resources (backend/src/test/resources/application.yml) and did not reveal any corresponding setting in the production configuration (typically inbackend/src/main/resources/application.yml). Please verify that a proper, production-appropriate JWT token validity duration is defined in your production configuration. If it’s missing or incorrect, update the configuration to reflect a safe and usable duration for production.backend/src/main/java/endolphin/backend/domain/auth/dto/OAuthResponse.java (1)
3-8: LGTM! Clean implementation of the expiration time field.The addition of the
expiredAtfield to theOAuthResponserecord is well-structured and immutable, which is ideal for DTOs.backend/src/main/java/endolphin/backend/global/security/JwtProvider.java (1)
14-15:Details
❓ Verification inconclusive
LGTM! Good practice to make token expiration configurable.
Moving from hardcoded value to configuration improves flexibility. However, let's verify the property is properly set.
🏁 Script executed:
#!/bin/bash # Check if jwt.expired is defined in application properties/yaml files find . -type f -name "application*.yml" -o -name "application*.properties" | while read -r file; do echo "Checking $file:" if [[ "$file" == *.yml ]] || [[ "$file" == *.yaml ]]; then grep -A 1 "jwt:" "$file" else grep "jwt.expired" "$file" fi doneLength of output: 691
Verify production configuration for 'jwt.expired' property
LGTM on moving token expiration to a configurable value—the use of @value is a good improvement over hardcoding. However, note that while your test configuration (./backend/src/test/resources/application.yml) defines some jwt settings (e.g., jwt.secret), the main configuration file (./backend/src/main/resources/application.yml) does not show any reference to jwt.expired. Please manually verify that your production configuration properly defines the jwt.expired property so that the injection in JwtProvider.java will work as expected.
backend/src/test/java/endolphin/backend/domain/auth/AuthServiceTest.java (1)
82-84: LGTM! Good test coverage for expiration time.The assertions properly verify both the presence and validity of the expiration time.
| String accessToken = jwtProvider.createToken(user.getId(), user.getEmail()); | ||
| return new OAuthResponse(accessToken); | ||
| LocalDateTime expiredAt = LocalDateTime.now().plus(expired, ChronoUnit.MILLIS); | ||
|
|
||
| return new OAuthResponse(accessToken, expiredAt); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider timezone implications and error handling.
A few considerations:
- The expiration time is created using the system's default timezone. Consider if this needs to be explicitly UTC.
- The JWT creation could throw exceptions that should be handled.
🏁 Script executed:
#!/bin/bash
# Check for timezone handling in the codebase
rg -l "ZoneId|ZoneOffset|TimeZone" .Length of output: 560
Timezone and Error Handling Adjustments in AuthService
- The expiration timestamp is currently determined by
LocalDateTime.now().plus(expired, ChronoUnit.MILLIS), which relies on the system default timezone. Please assess whether explicitly using UTC (e.g.,LocalDateTime.now(ZoneOffset.UTC)) could provide greater consistency—especially given that other modules (like CalendarService and CandidateEventService) appear to manage timezones more explicitly. - The call to
jwtProvider.createToken(user.getId(), user.getEmail())lacks any exception handling. Consider wrapping this call in a try-catch block to manage potential runtime exceptions more robustly.
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
access token 발급 시에 만료기한도 함께 응답하도록 수정
🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
Summary by CodeRabbit