-
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
[TNT-83] feat: 소셜 로그인 API 구현 #12
Conversation
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.
고생하셨습니다!! 특히 테스트 코드 아주 좋아요!👍
리뷰 일단 필요한거 반영 부탁드려요!
@@ -24,40 +22,48 @@ public class SessionService { | |||
static final long SESSION_DURATION = 2L * 24 * 60 * 60; // 48시간 | |||
private static final String AUTHORIZATION_HEADER = "Authorization"; | |||
private static final String SESSION_ID_PREFIX = "SESSION-ID "; | |||
private final RedisTemplate<String, SessionValue> redisTemplate; | |||
private final StringRedisTemplate redisTemplate; |
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 static final
, private final
을 개행으로 각자 구분하는건 어떨까요?
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.
좋습니다 개행으로 구분하겠습니다 !
Member findMember = findMemberFromDB(socialId, request.socialType()); | ||
|
||
if (findMember == null) { | ||
return OAuthLoginResponse.from(null, socialId, false); |
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 OAuthLoginResponse()
를 반환하는 것으로 보이네요!
어차피 같으니 from()
은 없애고 여기서 new OAuthLoginResponse()
부르면 되지 않을까요?
from()
같은 경우에는 다른 타입을 만들때 작성하는 것이 맞는것 같습니다!
그리고 내부에 @Builder
도 안쓰이면 제거하는게 좋을거 같아요!
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.
아하 고견 감사합니다. 불필요한 정적 팩토리 메서드 수정해서 푸쉬할게요 !
idToken = getAppleIdToken(request.authorizationCode()); | ||
} | ||
|
||
return verifyAndExtractUserInfo(idToken); |
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.
여기도 depth는 한번 올려도 될거 같아요!
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.
아하 여기 뎁스 수정해서 푸쉬할게요 !
|
||
@Repository | ||
public interface MemberRepository extends JpaRepository<Member, Long> { | ||
|
||
Optional<Member> findBySocialIdAndSocialType(String socialId, SocialType socialType); |
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.
Soft delete가 있기 때문에 AndDeletedIsNotNull
까지 해줘야 할 것 같아요!
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.
아하 해당 부분 수정해서 푸쉬할게요 !
KeyFactory kf = KeyFactory.getInstance("EC"); | ||
return (ECPrivateKey)kf.generatePrivate(keySpec); | ||
} catch (Exception e) { | ||
throw new OAuthException(FAILED_TO_FETCH_PRIVATE_KEY); |
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.
에러 스택 log.error로 출력해야합니다!
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.
아 여기 빠트렸네요 추가하겠습니다 !
return new ErrorResponse(exception.getBindingResult().getAllErrors().getFirst().getDefaultMessage()); | ||
log.error(errorMessage, exception); | ||
|
||
return new ErrorResponse(errorMessage); | ||
} | ||
|
||
// json 파싱, 날짜/시간 형식 예외 | ||
@ResponseStatus(HttpStatus.BAD_REQUEST) |
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.
여기들도 static import는 어떻게 생각하시나요!?
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.
아하 너무 좋습니다 static으로 올려버릴게요 !
@Tag(name = "로그인", description = "로그인 관련 API") | ||
@RestController | ||
@RequestMapping("/login") | ||
@RequiredArgsConstructor | ||
public class LoginController { | ||
|
||
private final OAuthService oauthService; | ||
|
||
@Operation(summary = "소셜 로그인") | ||
@PostMapping("/oauth") | ||
@ResponseStatus(value = HttpStatus.OK) | ||
public OAuthLoginResponse oauthLogin(@RequestBody @Valid OAuthLoginRequest request) { | ||
return oauthService.oauthLogin(request); | ||
} |
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.
아예 여기를 login, logout으로 하는 컨트롤러로 하는건 어떨까요?
그러면 클래스에는 RequestMapping을 빼고 메서드에 각각 Mapping하는 정도? + Swagger 설명 수정까지
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.
아하 고견 감사합니다 로그아웃도 같이 관리할 수 있는 컨트롤러가 될 수 있도록 할게요 !
@Test | ||
@DisplayName("존재하지 않는 회원 예외 발생") | ||
void member_not_found_error() { | ||
// given | ||
OAuthLoginRequest request = new OAuthLoginRequest(KAKAO, "kakao-access-token", null, null); | ||
|
||
mockWebServer.enqueue(new MockResponse() | ||
.setResponseCode(200) // 성공 응답으로 설정 | ||
.setHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE) | ||
.setBody("{\"id\": \"12345\", \"kakao_account\": {\"email\": \"[email protected]\"}}")); | ||
|
||
given(memberRepository.findBySocialIdAndSocialType("12345", SocialType.KAKAO)) | ||
.willReturn(Optional.empty()); | ||
|
||
// when | ||
OAuthLoginResponse response = oAuthService.oauthLogin(request); | ||
|
||
// then | ||
assertThat(response.sessionId()).isNull(); | ||
assertThat(response.socialId()).isEqualTo("12345"); | ||
assertThat(response.isSignUp()).isFalse(); | ||
} |
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.
아하 이름 명확하게 수정해서 푸쉬할게요 !
@@ -42,21 +42,53 @@ public class Member extends BaseTimeEntity { | |||
@Column(name = "profile", nullable = false) |
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.
profile, age 등이 erdcloud에서 업데이트돼서 반영 부탁드려요!
private Member(CreateMember create) { | ||
this.id = create.id; | ||
this.socialId = create.socialId; | ||
this.email = create.email; | ||
this.name = create.name; | ||
this.age = create.age; | ||
this.profile = create.profile; | ||
this.serviceAgreement = true; | ||
this.collectionAgreement = true; | ||
this.advertisementAgreement = create.advertisementAgreement; | ||
this.pushAgreement = true; | ||
this.socialType = create.socialType; | ||
} | ||
|
||
public static Member from(CreateMember create) { | ||
return new Member(create); | ||
} | ||
|
||
@Builder | ||
public Member(Long id, String socialId, String email, String name, int age, SocialType socialType) { | ||
this.id = id; | ||
this.socialId = socialId; | ||
this.email = email; | ||
this.name = name; | ||
this.age = age; | ||
this.profile = ""; | ||
this.socialType = socialType; | ||
public static class CreateMember { | ||
|
||
private Long id; | ||
private String socialId; | ||
private String email; | ||
private String name; | ||
private int age; | ||
private String profile; | ||
private boolean advertisementAgreement; |
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.
기존 생성자는 파라미터 수가 8개가 넘어가니깐 sonarqube의 "too many parameters" 경고가 떴었습니다.
해결 방법으로 파라미터 수에 제약이 없는 빌더 패턴을 적용했습니다.
(출처: https://inpa.tistory.com/entry/GOF-%F0%9F%92%A0-%EB%B9%8C%EB%8D%94Builder-%ED%8C%A8%ED%84%B4-%EB%81%9D%ED%8C%90%EC%99%95-%EC%A0%95%EB%A6%AC)
수정하는 과정에서 조금 가독성이 떨어지게 구현한 것 같습니다..
다시 아래와 같이 수정했고 푸쉬할게요 !
…etResponse를 static import로 수정
|
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.
👍
📋 Checklist
[APP2-77] feat: 회원 인증 Filter 구현
)🎟️ Issue
✅ Tasks
-> 받아온 요청 파라미터들로 kakao / apple 서버로 다시 요청해서 소셜 회원 정보 받아옴
-> 해당 회원 정보로 우리 DB에서 조회 후 없으면 예외 발생, 있으면 sessionId 발급 후 응답
에러 메시지 enum 클래스로 묶었습니다.
모든 에러에서 stack trace 출력하도록 수정했습니다.
🙋🏻 More