Conversation
WalkthroughThis pull request introduces significant updates to the project’s infrastructure and authentication. A new NestJS service is added to the Docker Compose configuration along with a dedicated Dockerfile. The application now integrates GraphQL with Apollo and leverages JWT authentication via Passport. Several outdated authentication controllers, DTOs, and modules (including tests) have been removed, while a new User module and associated resolver, service, and entity files have been introduced. The overall changes shift the architecture toward a more modular, JWT-based, GraphQL-driven design. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant UR as UserResolver
participant US as UserService
participant DB as Database
participant JWT as JwtService
C->>UR: login(username, password)
UR->>US: login(username, password)
US->>DB: findByUsername(username)
DB-->>US: user data
US->>JWT: generate access & refresh tokens
JWT-->>US: {accessToken, refreshToken}
US-->>UR: tokens
UR-->>C: tokens
sequenceDiagram
participant C as Client
participant UR as UserResolver
participant US as UserService
participant JWT as JwtService
C->>UR: refreshToken(token)
UR->>US: refreshToken(token)
US->>JWT: verify refresh token
JWT-->>US: payload/validity
US->>JWT: generate new tokens
JWT-->>US: new {accessToken, refreshToken}
US-->>UR: new tokens
UR-->>C: new tokens
Poem
✨ Finishing Touches
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: 11
🧹 Nitpick comments (6)
src/users/dto/create-user.input/create-user.input.ts (1)
1-10: Add validation to input fieldsThe
CreateUserInputclass is correctly structured as a GraphQL input type, but consider enhancing it with validation rules using class-validator decorators to ensure data integrity.import { InputType, Field } from '@nestjs/graphql'; +import { IsNotEmpty, IsString, IsInt, Min } from 'class-validator'; @InputType() export class CreateUserInput { @Field() + @IsNotEmpty() + @IsString() name: string; @Field() + @IsInt() + @Min(0) age: number; }This ensures that incoming data is validated before processing, providing clearer error messages and improved security.
src/users/users.resolver.ts (3)
6-8: Consider adding authorization/guard mechanisms.
UsersResolverpublicly exposes user-retrieval and creation. If user data is sensitive, you might use NestJS guards or interceptors to handle authentication/authorization.
10-13: Add error handling forfindAll()if needed.
If an unexpected error occurs in theUsersService, returning an empty array could mask underlying issues. Consider adding error handling or logging.
15-18: Validate input before user creation.
Currently, thecreateUsermutation directly passescreateUserInputto the service without validation. For production scenarios, consider leveraging class-validator or custom validation logic to ensure data integrity.src/auth/auth.module.ts (1)
13-13: Consider advanced token expiration strategies.
Having a fixed60mmight suffice for basic setups, but in production you may want refresh tokens or dynamic TTL.Also applies to: 15-15
src/users/users.service.ts (1)
17-18: Consider filtering, pagination, or partial retrieval.
As the user list grows, returning all users every time may affect performance. Provide pagination or filters infindAll()if the dataset can become large.
📜 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 (21)
docker-compose.yaml(1 hunks)dockerfile(1 hunks)package.json(1 hunks)src/app.module.ts(2 hunks)src/auth/auth.controller.spec.ts(0 hunks)src/auth/auth.controller.ts(0 hunks)src/auth/auth.guard.ts(0 hunks)src/auth/auth.module.ts(1 hunks)src/auth/auth.resolver.spec.ts(1 hunks)src/auth/auth.resolver.ts(1 hunks)src/auth/auth.service.ts(1 hunks)src/auth/constants.ts(0 hunks)src/auth/dto/sign-in.dto.ts(0 hunks)src/auth/dto/sign-up.dto.ts(0 hunks)src/users/dto/create-user.input/create-user.input.ts(1 hunks)src/users/entities/user/user.ts(1 hunks)src/users/user.entity.ts(0 hunks)src/users/users.module.ts(1 hunks)src/users/users.resolver.spec.ts(1 hunks)src/users/users.resolver.ts(1 hunks)src/users/users.service.ts(1 hunks)
💤 Files with no reviewable changes (7)
- src/auth/constants.ts
- src/auth/dto/sign-up.dto.ts
- src/auth/auth.controller.spec.ts
- src/auth/dto/sign-in.dto.ts
- src/users/user.entity.ts
- src/auth/auth.controller.ts
- src/auth/auth.guard.ts
✅ Files skipped from review due to trivial changes (1)
- dockerfile
🧰 Additional context used
🪛 ESLint
src/users/users.module.ts
[error] 6-6: Insert ,
(prettier/prettier)
🔇 Additional comments (12)
src/users/entities/user/user.ts (1)
1-15: Align TypeORM and GraphQL decoratorsThe
Userclass uses a mix of GraphQL and TypeORM decorators, but the TypeORM setup is incomplete which could lead to issues with database persistence.import { ObjectType, Field, Int } from '@nestjs/graphql'; -import { PrimaryGeneratedColumn } from 'typeorm'; +import { Entity, PrimaryGeneratedColumn, Column } from 'typeorm'; +@Entity() @ObjectType() export class User { @Field(() => Int) @PrimaryGeneratedColumn() id: number; @Field() + @Column() name: string; @Field() + @Column() age: number; }If you're intentionally not using this as a TypeORM entity (perhaps for an in-memory store or GraphQL-only type), consider removing the TypeORM decorators entirely to avoid confusion.
src/users/users.module.ts (2)
2-2: Module shift to GraphQL with UsersResolver.The addition of UsersResolver indicates a shift toward GraphQL-based user operations, which aligns with the JWT implementation goal mentioned in the PR title.
6-6: Architectural change: From TypeORM to in-memory user management.The providers array now includes UsersResolver alongside UsersService, and the TypeOrmModule import has been removed. This indicates a significant shift from a database-driven model to what appears to be an in-memory user management system.
Make sure this change is intentional and that there's a strategy for user data persistence, especially for authentication purposes.
🧰 Tools
🪛 ESLint
[error] 6-6: Insert
,(prettier/prettier)
src/auth/auth.resolver.ts (1)
14-16: Good security practice with generic error message.Using a generic error message for invalid credentials is a security best practice as it doesn't reveal whether the username or password was incorrect.
package.json (1)
23-23: GraphQL and JWT dependencies properly added.The addition of GraphQL and JWT-related dependencies aligns with the PR's objective of implementing JWT authentication in a GraphQL context.
Also applies to: 26-27, 32-32, 37-39
src/app.module.ts (3)
6-8: GraphQL integration properly configured.The imports for GraphQL and Apollo are correctly added to support the GraphQL functionality.
15-15: Database host changed for containerized environment.The database host has changed from 'localhost' to 'postgres', indicating a move to a containerized environment where 'postgres' is the service name in docker-compose.
Ensure that docker-compose is properly configured with a service named 'postgres' and that the appropriate network settings are in place for communication between the NestJS and postgres containers.
24-27: GraphQL module correctly configured.The GraphQLModule is correctly set up with ApolloDriver and automatic schema generation.
src/users/users.resolver.ts (1)
1-4: Ensure theUserimport path is correct.
Double-check that the file'./entities/user/user'indeed exports aUserclass (decorated with@ObjectType()) so that the resolver can properly resolve the GraphQL type.src/auth/auth.module.ts (1)
3-5: Remove references to hardcoded AuthService/Resolver if not in use.
As the application transitions to GraphQL-based auth, confirm that these references match your design plan. If the module is purely GraphQL-based now, check for leftover REST logic.docker-compose.yaml (1)
37-39: Verify network requirements.
A default bridge network in Docker Compose is typically sufficient for local development, but ensure you have the necessary isolation or bridging if additional containers or external services need to connect.src/auth/auth.service.ts (1)
3-3: Check performance and security trade-offs withbcryptjs
The usage ofbcryptjsmight differ from the standardbcryptlibrary. Ensure that the chosen library meets your performance and security requirements.
src/auth/auth.resolver.spec.ts
Outdated
| describe('AuthResolver', () => { | ||
| let resolver: AuthResolver; | ||
|
|
||
| beforeEach(async () => { | ||
| const module: TestingModule = await Test.createTestingModule({ | ||
| providers: [AuthResolver], | ||
| }).compile(); | ||
|
|
||
| resolver = module.get<AuthResolver>(AuthResolver); | ||
| }); | ||
|
|
||
| it('should be defined', () => { | ||
| expect(resolver).toBeDefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with AuthService mock
The current test only verifies that the resolver is defined, but it's missing proper dependency mocking and functional tests.
import { Test, TestingModule } from '@nestjs/testing';
import { AuthResolver } from './auth.resolver';
+import { AuthService } from './auth.service';
describe('AuthResolver', () => {
let resolver: AuthResolver;
+ let authService: AuthService;
beforeEach(async () => {
+ const mockAuthService = {
+ login: jest.fn().mockResolvedValue({ access_token: 'test-token' }),
+ };
+
const module: TestingModule = await Test.createTestingModule({
- providers: [AuthResolver],
+ providers: [
+ AuthResolver,
+ { provide: AuthService, useValue: mockAuthService },
+ ],
}).compile();
resolver = module.get<AuthResolver>(AuthResolver);
+ authService = module.get<AuthService>(AuthService);
});
it('should be defined', () => {
expect(resolver).toBeDefined();
});
+
+ describe('login', () => {
+ it('should return an access token when credentials are valid', async () => {
+ const mockUser = { username: 'testuser', password: 'password' };
+ const result = await resolver.login(mockUser);
+ expect(result).toEqual({ access_token: 'test-token' });
+ expect(authService.login).toHaveBeenCalledWith(mockUser);
+ });
+ });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('AuthResolver', () => { | |
| let resolver: AuthResolver; | |
| beforeEach(async () => { | |
| const module: TestingModule = await Test.createTestingModule({ | |
| providers: [AuthResolver], | |
| }).compile(); | |
| resolver = module.get<AuthResolver>(AuthResolver); | |
| }); | |
| it('should be defined', () => { | |
| expect(resolver).toBeDefined(); | |
| }); | |
| }); | |
| import { Test, TestingModule } from '@nestjs/testing'; | |
| import { AuthResolver } from './auth.resolver'; | |
| import { AuthService } from './auth.service'; | |
| describe('AuthResolver', () => { | |
| let resolver: AuthResolver; | |
| let authService: AuthService; | |
| beforeEach(async () => { | |
| const mockAuthService = { | |
| login: jest.fn().mockResolvedValue({ access_token: 'test-token' }), | |
| }; | |
| const module: TestingModule = await Test.createTestingModule({ | |
| providers: [ | |
| AuthResolver, | |
| { provide: AuthService, useValue: mockAuthService }, | |
| ], | |
| }).compile(); | |
| resolver = module.get<AuthResolver>(AuthResolver); | |
| authService = module.get<AuthService>(AuthService); | |
| }); | |
| it('should be defined', () => { | |
| expect(resolver).toBeDefined(); | |
| }); | |
| describe('login', () => { | |
| it('should return an access token when credentials are valid', async () => { | |
| const mockUser = { username: 'testuser', password: 'password' }; | |
| const result = await resolver.login(mockUser); | |
| expect(result).toEqual({ access_token: 'test-token' }); | |
| expect(authService.login).toHaveBeenCalledWith(mockUser); | |
| }); | |
| }); | |
| }); |
src/users/users.resolver.spec.ts
Outdated
| describe('UsersResolver', () => { | ||
| let resolver: UsersResolver; | ||
|
|
||
| beforeEach(async () => { | ||
| const module: TestingModule = await Test.createTestingModule({ | ||
| providers: [UsersResolver], | ||
| }).compile(); | ||
|
|
||
| resolver = module.get<UsersResolver>(UsersResolver); | ||
| }); | ||
|
|
||
| it('should be defined', () => { | ||
| expect(resolver).toBeDefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper service mocking and functional tests
The test suite is missing the UsersService dependency and functional tests for the resolver's methods.
import { Test, TestingModule } from '@nestjs/testing';
import { UsersResolver } from './users.resolver';
+import { UsersService } from './users.service';
+import { CreateUserInput } from './dto/create-user.input/create-user.input';
+import { User } from './entities/user/user';
describe('UsersResolver', () => {
let resolver: UsersResolver;
+ let service: UsersService;
beforeEach(async () => {
+ const mockUsersService = {
+ findAll: jest.fn().mockResolvedValue([]),
+ create: jest.fn().mockImplementation((createUserInput: CreateUserInput) => {
+ const user = new User();
+ user.id = 1;
+ user.name = createUserInput.name;
+ user.age = createUserInput.age;
+ return user;
+ }),
+ };
+
const module: TestingModule = await Test.createTestingModule({
- providers: [UsersResolver],
+ providers: [
+ UsersResolver,
+ { provide: UsersService, useValue: mockUsersService },
+ ],
}).compile();
resolver = module.get<UsersResolver>(UsersResolver);
+ service = module.get<UsersService>(UsersService);
});
it('should be defined', () => {
expect(resolver).toBeDefined();
});
+
+ describe('findAll', () => {
+ it('should return an array of users', async () => {
+ const result = await resolver.findAll();
+ expect(result).toEqual([]);
+ expect(service.findAll).toHaveBeenCalled();
+ });
+ });
+
+ describe('createUser', () => {
+ it('should create a new user', async () => {
+ const createUserInput: CreateUserInput = { name: 'Test User', age: 25 };
+ const result = await resolver.createUser(createUserInput);
+ expect(result).toEqual({
+ id: 1,
+ name: 'Test User',
+ age: 25,
+ });
+ expect(service.create).toHaveBeenCalledWith(createUserInput);
+ });
+ });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('UsersResolver', () => { | |
| let resolver: UsersResolver; | |
| beforeEach(async () => { | |
| const module: TestingModule = await Test.createTestingModule({ | |
| providers: [UsersResolver], | |
| }).compile(); | |
| resolver = module.get<UsersResolver>(UsersResolver); | |
| }); | |
| it('should be defined', () => { | |
| expect(resolver).toBeDefined(); | |
| }); | |
| }); | |
| import { Test, TestingModule } from '@nestjs/testing'; | |
| import { UsersResolver } from './users.resolver'; | |
| import { UsersService } from './users.service'; | |
| import { CreateUserInput } from './dto/create-user.input/create-user.input'; | |
| import { User } from './entities/user/user'; | |
| describe('UsersResolver', () => { | |
| let resolver: UsersResolver; | |
| let service: UsersService; | |
| beforeEach(async () => { | |
| const mockUsersService = { | |
| findAll: jest.fn().mockResolvedValue([]), | |
| create: jest.fn().mockImplementation((createUserInput: CreateUserInput) => { | |
| const user = new User(); | |
| user.id = 1; | |
| user.name = createUserInput.name; | |
| user.age = createUserInput.age; | |
| return user; | |
| }), | |
| }; | |
| const module: TestingModule = await Test.createTestingModule({ | |
| providers: [ | |
| UsersResolver, | |
| { provide: UsersService, useValue: mockUsersService }, | |
| ], | |
| }).compile(); | |
| resolver = module.get<UsersResolver>(UsersResolver); | |
| service = module.get<UsersService>(UsersService); | |
| }); | |
| it('should be defined', () => { | |
| expect(resolver).toBeDefined(); | |
| }); | |
| describe('findAll', () => { | |
| it('should return an array of users', async () => { | |
| const result = await resolver.findAll(); | |
| expect(result).toEqual([]); | |
| expect(service.findAll).toHaveBeenCalled(); | |
| }); | |
| }); | |
| describe('createUser', () => { | |
| it('should create a new user', async () => { | |
| const createUserInput: CreateUserInput = { name: 'Test User', age: 25 }; | |
| const result = await resolver.createUser(createUserInput); | |
| expect(result).toEqual({ | |
| id: 1, | |
| name: 'Test User', | |
| age: 25, | |
| }); | |
| expect(service.create).toHaveBeenCalledWith(createUserInput); | |
| }); | |
| }); | |
| }); |
src/auth/auth.resolver.ts
Outdated
| @Resolver() | ||
| export class AuthResolver { | ||
| constructor(private readonly authService: AuthService) {} | ||
|
|
||
| @Mutation(() => String) | ||
| async login( | ||
| @Args('name') name: string, | ||
| @Args('password') password: string, | ||
| ): Promise<{ accessToken: string; refreshToken: string }> { | ||
| const user = await this.authService.validateUser(name, password); | ||
| if (!user) { | ||
| throw new Error('Invalid credentials'); | ||
| } | ||
| return this.authService.login(user); | ||
| } | ||
| } |
There was a problem hiding this comment.
Return type mismatch in login mutation.
The login mutation decorator specifies a String return type but actually returns an object with accessToken and refreshToken.
Update the mutation return type to match the actual return value:
- @Mutation(() => String)
+ @Mutation(() => LoginResponse)And define a LoginResponse object type:
@ObjectType()
class LoginResponse {
@Field()
accessToken: string;
@Field()
refreshToken: string;
}Don't forget to import the necessary decorators:
import { Resolver, Mutation, Args, ObjectType, Field } from '@nestjs/graphql';| "test:e2e": "jest --config ./test/jest-e2e.json" | ||
| }, | ||
| "dependencies": { | ||
| "@nestjs/apollo": "^13.0.3", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Dependency redundancy: bcrypt and bcryptjs.
The dependencies include both 'bcrypt' (line 33) and 'bcryptjs' (line 34), which serve the same purpose (password hashing). This redundancy is unnecessary and could lead to inconsistent usage across the codebase.
Choose one password hashing library and remove the other:
"dependencies": {
"@nestjs/apollo": "^13.0.3",
"@nestjs/common": "^11.0.7",
"@nestjs/core": "^11.0.7",
"@nestjs/graphql": "^13.0.3",
"@nestjs/jwt": "^11.0.0",
"@nestjs/passport": "^11.0.5",
"@nestjs/platform-express": "^11.0.7",
"@nestjs/swagger": "^11.0.3",
"@nestjs/typeorm": "^11.0.0",
"apollo-server-express": "^3.13.0",
- "bcrypt": "^5.1.1",
"bcryptjs": "^3.0.2",
"class-transformer": "^0.5.1",
"class-validator": "^0.14.1",
"graphql": "^16.10.0",
"passport": "^0.7.0",
"passport-jwt": "^4.0.1",I recommend keeping 'bcryptjs' as it's a pure JavaScript implementation with no compilation requirements, making it more portable across environments.
Also applies to: 26-27, 32-33, 34-39
src/app.module.ts
Outdated
| import { User } from './users/user.entity'; | ||
| import { GraphQLModule } from '@nestjs/graphql'; | ||
| import { ApolloDriver, ApolloDriverConfig } from '@nestjs/apollo'; | ||
| import { User } from './users/entities/user/user'; |
There was a problem hiding this comment.
Potential entity import inconsistency.
The User entity is imported from a new path, but according to the changes in UsersModule, there appears to be a shift away from a database-driven model to an in-memory system.
Verify that the User entity at the new path './users/entities/user/user' exists and is properly defined. If the application is truly moving to an in-memory user management system without TypeORM entities, this import might need to be removed or updated.
| POSTGRES_USER: postgres | ||
| POSTGRES_PASSWORD: 1234 | ||
| POSTGRES_DB: stockdb |
There was a problem hiding this comment.
Store Postgres credentials securely.
Hardcoded credentials (POSTGRES_USER, POSTGRES_PASSWORD, POSTGRES_DB) can lead to security risks. If this is for production, consider using Docker secrets or environment variables injected by your CI/CD pipeline.
| nestjs: | ||
| build: . | ||
| container_name: nestjs_app | ||
| restart: always | ||
| depends_on: | ||
| - postgres | ||
| ports: | ||
| - '3000:3000' | ||
| environment: | ||
| - DATABASE_HOST=postgres | ||
| - DATABASE_PORT=5432 | ||
| - DATABASE_USERNAME=postgres | ||
| - DATABASE_PASSWORD=1234 | ||
| - DATABASE_NAME=stockdb | ||
| networks: | ||
| - default |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use environment variable files for the NestJS service.
Plainly setting credentials in the docker-compose file is convenient for development but risky in production. Move these values into an .env file and reference them with env_file: to mitigate secrets exposure.
src/users/users.service.ts
Outdated
| import { InjectRepository } from '@nestjs/typeorm'; | ||
| import { User } from './user.entity'; | ||
| import { Repository } from 'typeorm'; | ||
| import { CreateUserInput } from './dto/create-user.input/create-user.input'; |
There was a problem hiding this comment.
Be mindful of concurrency with in-memory data.
private users = [] can lead to inconsistent data when concurrent requests arrive. For production usage, consider a database, or at least protect writes with locking or concurrency-safe data structures.
Also applies to: 6-6, 8-15
src/auth/auth.service.ts
Outdated
| // 사용자 비밀번호 확인 | ||
| async validateUser(name: string, pass: string): Promise<any> { | ||
| // 실제로는 DB에서 사용자를 조회하고 비밀번호 비교 | ||
| const user = { name: 'user', password: 'password' }; // 예시 | ||
| const isPasswordValid = await bcrypt.compare(pass, user.password); | ||
|
|
||
| // 중복 아이디 체크 | ||
| const existingUser = await this.usersService.findOne(username); | ||
| if (existingUser) { | ||
| throw new ConflictException('Username already exists'); | ||
| if (isPasswordValid) { | ||
| return user; | ||
| } | ||
|
|
||
| // 비밀번호 암호화 | ||
| const hashedPassword = await bcrypt.hash(password, 10); | ||
|
|
||
| // 유저 생성 후 반환 | ||
| return this.usersService.createUser(username, hashedPassword); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Misleading placeholder password usage
Using bcrypt.compare on a hardcoded, non-hashed string ('password') will only succeed if the input password is 'password'. This approach is misleading and not secure unless it is strictly for placeholder logic. For production, ensure passwords are stored and compared in a secure, hashed form.
src/auth/auth.service.ts
Outdated
| // JWT 토큰 생성 | ||
| async login(user: any) { | ||
| const payload = { username: user.name, sub: user.id }; // 사용자 정보 | ||
| const accessToken = this.jwtService.sign(payload); // Access Token 생성 | ||
| const refreshToken = this.jwtService.sign(payload, { | ||
| secret: 'yourSecretKey', // Refresh Token의 secret은 다르게 설정할 수 있습니다. | ||
| expiresIn: '7d', // Refresh Token은 더 긴 기간 동안 유효합니다. | ||
| }); | ||
|
|
||
| const payload = { sub: user.id, username: user.username }; | ||
| return { | ||
| access_token: await this.jwtService.signAsync(payload), | ||
| }; | ||
| return { accessToken, refreshToken }; | ||
| } |
There was a problem hiding this comment.
Hardcoded refresh token secret & undefined user.id
- The code references
user.idwithout an actualidproperty in theuserobject, leading to potential runtime errors. - Storing secrets such as
'yourSecretKey'directly in code poses a security risk. Instead, inject these from environment variables (e.g.,process.env.JWT_REFRESH_SECRETor using a config service).
Here is a potential fix snippet to illustrate adjusting the sub field for a placeholder ID:
- const payload = { username: user.name, sub: user.id };
+ const payload = { username: user.name, sub: 1 }; // or retrieve user.id from your data source📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // JWT 토큰 생성 | |
| async login(user: any) { | |
| const payload = { username: user.name, sub: user.id }; // 사용자 정보 | |
| const accessToken = this.jwtService.sign(payload); // Access Token 생성 | |
| const refreshToken = this.jwtService.sign(payload, { | |
| secret: 'yourSecretKey', // Refresh Token의 secret은 다르게 설정할 수 있습니다. | |
| expiresIn: '7d', // Refresh Token은 더 긴 기간 동안 유효합니다. | |
| }); | |
| const payload = { sub: user.id, username: user.username }; | |
| return { | |
| access_token: await this.jwtService.signAsync(payload), | |
| }; | |
| return { accessToken, refreshToken }; | |
| } | |
| // JWT 토큰 생성 | |
| async login(user: any) { | |
| const payload = { username: user.name, sub: 1 }; // or retrieve user.id from your data source | |
| const accessToken = this.jwtService.sign(payload); // Access Token 생성 | |
| const refreshToken = this.jwtService.sign(payload, { | |
| secret: 'yourSecretKey', // Refresh Token의 secret은 다르게 설정할 수 있습니다. | |
| expiresIn: '7d', // Refresh Token은 더 긴 기간 동안 유효합니다. | |
| }); | |
| return { accessToken, refreshToken }; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
package.json (1)
32-34: 🛠️ Refactor suggestionDependency redundancy: bcrypt and bcryptjs.
The dependencies include both 'bcrypt' (line 33) and 'bcryptjs' (line 34), which serve the same purpose (password hashing). This redundancy is unnecessary and could lead to inconsistent usage across the codebase.
Choose one password hashing library and remove the other:
"dependencies": { "@nestjs/apollo": "^13.0.3", "@nestjs/common": "^11.0.7", "@nestjs/core": "^11.0.7", "@nestjs/graphql": "^13.0.3", "@nestjs/jwt": "^11.0.0", "@nestjs/passport": "^11.0.5", "@nestjs/platform-express": "^11.0.7", "@nestjs/swagger": "^11.0.3", "@nestjs/typeorm": "^11.0.0", "apollo-server-express": "^3.13.0", - "bcrypt": "^5.1.1", "bcryptjs": "^3.0.2",I recommend keeping 'bcryptjs' as it's a pure JavaScript implementation with no compilation requirements, making it more portable across environments.
src/auth/auth.module.ts (1)
13-13:⚠️ Potential issueAvoid storing the JWT secret in source code.
For security best practices, store
'your_secret_key'in environment variables or a secure vault rather than hardcoding it.- secret: 'your_secret_key', + secret: process.env.JWT_SECRET,
🧹 Nitpick comments (8)
src/user/dto/login-response.dto.ts (1)
4-11: Consider adding descriptions to GraphQL fields for better documentation.The
LoginResponseclass properly defines the structure for returning JWT tokens. For improved documentation and API clarity, consider adding descriptions to the@Field()decorators.@ObjectType() export class LoginResponse { - @Field() + @Field({ description: 'JWT access token for authenticating requests' }) accessToken: string; - @Field() + @Field({ description: 'JWT refresh token for obtaining new access tokens' }) refreshToken: string; }src/auth/jwt.strategy.ts (1)
1-26: Add error handling for token expiration.The current implementation doesn't handle JWT token expiration errors explicitly. Consider adding comprehensive error handling.
You could implement a custom exception filter to handle JWT-related errors:
// src/auth/jwt-exception.filter.ts import { ExceptionFilter, Catch, ArgumentsHost, HttpStatus } from '@nestjs/common'; import { Response } from 'express'; import { JsonWebTokenError, TokenExpiredError } from 'jsonwebtoken'; @Catch(JsonWebTokenError, TokenExpiredError) export class JwtExceptionFilter implements ExceptionFilter { catch(exception: JsonWebTokenError | TokenExpiredError, host: ArgumentsHost) { const ctx = host.switchToHttp(); const response = ctx.getResponse<Response>(); let status = HttpStatus.UNAUTHORIZED; let message = 'Unauthorized'; if (exception instanceof TokenExpiredError) { message = 'Token has expired'; } response.status(status).json({ statusCode: status, message, timestamp: new Date().toISOString(), }); } }Then register this filter in your application.
src/user/user.entity.ts (1)
1-18: User entity implementation looks good with proper separation of GraphQL and database concerns.The entity is properly configured with TypeORM and GraphQL decorators. Good security practice keeping the password field out of the GraphQL schema (no @field decorator) while still persisting it to the database.
Consider adding methods for password handling like hashing and validation in this entity to encapsulate this logic.
You could add a password validation method to encapsulate password checking logic:
import * as bcryptjs from 'bcryptjs'; @ObjectType() // GraphQL에서 타입으로 사용 @Entity() export class User { // existing fields... async validatePassword(password: string): Promise<boolean> { return bcryptjs.compare(password, this.password); } }src/user/user.resolver.ts (2)
53-55: Unused error variable in catch block.The error variable is defined but never used in the catch block.
- } catch (error) { + } catch (_) { throw new Error('Invalid refresh token'); }Or to provide better error information:
} catch (error) { - throw new Error('Invalid refresh token'); + throw new Error(`Invalid refresh token: ${error.message}`); }🧰 Tools
🪛 ESLint
[error] 53-53: 'error' is defined but never used.
(@typescript-eslint/no-unused-vars)
16-22: Implement input validation for user registration.The register mutation should validate the username and password inputs to ensure they meet security and business requirements.
Consider using class-validator with GraphQL input types:
// src/user/dto/register.input.ts import { InputType, Field } from '@nestjs/graphql'; import { IsString, MinLength, MaxLength, Matches } from 'class-validator'; @InputType() export class RegisterInput { @Field() @IsString() @MinLength(3) @MaxLength(20) username: string; @Field() @IsString() @MinLength(8) @Matches(/^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)[a-zA-Z\d]{8,}$/, { message: 'Password too weak', }) password: string; }Then update your resolver:
@Mutation(() => User) async register( - @Args('username') username: string, - @Args('password') password: string, + @Args('input') input: RegisterInput, ): Promise<User> { - return this.userService.register(username, password); + return this.userService.register(input.username, input.password); }src/auth/auth.module.ts (1)
14-14: Consider a shorter expiration time for better security.60 minutes is a relatively long time for a JWT token. For better security, consider reducing this time and implementing a refresh token strategy if you haven't already.
- signOptions: { expiresIn: '60m' }, + signOptions: { expiresIn: '15m' },src/user/user.service.ts (2)
49-51: Consider adding error handling for findByUsername.To maintain consistent behavior, consider adding error handling when a user isn't found.
async findByUsername(username: string): Promise<User> { - return this.userRepository.findOne({ where: { username } }); + const user = await this.userRepository.findOne({ where: { username } }); + if (!user) { + throw new NotFoundException(`User with username "${username}" not found`); + } + return user; }
53-57: Consider storing refresh tokens in database.For enhanced security and the ability to revoke tokens, consider storing refresh tokens in the database with associations to users.
📜 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 (17)
package.json(1 hunks)src/app.module.ts(2 hunks)src/app.resolver.ts(1 hunks)src/auth/auth.guard.ts(1 hunks)src/auth/auth.module.ts(1 hunks)src/auth/auth.service.spec.ts(0 hunks)src/auth/auth.service.ts(0 hunks)src/auth/interfaces/jwt-payload.interface.ts(1 hunks)src/auth/jwt.strategy.ts(1 hunks)src/user/dto/login-response.dto.ts(1 hunks)src/user/user.entity.ts(1 hunks)src/user/user.module.ts(1 hunks)src/user/user.resolver.ts(1 hunks)src/user/user.service.ts(1 hunks)src/users/users.module.ts(0 hunks)src/users/users.service.spec.ts(0 hunks)src/users/users.service.ts(0 hunks)
💤 Files with no reviewable changes (5)
- src/users/users.service.spec.ts
- src/auth/auth.service.ts
- src/users/users.service.ts
- src/auth/auth.service.spec.ts
- src/users/users.module.ts
✅ Files skipped from review due to trivial changes (1)
- src/auth/interfaces/jwt-payload.interface.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app.module.ts
🧰 Additional context used
🪛 ESLint
src/user/user.resolver.ts
[error] 53-53: 'error' is defined but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (6)
src/app.resolver.ts (1)
4-10: LGTM! Simple hello world GraphQL query.The
AppResolverimplements a basic GraphQL query that serves as a good starting point or health check endpoint.src/user/user.module.ts (1)
9-20: Add refresh token configuration.The
LoginResponseDTO includes a refresh token field, but there's no configuration for generating or validating refresh tokens in the module.Consider adding separate configuration for refresh tokens with a longer expiration time than access tokens:
JwtModule.register({ secret: process.env.JWT_SECRET, signOptions: { expiresIn: '60m' }, }), + // You may need to add a method in UserService to handle refresh token generation + // with different expiration settings (e.g., '7d' instead of '60m')src/auth/auth.guard.ts (1)
1-6: Good simplification of authentication guard.The switch to extending NestJS's built-in AuthGuard is a good practice that simplifies the code while leveraging the framework's capabilities. This approach is more maintainable and follows NestJS best practices.
src/auth/auth.module.ts (2)
10-11: LGTM: Good modular approach.The imports look good. The comment on line 11 is helpful for explaining why UserModule is necessary.
17-18: LGTM: Clean provider and export configuration.The simplified providers and exports array now focusing on JWT strategy looks good.
src/user/user.service.ts (1)
40-45: Good implementation of access and refresh tokens.The approach with short-lived access tokens and longer-lived refresh tokens follows security best practices.
| JwtModule.register({ | ||
| secret: 'your_secret_key', // 실제 운영 시에는 환경 변수 사용 권장 | ||
| signOptions: { expiresIn: '60m' }, | ||
| }), |
There was a problem hiding this comment.
Use environment variables for JWT secret key.
Hardcoding sensitive information like JWT secret keys directly in the source code is a security risk, even with comments noting it should be changed in production.
JwtModule.register({
- secret: 'your_secret_key', // 실제 운영 시에는 환경 변수 사용 권장
+ secret: process.env.JWT_SECRET, // Use environment variable for security
signOptions: { expiresIn: '60m' },
}),Also, consider adding validation to ensure the environment variable is set during application startup.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| JwtModule.register({ | |
| secret: 'your_secret_key', // 실제 운영 시에는 환경 변수 사용 권장 | |
| signOptions: { expiresIn: '60m' }, | |
| }), | |
| JwtModule.register({ | |
| secret: process.env.JWT_SECRET, // Use environment variable for security | |
| signOptions: { expiresIn: '60m' }, | |
| }), |
| constructor(private readonly userService: UserService) { | ||
| super({ | ||
| jwtFromRequest: ExtractJwt.fromAuthHeaderAsBearerToken(), | ||
| secretOrKey: 'your_secret_key', // JWT secret |
There was a problem hiding this comment.
Use environment variables for JWT secret key.
The hardcoded JWT secret should match the one used in JwtModule and should be stored in environment variables for security.
super({
jwtFromRequest: ExtractJwt.fromAuthHeaderAsBearerToken(),
- secretOrKey: 'your_secret_key', // JWT secret
+ secretOrKey: process.env.JWT_SECRET,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| secretOrKey: 'your_secret_key', // JWT secret | |
| super({ | |
| jwtFromRequest: ExtractJwt.fromAuthHeaderAsBearerToken(), | |
| secretOrKey: process.env.JWT_SECRET, | |
| }); |
| async validate(payload: JwtPayload) { | ||
| const { username } = payload; | ||
| const user = await this.userService.findByUsername(username); | ||
| if (!user) { | ||
| throw new Error('Unauthorized'); | ||
| } | ||
| return user; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use NestJS UnauthorizedException instead of generic Error.
For consistent error handling and proper HTTP status codes, use NestJS's built-in exceptions.
async validate(payload: JwtPayload) {
const { username } = payload;
const user = await this.userService.findByUsername(username);
if (!user) {
- throw new Error('Unauthorized');
+ throw new UnauthorizedException('User not found or token invalid');
}
return user;
}Don't forget to import UnauthorizedException:
+ import { Injectable, UnauthorizedException } from '@nestjs/common';
- import { Injectable } from '@nestjs/common';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async validate(payload: JwtPayload) { | |
| const { username } = payload; | |
| const user = await this.userService.findByUsername(username); | |
| if (!user) { | |
| throw new Error('Unauthorized'); | |
| } | |
| return user; | |
| } | |
| import { Injectable, UnauthorizedException } from '@nestjs/common'; | |
| @Injectable() | |
| export class JwtStrategy { | |
| async validate(payload: JwtPayload) { | |
| const { username } = payload; | |
| const user = await this.userService.findByUsername(username); | |
| if (!user) { | |
| throw new UnauthorizedException('User not found or token invalid'); | |
| } | |
| return user; | |
| } | |
| } |
| // refresh token 검증 (실제로는 별도의 secret을 사용할 수도 있음) | ||
| const payload = this.userService['jwtService'].verify(refreshToken, { | ||
| secret: 'your_secret_key', | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid accessing private properties and hard-coded secrets.
Direct access to the service's private property using bracket notation (this.userService['jwtService']) bypasses TypeScript's visibility modifiers and makes the code less maintainable. Additionally, hard-coded secrets should be moved to configuration.
Consider these changes:
- Update the UserService to provide a proper method for token verification
- Move the secret key to configuration
- const payload = this.userService['jwtService'].verify(refreshToken, {
- secret: 'your_secret_key',
- });
+ const payload = await this.userService.verifyRefreshToken(refreshToken);And in UserService:
async verifyRefreshToken(refreshToken: string) {
return this.jwtService.verify(refreshToken, {
secret: this.configService.get<string>('JWT_REFRESH_SECRET'),
});
}| const newAccessToken = this.userService['jwtService'].sign( | ||
| { username: payload.username, sub: payload.sub }, | ||
| { expiresIn: '15m' }, | ||
| ); | ||
| // (선택사항) refresh token 로테이션: 새 refresh token 발급 | ||
| const newRefreshToken = this.userService['jwtService'].sign( | ||
| { username: payload.username, sub: payload.sub }, | ||
| { expiresIn: '7d' }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoded token expiration times and duplicate token generation logic.
Token expiration times should be configurable, and the token generation logic is duplicated from what's likely in the UserService.
Extract this logic to the UserService:
- const newAccessToken = this.userService['jwtService'].sign(
- { username: payload.username, sub: payload.sub },
- { expiresIn: '15m' },
- );
- const newRefreshToken = this.userService['jwtService'].sign(
- { username: payload.username, sub: payload.sub },
- { expiresIn: '7d' },
- );
+ const { accessToken: newAccessToken, refreshToken: newRefreshToken } =
+ await this.userService.generateTokens(payload.username, payload.sub);Then implement in UserService:
async generateTokens(username: string, userId: number) {
const payload = { username, sub: userId };
return {
accessToken: this.jwtService.sign(payload, {
expiresIn: this.configService.get<string>('JWT_ACCESS_EXPIRES', '15m'),
}),
refreshToken: this.jwtService.sign(payload, {
expiresIn: this.configService.get<string>('JWT_REFRESH_EXPIRES', '7d'),
}),
};
}| if (!user) { | ||
| throw new Error('User not found'); | ||
| } | ||
| const isPasswordValid = await bcrypt.compare(password, user.password); | ||
| if (!isPasswordValid) { | ||
| throw new Error('Invalid credentials'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace generic Error with NestJS HTTP exceptions.
Using NestJS's built-in exception filters provides more consistent error handling and proper HTTP status codes.
if (!user) {
- throw new Error('User not found');
+ throw new NotFoundException('User not found');
}
const isPasswordValid = await bcrypt.compare(password, user.password);
if (!isPasswordValid) {
- throw new Error('Invalid credentials');
+ throw new UnauthorizedException('Invalid credentials');
}Don't forget to add the imports:
import { Injectable, NotFoundException, UnauthorizedException } from '@nestjs/common';| // 회원가입: username과 비밀번호를 받아 새 사용자를 생성합니다. | ||
| async register(username: string, password: string): Promise<User> { | ||
| const hashedPassword = await bcrypt.hash(password, 10); | ||
| const newUser = this.userRepository.create({ | ||
| username, | ||
| password: hashedPassword, | ||
| }); | ||
| return this.userRepository.save(newUser); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add check for existing username before registration.
Currently, there's no validation to prevent duplicate usernames during registration.
async register(username: string, password: string): Promise<User> {
+ const existingUser = await this.userRepository.findOne({ where: { username } });
+ if (existingUser) {
+ throw new ConflictException('Username already exists');
+ }
const hashedPassword = await bcrypt.hash(password, 10);
const newUser = this.userRepository.create({
username,
password: hashedPassword,
});
return this.userRepository.save(newUser);
}Add to imports:
import { Injectable, ConflictException, NotFoundException, UnauthorizedException } from '@nestjs/common';1 participant
Summary by CodeRabbit
New Features
Chores