-
Notifications
You must be signed in to change notification settings - Fork 1
MOSU-3 feat: OAuth2 로그인 기능 구현 #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
polyglot-k
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전반적으로 코드는 깔끔한고, 응집도 있게 묶으려고 한 노력이 보여서 좋았습니다.
일단 file change 가 너무 많아서 조금 리뷰하기는 힘들었습니다.
다음에는 조금씩 쪼개서 PR 단위를 더 작게 하는 것도 고려해보면 좋을 것 같습니다.
부분적으로 구조가 조금 복잡성을 띄지않나? 라는 생각을 해보면 좋을 것 같습니다! 파일에 대한 분리를 꼼꼼하게 한 것 같지만, 안정성에 대한 보장이 부족한 부분이 있었던 것 같습니다! 동일 기능이 가능하다면, 비교적 짧은 형태로 구성되는게 좋고, 한 메소드에 크기가 크다면, private 메소드를 활용해서 모듈 단위로 분리해서 흐름을 따라갈 수 있도록 하면 더 좋을 것 같습니다!
velkey에 대한 부분은 이전에 설명해드렸었는데 까먹으신 것 같아서 그 부분에 대한 적용을 해주시면 감사하겠습니다.
| ) | ||
| .successHandler(loginSuccessHandler)) | ||
| .addFilterBefore(accessTokenFilter, UsernamePasswordAuthenticationFilter.class) | ||
| .addFilterBefore(new TokenExceptionFilter(), accessTokenFilter.getClass()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 필터를 new 로 직접 등록한 이유를 여쭤봐도 될까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉 생성자 주입 하겠습니다!
| final HttpStatus status = HttpStatus.UNAUTHORIZED; | ||
| final String message = "로그인이 필요합니다."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런식으로 메소드 내에서 처리하는 것과, private final static 으로 처리 하는 것 중에 어느 것이 초기화 될 때, 자원 낭비가 적을지 바라보면 좋을거 같아요. JVM Memory Area 내에 어디를 차지하고 있는지를 바라보고 생명주기성에 따라 봤을 때는 private final static 이 더 나아보여요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵! 변경하겠습니다!
| private final ObjectMapper objectMapper; | ||
|
|
||
| @Override | ||
| public void commence( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
부분적으로 private method 로 분리하여 흐름을 만들면 조금 더 읽기 좋을거 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메소드의 크기가 조금 커서 흐름을 확인하는데 어려움이 조금 있었고, 코드 자체는 깔끔합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 private으로 조금 분리하겠습니다
| ) throws ServletException, IOException { | ||
| try { | ||
| filterChain.doFilter(request, response); | ||
| } catch (final CustomRuntimeException exception) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
자바 8부터 도입된 "Effective Final (유효적 최종)" 개념이 있는데, 선언 이후에 한번도 변경되지 않으면, 컴파일 라인에서 이변수를 final로 취급하기 때문에 오히려 현재 코드가 가독성을 저하되는 것 같아서 final 은 빼도 괜찮지 않을까라고 생각이 들어요. 지역변수나 인자 라인에 있는 것들이 보통 그렇게 되는 걸로 알고 있어서
다른 의견이나 잘못된 정보있으면 편하게 말씀해주세요
| ) { | ||
|
|
||
| public static OAuthUserInfo of( | ||
| final String registrationId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enum으로 처리하면 좀 더 명확할거 같아요!
| final Map<String, Object> account = (Map<String, Object>) attributes.get("kakao_account"); | ||
| final Map<String, Object> profile = (Map<String, Object>) account.get("profile"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ofKakao 메서드에서 attributes.get("kakao_account")나 account.get("profile")과 같이 Map에서 값을 가져올 때, 해당 키가 존재하지 않으면 NullPointerException이 발생할 수 있습니다.
| if (userRepository.existsByEmail(info.email())) { | ||
| final OAuthUserJpaEntity user = userRepository.findByEmail(info.email()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findByEmail 로 분기처리 충분히 가능할거 같은데, existsByEmail 로 불필요한 쿼리가 발생할 수도 있겠다는 생각이 드네요! 한번 고려해보세요!
| @Getter | ||
| @RedisHash(value = "token_verification") | ||
| public class RefreshTokenRedisEntity { | ||
|
|
||
| @Id | ||
| private Long userId; | ||
|
|
||
| @Indexed | ||
| private String refreshToken; | ||
|
|
||
| @TimeToLive(unit = TimeUnit.MILLISECONDS) | ||
| private Long expiration; | ||
|
|
||
| public static RefreshTokenRedisEntity from(final RefreshToken token) { | ||
| final RefreshTokenRedisEntity entity = new RefreshTokenRedisEntity(); | ||
| entity.userId = token.userId(); | ||
| entity.refreshToken = token.refreshToken(); | ||
| entity.expiration = token.expiration(); | ||
| return entity; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @Getter | |
| @RedisHash(value = "token_verification") | |
| public class RefreshTokenRedisEntity { | |
| @Id | |
| private Long userId; | |
| @Indexed | |
| private String refreshToken; | |
| @TimeToLive(unit = TimeUnit.MILLISECONDS) | |
| private Long expiration; | |
| public static RefreshTokenRedisEntity from(final RefreshToken token) { | |
| final RefreshTokenRedisEntity entity = new RefreshTokenRedisEntity(); | |
| entity.userId = token.userId(); | |
| entity.refreshToken = token.refreshToken(); | |
| entity.expiration = token.expiration(); | |
| return entity; | |
| } | |
| } | |
| @Getter | |
| @RedisHash(value = "token_verification") | |
| @AllArgsConstructor | |
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | |
| public class RefreshTokenRedisEntity { | |
| @Id | |
| private Long userId; | |
| @Indexed | |
| private String refreshToken; | |
| @TimeToLive(unit = TimeUnit.MILLISECONDS) | |
| private Long expiration; | |
| public static RefreshTokenRedisEntity from(final RefreshToken token) { | |
| return new RefreshTokenRedisEntity(token.userId(), token.refreshToken(), token.expiration()); | |
| } | |
| } |
일단 코드의 간결성 측면으로 볼 때, 수동 주입하는 형태보다는 이러한 형태는 어떤가요??
| final String url = String.format( | ||
| "/oauth2/authorization/%s?%s=%s", | ||
| registrationId, | ||
| REDIRECT_PARAM_KEY, | ||
| redirect | ||
| ); | ||
| return new RedirectView(url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| final String url = String.format( | |
| "/oauth2/authorization/%s?%s=%s", | |
| registrationId, | |
| REDIRECT_PARAM_KEY, | |
| redirect | |
| ); | |
| return new RedirectView(url); | |
| final String url = UriComponentsBuilder | |
| .fromPath("/oauth2/authorization/{registrationId}") | |
| .queryParam(REDIRECT_PARAM_KEY, redirect) | |
| .buildAndExpand(registrationId) | |
| .toUriString(); | |
| return new RedirectView(url); |
String.format 메소드는 안정성이 높은 메소드는 아닙니다. 해당 부분으로 변경하는건 어떨까요?
✨ 구현한 기능
📢 논의하고 싶은 내용
Long타입의id가 통일 되지 않고 있습니다. 그래서 나중에User를 조회하는 상황이 오면 조회 메서드를 분리할지, 각각의User테이블에OAuth를 사용한지 아닌지에 대한Enum필드값을 추가할지 고민입니다.🎸 기타
OAuthUserInfo에switch문에 다른 회사를 넣을 수 있게 했습니다.ofKakao메서드에 임시 메일을 넣어뒀는데 나중에 수정할 예정 입니다.