feat(services-bff): Refresh token with polling #16872
Conversation
WalkthroughThe pull request introduces significant changes across multiple service files in the authentication module. Key modifications include the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning eslint@8.57.0: This version is no longer supported. Please see https://eslint.org/version-support for other options. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
apps/services/bff/src/app/modules/auth/token-refresh.service.ts
Outdated
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: 11
🧹 Outside diff range and nitpick comments (11)
apps/services/bff/src/app/modules/proxy/proxy.module.ts (1)
13-19: Consider organizing providers by dependency order.The providers array correctly includes all necessary services for the token refresh feature. However, consider organizing them based on their dependency relationships for better maintainability.
Consider this organization:
providers: [ ProxyService, - CryptoService, - TokenRefreshService, - ErrorService, - IdsService, + ErrorService, // Base service + CryptoService, // Base service + IdsService, // Depends on base services + TokenRefreshService, // Depends on other services ],apps/services/bff/src/app/modules/user/user.controller.ts (2)
31-35: Consider adding type safety for service parametersThe object parameter passed to
userService.getUserwould benefit from an explicit interface definition.Consider adding this interface:
interface GetUserParams { req: Request; res: Response; refresh: boolean; }Then update the method call:
- return this.userService.getUser({ + const params: GetUserParams = { req, res, refresh: query.refresh === 'true', - }) + }; + return this.userService.getUser(params);
Line range hint
23-36: Architecture aligns with NestJS best practicesThe controller properly:
- Delegates business logic to the service layer
- Maintains single responsibility principle
- Uses dependency injection for service integration
The token refresh polling mechanism is correctly abstracted away from the controller layer.
apps/services/bff/src/app/modules/auth/auth.service.ts (1)
Line range hint
1-507: Consider implementing token refresh polling as a separate service.Given the PR objectives focus on token refresh with polling, consider:
- Creating a dedicated TokenRefreshService to handle polling logic
- Implementing a caching mechanism for refresh operations
- Adding retry logic with exponential backoff
- Including proper logging for refresh operations
This would maintain single responsibility principle and make the auth flow more maintainable.
apps/services/bff/src/app/modules/user/user.service.ts (2)
1-2: Consider abstracting Express-specific types from the service layerPassing
RequestandResponsetypes from Express into service methods couples the service layer to the HTTP framework, which can reduce modularity and make unit testing more complex. Consider extracting only the necessary data from theRequestobject in the controller and refactoring the service methods to be independent of Express types.
Line range hint
34-87: Add or update unit tests for the modifiedgetUsermethodWith significant changes to the
getUsermethod, including updated parameters and token refresh logic, it's important to add or update unit tests to ensure all new scenarios are properly tested.Do you want assistance in creating unit tests for the updated method or opening a GitHub issue to track this task?
apps/services/bff/src/app/services/error.service.ts (1)
18-33: Consider reducing duplication betweenOAuth2ErrorCodeandOAUTH2_ERROR_CODESTo ensure consistency and simplify maintenance, consider deriving the
OAUTH2_ERROR_CODESarray from theOAuth2ErrorCodetype to avoid duplication.Here's an example of how you might implement this:
export type OAuth2ErrorCode = | 'invalid_grant' | 'invalid_client' | 'invalid_request' | 'unauthorized_client' | 'unsupported_grant_type' | 'invalid_scope' export const OAUTH2_ERROR_CODES: OAuth2ErrorCode[] = [ 'invalid_grant', 'invalid_client', 'invalid_request', 'unauthorized_client', 'unsupported_grant_type', 'invalid_scope', ]Alternatively, you can create the type from the array:
const errorCodes = [ 'invalid_grant', 'invalid_client', 'invalid_request', 'unauthorized_client', 'unsupported_grant_type', 'invalid_scope', ] as const export type OAuth2ErrorCode = typeof errorCodes[number] export const OAUTH2_ERROR_CODES: OAuth2ErrorCode[] = [...errorCodes]apps/services/bff/src/app/modules/ids/ids.service.ts (1)
Line range hint
66-74: RestrictcreatePARAuthorizationHeaderto be a private methodThe
createPARAuthorizationHeadermethod is currently declared aspublic, but it appears to be used only within theIdsService. Making itprivateenhances encapsulation by hiding implementation details that are not intended for external use.Apply this diff to make the method private:
- createPARAuthorizationHeader() { + private createPARAuthorizationHeader() { const { ids } = this.config const basicAuth = `${encodeURIComponent(ids.clientId)}:${encodeURIComponent( ids.secret, )}` const base64Auth = Buffer.from(basicAuth).toString('base64') return `Basic ${base64Auth}` }apps/services/bff/src/app/modules/auth/token-refresh.service.ts (2)
14-15: Consider externalizingPOLL_INTERVALandMAX_POLL_TIMEto configurationCurrently, the polling interval and maximum polling time are hardcoded as static properties. Externalizing these constants to a configuration file or environment variables would enhance flexibility and make it easier to adjust these values without changing the codebase.
196-198: Include error details in the log for better debuggingThe current error log does not include the actual error information, which can make diagnosing issues more difficult. Including the error object provides valuable context when investigating failures.
Update the log statement to include the error details:
- this.logger.error(`Token refresh failed for sid: ${sid}`) + this.logger.error(`Token refresh failed for sid: ${sid}`, { error })apps/services/bff/src/app/modules/proxy/proxy.service.ts (1)
238-238: Provide more descriptive error messages for better debuggingThe error message
'Proxying url failed!'may not offer sufficient context for troubleshooting.Consider including additional details in the exception message to aid debugging while avoiding exposure of sensitive information. For example:
throw new BadRequestException(`Invalid proxy URL provided.`);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
apps/services/bff/src/app/modules/auth/auth.service.ts(6 hunks)apps/services/bff/src/app/modules/auth/token-refresh.service.ts(1 hunks)apps/services/bff/src/app/modules/ids/ids.service.ts(2 hunks)apps/services/bff/src/app/modules/ids/ids.types.ts(0 hunks)apps/services/bff/src/app/modules/proxy/proxy.module.ts(1 hunks)apps/services/bff/src/app/modules/proxy/proxy.service.ts(7 hunks)apps/services/bff/src/app/modules/user/user.controller.ts(2 hunks)apps/services/bff/src/app/modules/user/user.module.ts(1 hunks)apps/services/bff/src/app/modules/user/user.service.ts(2 hunks)apps/services/bff/src/app/services/error.service.ts(1 hunks)apps/services/bff/src/app/utils/get-cookie-options.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/services/bff/src/app/modules/ids/ids.types.ts
🧰 Additional context used
📓 Path-based instructions (10)
apps/services/bff/src/app/modules/auth/auth.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/auth/token-refresh.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/ids/ids.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/proxy/proxy.module.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/proxy/proxy.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.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.module.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.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/services/error.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/utils/get-cookie-options.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 (23)
apps/services/bff/src/app/utils/get-cookie-options.ts (2)
1-2: LGTM! Clean and focused imports.
The imports are appropriate and well-scoped for the functionality.
4-13: Consider enhancing cookie security configuration.
The cookie options implementation follows security best practices with httpOnly, secure, and sameSite: 'lax'. However, consider these improvements:
- Add
maxAgeorexpiresto explicitly control cookie lifetime - Validate
environment.keyPathto ensure it's not undefined - Consider memoizing the function since it returns the same configuration object
Here's a suggested implementation:
+ import { memoize } from 'lodash'
import { CookieOptions } from 'express'
import { environment } from '../../environment'
- export const getCookieOptions = (): CookieOptions => {
+ export const getCookieOptions = memoize((): CookieOptions => {
+ if (!environment.keyPath) {
+ throw new Error('Cookie path configuration is missing')
+ }
+
return {
httpOnly: true,
secure: true,
// The lax setting allows cookies to be sent on top-level navigations (such as redirects),
// while still providing some protection against CSRF attacks.
sameSite: 'lax',
path: environment.keyPath,
+ maxAge: 24 * 60 * 60 * 1000, // 24 hours
}
- }
+ })Let's verify the environment configuration:
✅ Verification successful
Let me analyze the environment configuration based on the shell script results.
The environment.keyPath is properly configured in the BFF service:
-
It's defined in the environment schema (
apps/services/bff/src/environment/environment.schema.ts) with validation:- Required field with error message
- Has validation to ensure it doesn't end with '/bff'
-
It's used consistently across the BFF service:
- As cookie path in
get-cookie-options.ts - As global API prefix in
main.ts - In auth service for client base URL construction
- As cookie path in
-
The value comes from
BFF_CLIENT_KEY_PATHenvironment variable with proper schema validation.
Cookie configuration is secure and well-implemented.
The current implementation follows security best practices and the environment configuration is properly validated. The suggestions for improvement can be revised:
maxAge: Not strictly necessary as session cookies (without expiry) are often preferred for auth cookiesenvironment.keyPathvalidation: Already handled by the environment schema- Memoization: Not critical as the object is small and the function is likely not called frequently
- The cookie path is properly validated at startup through Zod schema
- The implementation is used consistently across the BFF service
- The security settings (httpOnly, secure, sameSite) are appropriately configured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if environment.keyPath is properly configured across the codebase
# Check for environment configuration files
echo "Searching for environment configuration files..."
fd -e ts -e js environment
# Check for keyPath usage
echo "Checking keyPath usage..."
rg "keyPath" -A 5 -B 5
Length of output: 17783
apps/services/bff/src/app/modules/user/user.module.ts (3)
2-3: LGTM! Import statements follow NestJS best practices
The new service imports maintain proper module boundaries and follow the expected NestJS architectural patterns.
Also applies to: 5-5
13-19: LGTM! Module configuration follows NestJS patterns
The providers are correctly registered in the module, maintaining proper dependency injection patterns and service encapsulation.
13-19: Verify circular dependencies with AuthModule
Since we're importing AuthModule and also providing TokenRefreshService (which comes from the auth module), we should verify there are no circular dependencies.
✅ Verification successful
Let me gather more information to verify the potential circular dependencies.
Let me check one more aspect to verify if there's a circular dependency through the services.
No circular dependency detected between UserModule and AuthModule
Based on the analysis:
TokenRefreshServiceis defined in the auth module and is correctly imported by the user moduleUserServicedoes depend onTokenRefreshService, butAuthServicedoes not depend on any user module components- The auth module exports its services properly, making them available for use in other modules
The current architecture follows a unidirectional dependency flow from user module to auth module, which is a correct design pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential circular dependencies between UserModule and AuthModule
# Search for UserModule imports in auth module
echo "Checking for UserModule imports in AuthModule:"
rg -l "import.*UserModule" "apps/services/bff/src/app/modules/auth/"
# Search for service dependencies
echo -e "\nChecking for service dependencies:"
ast-grep --pattern 'constructor($$$) {
$$$
}' "apps/services/bff/src/app/modules/auth/"
Length of output: 346
Script:
#!/bin/bash
# Check imports and providers in both modules
# Check auth.module.ts content
echo "Content of auth.module.ts:"
fd -e ts "auth.module.ts" -x cat {}
echo -e "\nContent of user.module.ts:"
fd -e ts "user.module.ts" -x cat {}
# Check where TokenRefreshService is defined and its dependencies
echo -e "\nTokenRefreshService definition and dependencies:"
rg "class TokenRefreshService" -A 10
rg "import.*TokenRefreshService"
Length of output: 10362
Script:
#!/bin/bash
# Check dependencies in AuthService and UserService
echo "AuthService dependencies:"
rg "class AuthService" -A 10
echo -e "\nUserService dependencies:"
rg "class UserService" -A 10
echo -e "\nCheck if AuthService imports UserService:"
rg "import.*UserService" "apps/services/bff/src/app/modules/auth/"
Length of output: 7449
apps/services/bff/src/app/modules/user/user.controller.ts (2)
1-1: LGTM! Import changes follow NestJS conventions
The addition of Response type and Res decorator imports aligns with NestJS best practices for handling HTTP responses.
Also applies to: 4-11
27-27: Ensure test coverage for response handling
The addition of @Res({ passthrough: true }) requires careful testing to verify:
- Error scenarios are properly handled
- Response headers and cookies are correctly set
- Framework features like interceptors still work as expected
apps/services/bff/src/app/modules/auth/auth.service.ts (5)
29-29: LGTM! Good refactoring of cookie options to utils.
Moving getCookieOptions to a shared utility follows the DRY principle and improves code maintainability.
286-286: LGTM! Appropriate error handling approach.
The simplified error handling is acceptable as any errors from updateTokenCache will be caught by the surrounding try-catch block.
299-299: LGTM! Consistent use of shared cookie options.
The use of the shared getCookieOptions utility maintains consistency across the codebase.
407-407: LGTM! Consistent cookie clearing implementation.
The use of getCookieOptions for clearing cookies maintains consistency with other cookie operations in the service.
206-208: Verify error handling for PAR response.
The removal of error type checking could potentially lead to runtime errors if parResponse is undefined or doesn't contain request_uri.
apps/services/bff/src/app/modules/user/user.service.ts (3)
16-17: Dependencies injected according to NestJS best practices
Adding TokenRefreshService and ErrorService via dependency injection aligns with NestJS patterns for service encapsulation and promotes modularity.
21-23: Good documentation added for mapToBffUser method
Including JSDoc comments enhances code readability and maintainability by clearly explaining the purpose of the method.
31-33: Well-documented method description for getUser
Adding comments provides clarity on the method's functionality, improving understanding for future maintainers.
apps/services/bff/src/app/services/error.service.ts (3)
1-12: Imports are well-organized and follow best practices
The import statements are correctly grouped and use TypeScript's import type where appropriate.
44-50: Type guard isKnownOAuth2ErrorCode is effectively implemented
The use of a type guard enhances type safety when validating error codes.
79-82: Confirm the logic when handling missing sid or UnauthorizedException
Ensure that throwing an UnauthorizedException when sid is missing or the error is an UnauthorizedException aligns with the desired authentication flow.
If this logic is intentional, no action is needed. Otherwise, consider revisiting the condition.
apps/services/bff/src/app/modules/ids/ids.service.ts (1)
36-57: Confirm adherence to NestJS architecture and practices
The implementation of the IdsService follows NestJS best practices by utilizing dependency injection for configurations and services such as BffConfig, EnhancedFetchAPI, and CryptoService. The service methods are well-structured and encapsulated, promoting maintainability and testability.
apps/services/bff/src/app/modules/proxy/proxy.service.ts (4)
12-12: Good use of type-only imports for Request and Response
Using import type helps optimize the compiled output by ensuring only types are imported, which can reduce bundle size.
21-21: Consistent use of SESSION_COOKIE_NAME constant
Replacing hardcoded cookie names with the SESSION_COOKIE_NAME constant improves maintainability and reduces the risk of typos.
70-71: Proper dependency injection of TokenRefreshService and ErrorService
Injecting TokenRefreshService and ErrorService follows NestJS dependency injection patterns and enhances modularity and testability of the service.
189-191: Using HttpStatus constants for clarity in error responses
Utilizing HttpStatus.INTERNAL_SERVER_ERROR instead of hardcoded status codes enhances code readability and ensures consistency across HTTP responses.
apps/services/bff/src/app/modules/auth/token-refresh.service.ts
Outdated
Show resolved
Hide resolved
apps/services/bff/src/app/modules/auth/token-refresh.service.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16872 +/- ##
==========================================
+ Coverage 36.44% 36.46% +0.02%
==========================================
Files 6851 6903 +52
Lines 143567 144590 +1023
Branches 40980 41288 +308
==========================================
+ Hits 52328 52730 +402
- Misses 91239 91860 +621 Flags with carried forward coverage won't be shown. Click here to find out more. see 137 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 79 Total Test Services: 0 Failed, 77 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/services/bff/src/app/modules/auth/token-refresh.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/bff/src/app/modules/auth/token-refresh.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 (3)
apps/services/bff/src/app/modules/auth/token-refresh.service.ts (3)
34-47: LGTM! Well-documented utility methods
The key creation methods are well-implemented with clear documentation and proper separation of concerns.
102-132: LGTM with previous review comment
The polling implementation has proper cleanup, but please address the previous review comment about using setTimeout instead of setInterval for better async handling.
150-201: 🛠️ Refactor suggestion
Add comprehensive unit tests for the token refresh flow
The refreshToken method handles critical authentication logic and should be thoroughly tested. Key test cases should include:
- Successful token refresh
- Concurrent refresh requests
- Polling timeout and retry
- Error handling scenarios
Additionally, consider adding integration tests to verify the interaction with Redis cache.
apps/services/bff/src/app/modules/auth/token-refresh.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts (6)
20-41: Consider moving mock data to separate test fixtures fileThe mock data structures are well-defined but would be better organized in a separate test fixtures file for reusability and maintainability.
86-95: Add type annotations to mock implementationsThe mock implementations for cache operations would benefit from explicit type annotations for better type safety.
- save: jest.fn().mockImplementation(async ({ key, value }) => { + save: jest.fn().mockImplementation(async ({ key, value }: { key: string; value: unknown }) => { mockCacheStore.set(key, value) }), - get: jest + get: jest<unknown> .fn() .mockImplementation(async (key) => mockCacheStore.get(key)), - delete: jest + delete: jest<void> .fn() .mockImplementation(async (key) => mockCacheStore.delete(key)),
157-175: Enhance timeout test case clarity and coverageThe timeout test could be improved by:
- Making the timeout duration explicit in the test description
- Adding assertions to verify the timing
- Testing different timeout scenarios
- it('should retry refresh if polling times out', async () => { + it('should retry refresh after 2000ms polling timeout', async () => { // Arrange + const startTime = Date.now() await cacheService.save({ key: refreshInProgressKey, value: true, ttl: 3000, }) // Act const result = await service.refreshToken({ sid: testSid, encryptedRefreshToken: testRefreshToken, }) // Assert + const duration = Date.now() - startTime + expect(duration).toBeGreaterThanOrEqual(2000) expect(mockLogger.error).toHaveBeenCalled() expect(idsService.refreshToken).toHaveBeenCalledWith(testRefreshToken) expect(result).toEqual(mockTokenResponse) })
177-193: Add more comprehensive error handling test casesConsider adding test cases for:
- Network errors
- Invalid token errors
- Rate limiting errors
- Server errors with different status codes
Example addition:
it('should handle specific error types appropriately', async () => { const errorScenarios = [ { error: new Error('Network Error'), expectedMessage: 'Network error during token refresh' }, { error: new Error('Invalid Token'), expectedMessage: 'Invalid refresh token' }, { error: new Error('Rate Limit Exceeded'), expectedMessage: 'Rate limit exceeded during token refresh' } ]; for (const scenario of errorScenarios) { jest.spyOn(idsService, 'refreshToken').mockRejectedValueOnce(scenario.error); await expect(service.refreshToken({ sid: testSid, encryptedRefreshToken: testRefreshToken, })).rejects.toThrow(scenario.error); expect(mockLogger.error).toHaveBeenCalledWith( expect.stringContaining(scenario.expectedMessage) ); } });
195-256: Strengthen concurrent request testingWhile the test effectively verifies concurrent request handling, consider enhancing it with:
- Verification of request order
- Testing with varying delays between requests
- Assertion for maximum execution time
it('should maintain request order in concurrent scenarios', async () => { const requestOrder: number[] = []; const startTimes: number[] = []; jest.spyOn(idsService, 'refreshToken').mockImplementation(async () => { const requestIndex = startTimes.length; startTimes.push(Date.now()); await delay(Math.random() * 50); // Random delay requestOrder.push(requestIndex); return mockTokenResponse; }); const results = await Promise.all([ service.refreshToken({ sid: testSid, encryptedRefreshToken: testRefreshToken }), service.refreshToken({ sid: testSid, encryptedRefreshToken: testRefreshToken }), service.refreshToken({ sid: testSid, encryptedRefreshToken: testRefreshToken }) ]); expect(requestOrder).toEqual([0]); // Only first request should perform refresh expect(results).toHaveLength(3); expect(Math.max(...startTimes) - Math.min(...startTimes)).toBeLessThan(100); // All requests within 100ms });
53-257: Consider adding integration and performance testsWhile unit tests provide good coverage, consider adding:
- Integration tests with the actual authentication flow
- Performance benchmarks for the polling mechanism
- Load tests for concurrent request handling
This would help verify the service's behavior in real-world scenarios and ensure it meets performance requirements under load.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/bff/src/app/modules/auth/token-refresh.service.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."
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts (1)
Line range hint
1-234: Consider reorganizing tests for better coverage trackingWhile the test structure is good, consider organizing the token refresh related tests into a separate describe block. This would:
- Make it easier to track coverage of the new token refresh functionality
- Provide a clear separation between proxy functionality and token management tests
- Help identify gaps in testing the new polling and concurrency features
Suggested test organization:
describe('Token Refresh Handling', () => { // Group all token refresh related tests here // - Basic token refresh // - Concurrent refresh handling // - Polling timeout and retry // - Maximum retry attempts }) describe('Proxy Endpoints', () => { // Keep existing proxy endpoint tests here })apps/services/bff/src/app/modules/auth/token-refresh.service.ts (2)
34-47: Consider enhancing type safety for cache keys.The key creation methods are well-structured, but could benefit from type safety improvements.
Consider creating a type for session key types:
type SessionKeyType = 'current' | 'refresh_token_in_progress'; private createRefreshTokenKey(sid: string): `${SessionKeyType}:${string}` { return `refresh_token_in_progress:${sid}`; }
150-209: Consider breaking down the complex refresh logic.The
refreshTokenmethod handles multiple responsibilities. Consider extracting the retry logic and token response validation into separate methods.Example structure:
private async retryRefreshAfterPollingTimeout( refreshTokenKey: string, encryptedRefreshToken: string, ): Promise<CachedTokenResponse> { // Retry logic here } private async validateTokenResponse( sid: string, tokenResponseKey: string, ): Promise<CachedTokenResponse> { // Validation logic here }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts(2 hunks)apps/services/bff/src/app/modules/auth/token-refresh.service.ts(1 hunks)apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts(3 hunks)apps/services/bff/src/app/modules/proxy/proxy.service.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/services/bff/src/app/modules/proxy/proxy.service.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/services/bff/src/app/modules/auth/auth.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."
apps/services/bff/src/app/modules/auth/token-refresh.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/proxy/proxy.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 (4)
apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts (1)
42-44: LGTM! Mock implementations are correctly simplified
The mock implementations for refreshToken, getTokens, and revokeToken are properly aligned with the new token refresh service requirements. The simplified return values improve test readability while maintaining functional correctness.
apps/services/bff/src/app/modules/auth/token-refresh.service.ts (1)
1-8: LGTM! Dependencies are properly imported and organized.
The service follows NestJS best practices with proper imports and type definitions.
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts (2)
9-12: LGTM! Import organization improvements
The reordering of imports and renaming of mockedTokensResponse to tokensResponse improves code organization and clarity.
61-63: Verify test coverage for token refresh polling scenarios
The simplified mock implementations align well with the new token refresh polling feature. However, given the PR objectives of preventing multiple simultaneous refresh attempts, we should ensure adequate test coverage.
Let's verify the test coverage:
✅ Verification successful
Test coverage for token refresh polling is comprehensive
The test coverage for token refresh polling scenarios is thorough and well-implemented. The token-refresh.service.spec.ts includes specific tests that verify:
- Prevention of concurrent refresh token requests
- Polling behavior and timeout handling
- Waiting for ongoing refresh operations
- Error handling during refresh operations
- Cache management for refresh tokens
The implementation in token-refresh.service.ts aligns with the PR objectives of preventing multiple simultaneous refresh attempts, and the simplified mock implementations in the auth controller tests are appropriate given the comprehensive coverage in the dedicated token refresh service tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for token refresh polling scenarios
# Look for test cases covering concurrent token refresh
rg -l "concurrent|simultaneous|polling" "apps/services/bff/src/**/*spec.ts"
# Check for timing-related test utilities
rg -l "fakeTimers|advanceTimersByTime|tick" "apps/services/bff/src/**/*spec.ts"
Length of output: 307
Script:
#!/bin/bash
# Let's try with correct path patterns and check test coverage
# Look for test cases covering concurrent token refresh
rg -l "concurrent|simultaneous|polling" --type spec
# Check for timing-related test utilities
rg -l "fakeTimers|advanceTimersByTime|tick" --type spec
# Get content of the auth controller spec file to analyze test coverage
rg -p "describe.*auth.*controller" --type spec -A 10
# Look for refresh token related tests
rg "refresh.*token" --type spec -A 5
Length of output: 801
Script:
#!/bin/bash
# Let's first find the test files and then analyze them
# Find all test files
fd -t f ".*spec.ts$"
# Find all test files specifically in the auth module
fd -t f "auth.*spec.ts$"
# Try to get content of the specific file we're reviewing
cat "apps/services/bff/src/app/modules/auth/auth.controller.spec.ts"
Length of output: 83649
Script:
#!/bin/bash
# Let's check for token refresh polling tests and implementation
# Find token refresh service tests
cat "apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts"
# Look for token refresh related code in the auth controller
rg "token.*refresh" "apps/services/bff/src/app/modules/auth/"
# Check for any concurrent or polling related tests
rg "concurrent|simultaneous|polling" "apps/services/bff/src/app/modules/auth/"
Length of output: 10798
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
apps/services/bff/src/app/modules/proxy/proxy.service.ts (3)
92-96: Consider caching the session keyThe session key is constructed on every request. Consider moving this to a class property or memoizing it to avoid repeated string concatenation.
+ private createTokenResponseKey(sid: string): string { + return this.cacheService.createSessionKeyType('current', sid) + }
231-239: Add request validation for forwardGetApiRequestConsider adding validation for the request body and headers before processing.
private validateRequest(req: Request): void { if (!req.headers['content-type']?.includes('application/json')) { throw new BadRequestException('Invalid content type') } }
244-245: Enhance error message for invalid URLThe error message could be more descriptive to help clients understand the issue.
- throw new BadRequestException('Proxying url failed!') + throw new BadRequestException(`URL '${url}' is not in the allowed external API URLs list`)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/services/bff/src/app/modules/proxy/proxy.service.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/bff/src/app/modules/proxy/proxy.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/proxy/proxy.service.ts (2)
70-71: LGTM: Dependencies properly injected
The addition of TokenRefreshService and ErrorService follows NestJS dependency injection patterns correctly.
108-112: LGTM: Token refresh implementation
The token refresh logic correctly implements the polling mechanism as specified in the PR objectives, preventing multiple simultaneous refresh attempts.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
apps/services/bff/src/app/modules/user/user.controller.spec.ts (3)
18-24: Consider using a more robust caching mechanism for testsThe current implementation uses a simple Map for mocking cache operations. While this works, it might not accurately represent thread-safety concerns that the actual cache manager handles.
Consider using a more sophisticated mock that can simulate concurrent operations:
-const mockCacheStore = new Map() +class ThreadSafeMockCache { + private store = new Map() + async set(key: string, value: unknown): Promise<void> { + this.store.set(key, value) + } + async get(key: string): Promise<unknown> { + return this.store.get(key) + } + async del(key: string): Promise<void> { + this.store.delete(key) + } +} +const mockCacheStore = new ThreadSafeMockCache()
54-62: Add type annotations for mock servicesThe mock service objects lack TypeScript interfaces, which could lead to type mismatches with the actual services.
+type TokenRefreshServiceMock = Partial<TokenRefreshService> +type IdsServiceMock = Partial<IdsService> -const mockTokenRefreshService = { +const mockTokenRefreshService: TokenRefreshServiceMock = { refreshToken: jest.fn().mockResolvedValue(mockCachedTokenResponse), } -const mockIdsService = { +const mockIdsService: IdsServiceMock = { getTokens: jest.fn().mockResolvedValue(mockCachedTokenResponse), revokeToken: jest.fn().mockResolvedValue(undefined), getLoginSearchParams: jest.fn().mockImplementation(getLoginSearchParmsFn), }
96-100: Enhance error handling in afterAllThe app closure in afterAll should handle potential errors to ensure proper cleanup.
afterAll(async () => { if (app) { - await app.close() + try { + await app.close() + } catch (error) { + console.error('Failed to close test app:', error) + } } })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts(3 hunks)apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts(4 hunks)apps/services/bff/src/app/modules/user/user.controller.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/services/bff/src/app/modules/auth/auth.controller.spec.ts
- apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
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."
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/services/bff/src/app/modules/auth/token-refresh.service.ts (1)
63-92: Enhance error handling granularityThe error handling could be more specific to differentiate between cache operations and token refresh failures.
Consider implementing specific error types:
class TokenRefreshError extends Error { constructor( message: string, public readonly operation: 'CACHE_SAVE' | 'TOKEN_REFRESH' | 'CACHE_UPDATE', public readonly cause?: Error ) { super(message) } } // Usage: try { await this.cacheService.save(...) } catch (error) { throw new TokenRefreshError('Failed to set refresh flag', 'CACHE_SAVE', error) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/services/bff/src/app/modules/auth/token-refresh.service.ts(1 hunks)apps/services/bff/src/app/modules/user/user.controller.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/services/bff/src/app/modules/user/user.controller.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/bff/src/app/modules/auth/token-refresh.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/auth/token-refresh.service.ts (2)
34-47: LGTM! Well-documented utility methods
The key generation methods are well-documented and follow single responsibility principle.
104-134: Refer to existing comment about setInterval usage
The implementation of polling mechanism needs improvement. Please refer to the previous comment about avoiding async functions within setInterval.
apps/services/bff/src/app/modules/auth/token-refresh.service.ts
Outdated
Show resolved
Hide resolved
apps/services/bff/src/app/modules/auth/token-refresh.service.ts
Outdated
Show resolved
Hide resolved
apps/services/bff/src/app/modules/auth/token-refresh.service.ts
Outdated
Show resolved
Hide resolved
apps/services/bff/src/app/modules/auth/token-refresh.service.ts
Outdated
Show resolved
Hide resolved
apps/services/bff/src/app/modules/auth/token-refresh.service.ts
Outdated
Show resolved
Hide resolved
apps/services/bff/src/app/modules/auth/token-refresh.service.ts
Outdated
Show resolved
Hide resolved
apps/services/bff/src/app/modules/auth/token-refresh.service.ts
Outdated
Show resolved
Hide resolved
apps/services/bff/src/app/modules/auth/token-refresh.service.ts
Outdated
Show resolved
Hide resolved
|
@eirikurn I simplified the control flow with logging/errors in mind. Can you take a second look 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (12)
apps/services/bff/src/app/modules/user/user.service.ts (2)
1-17: Consider maintaining logging capabilityThe removal of the logger dependency might impact observability, especially for a critical authentication service. Consider retaining logging capabilities through the ErrorService or a dedicated logging service to maintain proper monitoring of authentication flows.
74-79: Enhance type safety in error handlingThe error object passed to handleAuthorizedError lacks type information, which could make it harder to maintain and debug error handling logic.
Consider adding a custom error type:
type AuthError = UnauthorizedException | { message: string; code?: string; // Add other relevant error properties }; // Then update the error handling: } catch (error: unknown) { return this.errorService.handleAuthorizedError({ error: error as AuthError, res, tokenResponseKey, operation: `${UserService.name}.getUser`, }); }apps/services/bff/src/app/modules/auth/token-refresh.service.ts (3)
14-25: LGTM! Consider environment-specific configurationThe service follows NestJS best practices with proper dependency injection. However, the timing constants should be configurable via environment variables for different deployment environments.
Consider using NestJS ConfigService to make
POLL_INTERVALandMAX_POLL_TIMEconfigurable.
85-115: Consider enhancing polling logsThe polling implementation is solid, but could benefit from more detailed logging to track the polling progress.
Consider adding debug logs for each polling attempt:
while (attempts < maxAttempts) { + this.logger.debug(`Polling attempt ${attempts + 1}/${maxAttempts} for session ${sid}`) const refreshTokenInProgress = await this.cacheService.get<boolean>(
161-202: Add comprehensive test coverageWhile the implementation is solid, it requires thorough testing to ensure reliability of the concurrent refresh mechanism.
Would you like assistance in generating unit tests for the following scenarios?
- Concurrent refresh requests
- Polling timeout and retry
- Cache miss scenarios
- Token expiration handling
apps/services/bff/src/app/modules/proxy/proxy.service.ts (2)
79-127: Consider adding retry mechanism for token refresh.While the token refresh logic is well-structured, consider adding a retry mechanism with exponential backoff for cases where the refresh operation fails due to temporary network issues.
if ( cachedTokenResponse && hasTimestampExpiredInMS(cachedTokenResponse.accessTokenExp) ) { + const maxRetries = 3; + let retryCount = 0; + while (retryCount < maxRetries) { + try { cachedTokenResponse = await this.tokenRefreshService.refreshToken({ sid, encryptedRefreshToken: cachedTokenResponse.encryptedRefreshToken, }) + break; + } catch (error) { + if (retryCount === maxRetries - 1) throw error; + retryCount++; + await new Promise(resolve => setTimeout(resolve, Math.pow(2, retryCount) * 1000)); + } + } }
247-247: Enhance error message for invalid URL.The current error message 'Proxying url failed!' doesn't provide enough context about why the URL was rejected. Consider including the validation rules or allowed domains in the error message.
- throw new BadRequestException('Proxying url failed!') + throw new BadRequestException( + `URL '${url}' is not in the allowed domains. Allowed domains: ${this.config.allowedExternalApiUrls.join(', ')}` + )apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts (5)
10-10: Consider moving utility functions to a separate test utilities fileThe
delayfunction is a generic test utility that could be reused across different test files.Consider moving it to a shared test utilities file, e.g.,
test/utils.ts:// test/utils.ts export const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));
23-42: Extract mock data to test fixtures fileThe mock data objects are well-structured but could be moved to a separate fixtures file to improve maintainability and reusability.
Consider creating a test fixtures file, e.g.,
test/fixtures/auth.fixtures.ts:// test/fixtures/auth.fixtures.ts export const mockTokenResponse: CachedTokenResponse = { // ... current mock data }; export const mockIdsTokenResponse: TokenResponse = { // ... current mock data };Also applies to: 44-52
113-127: Consider adding edge cases for token refreshWhile the happy path is well tested, consider adding tests for:
- Refresh with expired tokens
- Refresh with invalid tokens
- Rate limiting scenarios
178-194: Enhance error handling test coverageThe error handling test could be more specific about different types of errors:
- Network errors
- Invalid token errors
- Server errors
Example:
it('should handle specific refresh token errors', async () => { const cases = [ { error: new NetworkError('Connection failed'), expectedLog: 'Network error during token refresh' }, { error: new InvalidTokenError('Token expired'), expectedLog: 'Invalid refresh token' }, { error: new ServerError('Internal error'), expectedLog: 'Server error during refresh' } ]; for (const { error, expectedLog } of cases) { jest.spyOn(idsService, 'refreshToken').mockRejectedValueOnce(error); const result = await service.refreshToken({ sid: testSid, encryptedRefreshToken: testRefreshToken, }); expect(result).toBe(null); expect(mockLogger.warn).toHaveBeenCalledWith( expect.stringContaining(expectedLog), ); } });
196-256: Refactor concurrent requests test for better readabilityThe concurrent requests test is comprehensive but could be more maintainable:
- Extract the mock implementations to separate helper functions
- Use more descriptive variable names
- Add comments explaining the test flow
Example refactor:
it('should prevent concurrent refresh token requests', async () => { const CONCURRENT_REQUESTS = 5; const LOCK_DELAY_MS = 50; // Setup mock behavior const setupCacheMocks = () => { let isRefreshInProgress = false; jest.spyOn(cacheService, 'get') .mockImplementation(async (key) => key.includes(refreshInProgressPrefix) ? isRefreshInProgress : mockTokenResponse ); jest.spyOn(cacheService, 'save') .mockImplementation(async ({ key }) => { if (key.includes(refreshInProgressPrefix)) { isRefreshInProgress = true; await delay(LOCK_DELAY_MS); } }); jest.spyOn(cacheService, 'delete') .mockImplementation(async (key) => { if (key.includes(refreshInProgressPrefix)) { isRefreshInProgress = false; } }); }; // Setup mocks setupCacheMocks(); // Execute concurrent requests const refreshPromises = Array.from({ length: CONCURRENT_REQUESTS }, (_, index) => service.refreshToken({ sid: testSid, encryptedRefreshToken: testRefreshToken, }) ); const results = await Promise.all(refreshPromises); // Verify results expect(idsService.refreshToken).toHaveBeenCalledTimes(1); results.forEach(result => expect(result).toEqual(mockTokenResponse)); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
apps/services/bff/src/app/modules/auth/auth.service.ts(6 hunks)apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts(1 hunks)apps/services/bff/src/app/modules/auth/token-refresh.service.ts(1 hunks)apps/services/bff/src/app/modules/proxy/proxy.service.ts(7 hunks)apps/services/bff/src/app/modules/user/user.service.ts(1 hunks)apps/services/bff/src/app/services/error.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/services/bff/src/app/modules/auth/auth.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/auth/token-refresh.service.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."
apps/services/bff/src/app/modules/auth/token-refresh.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/proxy/proxy.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.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/services/error.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 (12)
apps/services/bff/src/app/modules/user/user.service.ts (1)
16-16: Verify TokenRefreshService implementation
The TokenRefreshService is crucial for preventing multiple simultaneous refresh attempts. Let's verify its implementation.
✅ Verification successful
TokenRefreshService implementation verified successfully
The TokenRefreshService implementation in apps/services/bff/src/app/modules/auth/token-refresh.service.ts has been verified and includes:
- Proper polling mechanism with configurable intervals (POLL_INTERVAL = 200ms) and timeout (MAX_POLL_TIME = 3s)
- Centralized token refresh handling to prevent multiple simultaneous refresh attempts
- Cache-based synchronization using CacheService
- Comprehensive error handling and logging
- Well-tested implementation with thorough test coverage in token-refresh.service.spec.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TokenRefreshService implementation to verify polling and concurrency handling
ast-grep --pattern 'class TokenRefreshService {
$$$
refreshToken($$$) {
$$$
}
$$$
}'
# Search for tests to verify proper handling of concurrent requests
rg -l "TokenRefreshService.*spec"
Length of output: 134
Script:
#!/bin/bash
# Let's try a broader search for TokenRefreshService implementation
rg -A 10 "class TokenRefreshService"
# Search for files containing TokenRefreshService
fd -t f "TokenRefreshService"
# Search for any refresh token related implementations
rg -A 5 "refreshToken"
# Look for any test files containing token refresh
rg -l "token.*refresh.*test|token.*refresh.*spec"
Length of output: 73134
apps/services/bff/src/app/modules/auth/token-refresh.service.ts (3)
27-45: LGTM! Well-documented helper methods
The helper methods are focused, well-documented, and follow consistent patterns for key generation.
51-79: LGTM! Robust error handling with proper cleanup
The implementation includes proper error handling with cleanup in the finally block and appropriate warning logs.
121-142: LGTM! Comprehensive token validation
The cache operations include proper null checks and token expiration validation with appropriate warning logs.
apps/services/bff/src/app/modules/proxy/proxy.service.ts (1)
5-5: LGTM! Clean dependency management and service organization.
The changes follow NestJS best practices with proper dependency injection and service organization. The type imports and new service dependencies improve code maintainability and separation of concerns.
Also applies to: 12-12, 21-22, 26-26, 70-71
apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts (1)
108-111: LGTM! Proper test cleanup implemented
The cleanup properly resets both the mock cache store and Jest mocks after each test, preventing test pollution.
apps/services/bff/src/app/modules/auth/auth.service.ts (4)
286-286: Consider potential race conditions in token update
The updateTokenCache operation might be susceptible to race conditions if multiple requests attempt to update the cache simultaneously. Consider using the new TokenRefreshService mentioned in the PR objectives for better concurrency control.
Let's verify the TokenRefreshService implementation:
206-208: Consider security implications of exposing client_id in URL parameters
While the implementation is functionally correct, exposing the client_id in URL parameters might make it visible in server logs and browser history. Consider if this poses any security risks in your context.
Let's check if this is a common pattern in the codebase:
295-298: Ensure atomic cookie operations
The sequence of clearing and setting cookies should be atomic to prevent potential edge cases where requests might be processed between these operations.
Let's check for similar patterns in the codebase:
Line range hint 1-524: Consider architectural improvements for better maintainability
The AuthService has grown quite complex with multiple responsibilities. Consider:
- Extracting token management into a separate service (as planned with TokenRefreshService)
- Creating a dedicated CookieService for consistent cookie handling
- Implementing a more robust error handling strategy with custom error types
This would improve maintainability and make the code more testable.
Let's analyze the current architecture:
apps/services/bff/src/app/services/error.service.ts (2)
81-81: Enhance robustness when extracting errorCode from error
To prevent potential runtime errors, add type checks to ensure error has the expected structure before accessing body.error.
95-95: Avoid replacing errors; consider rethrowing the original error
As previously suggested, rather than throwing a new InternalServerErrorException, rethrow the original error to preserve the stack trace and allow proper handling by the exception middleware.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/services/bff/src/app/services/error.service.ts (1)
47-58: Update JSDoc comments to reflect actual parametersThe JSDoc comment references
params.sid, butsidis not a parameter of thehandleAuthorizedErrormethod. Please update the documentation to accurately reflect the method's parameters.Apply this diff to correct the JSDoc comment:
* @param params Object containing error handling parameters * @param params.res - Express Response object for clearing cookies -* @param params.sid - Session ID of the user * @param params.operation - Name of the operation that failed (e.g., 'get user', 'refresh token') * @param params.error - The error that was caught * @param params.tokenResponseKey - Redis key for the token that needs to be cleaned up
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/services/bff/src/app/services/error.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/bff/src/app/services/error.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 (4)
apps/services/bff/src/app/services/error.service.ts (4)
1-28: Imports and OAuth2 error code definitions are appropriate
The imports are well-organized, and the OAuth2 error codes are accurately defined, aligning with the OAuth2 specification.
30-38: Dependency injection is properly implemented
The ErrorService class correctly uses dependency injection for Logger and CacheService, adhering to NestJS best practices.
43-45: Helper method isKnownOAuth2ErrorCode is correctly implemented
The isKnownOAuth2ErrorCode function accurately checks for known OAuth2 error codes, enhancing error handling efficiency.
59-89: handleAuthorizedError method effectively handles OAuth2 errors
The handleAuthorizedError method systematically manages OAuth2 errors by logging the error, clearing the session cookie, and deleting the cached token. It correctly throws an UnauthorizedException after cleanup, adhering to OAuth2 and NestJS error-handling practices.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
apps/services/bff/src/app/modules/user/user.service.ts (2)
54-65: Enhance type safety for token response handlingConsider using a more explicit type for the cached token response variable initialization.
- let cachedTokenResponse: CachedTokenResponse | null = + let cachedTokenResponse: Readonly<CachedTokenResponse> | null =
74-79: Consider adding specific error types for better error handlingThe error handling could be more specific about the types of errors that can occur during token refresh operations.
- } catch (error) { + } catch (error: unknown) { return this.errorService.handleAuthorizedError({ error, res, tokenResponseKey, operation: `${UserService.name}.getUser`, })apps/services/bff/src/app/modules/auth/token-refresh.service.ts (2)
49-51: Consider decoupling key generation from CacheServiceThe
createTokenResponseKeymethod directly depends on CacheService's implementation details. Consider moving the key format logic to a shared constant or configuration to reduce coupling.- private createTokenResponseKey(sid: string): string { - return this.cacheService.createSessionKeyType('current', sid) + private static readonly TOKEN_KEY_PREFIX = 'current' + private createTokenResponseKey(sid: string): string { + return `${TokenRefreshService.TOKEN_KEY_PREFIX}:${sid}` }
110-125: Consider implementing exponential backoff for pollingThe current polling implementation uses a fixed interval. Consider implementing exponential backoff to reduce load on the system during high concurrency scenarios.
+ private calculateBackoff(attempt: number): number { + return Math.min( + TokenRefreshService.POLL_INTERVAL * Math.pow(2, attempt), + TokenRefreshService.MAX_POLL_TIME + ) + } while (attempts < maxAttempts) { // ... existing polling logic ... - await this.delay(TokenRefreshService.POLL_INTERVAL) + await this.delay(this.calculateBackoff(attempts)) attempts++ }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/services/bff/src/app/modules/auth/token-refresh.service.ts(1 hunks)apps/services/bff/src/app/modules/user/user.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/services/bff/src/app/modules/auth/token-refresh.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.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 (5)
apps/services/bff/src/app/modules/user/user.service.ts (2)
1-2: LGTM! Constructor changes align with service responsibilities
The updated dependencies properly support the token refresh functionality while maintaining NestJS architectural patterns.
Also applies to: 7-7, 9-9, 16-17
33-41: Verify integration testing for concurrent token refresh scenarios
The token refresh implementation looks good, but we should ensure proper integration testing for concurrent scenarios.
Also applies to: 61-65
✅ Verification successful
Integration testing for concurrent token refresh is properly implemented
The codebase already includes comprehensive testing for concurrent token refresh scenarios:
-
In
token-refresh.service.spec.ts:- Tests preventing concurrent refresh token requests with multiple simultaneous calls
- Tests waiting for ongoing refresh and returning cached results
- Tests retry mechanism for polling timeouts
- Tests error handling for refresh token failures
-
In
user.controller.spec.ts:- Tests token refresh behavior when access token is expired
- Tests scenarios with refresh enabled and disabled
The implementation is well-tested for concurrent scenarios and meets the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for integration tests covering concurrent token refresh scenarios
rg -t typescript -l "describe.*token.*refresh" "apps/services/bff/src/**/*.(spec|test).ts"
Length of output: 127
Script:
#!/bin/bash
# Search for test files related to token refresh without file type restriction
rg -l "describe.*token.*refresh" "apps/services/bff/src/**/*.(spec|test).ts"
# Also search for test files containing user service tests
rg -l "describe.*UserService" "apps/services/bff/src/**/*.(spec|test).ts"
# Search for any test files that might test concurrent scenarios
rg -l "concurrent|simultaneous|race.condition" "apps/services/bff/src/**/*.(spec|test).ts"
Length of output: 493
Script:
#!/bin/bash
# First, let's find the correct test file paths
fd ".(spec|test).ts$" apps/services/bff/src/
# Then search within those files for token refresh tests
fd ".(spec|test).ts$" apps/services/bff/src/ -x rg -l "describe.*token.*refresh" {}
# Look for user service tests
fd ".(spec|test).ts$" apps/services/bff/src/ -x rg -l "describe.*UserService" {}
# Search for concurrent testing patterns
fd ".(spec|test).ts$" apps/services/bff/src/ -x rg -l "concurrent|simultaneous|race.condition" {}
Length of output: 817
Script:
#!/bin/bash
# Let's examine the token refresh service tests to see what scenarios are covered
rg -A 10 "describe|it\(" apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts
# Also check the user controller tests for any token refresh related tests
rg -A 10 "describe|it\(" apps/services/bff/src/app/modules/user/user.controller.spec.ts
Length of output: 4855
apps/services/bff/src/app/modules/auth/token-refresh.service.ts (3)
14-25: LGTM! Service setup follows NestJS best practices
The dependency injection is properly implemented with clear separation of concerns.
141-162: LGTM! Cache operations are well implemented
The token validation and cache operations are properly implemented with appropriate error handling and logging.
181-222: LGTM! Public interface is well designed
The method properly handles concurrent requests and implements appropriate retry logic with clear documentation.
Refresh token with polling
What
This feature prevents performance issues that could occur when multiple requests simultaneously attempt to refresh an expired token.
The main logic is in the new token service which manages token refresh operations with built-in concurrency protection.
I have also refactored a bit the enhancedFetch since it was handling error logic which was never reached.
Key features:
The service ensures that when multiple requests need a token refresh:
Why
This prevents multiple consecutive requests from triggering redundant refresh token requests.
NOTE
Comments generated mostly with AI
TODO tests
Checklist:
Summary by CodeRabbit
New Features
TokenRefreshServicefor managing token refresh operations with enhanced error handling.ErrorServicefor centralized management of authorization errors.getCookieOptionsfor flexible cookie configuration.Improvements
login,callbackLogin, andpostRequestmethods.ProxyServiceandUserServicefor better clarity and maintainability.IdsServiceby returning raw data instead of structured responses.UserControllerandUserServicewith updated method signatures.TokenRefreshServiceandUserControllerto ensure robust handling of various scenarios.Bug Fixes