refactor: ♻️ auth module #126
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request implements a refactoring of the authentication module, shifting from the Changes
Sequence DiagramsequenceDiagram
participant Resolver as AuthResolver
participant UseCase as SignInUseCase
participant Cognito as AwsCognitoService
participant JWT as JwtService
Resolver->>UseCase: execute(email, password)
UseCase->>Cognito: signIn(email, password)
alt Successful Sign-In
Cognito-->>UseCase: Cognito User
UseCase->>JWT: signAsync(user details)
JWT-->>UseCase: Access Token
UseCase-->>Resolver: Access Token
else Sign-In Failure
Cognito--xUseCase: UnauthorizedException
UseCase--xResolver: UnauthorizedException
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (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
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: 1
🔭 Outside diff range comments (4)
libs/auth/application/src/lib/use-cases/sign-in.use-case.ts (1)
Add Cognito user ID from authentication response to JWT payload
The Cognito
initiateAuthresponse fromsignInmethod contains the user's sub (unique identifier) in the ID token claims. You need to decode this token and use the sub claim in the JWT payload.
- In
SignInUseCase, extract the ID token from Cognito authentication response and decode it to get the user's sub- Use this sub as the user ID in the JWT payload instead of the commented TODO
🔗 Analysis chain
Line range hint
26-29: Add Cognito user ID to JWT payloadThe TODO comment indicates missing user ID in the JWT payload. This is important for user identification.
Let me gather more information about the authentication flow and Cognito integration.
Let me check how the Cognito authentication flow is implemented to see if we can get the user ID.
Let me check the complete implementation of the sign-in use case and the Cognito service response.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Cognito ID is used elsewhere rg "sub:|userId:|user.id"Length of output: 3427
Script:
#!/bin/bash # Check for Cognito-related code and authentication rg -A 3 "cognito|CognitoUser|authenticate|getUser"Length of output: 17363
Script:
#!/bin/bash # Check the sign-in use case implementation and related authentication code rg -A 10 "class SignInUseCase"Length of output: 949
Script:
#!/bin/bash # Get the complete implementation of sign-in use case and Cognito authentication cat libs/auth/application/src/lib/use-cases/sign-in.use-case.ts cat libs/shared/infrastructure/aws-cognito/src/lib/aws-cognito.service.tsLength of output: 3958
libs/shared/infrastructure/aws-cognito/src/lib/aws-cognito.service.ts (3)
Line range hint
33-38: Improve error handling in AWS operationsThe current try-catch blocks simply rethrow errors without transformation. Consider:
- Mapping AWS-specific errors to domain-specific exceptions
- Adding error context for better debugging
Example implementation:
try { return await this.cognito.signUp(params).promise(); } catch (error) { if (error.code === 'UsernameExistsException') { throw new ConflictException('User already exists'); } if (error.code === 'InvalidPasswordException') { throw new BadRequestException('Password does not meet requirements'); } throw new InternalServerErrorException('Failed to sign up user'); }Also applies to: 47-51, 63-67, 82-86
Line range hint
9-19: Add AWS configuration validationThe service should validate AWS configuration during initialization to fail fast if required configuration is missing.
constructor(private readonly configService: ConfigService) { const awsConfig = this.configService.get('aws'); + + if (!awsConfig?.region || !awsConfig?.accessKeyId || !awsConfig?.secretAccessKey) { + throw new Error('Missing required AWS configuration'); + } this.cognito = new AWS.CognitoIdentityServiceProvider({ region: awsConfig.region, accessKeyId: awsConfig.accessKeyId, secretAccessKey: awsConfig.secretAccessKey, }); }
Email verification is not implemented and needs to be addressed
The codebase shows a TODO comment in the AWS Cognito service indicating that email verification is not implemented yet. This is a security concern as email verification helps prevent unauthorized account creation and ensures user authenticity.
libs/shared/infrastructure/aws-cognito/src/lib/aws-cognito.service.ts: Theemail_verifiedattribute is currently commented out with a TODO note.🔗 Analysis chain
Line range hint
29-29: Consider implementing email verificationThe TODO comment about email verification should be addressed as it's a security best practice.
Would you like me to help create a GitHub issue to track the implementation of email verification?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if email verification is implemented elsewhere rg -l "email_verified"Length of output: 92
Script:
#!/bin/bash # Let's check the actual implementation in the aws-cognito service rg "email_verified" -A 5 -B 5Length of output: 1149
🧹 Nitpick comments (7)
libs/auth/application/src/lib/use-cases/sign-in.use-case.ts (2)
13-18: Consider adding input validationThe execute method should validate email format before attempting sign-in.
Consider adding:
async execute( email: string, pass: string, ): Promise<{ accessToken: string; }> { + if (!email || !this.isValidEmail(email)) { + throw new BadRequestException('Invalid email format'); + }
6-6: Document the use case responsibilityAdding class-level documentation would help explain the single responsibility of this use case.
Consider adding:
@Injectable() +/** + * SignInUseCase handles user authentication through AWS Cognito + * and generates a JWT token upon successful authentication. + */ export class SignInUseCase {libs/auth/interface-adapters/src/lib/resolver/auth.resolver.spec.ts (2)
36-39: Consider using more realistic test dataThe test email and password are quite basic. Consider using constants that better reflect your password policy requirements and email format restrictions.
const signInInput: SignInInputDto = { - email: '[email protected]', - password: 'password123', + email: '[email protected]', + password: 'P@ssw0rd!2023' };
57-69: Consider adding more edge casesThe error handling test is good, but consider adding tests for:
- Empty credentials
- Malformed email
- Password that doesn't meet complexity requirements
libs/auth/application/src/lib/use-cases/sign-in.use-case.spec.ts (2)
52-59: Enhance error case assertionsThe test verifies that UnauthorizedException is thrown but doesn't validate the error message. Consider asserting the specific error message to ensure proper error communication.
await expect(signInUseCase.execute(email, password)).rejects.toThrow( UnauthorizedException, ); +await expect(signInUseCase.execute(email, password)).rejects.toThrow( + 'Invalid credentials' +);
36-40: Add input validation testsConsider adding test cases for:
- Empty email/password
- Invalid email format
- Password not meeting minimum requirements
libs/shared/infrastructure/aws-cognito/src/lib/aws-cognito.service.ts (1)
7-7: Remove or implement commented codeThe commented configService declaration should either be removed or implemented if needed.
- // private readonly configService: ConfigService;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
libs/auth/application/src/index.ts(1 hunks)libs/auth/application/src/lib/auth.service.spec.ts(0 hunks)libs/auth/application/src/lib/use-cases/sign-in.use-case.spec.ts(1 hunks)libs/auth/application/src/lib/use-cases/sign-in.use-case.ts(1 hunks)libs/auth/interface-adapters/src/lib/auth.module.ts(3 hunks)libs/auth/interface-adapters/src/lib/resolver/auth.resolver.spec.ts(1 hunks)libs/auth/interface-adapters/src/lib/resolver/auth.resolver.ts(2 hunks)libs/shared/infrastructure/aws-cognito/src/lib/aws-cognito.service.ts(1 hunks)
💤 Files with no reviewable changes (1)
- libs/auth/application/src/lib/auth.service.spec.ts
✅ Files skipped from review due to trivial changes (1)
- libs/auth/application/src/index.ts
🔇 Additional comments (4)
libs/auth/interface-adapters/src/lib/resolver/auth.resolver.ts (2)
15-15: LGTM! Clean dependency injection of SignInUseCase
The constructor properly injects the new SignInUseCase, maintaining the dependency injection pattern.
22-22: Verify error handling in GraphQL context
The change from signIn to execute looks good, but we should ensure that UnauthorizedException from SignInUseCase is properly transformed into a GraphQL error.
Consider adding a GraphQL error interceptor if not already present, to properly format authentication errors for GraphQL clients.
✅ Verification successful
UnauthorizedException is properly handled in the GraphQL context
The verification shows that:
- The SignInUseCase properly throws UnauthorizedException in both authentication failure scenarios (in
sign-in.use-case.ts) - NestJS automatically transforms exceptions into GraphQL errors by default, and no special handling is needed
- The auth resolver's test (
auth.resolver.spec.ts) confirms the UnauthorizedException is properly propagated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GraphQL error handling setup
rg -l "GraphQLError|UnauthorizedException.*GraphQL"
Length of output: 53
Script:
#!/bin/bash
# Let's search for error handling patterns in the resolver and related files
rg -A 5 "catch.*Exception|@UseFilters|ExceptionFilter" --type ts
# Also search for any error handling setup in main.ts or similar bootstrap files
fd "main.ts|bootstrap.ts|app.module.ts" --exec rg -A 5 "useGlobalFilters|ExceptionFilter"
# Look for the SignInUseCase implementation to understand error handling
ast-grep --pattern 'class $_ implements SignInUseCase {
$$$
execute($$$) {
$$$
}
}'
Length of output: 254
Script:
#!/bin/bash
# Let's try a broader search for error handling patterns
rg -A 5 "throw.*Exception|catch|@Catch" --type ts
# Search for NestJS exception filter patterns
rg -A 5 "implements.*ExceptionFilter" --type ts
# Look for GraphQL specific error handling
rg -A 5 "@nestjs/graphql.*Error" --type ts
# Check the auth resolver implementation for any try-catch blocks
fd "auth.resolver.ts" --exec cat {}
Length of output: 10750
libs/auth/interface-adapters/src/lib/auth.module.ts (2)
10-10: LGTM! Provider configuration updated correctly
The providers array correctly includes SignInUseCase and its dependencies.
25-25: Verify impact of empty exports array
The exports array is now empty. This is a breaking change that could affect modules importing AuthModule.
|
🎉 This PR is included in version 1.19.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores