fix(services-bff): Add missing exp check on the user endpoint #17247
Conversation
Add a utility function to check if the access token has expired before attempting to refresh it. This change improves the efficiency of the token refresh process by ensuring that refresh operations are only triggered when necessary.
Add tests to verify token refresh behavior based on token expiration and refresh query parameter. Ensure that the refreshToken service is called only when the token exists, is expired, and refresh is set to true. This improves test coverage and ensures correct handling of token refresh scenarios.
WalkthroughThis pull request focuses on enhancing the token refresh logic and test coverage in the BFF (Backend for Frontend) user service. The changes involve updating the Changes
Sequence DiagramsequenceDiagram
participant Client
participant UserController
participant UserService
participant TokenCache
Client->>UserController: GET /user request
UserController->>UserService: getUser()
UserService->>TokenCache: Check cached token
alt Token exists and not expired
UserService-->>UserController: Return existing token
else Token expired and refresh=true
UserService->>TokenCache: Refresh token
TokenCache-->>UserService: New token
UserService-->>UserController: Return new token
else No valid token
UserService-->>UserController: Throw Unauthorized
end
UserController-->>Client: Respond with token/error
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/services/bff/src/app/modules/user/user.controller.spec.ts (1)
273-344: LGTM: Comprehensive test matrix for token refresh conditionsExcellent test coverage using a matrix approach to verify all possible combinations:
- Token existence
- Token expiration status
- Refresh flag value
The test structure ensures thorough validation of the new token refresh logic.
Consider adding a test case for invalid token format to improve edge case coverage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/services/bff/src/app/modules/user/user.controller.spec.ts(1 hunks)apps/services/bff/src/app/modules/user/user.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/services/bff/src/app/modules/user/user.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/user/user.controller.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (3)
apps/services/bff/src/app/modules/user/user.service.ts (2)
8-8: LGTM: Clean import of utility function
The addition of the hasTimestampExpiredInMS utility import follows NestJS best practices for code organization.
62-66: LGTM: Enhanced token refresh logic with expiration check
The modified condition properly implements the PR objective by checking three criteria before refreshing:
- Token exists (
cachedTokenResponse) - Token has expired (
hasTimestampExpiredInMS) - Refresh is enabled (
refresh)
This optimization prevents unnecessary token refreshes.
apps/services/bff/src/app/modules/user/user.controller.spec.ts (1)
234-271: LGTM: Well-structured test for non-expired token scenario
The test properly verifies that tokens are not refreshed when they haven't expired, even if refresh is requested. Good use of NestJS testing patterns and clear arrange-act-assert structure.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17247 +/- ##
=======================================
Coverage 35.74% 35.74%
=======================================
Files 6937 6937
Lines 148284 148284
Branches 42311 42311
=======================================
Hits 52998 52998
Misses 95286 95286
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
* fix(user): enhance token refresh logic with expiration check Add a utility function to check if the access token has expired before attempting to refresh it. This change improves the efficiency of the token refresh process by ensuring that refresh operations are only triggered when necessary. * test: enhance user token refresh logic in tests Add tests to verify token refresh behavior based on token expiration and refresh query parameter. Ensure that the refreshToken service is called only when the token exists, is expired, and refresh is set to true. This improves test coverage and ensures correct handling of token refresh scenarios. --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Fix for missing expiration check on the user endpoint
What
Add a utility function to check if the access token has expired before attempting to refresh it. This change improves the efficiency of the token refresh process by ensuring that refresh operations are only triggered when necessary.
Checklist:
Summary by CodeRabbit