Refactor/#17/auth response and test #18
Conversation
- 사용자 인증을 위해 CustomAuthGuard와 에러 메세지 추가
- 응답 포맷팅을 service로 이동 - 출력 처리를 service에 위임하여 controller를 단순화함
jungyoonho128
requested review from
sim16065,
Ryu-02,
dg1418,
gwgw123,
jeonheena and
a team
as code owners
jungyoonho128
requested review from
rrgks6221 and
kimjiwonpg98
and removed request for
a team
src/auth/user.controller.ts
Outdated
| export class CustomJwtAuthGuard extends AuthGuard('jwt') { | ||
| handleRequest(err, user, info) { | ||
| if (err || !user) { | ||
| if (info?.name === 'TokenExpiredError') { |
There was a problem hiding this comment.
info의 name보다는 이거는 어떤가요?
| if (info?.name === 'TokenExpiredError') { | |
| if (err instanceof TokenExpiredError) { |
There was a problem hiding this comment.
에러 이름이 아니라 인스턴스인지 확인하는 방식이네요! 새로 배워갑니다
There was a problem hiding this comment.
좋은 리뷰 감사합니다! instanceof를 사용하면 실제로 해당 에러가 TokenExpiredError 인스턴스인지 정확히 확인할 수 있어서 더 안전하다는 점은 이번에 처음 알게 됐어요. 말씀해주신 방식이 타입 안정성이나 유지보수 면에서 확실히 더 좋은 것 같아서, 해당 부분은 instanceof로 수정해보겠습니다. 감사합니다!
src/auth/user.controller.ts
Outdated
| import { AuthGuard } from '@nestjs/passport'; | ||
| import { ApiBearerAuth, ApiOperation, ApiTags } from '@nestjs/swagger'; | ||
|
|
||
| @Injectable() | ||
| export class CustomJwtAuthGuard extends AuthGuard('jwt') { |
There was a problem hiding this comment.
리뷰 감사합니다! 말씀해주신 대로 가드를 별도 파일로 분리해서 관리하도록 변경했습니다. 좋은 리뷰 감사드립니다!
src/auth/user.controller.ts
Outdated
| @Injectable() | ||
| export class CustomJwtAuthGuard extends AuthGuard('jwt') { | ||
| handleRequest(err, user, info) { | ||
| if (err || !user) { | ||
| if (info?.name === 'TokenExpiredError') { | ||
| throw new UnauthorizedException( | ||
| '토큰이 만료되었습니다. 다시 로그인해주세요.', | ||
| ); | ||
| } else if (info?.name === 'JsonWebTokenError') { | ||
| throw new UnauthorizedException('유효하지 않은 토큰입니다.'); | ||
| } else { | ||
| throw new UnauthorizedException('인증에 실패했습니다.'); | ||
| } | ||
| } | ||
| return user; | ||
| } | ||
| } |
There was a problem hiding this comment.
@Injectable()
export class CustomJwtAuthGuard extends AuthGuard('jwt') {
handleRequest(err, user, info) {
if (err instanceof TokenExpiredError) {
throw new UnauthorizedException(
'토큰이 만료되었습니다. 다시 로그인해주세요.',
);
}
if (err instanceof JsonWebTokenError) {
throw new UnauthorizedException('유효하지 않은 토큰입니다.');
}
if (err || !user) {
throw new UnauthorizedException('인증에 실패했습니다.');
}
return user;
}
}if else문을 정리해서 Early Return 형식으로 정리해봤습니다.
코드를 좀 더 읽기 쉽게 만들어 줘서 저는 이런 방식을 선호하는 편입니다.
There was a problem hiding this comment.
리뷰 감사합니다! 말씀해주신 방식도 깔끔해서 참고할 만한 것 같아요. 다만 지금은 현재 코드 흐름이 더 익숙해서 그대로 사용해봤습니다 :)
| httpOnly: true, | ||
| secure: true, | ||
| sameSite: 'strict', | ||
| maxAge: 7 * 24 * 60 * 60 * 1000, |
There was a problem hiding this comment.
쿠키나, 토큰 만료 시간 같은 상수값은 .env나 상수파일로 분리하는게 좋아보입니다 ~
There was a problem hiding this comment.
좋은 리뷰 감사합니다. 말씀 해 주신 대로 .env 파일로 따로 분리해주었습니다
src/auth/auth.service.ts
Outdated
| return { | ||
| status: 'success', | ||
| message: '회원가입 성공', | ||
| data: null, | ||
| }; |
There was a problem hiding this comment.
어떤 의도로 status, message를 service 리턴으로 추가하고 이런 규격으로 지정했는지는 모르겠습니다
status, message 값은 API의 응답으로 정의하신거로 보이는데요
controller, service, repository의 역할을 간단히 보면 아래처럼 볼 때 status나 message는 컨트롤러 영역에서 제공하는게 올바를거같습니다
controller: request, response에 대한 제어
service: repository 또는 컨트롤러와 연결짓는 레이어 (일반적으로 서버 외부 어뎁터(컨트롤러, repository)들을 이용해 비즈니스 로직을 수행함)
repository: 영속성을 관리
There was a problem hiding this comment.
리뷰 남겨주셔서 감사합니다. 모든 API 응답을 status, message, data 구조로 통일하고 있어서, service에서 이 형식의 응답 객체를 리턴하게 하면 controller에서 중복된 처리 없이 일관된 응답을 쉽게 구성할 수 있다고 판단했습니다. 하지만 관련 자료들을 찾아보고 리뷰 주신 내용을 확인해보니, 역할 분리 관점에서 controller에서 응답을 조립하는 방식이 더 적절하다는 걸 알게 되었습니다. 좋은 방법 알려주셔서 감사합니다!
There was a problem hiding this comment.
중복된 처리 없이 일관된 응답을 쉽게 구성
넵 이부분은 DTO를 생성하여 컨트롤러에서 처리하거나 인터셉터를 활용하는 방안도 있을거같습니다
- AuthService는 처리만, AuthController에서 응답 통일 - JWT·쿠키 설정은 .env 관리 - 인증 가드 분리해 예외 처리 명확화
관련 이슈
📝 제목
📋 작업 내용
🔧 기술 변경
🚀 테스트 사항(선택)
💬 리뷰 요구사항 (선택)