Conversation
Summary of ChangesHello @hyobin-yang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's authentication and user management capabilities. It introduces a structured approach to user registration, login, and password recovery, alongside features like email verification for different purposes and nickname availability checks. The changes involve creating new services, data transfer objects, and API endpoints, while also refactoring existing code for improved organization and maintainability. Comprehensive validation and error handling have been integrated into the new features. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이 PR은 인증(회원가입, 로그인, 비밀번호 재설정), 이메일 인증, 사용자 관리(닉네임 중복 확인) 기능을 추가하고, 패키지 구조를 리팩토링하는 등 많은 변경 사항을 포함하고 있습니다. 전반적으로 좋은 방향의 개선이지만, 몇 가지 중요한 수정이 필요합니다.
가장 시급한 문제는 비밀번호가 평문으로 저장되고 있다는 점입니다. 이는 심각한 보안 취약점이므로 반드시 PasswordEncoder를 사용하여 해싱해야 합니다. 또한, 컨트롤러에서 인증된 사용자 정보를 받아오는 방식에 문제가 있어 런타임 오류가 발생할 수 있습니다.
그 외에도 중복 사용자 가입 방지 로직 추가, API 응답 형식 개선, 코드 안정성 향상을 위한 몇 가지 제안 사항들을 리뷰에 포함했습니다. 추가된 테스트 코드는 매우 좋지만, 제안된 수정 사항들을 반영하여 함께 업데이트해야 합니다.
| public void resetPassword(PasswordDto dto, User user){ | ||
| user.changePassword(dto.getPassword()); | ||
| } |
There was a problem hiding this comment.
비밀번호를 데이터베이스에 평문으로 저장하는 것은 심각한 보안 취약점입니다. Spring Security의 PasswordEncoder를 주입받아 signup과 resetPassword 메소드에서 비밀번호를 저장하기 전에 반드시 해싱해야 합니다. login 메소드에서는 passwordEncoder.matches()를 사용하여 비밀번호를 검증해야 합니다. @RequiredArgsConstructor를 통해 주입받으려면 PasswordEncoder를 final 필드로 추가해야 합니다.
public void resetPassword(PasswordDto dto, User user){
user.changePassword(passwordEncoder.encode(dto.getPassword()));
}| public void resetPassword(@Valid @RequestBody PasswordDto request, User user){ | ||
| authService.resetPassword(request, user); | ||
| } |
There was a problem hiding this comment.
resetPassword 메소드의 User 파라미터에 어노테이션이 없어 Spring MVC가 이를 해석할 수 없습니다. 이는 런타임 오류를 발생시킬 것입니다. @AuthenticationPrincipal 같은 어노테이션을 사용하여 Security Context로부터 인증된 사용자 정보를 가져와야 합니다. 이를 위해서는 Spring Security 설정이 되어있고, User 엔티티가 UserDetails를 구현하거나 커스텀 argument resolver가 필요합니다.
| public void resetPassword(@Valid @RequestBody PasswordDto request, User user){ | |
| authService.resetPassword(request, user); | |
| } | |
| public void resetPassword(@Valid @RequestBody PasswordDto request, @org.springframework.security.core.annotation.AuthenticationPrincipal User user){ | |
| authService.resetPassword(request, user); | |
| } |
| public void signup(SignupDto dto){ | ||
| // TODO: bio, 닉네임에 금칙어 검사 | ||
| SignupVo vo = new SignupVo( | ||
| dto.getName(), | ||
| dto.getBirthdate(), | ||
| dto.getEmail(), | ||
| dto.getPassword(), | ||
| dto.getProfileImage(), | ||
| dto.getNickname(), | ||
| dto.getBio() | ||
| ); | ||
| User user = User.from(vo); | ||
| userRepository.save(user); | ||
| } |
There was a problem hiding this comment.
signup 메소드에서 이메일이나 닉네임이 이미 존재하는지 확인하는 로직이 없습니다. User 엔티티에서 email과 nickname이 유니크 필드이므로, 중복된 값으로 저장을 시도하면 DataIntegrityViolationException이 발생하여 사용자 경험을 해칠 수 있습니다. 중복 여부를 미리 확인하고, 명확한 BusinessException을 발생시키는 것이 좋습니다. AuthErrorStatus에 EMAIL_ALREADY_EXISTS, NICKNAME_ALREADY_EXISTS 같은 에러 상태를 추가하여 사용할 수 있습니다.
public void signup(SignupDto dto){
if (userRepository.existsByEmail(dto.getEmail())) {
throw new com.daramg.server.common.exception.BusinessException(com.daramg.server.auth.exception.AuthErrorStatus.EMAIL_ALREADY_EXISTS);
}
if (userRepository.existsByNickname(dto.getNickname())) {
throw new com.daramg.server.common.exception.BusinessException(com.daramg.server.auth.exception.AuthErrorStatus.NICKNAME_ALREADY_EXISTS);
}
// TODO: bio, 닉네임에 금칙어 검사
SignupVo vo = new SignupVo(
dto.getName(),
dto.getBirthdate(),
dto.getEmail(),
dto.getPassword(),
dto.getProfileImage(),
dto.getNickname(),
dto.getBio()
);
User user = User.from(vo);
userRepository.save(user);
}| public void login(LoginDto dto){ | ||
| } |
There was a problem hiding this comment.
login 메소드가 비어있습니다. 아직 개발 중인 것으로 보입니다. 로그인 로직 구현을 잊지 마세요. 일반적으로 이메일로 사용자를 찾고, 다른 리뷰에서 언급한 PasswordEncoder.matches()로 비밀번호를 확인한 뒤, 성공 시 인증 토큰(예: JWT)을 발급하는 과정이 포함되어야 합니다.
| switch (request.getEmailPurpose()) { | ||
| case SIGNUP -> sendForSignup(request); | ||
| case PASSWORD_RESET -> sendForPasswordReset(request); | ||
| default -> throw new BusinessException("지원하지 않는 이메일 발송 목적입니다."); |
There was a problem hiding this comment.
switch 문의 default 케이스에서 하드코딩된 문자열 메시지로 BusinessException을 발생시키고 있습니다. 에러 처리의 일관성과 유지보수 용이성을 위해 AuthErrorStatus와 같은 enum을 사용하여 구조화된 에러 코드를 사용하는 것이 좋습니다. AuthErrorStatus enum에 UNSUPPORTED_EMAIL_PURPOSE 같은 항목을 추가하여 사용하시는 것을 권장합니다.
| default -> throw new BusinessException("지원하지 않는 이메일 발송 목적입니다."); | |
| default -> throw new BusinessException(com.daramg.server.auth.exception.AuthErrorStatus.UNSUPPORTED_EMAIL_PURPOSE); |
| @@ -0,0 +1,3 @@ | |||
| package com.daramg.server.auth.dto | |||
|
|
|||
| class LoginDto(var email: String, var password: String) No newline at end of file | |||
There was a problem hiding this comment.
DTO의 프로퍼티가 var로 선언되어 변경 가능(mutable)합니다. DTO는 불변(immutable) 객체로 만드는 것이 좋은 관행입니다. val을 사용하면 의도치 않은 상태 변경을 막아 코드를 더 안전하고 예측 가능하게 만들 수 있습니다. 이 내용은 PasswordDto.kt에도 동일하게 적용됩니다.
| class LoginDto(var email: String, var password: String) | |
| class LoginDto(val email: String, val password: String) |
| regexp = "^(?=.*[a-z])(?=.*[A-Z])(?=.*[0-9])(?=.*[!@#\$%^&*()]).{10,}\$", | ||
| message = "비밀번호는 영어 대/소문자, 숫자, 특수문자를 모두 포함하여 10자 이상이어야 합니다" | ||
| ) | ||
| var password: String |
There was a problem hiding this comment.
password 프로퍼티가 var로 선언되어 변경 가능(mutable)합니다. DTO는 불변(immutable) 객체로 만들기 위해 val을 사용하는 것이 좋습니다. 이는 의도치 않은 상태 변경을 방지합니다.
| var password: String | |
| val password: String |
| ) | ||
| String nickname) { | ||
| boolean isAvailable = userService.isNicknameAvailable(nickname); | ||
| return ResponseEntity.ok(Map.of("닉네임 사용 가능 유무: ", isAvailable)); |
There was a problem hiding this comment.
API 응답의 JSON 키로 한글과 공백이 포함된 "닉네임 사용 가능 유무: "를 사용하고 있습니다. API 응답에서는 클라이언트 호환성과 예측 가능성을 위해 영문 camelCase 키(예: isAvailable)를 사용하는 것이 표준적인 방식입니다.
| return ResponseEntity.ok(Map.of("닉네임 사용 가능 유무: ", isAvailable)); | |
| return ResponseEntity.ok(Map.of("isAvailable", isAvailable)); |
| private UserService userService; | ||
|
|
||
| @Test | ||
| void 닉네임_중복_확인_사용가능() throws Exception { |
There was a problem hiding this comment.
테스트 이름 닉네임_중복_확인_사용가능은 오해의 소지가 있습니다. given(userService.isNicknameAvailable(nickname)).willReturn(false); 설정은 닉네임을 사용할 수 없는 경우(중복된 경우)를 테스트하고 있습니다. 테스트의 목적을 명확하게 나타내도록 닉네임_중복_확인_사용불가와 같이 이름을 변경하는 것이 좋겠습니다.
hyobin-yang
changed the title
1 participant
No description provided.