Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[FEAT] 로그아웃 구현 및 redis를 통한 임시 유저 저장 구현 #25

Merged
merged 37 commits into from
Jan 30, 2024

Conversation

mikekks
Copy link
Contributor

@mikekks mikekks commented Jan 26, 2024

구현 내용/방법

간단하게 구현한 내용과 방법에 대한 설명

  • 로그아웃을 구현했습니다. 자세한 방법은 노션 Back 공간에 첨부했습니다! 간단히 말하면 저장된 리프레시 토큰을 플러시 하는 방식으로 구현했습니다.
  • 기존에는 인증되지 않는 유저를 db에 저장했었는데요. 인증이 되지 않는 유저를 redis에 저장하여 db접근 횟수를 줄였고, 임시 유저이기에 redis에 저장하는 것이 적합해보였습니다!

리뷰 필요

  • 보안을 고려했을 때, 회원가입 과정에서 어쩔 수 없이 서버와의 통신을 3번 해야한다고 생각했습니다!
  • 구현에 급급하여 의존성이 이곳저곳 되어있는 부분이 많이 아쉽네요. 파사드 패턴을 적용하고 싶었지만 그렇게 되면 시간이 너무 오래걸릴거 같아서 일단 PR를 날렸습니다!

구현 캡쳐

스크린샷 2024-01-27 오전 12 41 08 스크린샷 2024-01-27 오전 12 41 46

@mikekks mikekks added Priority: High 우선순위: 높음 Status: Available 상태: 작업을 바로 시작할 수 있는 상태 labels Jan 26, 2024
@mikekks mikekks added the Type: Feature 유형: 기능 추가 label Jan 26, 2024
@mikekks mikekks requested a review from Goder-0 January 26, 2024 15:36
@mikekks mikekks self-assigned this Jan 26, 2024
Comment on lines 9 to 11
public static AuthUserResponseDto of(String platformId, AuthType authType, String userName, Role role,
String accessToken, String refreshToken) {
return new UserAuthResponseDTO(platformId, authType, userName, role, accessToken, refreshToken);
return new AuthUserResponseDto(platformId, authType, userName, role, accessToken, refreshToken);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DTO에서 Dto로 수정하셨네요!
그런데 of 함수가 정의된 일부 record dto가 보이는데, 혹시 어떠한 의도로 작성하신걸까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

팩토리 메서드를 의도한거였는데 조금 더 자세히 설명주실 수 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음 제가 원하는건 팩토리 메서드였고, 그렇기에 record 클래스에 생성 역할을 위임하고 싶었습니다! service 로직에서 new를 사용하는 것보다 유지보수 측면에서 좋다고 생각했습니다!
또한, 컨벤션으로 많이 사용하는 것으로 알고있기 때문에 사용했었습니다!

혹시 생성자를 사용하는 다른 의도나 이유가 있으신가요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하! 이해했습니다.
팩토리 메소드를 의도했다는 의미를 이해하지 못하고 있었네요!
정적 팩토리 메소드 네이밍 규칙이 있다는 사실도 처음알았습니다 ㅋㅋ
이거 같이 적용하면서 사용하도록 하죠!
관련 링크

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굳굳! 좋은 것 같아요!

Comment on lines 70 to 81
@PostMapping("/social/sign-up")
public ResponseEntity<User#ResponseDTO> #(
@RequestBody @Valid User#RequestDTO requestDTO
public ResponseEntity<#UserResponseDto> #(
@RequestBody @Valid #UserRequestDto requestDto
) {
if (!universityService.isValidRegex(requestDTO.universityName(), requestDTO.email())){
throw new AuthException(INVALID_MAIL_REGEX);
}
Long universityId = universityService.getUniversityId(requestDto.universityName(), requestDto.departmentName(),
requestDto.email());

userService.updateUniversityInfo(requestDTO);
mailService.sendMail(requestDTO);
authServiceProvider.getAuthService(requestDto.platformType()).updateUniversityInfo(requestDto, universityId);
mailService.sendMail(requestDto, requestDto.platformId());

return ResponseEntity.ok(User#ResponseDTO.of(requestDTO.platformId()));
return ResponseEntity.ok(#UserResponseDto.of(requestDto.platformId()));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

곰곰히 생각해보니, 해당 부분의 기능은 이메일을 보내는 부분이라는 생각이 듭니다. 함수명을 변경하는것은 어떻게 생각하시나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 emailVerify와 바꾸는게 좋을 것 같습니다!

Copy link
Member

@Goder-0 Goder-0 Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용자의 일부 정보를 입력받고 임시 유저도 생성하는 역할도 하고 있으니,
createTempUserAndSendEmail을 좀 줄여서 적용하면 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넹 좋아요

Comment on lines 83 to 95
@PostMapping("/email-verify")
public ResponseEntity<AuthUserResponseDto> verify(
@RequestBody @Valid VerifyUserRequestDto requestDto) {

UserVO userVO = mailService.verify(requestDto.emailCode());
User user = authServiceProvider.getAuthService(userVO.getPlatformType())
.createSocialUser(userVO, requestDto.nickName());

User user = mailService.verify(emailCode);
User#VO vo = User#VO.of(user, user.getPlatformType(), user.getRole(), AuthType.SIGN_UP);
UserAuthResponseDTO responseDTO = jwtService.issueToken(vo);
AuthUserResponseDto responseDTO = jwtService.issueToken(vo);

return ResponseEntity.status(HttpStatus.CREATED).body(responseDTO);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sign-up과 같은 맥락으로 해당 코드는 이메일 검증보다, 회원가입의 역할에 좀더 가깝다고 생각합니다.

Comment on lines 33 to +34
NOT_FOUND_USER(HttpStatus.NOT_FOUND, "유효한 유저를 찾지 못했습니다."),
NOT_FOUND_REFRESH_TOKEN(HttpStatus.NOT_FOUND, "유효한 리프레시 토큰을 찾지 못했습니다."),
NOT_FOUND_EMAIL_CODE(HttpStatus.NOT_FOUND, "유효한 이메일 코드를 찾지 못했습니다."),
NOT_FOUND_UNIVERSITY_AND_DEPARTMENT(HttpStatus.NOT_FOUND, "유효한 학교명 및 학과명을 찾지 못했습니다.");


NOT_FOUND_REFRESH_TOKEN(HttpStatus.NOT_FOUND, "유효한 리프레시 토큰을 찾지 못했습니다.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에러 코드의 위치를 옮기셨군요!

Comment on lines 38 to 45
return User.builder()
.email(email)
.name(name)
.nickname("tmpNickName")
.password(password)
.phoneNumber(phoneNumber)
.admissionYear(0)
.platformType(platformType)
.platformType(request.platformType())
.platformId(id)
.role(Role.GUEST)
.build();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 User 클래스 내에 생성자를 정의하는건 어떨까요?

Copy link
Contributor Author

@mikekks mikekks Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 개인적으로 생성자보단 빌더패턴을 선호해서 이렇게 했습니다! 특히 파라미터가 많은 경우에는 빌더패턴을 사용하는게 휴먼에러를 방지할 수 있는 좋은 방법이라고 생각합니다!

혹시 어떤 이유때문인지 궁금합니다!


@Getter
@NoArgsConstructor
@RedisHash(value = "user", timeToLive = 604800016)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 604800016에 의미가 따로 없다면 60480000으로 변경하는건어떨까요..?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 알겠습니다~


@Configuration
@EnableRedisRepositories(enableKeyspaceEvents = RedisKeyValueAdapter.EnableKeyspaceEvents.ON_STARTUP)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 컨피그가 무엇인줄 몰랐는데, 찾아보다 알게 되었습니다. 굳굳!!
참고자료

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 무슨 이유로 지우게 된 걸까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일단 현재는 사용안해서 나중에 사용할 때 다시 만들면 될 것 같습니다!

Comment on lines +107 to +108
public LogoutUserResponseDto logout(User user){
TokenVO foundRefreshToken = redisTokenRepository.findByPlatformIdOrElseThrowException(user.getPlatformId());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굿굿! 코드가 짧아졌네요 ㅎ

+ "<h1> 안녕하세요. Meeteam 입니다</h1>"
+ "<br>"
+ "<p>아래 링크를 클릭하면 이메일 인증이 완료됩니다.<p>"
+ "<a href='http://localhost:8080/auth/verify?emailCode=";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

번거롭게 해서 죄송합니다.
프론트의 도메인이 들어갈 부분을 환경변수로 빼고 파라미터만 남기는게 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음 어차피 나중에 여기 싹 다 수정해야 할 것 같은데 그 때 수정하는건 어떠신가요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굿굿 좋은 것 같습니다.
혹시 스키마 쪽은 나중에 하는걸까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

스키마쪽이 어떤걸 의미하는 걸까요??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전에 그 도메인 쪽도 Schema로 해서 swagger 하자고 하셨어서 여쭤봤습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dto 말씀하는걸까요?? Dto는 기억이 나는데 도메인 쪽은 제가� 기억이 안나서..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네네 맞습니다!

@Goder-0 Goder-0 self-requested a review January 30, 2024 10:29
@Goder-0
Copy link
Member

Goder-0 commented Jan 30, 2024

머지하시면 될 것 같습니다!

@mikekks mikekks merged commit 4406421 into develop Jan 30, 2024
1 check passed
@mikekks mikekks deleted the feat/#24_logout branch February 28, 2024 06:43
mikekks added a commit that referenced this pull request Apr 12, 2024
* [DOCS] PR 템플릿 수정

* [REFACTOR] redis 저장으로 로직 변경

* [MOVE] 이메일 관련 에러 코드 UniversityExceptionType으로 이동

* [DEL] VO 통합으로 인한 삭제

* [DEL] VO 통합으로 인한 삭제

* [DEL] 필요없는 코드 삭제

* [FEAT] redis에 저장할 임시 유저 정의

* [FIX] 요청받을 데이터 수정

* [DEL] Dto명 변경으로 인한 삭제

* [CHORE] 일부 필요없는 코드 삭제 및 데이터 길이 수정

* [REFACTOR] AuthService으로 의존성 정리

* [FEAT] redis관련 로직 구현

* [MOVE] 파일 위치 변경

* [RENAME] 파일 이름 변경

* [RENAME] 파일 이름 변경

* [FEAT] 로그아웃 구현

* [ADD] 로그아웃 Dto 추가

* [REFACTOR] 메일 서비스의 레포지토리 의존성 제거를 통한 개선

* [FEAT] redis, university 레포지토리 추가

* [ADD] secondary index TimeToLive 적용을 위한 코드 추가

* [ADD] RedisException 추가

* [ADD] RedisExceptionType enum 추가

* [FEAT] 임시 유저 저장을 위한 Redis 레포지토리 구현

* [RENAME] 파일 이름 변경

* [ADD] Id로 찾는 메서드 추가

* [FEAT] Id로 찾는 메서드 구현

* [DEL] 필요없는 의존성 제거

* [DEL] 필요없는 코드 제거

* [CHORE] 로그아웃 관련 코드 리뷰 반영

* [CHORE] 함수 이름 변경

* [CHORE] 함수 이름 변경

* [CHORE] 변수 이름 변경

* [CHORE] TTL 시간 변경

* [ADD] 매직리터럴 상수로 추가

* [DOCS] 스웨거 API 문서 작성

* [DOCS] 스웨거 API 문서 수정

* [DOCS] 스웨거 Dto 문서 추가
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Priority: High 우선순위: 높음 Status: Available 상태: 작업을 바로 시작할 수 있는 상태 Type: Feature 유형: 기능 추가
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] 로그아웃 구현 및 임시 유저 redis 저장 구현
2 participants