fix(services-bff): Fix refresh token condition #16851
Conversation
WalkthroughThe pull request introduces a modification to the Changes
Possibly related PRs
Suggested reviewers
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
🧹 Outside diff range and nitpick comments (1)
apps/services/bff/src/app/modules/user/user.service.ts (1)
Line range hint
82-85: Enhance error handling and logging securityWhile the error handling is functional, consider these security improvements:
- Use specific error types instead of generic UnauthorizedException to help debugging while maintaining security
- Ensure error logging doesn't expose sensitive token information
} catch (error) { - this.logger.error('Get user error: ', error) + this.logger.error('Failed to get user', { + errorType: error.constructor.name, + // Avoid logging the full error object as it might contain sensitive data + message: error instanceof Error ? error.message : 'Unknown error' + }) throw new UnauthorizedException() }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/services/bff/src/app/modules/user/user.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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."
🔇 Additional comments (2)
apps/services/bff/src/app/modules/user/user.service.ts (2)
58-58: Correct fix for refresh token condition
The condition change from !refresh to refresh fixes the core issue by only refreshing tokens when explicitly requested, rather than the opposite. This aligns with the intended behavior and the PR objectives.
Line range hint 44-44: Verify impact of changing default refresh behavior
Changing the default value of refresh from true to false is a breaking change that could affect existing callers. While this change improves security by preventing unnecessary token refreshes, we need to ensure all consumers are updated accordingly.
✅ Verification successful
Default 'refresh' Parameter Update Verified
The change of the default refresh parameter from true to false does not impact existing callers, as all calls to getUser explicitly set the refresh parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to getUser to identify affected code
ast-grep --pattern 'getUser($$$)'
# Search for potential test cases that might need updating
rg -l "getUser.*refresh" --type=test
Length of output: 287
Script:
#!/bin/bash
# Verify if 'refresh' parameter is explicitly set in getUser calls
rg 'getUser\s*\([^)]*refresh\s*=' apps/auth-admin-web/pages/admin/api-scope-user/[apiScopeUser].tsx apps/auth-admin-web/components/Admin/lists/UsersList.tsx
# Correct the file type for test files and search for getUser usages in tests
rg -l "getUser.*refresh" --type=js tests/
Length of output: 251
Script:
#!/bin/bash
# Find all test files in the repository
fd -t f -e test.js -e test.ts -e test.jsx -e test.tsx -e spec.js -e spec.ts -e spec.jsx -e spec.tsx
# Verify if 'refresh' parameter is explicitly set in all 'getUser' calls
rg 'getUser\s*\([^)]*refresh\s*='
Length of output: 71654
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/services/bff/src/app/modules/user/user.service.ts (1)
Line range hint
44-83: Add tests for the token refresh logic.This is a critical security-related change affecting authentication flows. Unit tests should be added to verify:
- Token refresh occurs when access token is expired and refresh=true
- Token refresh doesn't occur when access token is expired but refresh=false
- Token refresh doesn't occur when access token is valid (regardless of refresh parameter)
- Error handling for invalid/missing tokens
Would you like me to help generate comprehensive test cases for this authentication logic?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/services/bff/src/app/modules/user/user.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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."
🔇 Additional comments (1)
apps/services/bff/src/app/modules/user/user.service.ts (1)
58-58: LGTM! The refresh token condition is now correct.
The new condition if (accessTokenHasExpired && refresh) properly handles token refresh by only refreshing when both:
- The access token has expired
- The refresh parameter is explicitly set to true
This fixes the previous incorrect logic that would refresh tokens when told not to.
Let's verify the impact of this change on other parts of the codebase:
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16851 +/- ##
=======================================
Coverage 36.44% 36.44%
=======================================
Files 6852 6852
Lines 143530 143530
Branches 40963 40963
=======================================
Hits 52303 52303
Misses 91227 91227
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ✅ 0 Failed, 333 Passed, 0 Skipped, 57.51s Total Time |
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Fix refresh token condition
Why
Wrong condition
Checklist:
Summary by CodeRabbit
Bug Fixes
Improvements