-
Notifications
You must be signed in to change notification settings - Fork 2
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
[FEAT] OAuth 2.0 (Kakao) #51
Conversation
[FEAT] develop -> main
[Deploy] develop -> main
Test Results1 tests 1 ✅ 0s ⏱️ Results for commit aca8548. ♻️ This comment has been updated with latest results. |
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.
security로 구현하기 어려웠을거 같은데 고생하셨습니다! 몇가지 리뷰 부탁드립니다 😄
KAKAO_ADMIN_KEY: ${{ secrets.KAKAO_ADMIN_KEY}} | ||
CLIENT_ID: ${{ secrets.CLIENT_ID}} | ||
CLIENT_SECRET: ${{ secrets.CLIENT_SECRET}} |
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.
회의때 말한대로 일단 따로 관리한다고 알고 있겠습니다
@PostMapping("/unlink") | ||
private ApiResponse<Void> kakaoUnlink(@RequestBody OAuthUnlinkRequest request){ | ||
userService.unlinkKakaoAccount(request); | ||
return ApiUtil.successOnly(); | ||
} |
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.
백엔드에서만 사용하는 API는 따로 명시해도 좋을거 같아요
private final UserRepository userRepository; | ||
|
||
@Override | ||
public OAuth2User loadUser(OAuth2UserRequest userRequest) throws OAuth2AuthenticationException { |
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.
userRequest에는 엑세스 토큰이 있을까요? 있네요
public OAuth2User loadUser(OAuth2UserRequest userRequest) throws OAuth2AuthenticationException { | ||
OAuth2UserService<OAuth2UserRequest, OAuth2User> oAuth2UserService = new DefaultOAuth2UserService(); | ||
|
||
OAuth2User oAuth2User = oAuth2UserService.loadUser(userRequest); |
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.
엑세스 토큰으로 OAuth 정보를 받아오는 구간일까요?(OAuth attribute를 받아온다는 표현이 맞을까요?)
|
||
OAuth2Attribute oAuth2Attribute = OAuth2Attribute.of(registrationId, userNameAttributeName, oAuth2User.getAttributes()); | ||
|
||
Map<String, Object> userAttribute = oAuth2Attribute.convertToMap(); |
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.
DefaultOAuth2User에 왜 map을 넣어야할까요? 단일 유저 정보를 넣는게 흐름상 맞아보이는데...
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.
DefaultOAuth2User의 생성자에 Map 타입의 attributes가 포함되어야 하는 것으로 알고 있습니다. 해당 attribute에 유저 정보를 담는 것으로 이해했어요!
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.
Unlink API를 구현하면서, 카카오 계정 연결을 해제nlink API를 구현하면서, 카카오 계정 연결을 해제한 경우 DB에서도 유저를 제거하는 것이 옳다고 생각하여 추가하게 되었습니다
public static void sendSuccessResponse(HttpServletResponse response, ApiResponse<?> successMessage) throws RuntimeException, IOException { | ||
response.setContentType("application/json;charset=UTF-8"); | ||
|
||
ObjectMapper objectMapper = new ObjectMapper(); | ||
String result = objectMapper.writeValueAsString(successMessage); | ||
response.getWriter().print(result); | ||
} | ||
|
||
public static void sendErrorResponse(HttpServletResponse response, ApiResponse<?> errorMessage) throws RuntimeException, IOException { | ||
response.setContentType("application/json;charset=UTF-8"); | ||
|
||
ObjectMapper objectMapper = new ObjectMapper(); | ||
String result = objectMapper.writeValueAsString(errorMessage); | ||
response.getWriter().print(result); | ||
} |
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.
이건 지난 PR에서 하는게 맞을거 같은데, filter단에서 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.
의도는 성공과 실패 메소드를 분리하기 위해서였는데, 다시 보니 코드가 중복되네요..
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.
위 메소드는 filter에서의 doFilterInternal와 같은 오버라이딩한 메소드들의 반환 타입이 void이므로, response를 자체적으로 만들어 보내기 위한 메소드입니다.
|
||
public static TokenDto of(String accessToken, String refreshToken, Instant refreshTokenExpiredAt){ | ||
return TokenDto.builder() | ||
.grantType("Bearer") | ||
.accessToken(accessToken) | ||
.refreshToken(refreshToken) | ||
.refreshTokenExpiredAt(refreshTokenExpiredAt) | ||
.refreshTokenExpiredAt(refreshTokenExpiredAt.toString()) |
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.
비슷한 내용을 고민한적이 있는데 toString()을 실제 의미가 있는 값으로 사용해도 괜찮을까요?
여기도
switch (provider) { | ||
case "kakao": | ||
return ofKakao(provider,"email", attributes); | ||
default: | ||
throw new RuntimeException(); |
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.
switch도 지양해보죠
.addFilterBefore(new JwtAuthenticationFilter(jwtTokenProvider), UsernamePasswordAuthenticationFilter.class) | ||
.addFilterBefore(new JwtAuthenticationFilter(jwtTokenProvider), | ||
UsernamePasswordAuthenticationFilter.class) |
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.
중복된 코드
✏️ 작업 개요
⛳ 작업 분류
🔨 작업 상세 내용
💡 생각해볼 문제