feat: ✨ add auth and aws config #91
Conversation
WalkthroughThe pull request introduces several enhancements to the configuration management of a NestJS application. It adds a new environment variable Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (2)libs/users/interface-adapters/src/lib/users.module.ts (2)
The change from absolute to relative import path is a good practice as it makes the module more maintainable and helps prevent circular dependencies.
The addition of UsersService to the exports array suggests that other modules need to consume users functionality directly. While this is valid, it could lead to tight coupling if not managed carefully. Let's verify how this service is being used across the codebase: ✅ Verification successfulLet me gather more specific information about how UsersService is being used in the auth service, as this appears to be the main external consumer. Let me check one final aspect to see how the AuthModule is importing the UsersService. Exporting UsersService is necessary and correctly implemented The export of UsersService is justified as it's being used as a critical dependency in the AuthService for user authentication flow. The AuthService uses UsersService.findByEmail() during the sign-in process to validate and retrieve user information after AWS Cognito authentication. This is a legitimate use case for cross-module service usage in a NestJS application. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for UsersService usage across the codebase
# Expected results: Find imports and usages of UsersService to validate the export necessity
# Check for UsersService imports
echo "Checking for UsersService imports..."
rg -l "import.*UsersService"
# Check for UsersModule imports to see which modules depend on it
echo "Checking for UsersModule imports..."
rg -l "import.*UsersModule"
# Look for dependency injection of UsersService
echo "Checking for UsersService injection..."
ast-grep --pattern 'constructor($$$, private readonly usersService: UsersService, $$$)'
Length of output: 799 Script: #!/bin/bash
# Check the actual usage of UsersService in auth.service.ts
echo "Checking UsersService usage in auth.service.ts..."
rg -A 5 "UsersService" libs/auth/application/src/lib/auth.service.ts
# Check if AuthModule imports UsersModule
echo "Checking AuthModule imports..."
rg "UsersModule" libs/auth/interface-adapters/src/lib/auth.module.ts
# Check if there are any other potential consumers
echo "Checking for any other potential service injections..."
ast-grep --pattern 'private readonly usersService: UsersService'
Length of output: 874 Script: #!/bin/bash
# Check the AuthModule's imports and dependencies
echo "Checking AuthModule content..."
cat libs/auth/interface-adapters/src/lib/auth.module.ts
# Check if there are any direct usages of UsersService in auth.service implementation
echo "Checking auth.service implementation..."
cat libs/auth/application/src/lib/auth.service.ts
Length of output: 1783 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
libs/shared/config/src/lib/aws.config.ts (1)
4-9: Consider strengthening the AWS configuration validation.While the schema ensures non-empty strings, consider adding more specific validation:
regionshould match AWS region pattern (e.g., 'us-east-1')cognitoClientIdtypically follows a specific formatconst awsSchema = z.object({ - region: z.string().min(1), + region: z.string().regex(/^[a-z]{2}-[a-z]+-\d{1}$/), accessKeyId: z.string().min(1), secretAccessKey: z.string().min(1), - cognitoClientId: z.string().min(1), + cognitoClientId: z.string().regex(/^[a-zA-Z0-9]+$/), });libs/shared/config/src/lib/auth.config.spec.ts (2)
22-26: Improve test error case handling and avoid delete operator.The test correctly verifies error throwing, but could be improved:
- Replace
deletewith undefined assignment (performance improvement)- Add specific error message verification
it('should throw error if JWT_SECRET is not set', () => { - delete process.env['JWT_SECRET']; + process.env['JWT_SECRET'] = undefined; - expect(() => authConfig()).toThrow(); + expect(() => authConfig()).toThrow('Invalid auth config'); });🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
3-20: Add more test coverage for auth config.Consider adding tests for:
- Invalid JWT secret formats
- Edge cases (empty string, whitespace-only)
Would you like me to help generate additional test cases?
libs/shared/config/src/lib/applications.config.ts (2)
Line range hint
13-19: Consider environment-specific default values.The hardcoded default values might not be suitable for all environments.
Consider:
- Moving defaults to environment-specific configuration files
- Adding validation for development-only defaults
- Documenting the expected port ranges in comments
Line range hint
21-42: Enhance configuration type safety and validation.While the implementation is functional, consider these improvements:
- Add environment-specific validation
- Validate protocol against allowed values
- Add host format validation
+const protocolSchema = z.enum(['http', 'https']); + const serviceSchema = z.object({ - protocol: z.string().min(1), + protocol: protocolSchema, host: z.string().min(1), port: z.coerce.number().int().min(1).max(65535), name: z.string().optional(), }); +const isProduction = process.env.NODE_ENV === 'production'; + export const gatewayConfig = registerAs( 'gateway', (): ServiceConfig => { + if (isProduction && process.env['PROTOCOL'] === 'http') { + throw new Error('HTTP protocol not allowed in production'); + } return serviceSchema.parse({ protocol: process.env['PROTOCOL'] ?? DEFAULT_PROTOCOL, host: process.env['GATEWAY_HOST'] ?? DEFAULT_HOST, port: process.env['GATEWAY_PORT'] ? Number(process.env['GATEWAY_PORT']) : DEFAULT_PORT.gateway, }); }, );libs/shared/config/src/lib/aws.config.spec.ts (1)
28-50: Consider enhancing error case validation.While the error cases are well-covered, consider validating specific error messages to ensure proper error handling.
- expect(() => awsConfig()).toThrow(); + expect(() => awsConfig()).toThrow('AWS_REGION is required');🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 47-47: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
libs/shared/infrastructure/aws-cognito/src/lib/aws-cognito.service.ts (3)
9-9: Remove commented code.Remove the commented line as it's no longer needed and can cause confusion.
- // private readonly configService: ConfigService;
27-27: Consider caching the AWS config.The config is retrieved repeatedly in each method. Consider caching it as a class property during initialization to reduce overhead.
export class AwsCognitoService { private readonly logger = new Logger(AwsCognitoService.name); private cognito: AWS.CognitoIdentityServiceProvider; + private readonly awsConfig: ReturnType<ConfigService['get']>['aws']; constructor(private readonly configService: ConfigService) { - const awsConfig = this.configService.get('aws'); + this.awsConfig = this.configService.get('aws'); this.cognito = new AWS.CognitoIdentityServiceProvider({ - region: awsConfig.region, - accessKeyId: awsConfig.accessKeyId, - secretAccessKey: awsConfig.secretAccessKey, + region: this.awsConfig.region, + accessKeyId: this.awsConfig.accessKeyId, + secretAccessKey: this.awsConfig.secretAccessKey, }); }Then use
this.awsConfiginstead of retrieving it in each method.Also applies to: 50-50, 71-71, 89-89
Line range hint
31-36: Consider adding email format validation.While AWS Cognito handles basic validation, consider adding email format validation before making the API call to provide better user feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
.env.sample(1 hunks)apps/users/src/app/app.module.ts(2 hunks)libs/shared/config/src/index.ts(1 hunks)libs/shared/config/src/lib/applications.config.ts(1 hunks)libs/shared/config/src/lib/auth.config.spec.ts(1 hunks)libs/shared/config/src/lib/auth.config.ts(1 hunks)libs/shared/config/src/lib/aws.config.spec.ts(1 hunks)libs/shared/config/src/lib/aws.config.ts(1 hunks)libs/shared/config/src/lib/database.config.ts(1 hunks)libs/shared/infrastructure/aws-cognito/src/lib/aws-cognito.service.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env.sample
🧰 Additional context used
📓 Learnings (1)
libs/shared/infrastructure/aws-cognito/src/lib/aws-cognito.service.ts (1)
Learnt from: zhumeisongsong
PR: zhumeisongsong/graphql-federation-workspace#76
File: libs/shared/infrastructure/aws-cognito/src/lib/aws-cognito.service.ts:20-27
Timestamp: 2024-11-29T02:25:16.648Z
Learning: In the `AwsCognitoService` class (`libs/shared/infrastructure/aws-cognito/src/lib/aws-cognito.service.ts`), explicit validation of environment variables in the constructor is not required as per the developer's preference.
🪛 Biome (1.9.4)
libs/shared/config/src/lib/aws.config.spec.ts
[error] 31-31: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 47-47: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
libs/shared/config/src/lib/auth.config.spec.ts
[error] 23-23: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (11)
libs/shared/config/src/lib/database.config.ts (2)
12-12: LGTM! Good addition of return type annotation.
The explicit return type annotation improves type safety and makes the code more maintainable.
Line range hint 13-17: Consider environment-specific validation for database configuration.
The current implementation uses default values (localhost, 27017, main) when environment variables are not set. While this is convenient for development, it could lead to unexpected behavior in production if environment variables are accidentally unset.
Let's check if there are any environment-specific configurations:
Consider adding strict validation for production environments:
export const databaseConfig = registerAs('database', (): DatabaseConfig => {
+ const nodeEnv = process.env.NODE_ENV;
+
const config = {
- host: process.env['DATABASE_HOST'] || 'localhost',
- port: process.env['DATABASE_PORT'] || 27017,
- name: process.env['DATABASE_NAME'] || 'main',
+ host: process.env['DATABASE_HOST'] || (nodeEnv === 'production' ? undefined : 'localhost'),
+ port: process.env['DATABASE_PORT'] || (nodeEnv === 'production' ? undefined : 27017),
+ name: process.env['DATABASE_NAME'] || (nodeEnv === 'production' ? undefined : 'main'),
};
return databaseSchema.parse(config);
});libs/shared/config/src/index.ts (1)
3-4: LGTM! New config modules are properly exported
The new AWS and auth configuration exports are correctly added to the barrel file.
apps/users/src/app/app.module.ts (3)
3-8: LGTM! Imports are properly organized
The config imports are correctly structured using the barrel file exports.
17-19: Verify global config module usage
The ConfigModule is marked as global. Ensure this is intentional and document the reasoning to prevent potential issues with module isolation.
✅ Verification successful
Let me gather more information about the config usage to better understand the architecture.
Based on the gathered information, I can now provide a final response:
Global ConfigModule usage is justified in this architecture
The global ConfigModule usage is intentional and follows a well-structured pattern across the application:
- It's used in the main app modules (gateway, users) to provide configuration for the entire application
- The shared infrastructure modules (mongoose, aws-cognito) properly consume the ConfigService for their specific needs
- The architecture documentation shows a clear separation of concerns where infrastructure layer handles technical configurations
This setup aligns with NestJS best practices for configuration management in a modular monorepo structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other global module declarations
rg "isGlobal:\s*true" -g "*.ts"
# Check for direct config imports in feature modules
rg "ConfigModule" -g "*.module.ts"
Length of output: 845
Script:
#!/bin/bash
# Check how the config values are consumed across the codebase
rg "ConfigService" -A 3 -g "*.ts"
# Check for any documentation about the config architecture
fd "README" --exec cat {}
Length of output: 13342
18-18: Consider config loading order
While the configuration loading looks correct, consider documenting or verifying if there are any dependencies between these configs that would require a specific loading order.
✅ Verification successful
Based on the analysis of the shell script results, I can now provide a final response about the configuration loading order:
No specific config loading order dependencies found
The codebase analysis shows that while these configs are used together, there are no direct dependencies between them:
databaseConfig: Standalone MongoDB connection settingsawsConfig: Independent AWS/Cognito credentials and settingsauthConfig: Simple JWT secret configurationuserAppConfig: Basic service configuration (protocol, host, port)
Each config module is independently registered and validated using Zod schemas, with no cross-references between them. The order in the load array won't impact functionality since:
- Each config loads from distinct environment variables
- The configs are consumed independently by their respective services
- No config initialization depends on values from other configs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cross-config dependencies
rg "config\(\)" -A 5 -g "*.ts" | grep -E "database|aws|auth|userApp"
Length of output: 70
Script:
#!/bin/bash
# Search for config files and their content
fd -e ts config$ | xargs cat
Length of output: 32
Script:
#!/bin/bash
# Search for config usage patterns
rg -A 5 "userAppConfig|databaseConfig|awsConfig|authConfig" -g "*.ts"
Length of output: 20480
Script:
#!/bin/bash
# Search for configuration dependencies using ast-grep
ast-grep --pattern 'const $_ = registerAs($_, $_)'
Length of output: 3793
Script:
#!/bin/bash
# Look for configuration registration and dependencies
rg "registerAs|ConfigModule.forRoot" -g "*.ts" -A 5
Length of output: 5623
libs/shared/config/src/lib/applications.config.ts (1)
4-9: LGTM! Well-structured service schema validation.
The schema correctly validates:
- Required string fields with minimum length
- Port number range with proper type coercion
- Optional name field
libs/shared/config/src/lib/aws.config.spec.ts (2)
3-13: LGTM! Well-structured test setup.
The test setup properly manages environment variables with appropriate beforeEach and afterAll hooks.
15-26: LGTM! Comprehensive happy path test.
The test properly validates all required AWS configuration fields.
libs/shared/infrastructure/aws-cognito/src/lib/aws-cognito.service.ts (2)
12-20: LGTM! Proper AWS configuration initialization.
The service correctly initializes the AWS Cognito provider with configuration from ConfigService.
17-19: Verify AWS SDK configuration.
Let's verify that the AWS SDK is properly configured across the codebase.
✅ Verification successful
Let me gather more information about AWS configuration patterns in the codebase.
Let me check the content of the AWS config file to ensure it matches the configuration being used.
AWS SDK configuration is properly implemented and consistent
The AWS configuration is well-structured with:
- Centralized config in
libs/shared/config/src/lib/aws.config.ts - Environment variables validation using Zod schema
- Consistent usage across the codebase through NestJS ConfigService
- Proper AWS SDK initialization in the Cognito service
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other AWS SDK configurations to ensure consistency
rg -l "new AWS\." | while read -r file; do
echo "=== $file ==="
rg -A 5 "new AWS\." "$file"
done
Length of output: 502
Script:
#!/bin/bash
# Search for AWS configuration patterns and imports
echo "=== Searching for AWS config imports and usage ==="
rg -A 3 "import.*AWS"
echo -e "\n=== Searching for AWS configuration patterns ==="
rg -A 3 "AWS\.config"
echo -e "\n=== Searching for aws-config files ==="
fd -g "*aws*config*"
echo -e "\n=== Checking environment configurations ==="
rg "AWS_"
Length of output: 2722
Script:
#!/bin/bash
# Check AWS config implementation
echo "=== AWS Config Implementation ==="
cat libs/shared/config/src/lib/aws.config.ts
echo -e "\n=== Checking AWS Config Usage ==="
rg -A 3 "awsConfig"
Length of output: 7447
|
🎉 This PR is included in version 1.8.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Related #71
Summary by CodeRabbit
Release Notes
New Features
JWT_SECRETfor enhanced security.users.moduleavailable for import in other parts of the application.Improvements
AwsCognitoServiceto utilize injected configuration settings instead of direct environment variable access.UsersServiceto the exports of theUsersModulefor broader accessibility.Bug Fixes
Tests