[backend] Implement POST /auth/login #30
Conversation
- `docker compose exec backend yarn run nest generate resource` - `npm install --save @nestjs/passport passport @nestjs/jwt passport-jwt` - `npm install --save-dev @types/passport-jwt`
- Create a new file `backend/src/auth/dto/login.dto.ts` - Create a new file `backend/src/auth/entity/auth.entity.ts`
- Export the UserService from the UserModule
- Add JwtAuthGuard to user controller
- `docker compose exec backend yarn add bcrypt` - `docker compose exec backend yarn add -d @types/bcrypt`
|
Warning Rate Limit Exceeded@usatie has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 9 minutes and 44 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per repository. Your existing walkthrough is comprehensive and aligns with the provided changes. I'll proceed to include the poem as per your request. WalkthroughThe changes introduce user authentication to the backend of the application. They add JWT-based authentication, password hashing, and database seeding. The updates also include new tests for the authentication service and controller, and modifications to the user service and controller to incorporate authentication. Changes
Poem
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to filter (5)
- backend/package.json
- backend/yarn.lock
- compose.yml
- package-lock.json
- package.json
Files selected for processing (17)
- .env.template (1 hunks)
- README.md (2 hunks)
- backend/prisma/seed.ts (1 hunks)
- backend/src/app.module.ts (1 hunks)
- backend/src/auth/auth.controller.spec.ts (1 hunks)
- backend/src/auth/auth.controller.ts (1 hunks)
- backend/src/auth/auth.module.ts (1 hunks)
- backend/src/auth/auth.service.spec.ts (1 hunks)
- backend/src/auth/auth.service.ts (1 hunks)
- backend/src/auth/dto/login.dto.ts (1 hunks)
- backend/src/auth/entity/auth.entity.ts (1 hunks)
- backend/src/auth/jwt-auth.guard.ts (1 hunks)
- backend/src/auth/jwt.strategy.ts (1 hunks)
- backend/src/main.ts (1 hunks)
- backend/src/user/user.controller.ts (3 hunks)
- backend/src/user/user.module.ts (1 hunks)
- backend/src/user/user.service.ts (1 hunks)
Files skipped from review due to trivial changes (5)
- README.md
- backend/src/auth/auth.service.spec.ts
- backend/src/auth/jwt-auth.guard.ts
- backend/src/main.ts
- backend/src/user/user.module.ts
Additional comments: 19
.env.template (1)
- 5-5: Ensure that the
BACKEND_JWT_SECRETis generated securely and kept confidential. It should not be committed to the repository or shared publicly. Also, make sure to update the production environment with this new variable.backend/src/auth/entity/auth.entity.ts (1)
- 1-6: The
AuthEntityclass is well defined with theaccessTokenproperty. Ensure that theaccessTokenis being used correctly throughout the application and that it is being properly validated and sanitized.backend/src/app.module.ts (1)
- 3-12: The
AuthModulehas been added to the imports and the module decorator. This is a necessary step to include the authentication functionality in the application. Ensure that theAuthModuleis correctly implemented and that it doesn't conflict with other modules.backend/src/auth/dto/login.dto.ts (1)
- 1-15: The
LoginDtoclass is well defined with appropriate validation decorators for thepasswordproperties. The use of@ApiProperty()decorator will help in generating Swagger API documentation. The@IsEmail(),@IsNotEmpty(),@IsString(), and@MinLength(6)decorators will ensure that the input data is validated before processing.backend/src/user/user.service.ts (1)
- 28-40: The update function now hashes the password before updating the user data. This is a good security practice as it ensures that passwords are not stored in plain text. However, ensure that all calls to this function throughout the codebase have been updated to match the new signature.
backend/src/auth/auth.controller.ts (1)
- 1-17: The
AuthControllerclass and itsloginmethod look good. The method correctly uses the@Bodydecorator to extractpasswordfrom the request body. Theloginmethod also correctly returns aPromiseofAuthEntity. Ensure that theAuthService'sloginmethod is implemented correctly and handles any potential errors.backend/src/auth/jwt.strategy.ts (3)
1-5: Ensure that all imported modules are available and correctly installed in your project dependencies.
7-14: The
JwtStrategyclass is correctly set up to use the JWT strategy from Passport. It uses theExtractJwt.fromAuthHeaderAsBearerToken()method to extract the JWT token from the Authorization header and thejwtConstants.secretas the secret key to decode the token.16-24: The
validatemethod correctly checks if a user with the givenuserIdexists. If the user does not exist, it throws anUnauthorizedException. If the user exists, it returns the user object. This is a good practice for validating JWT tokens.backend/src/auth/auth.module.ts (3)
10-12: Ensure that the
JWT_SECRETenvironment variable is set in all environments where this code will run. If it's not set,jwtConstants.secretwill beundefined, which could lead to unexpected behavior.19-21: The JWT token expiration is set to 30 minutes. Make sure this duration is appropriate for your application's needs. If sessions are too short, users may have to re-authenticate frequently, which could negatively impact the user experience. If sessions are too long, it could pose a security risk if a token is stolen.
25-25: The
AuthServiceandJwtStrategyare provided at the module level. Ensure that these services do not maintain any state between requests, as this could lead to data leaks or inconsistent behavior.backend/src/user/user.controller.ts (7)
8-12: The
UseGuardsdecorator is imported correctly from@nestjs/common.18-23: The
ApiBearerAuthdecorator is imported correctly from@nestjs/swagger.25-25: The
JwtAuthGuardis imported correctly.40-41: The
findAllmethod is now protected by theJwtAuthGuardand requires a valid JWT token for access.49-50: The
findOnemethod is now protected by theJwtAuthGuardand requires a valid JWT token for access.57-58: The
updatemethod is now protected by theJwtAuthGuardand requires a valid JWT token for access.69-70: The
removemethod is now protected by theJwtAuthGuardand requires a valid JWT token for access.
| import { Test, TestingModule } from '@nestjs/testing'; | ||
| import { AuthController } from './auth.controller'; | ||
| import { AuthService } from './auth.service'; | ||
|
|
||
| describe('AuthController', () => { | ||
| let controller: AuthController; | ||
|
|
||
| beforeEach(async () => { | ||
| const module: TestingModule = await Test.createTestingModule({ | ||
| controllers: [AuthController], | ||
| providers: [AuthService], | ||
| }).compile(); | ||
|
|
||
| controller = module.get<AuthController>(AuthController); | ||
| }); | ||
|
|
||
| it('should be defined', () => { | ||
| expect(controller).toBeDefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test suite for AuthController is well-structured and follows the standard NestJS testing pattern. However, it only checks if the controller is defined. Consider adding more tests to verify the functionality of the controller, such as testing the login and logout methods, and checking if the JWT token is correctly generated and validated.
| import { | ||
| Injectable, | ||
| NotFoundException, | ||
| UnauthorizedException, | ||
| } from '@nestjs/common'; | ||
| import { PrismaService } from 'src/prisma/prisma.service'; | ||
| import { JwtService } from '@nestjs/jwt'; | ||
| import { AuthEntity } from './entity/auth.entity'; | ||
| import * as bcrypt from 'bcrypt'; | ||
|
|
||
| @Injectable() | ||
| export class AuthService { | ||
| constructor( | ||
| private prisma: PrismaService, | ||
| private jwtService: JwtService, | ||
| ) {} | ||
|
|
||
| async login(email: string, password: string): Promise<AuthEntity> { | ||
| const user = await this.prisma.user.findUnique({ where: { email } }); | ||
|
|
||
| if (!user) { | ||
| throw new NotFoundException(`No user found for email: ${email}`); | ||
| } | ||
|
|
||
| const isPasswordValid = await bcrypt.compare(password, user.password); | ||
|
|
||
| if (!isPasswordValid) { | ||
| throw new UnauthorizedException('Invalid password'); | ||
| } | ||
|
|
||
| return { | ||
| accessToken: this.jwtService.sign({ userId: user.id }), | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
The AuthService class and its login method are well implemented. It correctly uses the PrismaService to interact with the database and the JwtService to generate JWT tokens. The error handling for non-existent users and invalid passwords is also correctly done. However, consider adding a try-catch block around the database call and JWT token generation to handle any potential exceptions that might occur during these operations.
async login(email: string, password: string): Promise<AuthEntity> {
+ try {
const user = await this.prisma.user.findUnique({ where: { email } });
if (!user) {
throw new NotFoundException(`No user found for email: ${email}`);
}
const isPasswordValid = await bcrypt.compare(password, user.password);
if (!isPasswordValid) {
throw new UnauthorizedException('Invalid password');
}
return {
accessToken: this.jwtService.sign({ userId: user.id }),
};
+ } catch (error) {
+ throw new Error(`Error during authentication: ${error.message}`);
+ }
}Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| import { | |
| Injectable, | |
| NotFoundException, | |
| UnauthorizedException, | |
| } from '@nestjs/common'; | |
| import { PrismaService } from 'src/prisma/prisma.service'; | |
| import { JwtService } from '@nestjs/jwt'; | |
| import { AuthEntity } from './entity/auth.entity'; | |
| import * as bcrypt from 'bcrypt'; | |
| @Injectable() | |
| export class AuthService { | |
| constructor( | |
| private prisma: PrismaService, | |
| private jwtService: JwtService, | |
| ) {} | |
| async login(email: string, password: string): Promise<AuthEntity> { | |
| const user = await this.prisma.user.findUnique({ where: { email } }); | |
| if (!user) { | |
| throw new NotFoundException(`No user found for email: ${email}`); | |
| } | |
| const isPasswordValid = await bcrypt.compare(password, user.password); | |
| if (!isPasswordValid) { | |
| throw new UnauthorizedException('Invalid password'); | |
| } | |
| return { | |
| accessToken: this.jwtService.sign({ userId: user.id }), | |
| }; | |
| } | |
| } | |
| async login(email: string, password: string): Promise<AuthEntity> { | |
| try { | |
| const user = await this.prisma.user.findUnique({ where: { email } }); | |
| if (!user) { | |
| throw new NotFoundException(`No user found for email: ${email}`); | |
| } | |
| const isPasswordValid = await bcrypt.compare(password, user.password); | |
| if (!isPasswordValid) { | |
| throw new UnauthorizedException('Invalid password'); | |
| } | |
| return { | |
| accessToken: this.jwtService.sign({ userId: user.id }), | |
| }; | |
| } catch (error) { | |
| throw new Error(`Error during authentication: ${error.message}`); | |
| } | |
| } |
| import { PrismaClient } from '@prisma/client'; | ||
| import * as bcrypt from 'bcrypt'; | ||
|
|
||
| // Initialize Prisma client | ||
| const prisma = new PrismaClient(); | ||
|
|
||
| const roundsOfHashing = 10; | ||
|
|
||
| async function main() { | ||
| const passwordSusami = await bcrypt.hash('password-susami', roundsOfHashing); | ||
| const passwordThara = await bcrypt.hash('password-thara', roundsOfHashing); | ||
| const passwordKakiba = await bcrypt.hash('password-kakiba', roundsOfHashing); | ||
| const passwordShongou = await bcrypt.hash( | ||
| 'password-shongou', | ||
| roundsOfHashing, | ||
| ); | ||
|
|
||
| const user1 = await prisma.user.upsert({ | ||
| where: { email: '[email protected]' }, | ||
| update: {}, | ||
| create: { | ||
| email: '[email protected]', | ||
| name: 'Susami', | ||
| password: passwordSusami, | ||
| }, | ||
| }); | ||
|
|
||
| const user2 = await prisma.user.upsert({ | ||
| where: { email: '[email protected]' }, | ||
| update: {}, | ||
| create: { | ||
| email: '[email protected]', | ||
| name: 'Thara', | ||
| password: passwordThara, | ||
| }, | ||
| }); | ||
|
|
||
| const user3 = await prisma.user.upsert({ | ||
| where: { email: '[email protected]' }, | ||
| update: {}, | ||
| create: { | ||
| email: '[email protected]', | ||
| name: 'Kakiba', | ||
| password: passwordKakiba, | ||
| }, | ||
| }); | ||
|
|
||
| const user4 = await prisma.user.upsert({ | ||
| where: { email: '[email protected]' }, | ||
| update: {}, | ||
| create: { | ||
| email: '[email protected]', | ||
| name: 'Shongou', | ||
| password: passwordShongou, | ||
| }, | ||
| }); | ||
|
|
||
| console.log({ user1, user2, user3, user4 }); | ||
| } | ||
|
|
||
| main() | ||
| .catch((e) => { | ||
| console.error(e); | ||
| process.exit(1); | ||
| }) | ||
| .finally(async () => { | ||
| // close Prisma Client at the end | ||
| await prisma.$disconnect(); | ||
| }); |
There was a problem hiding this comment.
The code looks good overall. It correctly hashes passwords before storing them in the database, which is a good security practice. However, it's important to note that the passwords are hardcoded. In a production environment, you should never hardcode passwords, even for seeding purposes. Consider using environment variables or a secure method to store these passwords.
| const passwordSusami = await bcrypt.hash('password-susami', roundsOfHashing); | ||
| const passwordThara = await bcrypt.hash('password-thara', roundsOfHashing); | ||
| const passwordKakiba = await bcrypt.hash('password-kakiba', roundsOfHashing); | ||
| const passwordShongou = await bcrypt.hash( | ||
| 'password-shongou', | ||
| roundsOfHashing, | ||
| ); |
There was a problem hiding this comment.
The bcrypt hashing function is called with a static salt round value. While this is not necessarily a problem, it's worth noting that as computer processing power increases, you may need to increase the number of salt rounds to maintain the same level of security. Consider making this value configurable so it can be easily updated in the future.
- const roundsOfHashing = 10;
+ const roundsOfHashing = process.env.BCRYPT_SALT_ROUNDS || 10;Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| const passwordSusami = await bcrypt.hash('password-susami', roundsOfHashing); | |
| const passwordThara = await bcrypt.hash('password-thara', roundsOfHashing); | |
| const passwordKakiba = await bcrypt.hash('password-kakiba', roundsOfHashing); | |
| const passwordShongou = await bcrypt.hash( | |
| 'password-shongou', | |
| roundsOfHashing, | |
| ); | |
| const roundsOfHashing = process.env.BCRYPT_SALT_ROUNDS || 10; | |
| const passwordSusami = await bcrypt.hash('password-susami', roundsOfHashing); | |
| const passwordThara = await bcrypt.hash('password-thara', roundsOfHashing); | |
| const passwordKakiba = await bcrypt.hash('password-kakiba', roundsOfHashing); | |
| const passwordShongou = await bcrypt.hash( | |
| 'password-shongou', | |
| roundsOfHashing, | |
| ); |
| const user1 = await prisma.user.upsert({ | ||
| where: { email: '[email protected]' }, | ||
| update: {}, | ||
| create: { | ||
| email: '[email protected]', | ||
| name: 'Susami', | ||
| password: passwordSusami, | ||
| }, | ||
| }); | ||
|
|
||
| const user2 = await prisma.user.upsert({ | ||
| where: { email: '[email protected]' }, | ||
| update: {}, | ||
| create: { | ||
| email: '[email protected]', | ||
| name: 'Thara', | ||
| password: passwordThara, | ||
| }, | ||
| }); | ||
|
|
||
| const user3 = await prisma.user.upsert({ | ||
| where: { email: '[email protected]' }, | ||
| update: {}, | ||
| create: { | ||
| email: '[email protected]', | ||
| name: 'Kakiba', | ||
| password: passwordKakiba, | ||
| }, | ||
| }); | ||
|
|
||
| const user4 = await prisma.user.upsert({ | ||
| where: { email: '[email protected]' }, | ||
| update: {}, | ||
| create: { | ||
| email: '[email protected]', | ||
| name: 'Shongou', | ||
| password: passwordShongou, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The upsert function is used to create or update users. This is fine for a seed script, but be aware that if this script is run multiple times, it will not change the passwords for existing users because the update object is empty. If you want to update the passwords for existing users when the script is run, you need to include the password in the update object.
- update: {},
+ update: { password: passwordSusami },Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| const user1 = await prisma.user.upsert({ | |
| where: { email: '[email protected]' }, | |
| update: {}, | |
| create: { | |
| email: '[email protected]', | |
| name: 'Susami', | |
| password: passwordSusami, | |
| }, | |
| }); | |
| const user2 = await prisma.user.upsert({ | |
| where: { email: '[email protected]' }, | |
| update: {}, | |
| create: { | |
| email: '[email protected]', | |
| name: 'Thara', | |
| password: passwordThara, | |
| }, | |
| }); | |
| const user3 = await prisma.user.upsert({ | |
| where: { email: '[email protected]' }, | |
| update: {}, | |
| create: { | |
| email: '[email protected]', | |
| name: 'Kakiba', | |
| password: passwordKakiba, | |
| }, | |
| }); | |
| const user4 = await prisma.user.upsert({ | |
| where: { email: '[email protected]' }, | |
| update: {}, | |
| create: { | |
| email: '[email protected]', | |
| name: 'Shongou', | |
| password: passwordShongou, | |
| }, | |
| }); | |
| const user1 = await prisma.user.upsert({ | |
| where: { email: '[email protected]' }, | |
| update: { password: passwordSusami }, | |
| create: { | |
| email: '[email protected]', | |
| name: 'Susami', | |
| password: passwordSusami, | |
| }, | |
| }); | |
| const user2 = await prisma.user.upsert({ | |
| where: { email: '[email protected]' }, | |
| update: { password: passwordThara }, | |
| create: { | |
| email: '[email protected]', | |
| name: 'Thara', | |
| password: passwordThara, | |
| }, | |
| }); | |
| const user3 = await prisma.user.upsert({ | |
| where: { email: '[email protected]' }, | |
| update: { password: passwordKakiba }, | |
| create: { | |
| email: '[email protected]', | |
| name: 'Kakiba', | |
| password: passwordKakiba, | |
| }, | |
| }); | |
| const user4 = await prisma.user.upsert({ | |
| where: { email: '[email protected]' }, | |
| update: { password: passwordShongou }, | |
| create: { | |
| email: '[email protected]', | |
| name: 'Shongou', | |
| password: passwordShongou, | |
| }, | |
| }); |
| main() | ||
| .catch((e) => { | ||
| console.error(e); | ||
| process.exit(1); | ||
| }) | ||
| .finally(async () => { | ||
| // close Prisma Client at the end | ||
| await prisma.$disconnect(); | ||
| }); |
There was a problem hiding this comment.
The error handling and Prisma client disconnection are correctly implemented. However, consider adding more descriptive error messages to help with debugging if something goes wrong.
|
そうか、ユーザー一覧画面の取得をログイン必須にしてしまったので、そこのprerenderingがビルドができないのか。 |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- backend/src/auth/auth.controller.spec.ts (1 hunks)
- backend/src/auth/auth.service.spec.ts (1 hunks)
- backend/src/user/user.controller.ts (3 hunks)
Files skipped from review due to trivial changes (1)
- backend/src/auth/auth.controller.spec.ts
Additional comments: 5
backend/src/auth/auth.service.spec.ts (1)
- 1-4: Imports look good and are correctly ordered.
backend/src/user/user.controller.ts (4)
8-12: The
UseGuardsdecorator is imported but not used in thecreatemethod. If thecreatemethod should be protected, add theUseGuardsdecorator andJwtAuthGuardto it. If not, this import is fine as it is used in other methods.47-53: The
findOnemethod is correctly protected with bothUseGuards(JwtAuthGuard)andApiBearerAuth().55-60: The
updatemethod is correctly protected with bothUseGuards(JwtAuthGuard)andApiBearerAuth().66-72: The
removemethod is correctly protected with bothUseGuards(JwtAuthGuard)andApiBearerAuth().
| describe('AuthService', () => { | ||
| let service: AuthService; | ||
|
|
||
| beforeEach(async () => { | ||
| const module: TestingModule = await Test.createTestingModule({ | ||
| providers: [AuthService, PrismaService, JwtService], | ||
| }).compile(); | ||
|
|
||
| service = module.get<AuthService>(AuthService); | ||
| }); | ||
|
|
||
| it('should be defined', () => { | ||
| expect(service).toBeDefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test suite setup is correct, but it only contains a single test to check if the service is defined. Consider adding more tests to verify the functionality of the AuthService. For example, you could test if the service correctly validates credentials and generates JWT tokens.
|
|
||
| beforeEach(async () => { | ||
| const module: TestingModule = await Test.createTestingModule({ | ||
| providers: [AuthService, PrismaService, JwtService], |
There was a problem hiding this comment.
The JwtService and PrismaService are being provided directly in the test module. This might not be the best approach as it could lead to tests that are not isolated. Consider mocking these services to ensure that your tests are not dependent on their implementation. This will make your tests more reliable and easier to maintain.
- providers: [AuthService, PrismaService, JwtService],
+ providers: [
+ AuthService,
+ {
+ provide: PrismaService,
+ useValue: {
+ // mock methods here
+ },
+ },
+ {
+ provide: JwtService,
+ useValue: {
+ // mock methods here
+ },
+ },
+ ],Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| providers: [AuthService, PrismaService, JwtService], | |
| providers: [ | |
| AuthService, | |
| { | |
| provide: PrismaService, | |
| useValue: { | |
| // mock methods here | |
| }, | |
| }, | |
| { | |
| provide: JwtService, | |
| useValue: { | |
| // mock methods here | |
| }, | |
| }, | |
| ], |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/workflows/test.yml (1 hunks)
Additional comments: 1
.github/workflows/test.yml (1)
- 15-21: The new environment variable
BACKEND_JWT_SECRETis correctly added to the.envfile. Ensure that the secret is securely stored and not exposed in logs or error messages. Also, verify that theBACKEND_JWT_SECRETis set in the GitHub secrets to avoid breaking the CI/CD pipeline.
usatie
force-pushed
the
feat/backend/login
branch
from
a17f8d8
to
bc584b3
Compare
.envファイルに新たに追加する必要のある環境変数が一つ増えたのでお気をつけください。Prismaの公式チュートリアル Part5の内容をもとにログイン機能を実装しました。
まだ、ログイン状態/ログアウト状態でのリソース保護しか管理していないので、「AというユーザーをBもCもログインさえしていればアップデートできる」という機能になっています。
ここから「Aというユーザーをアップデートできるのは、Aとしてログインしている場合のみ」のように変更する必要があります。
https://www.prisma.io/blog/nestjs-prisma-authentication-7D056s1s0k3l#implement-authentication-in-your-rest-api
Summary by CodeRabbit
BACKEND_JWT_SECRETin the README.AuthControllerandAuthService.BACKEND_JWT_SECRET.