-
Notifications
You must be signed in to change notification settings - Fork 0
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: OAuth2 로그인 구현 #5
Conversation
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
- 기존의 토큰 로그인 방식에서 OAuth 로그인으로 변경 Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
- 기존의 토큰 로그인 방식에서 OAuth 로그인으로 변경 Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
- 기존의 토큰 로그인 방식에서 OAuth 로그인으로 변경 Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
- OAuthProperties 내부 클래스 이름 변경 (Client -> User) (yml 설정 파일과 일치 시키기 위한 변경) Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
- InMemoryProviderRepository -> OAuthProviderRepository Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
- OAuthTokenResponse Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
- 불필요한 개행 삭제 - 메서드 분리 Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
- getAccessToken -> getAccessTokenResponse Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
- 기존의 불필요했던 구조를 최적화 Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
- OAuthProviderFixture Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
Co-authored-by: devholic22 <hyunjoon.tech@gmail.com>
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.
approve 하기 전에 몇 가지가 궁금해서 rc 누릅니다! 수고하셨습니다~
import java.nio.charset.Charset; | ||
import java.nio.charset.StandardCharsets; | ||
|
||
public class CustomRequestWrapper extends HttpServletRequestWrapper { |
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.
이건 뭘 하는 클래스인가요??
|
||
@Transactional | ||
public String signup(final SignupRequest request) { | ||
validateExistedMember(request.email()); | ||
public String login(final LoginRequest request, final OAuthProvider provider) { |
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.
불필요한 공백 삭제해주세요
import org.springframework.stereotype.Component; | ||
|
||
@Component | ||
public class JackonJsonMapper implements JsonMapper { |
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.
이거 잭슨 말고 resttemplate에서 할 수 없을까요?
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.
restTemplate로 통신을 담당하는 클래스를 만들었는데 그 안에 JSON을 매핑하는 라이브러리까지 존재하면 나중에 JSON을 변환해주는 라이브러리가 바뀔 경우 유지보수에 불편함이 있을 것 같아서 분리했습니다. 또한 JSON을 매핑하는 라이브러리를 다른 클래스에서도 사용하기 때문에 클래스를 따로 분리하여 관리하는 것이 이점이 더 있다고 생각했습니다:)
memberService.create(email, nickname); | ||
|
||
// then | ||
Member member = memberRepository.findByEmail(email).get(); |
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.
이렇게 테스트하기 보다는
softly로 생성이 됐는지 optinal.isPresent()
로 먼저 검증하고 그다음 ast를 검증하는게 좋지 않을까요?
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.
계층 분리랑 다른 부분도 너무 잘해서 approve 하겠습니다!
리뷰 남긴 몇 부분만 수정하고 머지해주세요!
import lombok.Getter; | ||
import lombok.NoArgsConstructor; | ||
import org.springframework.beans.factory.annotation.Value; | ||
import org.springframework.stereotype.Component; | ||
|
||
@Getter |
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는 필요없어 보입니다!
return Arrays.stream(values()) | ||
.filter(platform -> name.equals(platform.name)) | ||
.findFirst() | ||
.orElseThrow(IllegalArgumentException::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.
이거 정확히 어떤 오류인지 파악하기 위해서 CustomException으로 바꾸는 게 좋아보입니다
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.
수고하셨습니다! 자잘한 몇 개만 수정해주시면 될 거 같습니다 :)
public interface RequestProcessor { | ||
|
||
String readJsonFromBody(HttpServletRequest httpServletRequest) throws IOException; | ||
} |
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.
사용 위치가 0곳으로 나오는데 다른 곳에서 대신 처리하신 것 같습니다! 없애도 괜찮을까요?
|
||
MemberInfoResponse extractMemberInfoFrom(String memberInfoResponse, | ||
MemberInfoKeyWordRequest memberInfoKeyWordRequest); | ||
} |
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 속성 붙여주시면 좋을 것 같습니다!
private String getValue(final String keyWord, final JsonNode jsonNode) { | ||
|
||
return Arrays.stream(keyWord.split(DELIMITER)) | ||
.reduce(jsonNode, JsonNode::get, (a, b) -> b) |
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.
혹시 a, b 말고 다른 이름으로 바꿔주실 수 있을까요?
String tokenUri, | ||
String userInfoUri, | ||
MemberInfoKeyWordRequest memberInfoKeyWordRequest | ||
) { |
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.
지금으로선 굳이 필요하지 않을 수도 있을 것 같긴 한데, 만약 OAuthProvider 회사들에서 제공하는 이름 형식이 달라지거나 위의 값이 없다면 (ex: clientSecret) 해당 OAuthProvider 방식은 지원이 안된다는 메시지를 전달해야 할 수도 있을 것 같습니다. 컨트롤러에 요청 시 넘어오는 정보인만큼 LoginRequest와 같이 @Valid
및 예외처리를 해 주면 어떨까요? (MemberInfoKeyWordRequest도 동일)
@@ -43,9 +42,9 @@ private void init() { | |||
} | |||
|
|||
@Override | |||
public String create(final Long id) { | |||
public String create(final String email) { | |||
Claims claims = Jwts.claims(); |
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.
파라미터로 넘겨온 인자를 이용해 Claims를 만들고 그것을 이용해 토큰을 만든 뒤 반환하는 것인데, createTokenWith
와 비슷하게 어떤 것을 이용해 토큰을 만든다는 느낌이 들게 메서드명을 정해주면 좋을 것 같습니다!
params.add("client_secret", oAuthProviderRequest.clientSecret()); | ||
params.add("redirect_uri", oAuthProviderRequest.redirectUri()); | ||
params.add("code", decode); | ||
|
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.
params.add에 쓰인 키 (grant_type, client_id 등)들을 상수처리하지 않은 이유가 있으실까요?
아니면 params에 여러 키를 저장하니, 메서드로 분리해주시면 좋을 것 같습니다.
} | ||
|
||
|
||
|
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.
공백 지워주시면 좋을 것 같습니다 :)
- a, b -> parentPath, childPath
OAuth2 기반 설정 작업을 하였습니다.
📄 Summary
카카오 환경에서의 OAuth2 로그인 처리를 하였습니다.
🙋🏻 More
login/{provider}
로 POST 요청을 보내는데, provider는 kakao, naver 등의 OAuth2 회사 이름을 의미합니다.close #4