feat(license-service): Barcode session #17541
Conversation
WalkthroughThis pull request introduces comprehensive changes to session management and configuration across multiple libraries and services. The modifications primarily focus on enhancing barcode and license service functionality by adding session key generation, session verification, and configurable expiration times. New methods for session caching, additional configuration parameters, and improved error handling for session-related scenarios have been implemented. The changes span authentication tools, license services, and problem type definitions to support more robust session management. Changes
Sequence DiagramsequenceDiagram
participant User
participant LicenseService
participant BarcodeService
participant CacheManager
User->>LicenseService: Request Barcode
LicenseService->>LicenseService: Generate Session Key
LicenseService->>BarcodeService: Check Existing Session
BarcodeService->>CacheManager: Verify Active Session
alt Session Exists
BarcodeService-->>LicenseService: Throw ProblemError
else No Active Session
LicenseService->>BarcodeService: Create Barcode
BarcodeService->>CacheManager: Store Session
end
Possibly related PRs
Suggested labels
📜 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
libs/auth-nest-tools/src/lib/mock-auth.guard.ts (1)
15-16: Consider meaningful default values for required fields.Empty strings for required fields
sidandsubmight not be ideal for testing scenarios. Consider using meaningful default values that better represent real authentication data.- sid: '', - sub: '', + sid: 'mock-session-id', + sub: 'mock-subject-id',libs/services/license/src/lib/barcode.service.ts (2)
13-13: Consider making session expiry time configurable.While 30 minutes is a reasonable default, consider moving this to the configuration to allow for environment-specific adjustments.
-export const BARCODE_SESSION_EXPIRE_TIME_IN_SEC = 1800 // 30 minutes +export const DEFAULT_BARCODE_SESSION_EXPIRE_TIME_IN_SEC = 1800 // 30 minutes
15-15: Make the session key prefix more specific.Consider using a more specific prefix to avoid potential key collisions in Redis.
-export const BARCODE_ACTIVE_SESSION_KEY = 'activeSession' +export const BARCODE_ACTIVE_SESSION_KEY = 'barcode:activeSession'libs/api/domains/license-service/src/lib/licenseService.service.ts (2)
474-476: Consider making getBarcodeSessionKey privateSince this utility method is only used internally within the service, consider making it private by adding the private modifier.
- getBarcodeSessionKey(licenseType: LicenseType, sub: string) { + private getBarcodeSessionKey(licenseType: LicenseType, sub: string) {
Line range hint
474-541: Consider implementing session cleanup mechanismThe current implementation lacks a mechanism to clean up expired sessions. Consider implementing a background job or using Redis TTL to automatically remove expired sessions and prevent resource exhaustion.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/api/domains/license-service/src/lib/licenseService.service.ts(5 hunks)libs/auth-nest-tools/src/lib/auth.ts(1 hunks)libs/auth-nest-tools/src/lib/current-actor.decorator.ts(1 hunks)libs/auth-nest-tools/src/lib/jwt.payload.ts(1 hunks)libs/auth-nest-tools/src/lib/jwt.strategy.spec.ts(1 hunks)libs/auth-nest-tools/src/lib/jwt.strategy.ts(1 hunks)libs/auth-nest-tools/src/lib/mock-auth.guard.ts(1 hunks)libs/services/license/src/index.ts(1 hunks)libs/services/license/src/lib/barcode.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
libs/services/license/src/index.ts (2)
Pattern libs/services/**/*: "Confirm that the code adheres to the following:
- Shared services and modules that can be utilized across NestJS apps.
- Proper implementation of NestJS providers, including custom providers.
- Adherence to NestJS module encapsulation and isolation principles."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-nest-tools/src/lib/jwt.strategy.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-nest-tools/src/lib/auth.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-nest-tools/src/lib/mock-auth.guard.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-nest-tools/src/lib/current-actor.decorator.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-nest-tools/src/lib/jwt.payload.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-nest-tools/src/lib/jwt.strategy.spec.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/services/license/src/lib/barcode.service.ts (2)
Pattern libs/services/**/*: "Confirm that the code adheres to the following:
- Shared services and modules that can be utilized across NestJS apps.
- Proper implementation of NestJS providers, including custom providers.
- Adherence to NestJS module encapsulation and isolation principles."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (6)
libs/services/license/src/index.ts (1)
8-8: LGTM! Export addition follows module encapsulation principles.The new export
BARCODE_SESSION_EXPIRE_TIME_IN_SECaligns with the session management feature and follows proper naming conventions.libs/auth-nest-tools/src/lib/auth.ts (1)
9-10: Breaking change: Verify impact of required 'sub' property.The change from optional to required
subproperty is a breaking change that could affect existing implementations.Let's verify the impact:
Session ID property addition looks good.
The addition of
sidproperty aligns with the session management feature requirements.libs/auth-nest-tools/src/lib/current-actor.decorator.ts (1)
28-28: LGTM! Session ID integration looks good.The addition of the
sidproperty to the user object is well-implemented and maintains backward compatibility.libs/auth-nest-tools/src/lib/jwt.strategy.ts (1)
58-58: LGTM! JWT strategy properly includes session ID.The session ID is correctly propagated from the JWT payload to the authenticated user object.
libs/services/license/src/lib/barcode.service.ts (1)
102-111: LGTM! Session cache methods are well-implemented.The new session cache methods follow existing patterns and properly handle key prefixing.
libs/auth-nest-tools/src/lib/jwt.strategy.spec.ts (1)
27-28: Add test cases for session ID handling.While the test payload has been updated with
subandsidproperties, there are no specific test cases verifying the session ID handling. Consider adding test cases to verify:
- Session ID propagation
- Missing session ID scenarios
- Invalid session ID handling
libs/api/domains/license-service/src/lib/licenseService.service.ts
Outdated
Show resolved
Hide resolved
libs/api/domains/license-service/src/lib/licenseService.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
541-541:⚠️ Potential issueEnsure asynchronous operation completes.
The call to
this.barcodeService.setSessionCacheshould be awaited to ensure it completes before proceeding.Apply this diff to await the asynchronous call:
await Promise.all([ // Other async operations... - this.barcodeService.setSessionCache(barcodeSessionKey, user.sid), + this.barcodeService.setSessionCache(barcodeSessionKey, user.sid), ])
🧹 Nitpick comments (2)
libs/api/domains/license-service/src/lib/licenseService.service.ts (2)
291-291: Unnecessary indentation change—maintain consistency.The indentation change on line 291 seems unintended and does not affect functionality. Please ensure consistent code formatting throughout the file.
Apply this diff to correct the indentation:
if (!client) { const msg = `Invalid license type. "${type}"` this.logger.warn(msg, { category: LOG_CATEGORY }) - throw new InternalServerErrorException(msg) + throw new InternalServerErrorException(msg) }
496-499: Use appropriate HTTP status code inProblemError.When throwing a
ProblemErrorfor an active session, consider specifying an appropriate HTTP status code (e.g., 429 Too Many Requests or 409 Conflict) to accurately represent the error condition.Apply this diff to include a status code:
throw new ProblemError({ type: ProblemType.BAD_SUBJECT, + status: 409, title: `User has an active session for license type: ${licenseType}`, })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/api/domains/license-service/src/lib/licenseService.service.ts(5 hunks)libs/services/license/src/lib/barcode.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/services/license/src/lib/barcode.service.ts (2)
Pattern libs/services/**/*: "Confirm that the code adheres to the following:
- Shared services and modules that can be utilized across NestJS apps.
- Proper implementation of NestJS providers, including custom providers.
- Adherence to NestJS module encapsulation and isolation principles."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (8)
libs/services/license/src/lib/barcode.service.ts (4)
13-13: Consistent constant naming and documentation.The constant
BARCODE_SESSION_EXPIRE_TIME_IN_SECis correctly named and follows the existing naming conventions. The inline comment provides clarity on the duration.
15-15: ConstantBARCODE_ACTIVE_SESSION_KEYcorrectly introduced.The addition of
BARCODE_ACTIVE_SESSION_KEYenhances the readability of cache key construction and prevents hardcoding the key prefix multiple times.
102-112: Ensure cache keys are unique and collision-free.When constructing cache keys using
BARCODE_ACTIVE_SESSION_KEYandkey, consider potential key collisions. Ifkeycan contain colons or other separators, it might inadvertently overlap with other keys. Ensure thatkeyis appropriately sanitized or structured to prevent this.
Line range hint
91-94: Clarify the type modification insetCachemethod.The
setCachemethod'svalueparameter type has been modified to includestring. If the method is intended to handle bothBarcodeData<Type>andstring, please document the use case. If not, consider reverting to the original type for type safety.libs/api/domains/license-service/src/lib/licenseService.service.ts (4)
51-52: Imports updated correctly for error handling.The addition of
ProblemErrorandProblemTypeimports aligns with the new error handling strategies implemented below.
474-476: MethodgetBarcodeSessionKeyappropriately added.The
getBarcodeSessionKeymethod provides a clear way to generate session keys based on license type andsub. This enhances code maintainability and readability.
487-500: 🛠️ Refactor suggestionAdd error handling for cache operations.
The calls to
getSessionCacheandsetSessionCacheshould include error handling to manage potential cache failures gracefully.Apply this diff to add try-catch blocks:
const barcodeSessionKey = this.getBarcodeSessionKey(licenseType, user.sub) -const activeBarcodeSession = await this.barcodeService.getSessionCache(barcodeSessionKey) +let activeBarcodeSession: string | undefined +try { + activeBarcodeSession = await this.barcodeService.getSessionCache(barcodeSessionKey) +} catch (error) { + this.logger.error('Failed to get session cache', { error, category: LOG_CATEGORY }) + // Decide whether to proceed or rethrow the error +} if (activeBarcodeSession && activeBarcodeSession !== user.sid) { // Existing logic... } // Later in the code... - this.barcodeService.setSessionCache(barcodeSessionKey, user.sid), + try { + await this.barcodeService.setSessionCache(barcodeSessionKey, user.sid) + } catch (error) { + this.logger.error('Failed to set session cache', { error, category: LOG_CATEGORY }) + // Decide on fail-open or fail-closed strategy + }Likely invalid or redundant comment.
487-500: Consider potential issues withuser.sidbeingundefined.When using
user.sidin session management, ensure thatsidis always defined. Ifsidcan beundefined, additional checks or fallbacks may be necessary to prevent issues when setting or comparing session IDs.Run the following script to identify any instances where
user.sidmight beundefined:
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
489-490:⚠️ Potential issueAdd error handling for Redis operations.
The Redis operations (getSessionCache and setSessionCache) should include error handling to gracefully handle cache failures.
- const activeBarcodeSession = await this.barcodeService.getSessionCache(barcodeSessionKey) + try { + const activeBarcodeSession = await this.barcodeService.getSessionCache(barcodeSessionKey) + } catch (error) { + this.logger.error('Failed to get session cache', { + error, + category: LOG_CATEGORY, + }); + // Fail open: allow barcode creation if we can't check the session + } - barcodeSessionKey && user.sid && this.barcodeService.setSessionCache(barcodeSessionKey, user.sid), + barcodeSessionKey && user.sid ? + this.barcodeService.setSessionCache(barcodeSessionKey, user.sid) + .catch(error => { + this.logger.error('Failed to set session cache', { + error, + category: LOG_CATEGORY, + }); + }) + : undefined,Also applies to: 544-544
🧹 Nitpick comments (1)
libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
474-476: Consider using a constant for the key separator.The key generation logic is correct, but using a string literal for the separator could lead to maintenance issues.
+ private readonly BARCODE_SESSION_KEY_SEPARATOR = '-'; + getBarcodeSessionKey(licenseType: LicenseType, sub: string) { - return `${licenseType}-${sub}` + return `${licenseType}${this.BARCODE_SESSION_KEY_SEPARATOR}${sub}` }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/api/domains/license-service/src/lib/licenseService.service.ts(5 hunks)libs/auth-nest-tools/src/lib/auth.ts(1 hunks)libs/auth-nest-tools/src/lib/jwt.payload.ts(1 hunks)libs/auth-nest-tools/src/lib/jwt.strategy.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/auth-nest-tools/src/lib/auth.ts
- libs/auth-nest-tools/src/lib/jwt.payload.ts
- libs/auth-nest-tools/src/lib/jwt.strategy.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/api/domains/license-service/src/lib/licenseService.service.ts (2)
51-52: LGTM! Improved error handling with problem details format.The changes enhance error handling by using the standardized problem details format and maintaining consistent indentation.
Also applies to: 291-291
487-502: LGTM! Robust session management implementation.The implementation effectively prevents users from generating barcodes for licenses on multiple devices in a short timeframe by:
- Checking for existing sessions
- Comparing session IDs
- Using appropriate error handling
Datadog ReportAll test runs ❌ 10 Total Test Services: 1 Failed, 9 Passed Test ServicesThis report shows up to 10 services
❌ Failed Tests (11)
🔻 Code Coverage Decreases vs Default Branch (5) |
libs/api/domains/license-service/src/lib/licenseService.service.ts
Outdated
Show resolved
Hide resolved
libs/api/domains/license-service/src/lib/licenseService.service.ts
Outdated
Show resolved
Hide resolved
move contants to env and refactor barcodeSession check to function
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libs/services/license/src/lib/barcode.service.ts (1)
100-109: 🛠️ Refactor suggestionAdd error handling for cache operations.
The cache operations should include error handling to gracefully handle failures.
Apply this diff to add error handling:
async setSessionCache(key: string, value: string) { + try { return this.cacheManager.set( `${BARCODE_ACTIVE_SESSION_KEY}:${key}`, value, this.config.barcodeSessionExpireTimeInSec * 1000, ) + } catch (error) { + this.logger.error('Failed to set session cache', { error }) + throw error + } } async getSessionCache(key: string): Promise<string | undefined> { + try { return this.cacheManager.get(`${BARCODE_ACTIVE_SESSION_KEY}:${key}`) + } catch (error) { + this.logger.error('Failed to get session cache', { error }) + return undefined + } }libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
553-555: 🛠️ Refactor suggestionAdd error handling for session cache operation.
The session cache operation should include error handling.
Apply this diff to add error handling:
- barcodeSessionKey && - user.sid && - this.barcodeService.setSessionCache(barcodeSessionKey, user.sid), + barcodeSessionKey && user.sid + ? this.barcodeService + .setSessionCache(barcodeSessionKey, user.sid) + .catch((error) => { + this.logger.error('Failed to set session cache', { + error, + category: LOG_CATEGORY, + }) + }) + : undefined,
🧹 Nitpick comments (4)
libs/services/license/src/lib/license.config.ts (2)
8-9: Add JSDoc comments for the new configuration properties.Consider adding documentation to explain the purpose and impact of these timing configurations:
+ /** Time in seconds before a generated barcode expires */ barcodeExpireTimeInSec: z.number(), + /** Time in seconds before a barcode session expires, preventing multiple device access */ barcodeSessionExpireTimeInSec: z.number(),
22-23: Consider adding validation for minimum values.To prevent potential issues with too short expiration times:
- barcodeExpireTimeInSec: env.requiredJSON('BARCODE_EXPIRE_TIME_IN_SEC', 60), - barcodeSessionExpireTimeInSec: env.requiredJSON('BARCODE_SESSION_EXPIRE_TIME_IN_SEC', 1800), + barcodeExpireTimeInSec: Math.max(30, env.requiredJSON('BARCODE_EXPIRE_TIME_IN_SEC', 60)), + barcodeSessionExpireTimeInSec: Math.max(300, env.requiredJSON('BARCODE_SESSION_EXPIRE_TIME_IN_SEC', 1800)),apps/services/license-api/infra/license-api.ts (1)
46-47: Consider using environment-specific configurations.The barcode timing values are currently hardcoded. Consider making them environment-specific like other configurations:
- BARCODE_EXPIRE_TIME_IN_SEC: '60', - BARCODE_SESSION_EXPIRE_TIME_IN_SEC: '1800', + BARCODE_EXPIRE_TIME_IN_SEC: { + dev: '120', // Longer timeout for development + staging: '60', + prod: '60', + }, + BARCODE_SESSION_EXPIRE_TIME_IN_SEC: { + dev: '3600', // Longer timeout for development + staging: '1800', + prod: '1800', + },libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
507-512: Combine session key generation and check into a single operation.The session key generation and check could be combined into a single operation for better readability.
Apply this diff to refactor the code:
- const barcodeSessionKey = user.sub - ? this.getBarcodeSessionKey(licenseType, user.sub) - : undefined - - await this.checkBarcodeSession(barcodeSessionKey, user, licenseType) + if (user.sub) { + await this.checkBarcodeSession( + this.getBarcodeSessionKey(licenseType, user.sub), + user, + licenseType, + ) + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/api/infra/api.ts(1 hunks)apps/services/license-api/infra/license-api.ts(1 hunks)libs/api/domains/license-service/src/lib/licenseService.service.ts(4 hunks)libs/services/license/src/lib/barcode.service.ts(4 hunks)libs/services/license/src/lib/license.config.ts(2 hunks)libs/shared/problem/src/Problem.ts(2 hunks)libs/shared/problem/src/ProblemType.ts(1 hunks)libs/shared/problem/src/problems.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
libs/shared/problem/src/Problem.ts (2)
Pattern libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/shared/problem/src/problems.ts (2)
Pattern libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/services/license-api/infra/license-api.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."
libs/shared/problem/src/ProblemType.ts (2)
Pattern libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/api/infra/api.ts (1)
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."
libs/services/license/src/lib/license.config.ts (2)
Pattern libs/services/**/*: "Confirm that the code adheres to the following:
- Shared services and modules that can be utilized across NestJS apps.
- Proper implementation of NestJS providers, including custom providers.
- Adherence to NestJS module encapsulation and isolation principles."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/services/license/src/lib/barcode.service.ts (2)
Pattern libs/services/**/*: "Confirm that the code adheres to the following:
- Shared services and modules that can be utilized across NestJS apps.
- Proper implementation of NestJS providers, including custom providers.
- Adherence to NestJS module encapsulation and isolation principles."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (5)
libs/shared/problem/src/Problem.ts (1)
8-8: LGTM! Well-structured type definition.The addition of
BadSessionProblemto the Problem type union is clean and follows TypeScript best practices for shared types.Also applies to: 18-18
libs/shared/problem/src/ProblemType.ts (1)
12-12: Verify documentation URL existence.The URL pattern follows the established convention, but we should ensure the documentation exists at the specified path.
✅ Verification successful
Documentation URL verified and accessible
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the documentation exists for the new problem type curl -I https://docs.devland.is/reference/problems/bad-sessionLength of output: 2407
libs/shared/problem/src/problems.ts (1)
34-36: LGTM!The
BadSessionProbleminterface follows the established pattern and correctly extends theBaseProbleminterface.libs/services/license/src/lib/barcode.service.ts (1)
13-13: LGTM!The changes improve configurability by replacing hardcoded values with configuration-based values.
Also applies to: 69-69, 86-86, 97-97
libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
474-476: LGTM!The session key generation follows a consistent format and is well-encapsulated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
charts/islandis/values.staging.yaml (1)
1440-1441: LGTM! Consider documenting the timeout values.The timeout values look reasonable:
- 60-second barcode expiration provides good security
- 30-minute session duration allows adequate user interaction time
Consider adding comments in the YAML file to document:
- The reasoning behind these specific timeout values
- The impact of modifying these values
- Any related configuration values in other services that should be kept in sync
charts/services/api/values.prod.yaml (2)
38-39: Consider moving timeout values to secrets management.The timeout values look reasonable from a security perspective (60s for barcode, 30min for session). However, consider moving these configuration values to the secrets section for better flexibility in managing different environments.
env: - BARCODE_EXPIRE_TIME_IN_SEC: '60' - BARCODE_SESSION_EXPIRE_TIME_IN_SEC: '1800' secrets: + BARCODE_EXPIRE_TIME_IN_SEC: '/k8s/api/BARCODE_EXPIRE_TIME_IN_SEC' + BARCODE_SESSION_EXPIRE_TIME_IN_SEC: '/k8s/api/BARCODE_SESSION_EXPIRE_TIME_IN_SEC'
38-39: Add documentation for timeout values.Please add comments explaining the purpose and impact of these timeout values to help other developers understand their significance in the barcode session management feature.
env: + # Maximum lifetime of a generated barcode in seconds BARCODE_EXPIRE_TIME_IN_SEC: '60' + # Maximum duration of a barcode creation session in seconds BARCODE_SESSION_EXPIRE_TIME_IN_SEC: '1800'charts/services/api/values.staging.yaml (1)
38-39: LGTM! Configuration maintains consistency across services and environments.The timeout values are consistent with:
- Other environments (dev, staging, prod)
- The license-api service configuration
This ensures reliable behavior of the barcode session management feature throughout the system.
Consider documenting these timeout values in the service's technical documentation to help other developers understand the session management behavior.
charts/islandis/values.prod.yaml (1)
277-278: LGTM! Consider documenting timeout rationales.The timeout values look reasonable:
- 60 seconds for barcode expiration provides sufficient time for legitimate use
- 30 minutes for session expiration balances usability with security
Consider adding comments to document why these specific timeout values were chosen, to help future maintainers understand the rationale.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
charts/islandis/values.dev.yaml(2 hunks)charts/islandis/values.prod.yaml(2 hunks)charts/islandis/values.staging.yaml(2 hunks)charts/services/api/values.dev.yaml(1 hunks)charts/services/api/values.prod.yaml(1 hunks)charts/services/api/values.staging.yaml(1 hunks)charts/services/license-api/values.dev.yaml(1 hunks)charts/services/license-api/values.prod.yaml(1 hunks)charts/services/license-api/values.staging.yaml(1 hunks)
🔇 Additional comments (5)
charts/services/license-api/values.prod.yaml (1)
22-23: Verify the timeout values align with security requirements.The timeout values look reasonable:
BARCODE_EXPIRE_TIME_IN_SEC: '60'provides a short window that helps prevent barcode reuseBARCODE_SESSION_EXPIRE_TIME_IN_SEC: '1800'(30 minutes) balances security with user experiencePlease confirm these timeout values have been approved by the security team, especially for the production environment.
charts/services/license-api/values.dev.yaml (1)
22-23: LGTM! Configuration matches production environment.Good practice to maintain consistent timeout values across environments. This helps catch timing-related issues during development.
charts/services/license-api/values.staging.yaml (1)
22-23: LGTM! Configuration matches other environments.Maintaining consistent timeout values across all environments (dev, staging, prod) is a good practice.
charts/services/api/values.dev.yaml (1)
38-39: LGTM! Configuration aligns with license-api service.Good practice to maintain consistent timeout values between the API and license-api services to ensure proper integration.
charts/islandis/values.dev.yaml (1)
289-290: LGTM! Environment parity maintained.The timeout values match production, maintaining consistency across environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/services/license-api/src/app/modules/license/test/license.service.spec.ts (1)
381-381: Consider adding validation for the expiration time.While using the configured expiration time is good, consider adding an assertion to validate that
config.barcodeExpireTimeInSecis positive before using it in the test. This would help catch configuration issues early.+ expect(config.barcodeExpireTimeInSec).toBeGreaterThan(0) jest.advanceTimersByTime(config.barcodeExpireTimeInSec * 1000)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/services/license-api/src/app/modules/license/test/license.service.spec.ts(3 hunks)libs/services/license/src/index.ts(0 hunks)
💤 Files with no reviewable changes (1)
- libs/services/license/src/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/license-api/src/app/modules/license/test/license.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."
🔇 Additional comments (2)
apps/services/license-api/src/app/modules/license/test/license.service.spec.ts (2)
228-228: LGTM! Config type is properly declared.The config variable is correctly typed using NestJS's ConfigType utility.
297-298: LGTM! Config is properly initialized.The config is correctly retrieved from the test module using the appropriate configuration key.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
charts/islandis-services/api/values.dev.yaml(1 hunks)charts/islandis-services/api/values.prod.yaml(1 hunks)charts/islandis-services/api/values.staging.yaml(1 hunks)charts/islandis-services/license-api/values.dev.yaml(1 hunks)charts/islandis-services/license-api/values.prod.yaml(1 hunks)charts/islandis-services/license-api/values.staging.yaml(1 hunks)
🔇 Additional comments (7)
charts/islandis-services/license-api/values.prod.yaml (1)
22-23: LGTM! The timeout values are reasonable.The barcode expiration (60s) and session expiration (1800s) times align well with the PR objectives. The barcode expires quickly to prevent reuse, while the session timeout provides a reasonable window for user interaction.
charts/islandis-services/api/values.prod.yaml (1)
38-39: LGTM! Values are consistent with license-api configuration.The timeout values match those in the license-api service, ensuring consistent behavior across services.
charts/islandis-services/license-api/values.dev.yaml (3)
23-23: LGTM! Session expiration time is reasonable.30-minute session duration is a good balance between security and user experience.
Line range hint
1-5: Important: This file is auto-generated.As per the warning comment, this file should not be edited manually. Please ensure you're following the correct process by running
yarn chartsto generate these values.Please confirm the source of these configurations and ensure they're being managed through the proper configuration pipeline.
22-22: Consider increasing the barcode expiration time.60 seconds might be too short for users to successfully scan their barcode, potentially leading to a poor user experience. Consider increasing this value based on real-world usage patterns.
Run this script to check barcode expiration times across environments:
charts/islandis-services/api/values.staging.yaml (1)
38-39: Review the timeout values for business requirements alignment.The new environment variables introduce timeouts for barcode management:
BARCODE_EXPIRE_TIME_IN_SEC: '60'- Barcodes expire after 1 minuteBARCODE_SESSION_EXPIRE_TIME_IN_SEC: '1800'- Sessions expire after 30 minutesWhile these values seem reasonable (short-lived barcodes for security, comfortable session duration), please verify:
- Is 1 minute sufficient for users to use the barcode?
- Does the 30-minute session timeout align with other session timeouts in the system?
charts/islandis-services/license-api/values.staging.yaml (1)
22-23: Consider managing timeout values through secrets.While hardcoded values are acceptable for staging, consider:
- Moving these values to secrets for better security and flexibility
- Adding documentation about the purpose and relationship between these timeouts
- Evaluating if different license types need different timeout configurations
Let's verify if any documentation exists about these timeouts:
✅ Verification successful
Hardcoded timeout values are appropriate, no changes needed.
The current implementation:
- Uses short-lived tokens (60s) for security with proper error handling
- Maintains good UX with reasonable session duration (30min)
- Has consistent values across environments by design
- Includes proper validation and type safety
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for documentation about the timeout configuration rg -i "barcode.*expire.*time|session.*expire.*time" README.md docs/Length of output: 115
Script:
#!/bin/bash # Find all markdown files echo "=== Markdown files ===" fd -e md # Search for related configuration files echo -e "\n=== Related configuration files ===" fd -e yaml -e yml | grep -i "license\|barcode" # Search for usage of these environment variables echo -e "\n=== Environment variable usage ===" rg "BARCODE_.*EXPIRE.*TIME" --type-add 'yaml:*.{yaml,yml}' --type yamlLength of output: 24073
Script:
#!/bin/bash # Search for code using these environment variables echo "=== TypeScript/JavaScript usage ===" rg "BARCODE_.*EXPIRE.*TIME" --type ts --type js -A 2 -B 2 # Look for related barcode implementation echo -e "\n=== Barcode related code ===" rg "barcode.*expire|expire.*barcode" --type ts --type js -i -A 2 -B 2Length of output: 12224
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/services/license/src/lib/license.config.ts (1)
8-9: Add documentation for the new configuration properties.Consider adding JSDoc comments to document:
- The purpose of these timeouts
- The units (seconds)
- The implications of these values on the system behavior
+ /** + * Time in seconds before a generated barcode expires + * @default 60 + */ barcodeExpireTimeInSec: z.number(), + /** + * Time in seconds before a barcode session expires + * Used to prevent multiple active sessions for the same license type + * @default 1800 + */ barcodeSessionExpireTimeInSec: z.number(),libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
Line range hint
511-559: Consider grouping session-related methods together.The session management methods (
getBarcodeSessionKey,checkBarcodeSession) should be moved closer to where they are used increateBarcode.Move the session management methods just above the
createBarcodemethod for better code organization.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/services/license-api/src/app/modules/license/test/license.service.spec.ts(3 hunks)libs/api/domains/license-service/src/lib/licenseService.service.ts(4 hunks)libs/services/license/src/lib/barcode.service.ts(4 hunks)libs/services/license/src/lib/license.config.ts(2 hunks)libs/shared/problem/src/Problem.ts(2 hunks)libs/shared/problem/src/problems.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/shared/problem/src/problems.ts
- libs/shared/problem/src/Problem.ts
- apps/services/license-api/src/app/modules/license/test/license.service.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
libs/services/license/src/lib/license.config.ts (2)
Pattern libs/services/**/*: "Confirm that the code adheres to the following:
- Shared services and modules that can be utilized across NestJS apps.
- Proper implementation of NestJS providers, including custom providers.
- Adherence to NestJS module encapsulation and isolation principles."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/services/license/src/lib/barcode.service.ts (2)
Pattern libs/services/**/*: "Confirm that the code adheres to the following:
- Shared services and modules that can be utilized across NestJS apps.
- Proper implementation of NestJS providers, including custom providers.
- Adherence to NestJS module encapsulation and isolation principles."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (5)
libs/services/license/src/lib/barcode.service.ts (3)
13-13: LGTM! Well-defined constant for session key prefix.The constant name is descriptive and follows the naming convention.
Line range hint
69-87: LGTM! Good refactor to use configuration values.The change from hardcoded values to configuration-based values improves flexibility and maintainability.
105-114: 🛠️ Refactor suggestionAdd error handling for Redis operations.
The session cache methods should handle potential Redis failures gracefully.
async setSessionCache(key: string, value: string) { + try { return this.cacheManager.set( `${BARCODE_ACTIVE_SESSION_KEY}:${key}`, value, this.config.barcodeSessionExpireTimeInSec * 1000, ) + } catch (error) { + this.logger.error('Failed to set session cache', { error }) + throw error + } } async getSessionCache(key: string): Promise<string | undefined> { + try { return this.cacheManager.get(`${BARCODE_ACTIVE_SESSION_KEY}:${key}`) + } catch (error) { + this.logger.error('Failed to get session cache', { error }) + return undefined + } }Likely invalid or redundant comment.
libs/api/domains/license-service/src/lib/licenseService.service.ts (2)
474-476: LGTM! Clear and concise key generation.The method follows single responsibility principle and is well-implemented.
478-500: LGTM! Robust session validation with proper error handling.The implementation:
- Properly validates session ownership
- Uses appropriate error type (ProblemError)
- Includes helpful logging
|
@magnearun can we get this to main tomorrow? 🙏 |
What
Store in Redis session id when user creates a barcode
Check when creating a barcode if there is a key in redis for this user, license type, if the session id is the same we create the barcode, else we do not.
Why
We do not want users to be able to request a barcode for a licence on different devices within a short amount of time.
This will prevent users from being able to for example request a barcode for their driving license to get into a bar and the immediately login to their island.is app on another persons phone and request a barcode again for that person to "scam" their way in.
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Configuration Changes
Bug Fixes