feat(auth): [#2] Implement user login with JWT #14
Conversation
…ject config service
WalkthroughAdds a JWT-based login flow: new DTOs, AuthController.login, AuthService.login (password verification + JWT signing), UsersService.findOneByEmail with TypeORM repository, JwtModule wiring in AuthModule, package dependency updates, and unit tests for login and related error propagation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthCtrl as AuthController
participant AuthSvc as AuthService
participant UsersSvc as UsersService
participant JwtSvc as JwtService
participant DB as Database
Client->>AuthCtrl: POST /auth/login (email, password)
AuthCtrl->>AuthSvc: login(LoginUserDto)
AuthSvc->>UsersSvc: findOneByEmail(email)
UsersSvc->>DB: SELECT user, include password
DB-->>UsersSvc: User | null
UsersSvc-->>AuthSvc: User | null
alt user not found
AuthSvc-->>Client: 401 Unauthorized ("Invalid credentials.")
else user found
AuthSvc->>AuthSvc: bcrypt.compare(password, user.password)
alt mismatch
AuthSvc-->>Client: 401 Unauthorized ("Invalid credentials.")
else match
AuthSvc->>JwtSvc: sign({ sub: user.id, email })
JwtSvc-->>AuthSvc: token
AuthSvc-->>AuthCtrl: AccessTokenDto { access_token }
AuthCtrl-->>Client: 200 { access_token }
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
apps/backend/package.json (1)
29-29: Move @types/passport-jwt to devDependencies.Type definition packages should be in
devDependenciessince they're only required at compile time, not runtime.Apply this diff:
- "@nestjs/jwt": "^11.0.1", "@nestjs/platform-express": "^11.0.1", "@nestjs/typeorm": "^11.0.0", - "@types/passport-jwt": "^4.0.1", "bcrypt": "^6.0.0",And add it to devDependencies:
"@types/bcrypt": "^6.0.0", "@types/express": "^5.0.0", "@types/jest": "^30.0.0", "@types/node": "^22.10.7", + "@types/passport-jwt": "^4.0.1", "@types/supertest": "^6.0.2",apps/backend/src/auth/auth.controller.ts (1)
19-22: Login endpoint implementation looks good, but consider adding rate limiting.The login endpoint correctly delegates authentication to the service and returns the access token. However, login endpoints are high-value targets for brute-force attacks.
Consider adding rate limiting middleware to protect against brute-force attacks:
- Use
@nestjs/throttlerpackage- Limit to ~5-10 login attempts per minute per IP
- Consider implementing account lockout after repeated failures
Example configuration:
import { Throttle } from '@nestjs/throttler'; @Post('login') @Throttle({ default: { limit: 5, ttl: 60000 } }) // 5 attempts per minute async login(@Body() loginUserDto: LoginUserDto): Promise<AccessTokenDto> { return this.authService.login(loginUserDto); }apps/backend/src/auth/dto/access-token.dto.ts (1)
1-3: Consider adding readonly modifier for clarity.Since this is a response DTO, marking the property as
readonlywould make the intent clearer and prevent accidental mutations.Apply this diff:
export class AccessTokenDto { - access_token: string; + readonly access_token: string; }apps/backend/src/auth/auth.controller.spec.ts (1)
66-86: Good test coverage for happy path. Consider adding error case tests.The test correctly verifies the controller delegates to the auth service and returns the expected response. However, consider adding tests for error scenarios.
Consider adding these test cases:
it('should propagate UnauthorizedException from auth service', async () => { // Arrange const mockLoginUserDto: LoginUserDto = { email: 'invalid@test.com', password: 'WrongPassword', }; (authService.login as jest.Mock).mockRejectedValue( new UnauthorizedException('Invalid credentials'), ); // Act & Assert await expect(controller.login(mockLoginUserDto)).rejects.toThrow( UnauthorizedException, ); });apps/backend/src/auth/dto/login-user.dto.ts (1)
1-11: Consider adding maximum length constraints for security.The validation decorators are appropriate, but adding
@MaxLength()constraints can help prevent potential DoS attacks from extremely large payloads.Apply this diff:
-import { IsEmail, IsNotEmpty, IsString } from 'class-validator'; +import { IsEmail, IsNotEmpty, IsString, MaxLength } from 'class-validator'; export class LoginUserDto { @IsNotEmpty() @IsEmail() + @MaxLength(255) email: string; @IsNotEmpty() @IsString() + @MaxLength(100) password: string; }apps/backend/src/auth/auth.module.ts (1)
14-16: Confirm global JwtModule is intended.Making JwtModule global affects the entire app. If JWT is only used in auth, consider removing global to limit surface area.
Do you want JwtService available app-wide? If not, drop
global: true.apps/backend/src/users/users.service.ts (2)
13-19: Consider case-insensitive email match and narrower select.Emails are typically case-insensitive; also limit selected columns to what you need.
async findOneByEmail(email: string): Promise<User | null> { - return this.userRespository - .createQueryBuilder('user') - .where('user.email = :email', { email }) - .addSelect('user.password') - .getOne(); + return this.userRepository + .createQueryBuilder('user') + .select(['user.id', 'user.email']) // fetch minimal fields + .addSelect('user.password') // include password for compare + .where('LOWER(user.email) = LOWER(:email)', { email }) + .getOne(); }
8-11: Rename typo: userRespository → userRepository across service and spec files.Typo confirmed in both service implementation and test file. Must be updated in both locations to avoid test failures.
Locations requiring fixes:
apps/backend/src/users/users.service.ts(lines 10, 14)apps/backend/src/users/users.service.spec.ts(lines 19, 33, 49, 56)apps/backend/src/auth/auth.service.ts (3)
15-15: Unify import path style.Keep imports consistent with path alias usage elsewhere.
-import { AccessTokenDto } from './dto/access-token.dto'; +import { AccessTokenDto } from '@/auth/dto/access-token.dto';
96-105: Prefer async signing to avoid blocking the event loop.Use
signAsyncfor non-blocking token generation.- const accessToken = this.jwtService.sign(payload); + const accessToken = await this.jwtService.signAsync(payload); return { access_token: accessToken, };
76-106: LGTM on auth flow; add rate-limiting/lockout upstream.Login logic is correct and returns a short, generic error. Next step: add rate limiting or incremental backoff/lockout to mitigate brute force at the route or gateway layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
apps/backend/package.json(1 hunks)apps/backend/src/auth/auth.controller.spec.ts(2 hunks)apps/backend/src/auth/auth.controller.ts(1 hunks)apps/backend/src/auth/auth.module.ts(1 hunks)apps/backend/src/auth/auth.service.spec.ts(4 hunks)apps/backend/src/auth/auth.service.ts(2 hunks)apps/backend/src/auth/dto/access-token.dto.ts(1 hunks)apps/backend/src/auth/dto/login-user.dto.ts(1 hunks)apps/backend/src/users/users.module.ts(1 hunks)apps/backend/src/users/users.service.spec.ts(1 hunks)apps/backend/src/users/users.service.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-19T16:44:42.654Z
Learnt from: Zafar7645
PR: Zafar7645/syncup#12
File: apps/backend/src/users/users.service.spec.ts:15-17
Timestamp: 2025-10-19T16:44:42.654Z
Learning: In the syncup repository (apps/backend/src/users/users.service.ts), the UsersService is scaffolding for future implementation and currently contains no methods or business logic. Basic "should be defined" smoke tests are sufficient for empty scaffolding services.
Applied to files:
apps/backend/src/users/users.service.spec.tsapps/backend/src/users/users.service.ts
🧬 Code graph analysis (7)
apps/backend/src/auth/auth.service.ts (3)
apps/backend/src/users/users.service.ts (1)
Injectable(6-20)apps/backend/src/auth/dto/login-user.dto.ts (1)
LoginUserDto(3-11)apps/backend/src/auth/dto/access-token.dto.ts (1)
AccessTokenDto(1-3)
apps/backend/src/auth/auth.controller.spec.ts (2)
apps/backend/src/auth/dto/login-user.dto.ts (1)
LoginUserDto(3-11)apps/backend/src/auth/dto/access-token.dto.ts (1)
AccessTokenDto(1-3)
apps/backend/src/auth/auth.module.ts (2)
apps/backend/src/users/users.module.ts (1)
Module(6-11)apps/backend/src/app.module.ts (1)
Module(9-34)
apps/backend/src/users/users.module.ts (2)
apps/backend/src/auth/auth.module.ts (1)
Module(8-24)apps/backend/src/app.module.ts (1)
Module(9-34)
apps/backend/src/auth/auth.service.spec.ts (2)
apps/backend/src/auth/dto/login-user.dto.ts (1)
LoginUserDto(3-11)apps/backend/src/auth/dto/access-token.dto.ts (1)
AccessTokenDto(1-3)
apps/backend/src/auth/auth.controller.ts (2)
apps/backend/src/auth/dto/login-user.dto.ts (1)
LoginUserDto(3-11)apps/backend/src/auth/dto/access-token.dto.ts (1)
AccessTokenDto(1-3)
apps/backend/src/users/users.service.ts (1)
apps/backend/src/auth/auth.service.ts (1)
Injectable(17-107)
🔇 Additional comments (4)
apps/backend/src/auth/auth.controller.ts (1)
12-17: LGTM! Proper async handling for register endpoint.The register endpoint is correctly made asynchronous and delegates to the auth service.
apps/backend/src/users/users.module.ts (1)
1-11: LGTM! Proper TypeORM integration.The module correctly registers the User entity with TypeORM, enabling repository injection in UsersService. This follows NestJS best practices.
apps/backend/src/auth/auth.service.spec.ts (1)
286-375: Excellent test coverage for login flow!The test suite comprehensively covers the login functionality:
- Valid credentials path with JWT generation
- User not found error case
- Password mismatch error case
The tests properly verify all service interactions and ensure that sensitive operations (password comparison, token generation) are only called when appropriate.
apps/backend/src/users/users.service.spec.ts (1)
42-65: Good test coverage for findOneByEmail with proper query builder verification.The test correctly verifies the query builder chain, including the important
addSelect('user.password')call, which ensures the password field is included for authentication. This is crucial since password fields are typically excluded by default in the User entity.
| "bcrypt": "^6.0.0", | ||
| "class-transformer": "^0.5.1", | ||
| "class-validator": "^0.14.2", | ||
| "passport-jwt": "^4.0.1", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if passport core package is required.
You've added passport-jwt but there's no passport core package in dependencies. If you're using passport-jwt strategy, you typically need the passport core package as well.
Run the following script to check if passport is already present or if passport-jwt works standalone in your implementation:
🏁 Script executed:
#!/bin/bash
# Check for passport imports and usage in the codebase
rg -n --type=ts "from 'passport'" -A 2 -B 2
rg -n --type=ts "import.*passport[^-]" -A 2 -B 2Length of output: 0
I need to gather more comprehensive information about the codebase to verify this dependency concern.
🏁 Script executed:
#!/bin/bash
# Check package.json for 'passport' and 'passport-jwt' entries
grep -E "(passport|passport-jwt)" apps/backend/package.jsonLength of output: 66
🏁 Script executed:
#!/bin/bash
# Search more broadly for passport usage - include .js, require(), and variations
rg -n "passport" apps/backend/src -A 1 -B 1 --type=ts --type=js || echo "No passport mentions found in src"Length of output: 33
🏁 Script executed:
#!/bin/bash
# Check if there are passport-jwt usage patterns (JwtStrategy, ExtractJwt, etc)
rg -n "JwtStrategy|ExtractJwt|passport\.use" apps/backend/src -A 1 -B 1 || echo "No JwtStrategy/ExtractJwt found"Length of output: 31
🏁 Script executed:
#!/bin/bash
# Look for any strategy setup or auth configuration files that might use passport
fd -e ts -e js . apps/backend/src | head -20 | xargs grep -l "auth" 2>/dev/null || echo "No auth-related files easily identified"Length of output: 235
🏁 Script executed:
#!/bin/bash
# Check auth.module.ts to see the actual auth strategy being used
cat apps/backend/src/auth/auth.module.tsLength of output: 727
🏁 Script executed:
#!/bin/bash
# Check auth.service.ts to see JWT implementation
cat apps/backend/src/auth/auth.service.tsLength of output: 2918
🏁 Script executed:
#!/bin/bash
# Check if @nestjs/jwt or other JWT strategies are in dependencies
grep -E "(@nestjs/jwt|jsonwebtoken|jwt)" apps/backend/package.jsonLength of output: 96
🏁 Script executed:
#!/bin/bash
# Check app.module.ts to see module imports
cat apps/backend/src/app.module.tsLength of output: 1164
Remove unused passport-jwt dependency and its type definitions.
The codebase uses @nestjs/jwt for JWT authentication (see auth.module.ts and auth.service.ts). The passport-jwt package is not imported or used anywhere in the implementation, making it an unused dependency. Remove "passport-jwt": "^4.0.1" and "@types/passport-jwt": "^4.0.1" from apps/backend/package.json.
🤖 Prompt for AI Agents
In apps/backend/package.json around line 33, remove the unused dependency
entries for "passport-jwt": "^4.0.1" and the corresponding
"@types/passport-jwt": "^4.0.1" from the dependencies/devDependencies section;
update the package.json by deleting those two lines, run npm/yarn install to
update lockfile, and ensure no imports/use of passport-jwt remain before
committing.
There was a problem hiding this comment.
You're right that passport-jwt isn't used yet.
While @nestjs/jwt handles the creation of tokens (which is done in the login function), passport-jwt is the standard library used with NestJS's AuthGuard to validate incoming tokens and protect routes.
I've included it now because the next step, after login, will be to implement route protection using AuthGuard('jwt'), which requires passport-jwt as its underlying strategy. Keeping it ensures the necessary dependency is ready for that implementation.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| JwtModule.registerAsync({ | ||
| imports: [ConfigModule], | ||
| useFactory: (configService: ConfigService) => ({ | ||
| global: true, | ||
| secret: configService.get<string>('JWT_SECRET'), | ||
| signOptions: { expiresIn: '60m' }, | ||
| }), | ||
| inject: [ConfigService], | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
Fail fast if JWT_SECRET is missing; make expiry configurable.
Guard against undefined secret at bootstrap and allow configuring expiry via env. This prevents runtime signing errors and enforces security hygiene.
JwtModule.registerAsync({
imports: [ConfigModule],
useFactory: (configService: ConfigService) => ({
- global: true,
- secret: configService.get<string>('JWT_SECRET'),
- signOptions: { expiresIn: '60m' },
+ global: true,
+ // Fail fast if secret is absent
+ secret: (() => {
+ const secret = configService.get<string>('JWT_SECRET');
+ if (!secret) throw new Error('JWT_SECRET is required');
+ return secret;
+ })(),
+ // Allow override; default remains 60m
+ signOptions: {
+ expiresIn: configService.get<string>('JWT_EXPIRES_IN') ?? '60m',
+ // Optional: uncomment if you want stronger validation
+ // issuer: configService.get<string>('JWT_ISSUER') ?? 'syncup',
+ // audience: configService.get<string>('JWT_AUDIENCE') ?? 'syncup-api',
+ },
}),
inject: [ConfigService],
}),🏁 Script executed:
#!/bin/bash
# Check env/config presence for JWT_SECRET and optional JWT_EXPIRES_IN
rg -nC2 'ConfigModule\.forRoot' apps/backend/src || true
rg -n 'JWT_SECRET|JWT_EXPIRES_IN|JWT_ISSUER|JWT_AUDIENCE' -S --apps backend -g '!**/dist/**' || true
fd -a ".env*" | xargs -I{} sh -c 'echo "--- {} ---"; rg -n "JWT_SECRET|JWT_EXPIRES_IN|JWT_ISSUER|JWT_AUDIENCE" "{}" || true'Length of output: 1029
I need to verify a few more things about how the codebase handles configuration validation and the actual behavior of ConfigService.get().
Now let me check for test files and verify the environment configuration:
Now let me check for test files and existing validation patterns in the auth module:
Based on my verification, the review comment raises a valid and important concern. ConfigService.get() returns undefined if the environment variable is not set, and best practice is to throw an exception during server startup if required environment variables are missing.
Multiple industry examples show using configService.getOrThrow() for JWT_SECRET, which is the idiomatic NestJS approach. The review comment's suggested inline validation with an IIFE is functionally correct but less conventional than getOrThrow().
Regarding configurable expiry: Real-world examples show making JWT_ACCESS_TOKEN_EXPIRES_IN configurable is a useful pattern, though not strictly required.
The review comment's concerns are valid and align with security best practices. I'll now generate the refined version:
Use configService.getOrThrow() for JWT_SECRET to fail fast; make expiry configurable.
Missing JWT_SECRET should cause immediate startup failure, not silent signing errors at runtime. Use the idiomatic NestJS pattern instead of inline validation, and allow expiry override via environment.
JwtModule.registerAsync({
imports: [ConfigModule],
useFactory: (configService: ConfigService) => ({
global: true,
- secret: configService.get<string>('JWT_SECRET'),
+ secret: configService.getOrThrow<string>('JWT_SECRET'),
signOptions: {
- expiresIn: '60m',
+ expiresIn: configService.get<string>('JWT_EXPIRES_IN') ?? '60m',
},
}),
inject: [ConfigService],
}),🤖 Prompt for AI Agents
In apps/backend/src/auth/auth.module.ts around lines 11 to 19, replace the
current use of configService.get('JWT_SECRET') with the idiomatic NestJS
getOrThrow to fail fast when JWT_SECRET is missing, and make the token expiry
configurable by reading a JWT_ACCESS_TOKEN_EXPIRES_IN env var (falling back to
'60m' if absent); update the JwtModule.registerAsync factory to call
configService.getOrThrow<string>('JWT_SECRET') for secret and
configService.get<string>('JWT_ACCESS_TOKEN_EXPIRES_IN') ?? '60m' for
signOptions.expiresIn so startup errors occur on missing secret and expiry can
be customized.
There was a problem hiding this comment.
- Configurable expiry is not strictly required, hence deferring it for now.
- Used getOrThrow for JWT_SECRET to fail instantly.
There was a problem hiding this comment.
@Zafar7645, perfect! Using getOrThrow ensures the application won't start with a misconfigured JWT secret, which is the critical safeguard. Deferring the configurable expiry is a reasonable prioritization—it's a nice-to-have that can be added when needed. Thanks for addressing the security concern! 🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/backend/src/auth/auth.controller.spec.ts (2)
79-84: Combine redundant exception assertions.The test calls
controller.register()twice to verify the same exception. Both the exception type and message can be checked in a single assertion chain, improving efficiency and readability.Apply this diff to combine the assertions:
- // Act & Assert - await expect(controller.register(mockRegisterUserDto)).rejects.toThrow( - ConflictException, - ); - await expect(controller.register(mockRegisterUserDto)).rejects.toThrow( - 'Unable to complete registration at this time.', - ); + // Act & Assert + await expect(controller.register(mockRegisterUserDto)).rejects.toMatchObject({ + name: 'ConflictException', + message: 'Unable to complete registration at this time.', + });Alternatively, if you prefer separate type and message checks:
- // Act & Assert - await expect(controller.register(mockRegisterUserDto)).rejects.toThrow( - ConflictException, - ); - await expect(controller.register(mockRegisterUserDto)).rejects.toThrow( - 'Unable to complete registration at this time.', - ); + // Act & Assert + const promise = controller.register(mockRegisterUserDto); + await expect(promise).rejects.toThrow(ConflictException); + await expect(promise).rejects.toThrow('Unable to complete registration at this time.');
139-144: Combine redundant exception assertions.Similar to the register test, this test calls
controller.login()twice to verify the same exception. Consider combining into a single assertion chain.Apply this diff to combine the assertions:
- // Act & Assert - await expect(controller.login(mockLoginUserDto)).rejects.toThrow( - UnauthorizedException, - ); - await expect(controller.login(mockLoginUserDto)).rejects.toThrow( - 'Invalid credentials.', - ); + // Act & Assert + await expect(controller.login(mockLoginUserDto)).rejects.toMatchObject({ + name: 'UnauthorizedException', + message: 'Invalid credentials.', + });Alternatively:
- // Act & Assert - await expect(controller.login(mockLoginUserDto)).rejects.toThrow( - UnauthorizedException, - ); - await expect(controller.login(mockLoginUserDto)).rejects.toThrow( - 'Invalid credentials.', - ); + // Act & Assert + const promise = controller.login(mockLoginUserDto); + await expect(promise).rejects.toThrow(UnauthorizedException); + await expect(promise).rejects.toThrow('Invalid credentials.');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
apps/backend/package.json(2 hunks)apps/backend/src/auth/auth.controller.spec.ts(2 hunks)apps/backend/src/auth/auth.controller.ts(1 hunks)apps/backend/src/auth/auth.module.ts(1 hunks)apps/backend/src/auth/auth.service.spec.ts(4 hunks)apps/backend/src/auth/auth.service.ts(2 hunks)apps/backend/src/auth/dto/access-token.dto.ts(1 hunks)apps/backend/src/auth/dto/login-user.dto.ts(1 hunks)apps/backend/src/users/users.service.spec.ts(1 hunks)apps/backend/src/users/users.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/backend/src/auth/auth.service.ts
- apps/backend/src/users/users.service.spec.ts
- apps/backend/src/auth/auth.service.spec.ts
- apps/backend/package.json
- apps/backend/src/auth/dto/login-user.dto.ts
- apps/backend/src/auth/dto/access-token.dto.ts
- apps/backend/src/auth/auth.controller.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-19T16:44:42.665Z
Learnt from: Zafar7645
PR: Zafar7645/syncup#12
File: apps/backend/src/users/users.service.spec.ts:15-17
Timestamp: 2025-10-19T16:44:42.665Z
Learning: In the syncup repository (apps/backend/src/users/users.service.ts), the UsersService is scaffolding for future implementation and currently contains no methods or business logic. Basic "should be defined" smoke tests are sufficient for empty scaffolding services.
Applied to files:
apps/backend/src/users/users.service.ts
🧬 Code graph analysis (3)
apps/backend/src/auth/auth.controller.spec.ts (2)
apps/backend/src/auth/dto/login-user.dto.ts (1)
LoginUserDto(3-13)apps/backend/src/auth/dto/access-token.dto.ts (1)
AccessTokenDto(1-3)
apps/backend/src/users/users.service.ts (1)
apps/backend/src/auth/auth.service.ts (1)
Injectable(17-108)
apps/backend/src/auth/auth.module.ts (2)
apps/backend/src/app.module.ts (1)
Module(9-34)apps/backend/src/users/users.module.ts (1)
Module(6-11)
🔇 Additional comments (8)
apps/backend/src/auth/auth.controller.spec.ts (4)
6-8: LGTM! Clean imports for the new test coverage.The new imports for DTOs and exceptions are necessary and properly structured for testing the login flow and error handling.
10-13: LGTM! Mock service properly extended.The mock service correctly includes both
registerandloginmethods to support the test suite.
87-103: LGTM! General error propagation test is appropriate.The test correctly verifies that the controller propagates general errors from the service without swallowing them.
107-125: LGTM! Login success test is well-structured.The test properly verifies the login happy path, ensuring the controller delegates to the service with the correct DTO and returns the expected access token response.
apps/backend/src/auth/auth.module.ts (1)
1-26: Excellent work addressing the previous security concern!The module configuration is solid. Using
getOrThrowforJWT_SECRETon line 15 ensures the application fails fast at startup if the secret is missing, which prevents runtime authentication errors. The JWT module setup with async configuration, global registration, and proper dependency injection follows NestJS best practices perfectly.apps/backend/src/users/users.service.ts (3)
1-4: LGTM! Imports are correctly structured.The imports follow NestJS and TypeORM conventions, bringing in the necessary decorators, entity, and repository types.
6-11: LGTM! Repository injection follows NestJS patterns.The constructor correctly uses
@InjectRepository(User)to inject the TypeORM repository, following standard NestJS dependency injection practices.
13-19: ****The email case sensitivity concern does not exist. The User entity (user.entity.ts) includes a
@BeforeInsert()and@BeforeUpdate()hook that automatically normalizes emails:@BeforeInsert() @BeforeUpdate() normalizeEmail() { if (this.email && typeof this.email === 'string') { this.email = this.email.trim().toLowerCase(); } }This means emails are lowercased before being persisted to the database during registration, making them consistent with the lowercased email used in the login flow. The
findOneByEmailimplementation is correct and functions properly.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/backend/src/auth/auth.controller.spec.ts (2)
6-7: Consider using absolute imports for consistency.The imports here use relative paths (
./dto/...) while the rest of the file uses absolute path aliases (@/auth/...). For consistency and maintainability, consider aligning with the absolute import style.Apply this diff to use absolute imports:
-import { LoginUserDto } from './dto/login-user.dto'; -import { AccessTokenDto } from './dto/access-token.dto'; +import { LoginUserDto } from '@/auth/dto/login-user.dto'; +import { AccessTokenDto } from '@/auth/dto/access-token.dto';
106-144: Add general error propagation test for comprehensive coverage.The login tests correctly cover the success path and UnauthorizedException propagation. However, for consistency with the register tests and comprehensive error handling coverage, consider adding a test for general error propagation.
Add this test after line 143:
it('should propagate any general error from the Auth Service', async () => { // Arrange const mockLoginUserDto: LoginUserDto = { email: 'test.user@test.com', password: 'PlainTextPassword', }; (authService.login as jest.Mock).mockRejectedValue( new Error('Something went wrong.'), ); // Act & Assert await expect(controller.login(mockLoginUserDto)).rejects.toThrow( 'Something went wrong.', ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/backend/src/auth/auth.controller.spec.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/backend/src/auth/auth.controller.spec.ts (2)
apps/backend/src/auth/dto/login-user.dto.ts (1)
LoginUserDto(3-13)apps/backend/src/auth/dto/access-token.dto.ts (1)
AccessTokenDto(1-3)
🔇 Additional comments (1)
apps/backend/src/auth/auth.controller.spec.ts (1)
40-104: Excellent test coverage for the register flow.The register tests comprehensively cover success, specific exceptions (ConflictException), and general error propagation. The AAA pattern with clear comments makes the tests easy to follow.
github-project-automation
bot
moved this from In Progress
to Done
in SyncUp Development Plan
Description
This PR introduces the complete user login feature. It adds a POST /auth/login endpoint that authenticates users based on their email and password and returns a JSON Web Token (JWT) upon success.
Related Issue
Closes #2
Summary by CodeRabbit
New Features
Tests