Feature/#41 깃허브 로그인 기능 구현 #50

hyeonjerry · 2023-07-18T05:43:04Z

#️⃣연관된 이슈

깃허브 로그인 기능 구현 #41

📝작업 내용

깃허브 redirect uri로 요청이 들어왔을 때 토큰을 반환하는 기능 구현
사용자를 조회하고 토큰을 생성하는 로직은 다음 이슈에서 진행 예정 (TODO 참고)

💬리뷰 요구사항(선택)

작업의 범위가 모호하여 로그인 기능을 위한 틀만 구현했습니다.
커스텀 예외는 아직 논의되지 않아 TODO 주석만 남겨두고 넘어갔습니다.
기능 구현에 미흡한 점 있으면 말씀해 주세요.

#41

into Feature/#41-깃허브_로그인_기능_구현

#41

hong-sile

전반적으로 잘 짜주신 것 같아요.
프롤로그랑 비슷한 flow라 좋네요.

OAuth같이 외부 API에 의존적이기도 하고,
지금 회원을 등록하는 기능이 없어서 테스트하기 어려운 점이 있긴하네요.
나중에 추가되면 보완하면 될 것 같아요.

수고했습니다.

hong-sile · 2023-07-20T00:57:23Z

backend/emm-sale/src/main/java/com/emmsale/login/application/dto/GithubAccessTokenResponse.java

+import lombok.NoArgsConstructor;
+
+@AllArgsConstructor
+@NoArgsConstructor(access = AccessLevel.PROTECTED)


혹시 NoArgsCounstructor를 PROTECTED로 추가하신 이유가 있을까요?

hong-sile · 2023-07-20T00:57:35Z

backend/emm-sale/src/main/java/com/emmsale/login/application/dto/GithubProfileResponse.java

+import lombok.NoArgsConstructor;
+
+@AllArgsConstructor
+@NoArgsConstructor(access = AccessLevel.PROTECTED)


얘도요!!

hong-sile · 2023-07-20T00:58:41Z

backend/emm-sale/src/main/java/com/emmsale/login/ui/LoginController.java

+  @GetMapping("/github/callback")
+  public ResponseEntity<TokenResponse> login(@RequestParam final String code) {
+    if (code == null) {
+      return ResponseEntity.badRequest().build();


badRequest에 code관련된 에러 메시지를 추가해주면 어떨까요?
CustomException으로 처리해줘도 좋을 것 같아요

hong-sile · 2023-07-20T01:04:18Z

backend/emm-sale/src/main/java/com/emmsale/login/ui/LoginController.java

+    if (code == null) {
+      return ResponseEntity.badRequest().build();
+    }
+    return ResponseEntity.ok().body(loginService.createToken(code));


제가 코드를 이해한 바로는 TokenResponse의 accessToken이
실제 저희가 사용하는 Jwt 토큰인 거 같은데, 혹시 jwt 토큰만이 아닌 id까지 포함된
TokenResponse를 반환하는 이유가 있나요?

안드로이드에서 사용자의 id를 함께 보내줄 것을 요청했기 때문입니다.
사용자의 id를 복사해서 친구에게 공유하여 친구신청 할 수 있는 기능 때문에 id를 함께 반환하도록 협의 했습니다.

아 그렇게 논의했군요. 이해했습니다.

음.. 친구 신청에서 필요한 사용자의 Id는 실제 DB에서 관리되는 member pk인 member_id가 아니라,
member의 uuid(figma에서 정의된 영어+숫자로 구성된 id목록)잖아요?

지금은 member에서 uuid가 지정되진 않아서, 뭔가 추가적인 작업을 할 순 없는데, uuid가 추가되면 변경해야할 것 같긴하네요

지금은 이대로만 두고, 추후에 추가해야 할 것 같아요. 수고했습니다.

hong-sile · 2023-07-20T01:05:13Z

backend/emm-sale/src/test/java/com/emmsale/login/application/LoginServiceTest.java

+  @MockBean
+  private GithubClient githubClient;
+  @MockBean
+  private JwtTokenProvider tokenProvider;


TokenProvider는 그냥 주입받게 하는건 어떤가요?

토큰 생성시 발급 시간이 들어가므로 매 테스트마다 발급되는 토큰의 값이 달라지는 문제가 있어 Mocking하여 테스트를 구현했습니다.
혹시 위 문제를 해결할 수 있는 다른 방법이 있다면 알려주세요 :)

음 어떤 것 때문에 그랬냐면, getAccessTokenFormGithub 테스트 코드에서

() -> assertEquals(expectTokenResponse.getAccessToken(), actualToken.getAccessToken())
구문이 있잖아요?

사실 모킹하면 이 부분은 의미가 없는 테스트가 되기도 하고, accessToken에서 memberId만 추출해서 이것만 같다고 체크하면 된다고 생각해요.

이렇게 짜면 굳이 mocking을 할 필요도 없고요.

제가 mocking을 경계하는 이유가 뭐냐면, 여기서 TokenProvider가 mockBean으로 사용됬다가 나중에 TokenProvider의 createToken의 내부코드가 변경되었을 때 테스트를 이 테스트 코드는 아무런 역할을 못할 것 같거든요.

사실 이 부분은 TokenProvider가 변경사항이 자주 생기는 코드가 아니라서 사실 큰 문제는 없긴해요.
읽어보고 납득이 안 되시면, 그대로 유지해도 괜찮을 것 같습니다.

#41

java-saeng · 2023-07-20T12:35:50Z

backend/emm-sale/src/main/java/com/emmsale/login/application/GithubClient.java

+    );
+
+    final HttpHeaders headers = new HttpHeaders();
+    headers.add("Accept", MediaType.APPLICATION_JSON_VALUE);


상수는 따로 빼주거나, 아니면 spring 이 제공해주는 HttpHeaders.ACCEPT 를 사용해보시는건 어떠신가요?

java-saeng · 2023-07-20T12:36:28Z

backend/emm-sale/src/main/java/com/emmsale/login/application/GithubClient.java

+    headers.add("Accept", MediaType.APPLICATION_JSON_VALUE);
+
+    final HttpEntity httpEntity = new HttpEntity(githubAccessTokenRequest, headers);
+    final RestTemplate restTemplate = new RestTemplate();


RestTemplate 객체를 싱글톤으로 관리해보시는건 어떠신가요? 반복적으로 사용되는 것 같아서요~

java-saeng · 2023-07-20T12:37:01Z

backend/emm-sale/src/main/java/com/emmsale/login/application/GithubClient.java

+    final HttpHeaders headers = new HttpHeaders();
+    headers.add("Accept", MediaType.APPLICATION_JSON_VALUE);
+
+    final HttpEntity httpEntity = new HttpEntity(githubAccessTokenRequest, headers);


HttpEntity를 row 타입으로 사용하고 있는데 이유가 있으실까요?

다른 코드를 참고하며 작성하다 보니 놓친 것 같습니다.
raw 타입을 사용할 경우 타입 안정성이 떨어질 뿐만 아니라 슈퍼 클래스의 메서드들도 모두 지워져서 GithubAccessTokenRequest 클래스의 메서드 또한 사용할 수 없는 문제가 발생하네요!!

HttpEntity<GithubAccessTokenRequest> httpEntity; httpEntity.getBody().getClientId(); // O HttpEntity httpEntity; httpEntity.getBody().getClientId(); // Cannot resolve method 'getClientId' in 'Object'

java-saeng · 2023-07-20T12:38:22Z

backend/emm-sale/src/main/java/com/emmsale/login/application/GithubClient.java

+    final String accessToken = restTemplate
+        .exchange(accessTokenUrl, HttpMethod.POST, httpEntity, GithubAccessTokenResponse.class)
+        .getBody()
+        .getAccessToken();


제가 잘 몰라서 그러는데 get함과 동시에 accessToken이 존재하지 않는다면 바로 NPE가 발생하나요??

Method invocation 'getAccessToken' may produce 'NullPointerException' 인텔리제이에서 이런 문구가 보여서요

final String accessToken = REST_TEMPLATE .exchange(accessTokenUrl, HttpMethod.POST, httpEntity, GithubAccessTokenResponse.class) .getBody() .getAccessToken();

위 코드에서 .exchange() 메서드의 내부를 보면 return nonNull(execute(...));을 반환하고 있습니다.
여기서 nonNull() 메서드를 보면 Assert.state(result != null, "No result");를 통해 result가 null일 경우 에러를 발생시킵니다.
따라서, .getBody().getAccessToken()을 했을 때 NPE가 발생할 일은 없을 것 같습니다!!

RestTemplate과 동일한 라이브러리는 아니지만 유사항 사례가 있어 참고로 남깁니다.
Method invocation may produce NullPointerException Retrofit Body

java-saeng · 2023-07-20T12:39:51Z

backend/emm-sale/src/main/java/com/emmsale/login/application/GithubClient.java

+
+  public GithubProfileResponse getGithubProfileFromGithub(final String accessToken) {
+    final HttpHeaders headers = new HttpHeaders();
+    headers.add("Authorization", "token " + accessToken);


위 리뷰와 같이 상수화를 시키거나 스프링이 제공해주는 것을 사용해보는건 어떨까요?

java-saeng · 2023-07-20T12:42:28Z

backend/emm-sale/src/test/java/com/emmsale/login/ui/LoginControllerTest.java

+import org.springframework.test.web.servlet.ResultActions;
+
+@WebMvcTest(LoginController.class)
+class LoginControllerTest {


이 테스트는 Rest docs가 필요없는걸까요?

java-saeng · 2023-07-20T12:47:08Z

고생하셨습니다!!

현재 controller 의 패키지 이름이 api와 ui 로 통일이 되지 않아서 이 부분은 다시 얘기해보면 좋을 것 같습니다.
제 리뷰가 정답이 아니니까 제리의 생각을 편안하게 말씀해주시면 감사하겠습니다.

hong-sile

몇몇 코드가 추가되었네요. 혼자 작성하느라 수고하셨습니다.

약간 리뷰라기 보다 궁금한 점이 많아서 코멘트를 남겼어요.

전체적인 흐름엔 문제 없어서 Approve합니다.
대신에, 코멘트는 꼭 남겨주세요!

hong-sile · 2023-07-21T00:45:55Z

backend/emm-sale/src/main/java/com/emmsale/login/ui/LoginController.java

+    if (code == null) {
+      return ResponseEntity.badRequest().build();
+    }
+    return ResponseEntity.ok().body(loginService.createToken(code));


아 그렇게 논의했군요. 이해했습니다.

음.. 친구 신청에서 필요한 사용자의 Id는 실제 DB에서 관리되는 member pk인 member_id가 아니라,
member의 uuid(figma에서 정의된 영어+숫자로 구성된 id목록)잖아요?

지금은 member에서 uuid가 지정되진 않아서, 뭔가 추가적인 작업을 할 순 없는데, uuid가 추가되면 변경해야할 것 같긴하네요

지금은 이대로만 두고, 추후에 추가해야 할 것 같아요. 수고했습니다.

hong-sile · 2023-07-21T00:55:23Z

backend/emm-sale/src/main/java/com/emmsale/login/utils/AuthorizationExtractor.java

+  private static final String BEARER_TYPE = "Bearer";
+
+  public static String extract(final HttpServletRequest request) {
+    final Enumeration<String> headers = request.getHeaders(AUTHORIZATION);


혹시 Enmeration을 반환받는 getHeaders 메서드로 작성하신 이유가 있나요?
getHeader를 사용하면, Authorization이 key인 header의 value를 바로 뽑을 수 있는걸로 알아서요.

이 부분은 제가 많이 안 짜봐서 잘 모르는 걸 수도 있으니 이유를 알려주시면 감사하겠습니다.

hong-sile · 2023-07-21T00:57:59Z

backend/emm-sale/src/main/java/com/emmsale/resolver/MemberArgumentResolver.java

  private final MemberQueryService memberQueryService;

  @Override
  public boolean supportsParameter(final MethodParameter parameter) {
-    return parameter.getParameterType().equals(Member.class);
+    return parameter.getParameterType().equals(Member.class) &&
+        parameter.hasParameterAnnotation(MemberOnly.class);


혹시 여기서 parameterAnnotation MemberOnly를 설정해주신 이유가 있나요?

어떤 의도를 추가하셨는지 잘 모르겠어서요.

hong-sile · 2023-07-21T01:04:41Z

backend/emm-sale/src/test/java/com/emmsale/login/application/LoginServiceTest.java

+  @MockBean
+  private GithubClient githubClient;
+  @MockBean
+  private JwtTokenProvider tokenProvider;


음 어떤 것 때문에 그랬냐면, getAccessTokenFormGithub 테스트 코드에서

() -> assertEquals(expectTokenResponse.getAccessToken(), actualToken.getAccessToken())
구문이 있잖아요?

사실 모킹하면 이 부분은 의미가 없는 테스트가 되기도 하고, accessToken에서 memberId만 추출해서 이것만 같다고 체크하면 된다고 생각해요.

이렇게 짜면 굳이 mocking을 할 필요도 없고요.

제가 mocking을 경계하는 이유가 뭐냐면, 여기서 TokenProvider가 mockBean으로 사용됬다가 나중에 TokenProvider의 createToken의 내부코드가 변경되었을 때 테스트를 이 테스트 코드는 아무런 역할을 못할 것 같거든요.

사실 이 부분은 TokenProvider가 변경사항이 자주 생기는 코드가 아니라서 사실 큰 문제는 없긴해요.
읽어보고 납득이 안 되시면, 그대로 유지해도 괜찮을 것 같습니다.

- 문자열 상수화 - RestTemplate 싱글톤 관리 - raw type 제거 #41

#41

amaran-th

수고 많으셨습니다~!
다른 분들이 리뷰를 너무 잘 남겨주셔서 저도 많이 배웠네요ㅎㅎ
몇 가지 의견 코멘트로 남겨놓았으니 확인 부탁드릴게요!

amaran-th · 2023-07-22T08:07:10Z

backend/emm-sale/src/main/java/com/emmsale/resolver/MemberArgumentResolver.java

+
+    if (token == null || token.isEmpty()) {
+      throw new LoginException(LoginExceptionType.NOT_FOUND_AUTHORIZATION_TOKEN);
+    }


이 부분은 메서드 분리를 해주는 편이 가독성 측면에서 더 좋을 것 같아요~!

amaran-th · 2023-07-22T08:25:44Z

backend/emm-sale/src/main/java/com/emmsale/login/application/GithubClient.java

@@ -0,0 +1,68 @@
+package com.emmsale.login.application;


패키지 상으로도 그렇고 기능적으로도 Service에 해당하는 것 같은데 클래스명을 GithubClientService가 아니라 GithubClient로 지어주신 이유가 있을까요??

제가 OAuth쪽은 익숙하지 않아서 잘 모르는 것일 수도 있습니다...!

컨트롤러의 요청을 받아 도메인 로직을 실행하고 이것을 반환하는 것이 아니므로 서비스라고 짓지 않았습니다.

amaran-th · 2023-07-22T08:33:11Z

backend/emm-sale/src/main/java/com/emmsale/login/application/GithubClient.java

+
+    if (accessToken == null) {
+      throw new LoginException(LoginExceptionType.NOT_FOUND_GITHUB_ACCESS_TOKEN);
+    }


유효성 검사 부분은 메서드로 추출해주는 편이 가독성에 더 좋을 것 같습니다!

amaran-th · 2023-07-22T08:48:41Z

backend/emm-sale/src/main/java/com/emmsale/login/exception/LoginExceptionType.java

+  ),
+  INVALID_GITHUB_ACCESS_TOKEN(
+      HttpStatus.NOT_FOUND,
+      "GitHub Profile을 찾을 수 없습니다."


'~을 찾을 수 없습니다'라는 종류의 에러라면 INVALID_~보다는 NOT_FOUND_~라는 수식어가 더 적절한 것 같다는 생각이 듭니다!
INVALID_GITHUB_ACCESS_TOKEN=>NOT_FOUND_GITHUB_PROFILE

저는 GithubProfile을 찾을 수 없는 이유가 유효하지 않은 github access token 때문이라고 생각하여 INVALID_GITHUB_ACCESS_TOKEN라고 작성하였습니다.
오류 메시지를 통해 현상을 알려주는게 좋을까요? 원인을 알려주는게 좋을까요?🤔

저는 프롤로그 에러 목록에서 에러 명과 에러 메시지가 전달하는 내용이 동일한 것을 보고 코멘트를 드렸었는데,
말씀을 듣고 보니 원인과 현상을 어떻게 나타내야 할지 고민이 되네요🤔
이건 저희끼리 함께 이야기해보고 커스텀 예외에 대한 규칙을 보완해도 좋을 것 같아요.

제 생각을 말씀드리자면 원인이 조금 더 디테일한 정보를 줄 수 있으니 오류 메시지를 'Github Access Token이 유효하지 않다'라고 바꾸는 것도 좋아 보여요.

이러한 에러 메시지의 경우에는, 어떻게 하면 이 API를 사용하는 사람(저희한테선 안드로이드겠죠?)에게 더 잘 이해가 되는지를 고려해볼 필요가 있다고 생각합니다.

저는 그런 입장에서 유효하지 않다는 의미보다 "찾을 수 없다"는 의미가 더 중요한 것 같아요.
그래서 NotFound를 유지하는 것이 더 괜찮아 보입니다.

amaran-th · 2023-07-22T11:19:00Z

backend/emm-sale/src/main/java/com/emmsale/login/utils/AuthorizationExtractor.java

+      }
+    }
+
+    return null;


NIT: null을 반환하고 호출부에서 null 체크를 해주는 것보다 Optional<> 객체를 반환하게 하면 호출부에서 더 안전하게 사용할 수 있을 것 같아요!
해당 메서드를 사용하는 곳이 한 곳밖에 없으면 당장은 신경쓰지 않아도 될 것 같긴 합니다.

amaran-th · 2023-07-22T11:26:23Z

backend/emm-sale/src/main/resources/application.yml

+security:
+  jwt:
+    token:
+      secret-key: secret_key


이 데이터를 추후 어디에 보관할지에 대한 논의가 필요할듯 하네요!(.env 파일을 사용한다든지)
참고 링크
추후 이 글의 yml 파일 내 환경변수 설정 섹션을 참조해보면 좋을 것 같아요. 저도 잘 몰라서 남겨봅니다ㅎㅎ;

#41

- githubId를 기준으로 사용자 조회 기능 구현 - 최초 사용자일 경우 회원가입 후 사용자 반환 기능 구현 #41

#41

amaran-th

커스텀 예외 관련해서 코멘트 하나 더 남겼으니 확인 부탁드릴게요!

java-saeng

resttemplate이랑 패키지 이름에 대한 리뷰 남겨두었으니 참고부탁드립니다

고생하셨습니다.

java-saeng · 2023-07-23T07:05:56Z

backend/emm-sale/src/main/java/com/emmsale/login/application/GithubClient.java

@@ -17,6 +17,8 @@
 @Component
 public class GithubClient {

+  private static final RestTemplate REST_TEMPLATE = new RestTemplate();


제 의도는 RestTemplate 도 그냥 주입 받으면 되지 않을까 생각해서 남겨뒀습니다.

굳이 상수화를 시키신 이유가 있으신가요??

java-saeng · 2023-07-23T07:06:28Z

backend/emm-sale/src/main/java/com/emmsale/login/ui/LoginApi.java

@@ -12,11 +12,11 @@

 @RestController
 @RequestMapping("/login")
-public class LoginController {
+public class LoginApi {


패키지 이름도 같이 변경해야하지 않을까요~~?

#41

hyeonjerry added 5 commits July 18, 2023 13:19

chore: 깃허브 로그인 변수 포맷 추가

ba1c7c3

#41

feat: 깃허브 accessToken 요청 클라이언트 구현

2f33d70

#41

feat: 깃허브 프로필 요청 클라이언트 구현

8c528a6

#41

feat: github code로부터 사용자 토큰 반환 기능 구현

52eb783

#41

feat: github 로그인 redirect api 구현

1f9b17c

#41

hyeonjerry requested review from hong-sile, java-saeng and amaran-th July 18, 2023 06:18

hyeonjerry self-assigned this Jul 18, 2023

hyeonjerry added Backend 백엔드 관련 이슈 기능 추가 새로운 기능 추가 및 기존 기능 변경 labels Jul 18, 2023

hyeonjerry added 4 commits July 18, 2023 17:41

Merge branch 'main' of https://github.com/woowacourse-teams/2023-emmsale

9d99baf

into Feature/#41-깃허브_로그인_기능_구현

fix: ServiceIntegrationTestHelper 추가

d1cd05c

#41

refactor: 커스텀 예외 추가

85e3454

#41

test: 로그인 서비스 테스트 구현

eff599f

#41

hong-sile changed the base branch from main to backend-main July 19, 2023 05:09

hyeonjerry added 3 commits July 19, 2023 18:14

chore: jwt 라이브러리 추가

1a101cb

#41

feat: 토큰 생성 및 발급 기능 구현

bb1b348

#41

feat: 새로운 유저 여부 정보 추가

5422e89

#41

hong-sile reviewed Jul 20, 2023

View reviewed changes

hyeonjerry added 7 commits July 20, 2023 14:08

refactor: JwtTokenProvider 패키지 이동

38a1afb

#41

feat: JwtTokenProvider Subject 추출 기능 구현

4701ddb

#41

feat: Authorization 헤더 추출 기능 구현

6179009

#41

feat: ArgumentResolver에 TokenProvider 등록

7a7b853

#41

test: TokenProvider 테스트 구현

5abbf9b

#41

refactor: 불필요한 코드 제거

28f71c0

#41

refactor: Custom 예외 추가

feeab0e

#41

java-saeng reviewed Jul 20, 2023

View reviewed changes

hong-sile approved these changes Jul 21, 2023

View reviewed changes

hyeonjerry added 3 commits July 22, 2023 11:53

refactor: 피드백 반영

e7f913d

- 문자열 상수화 - RestTemplate 싱글톤 관리 - raw type 제거 #41

refactor: 패키지 및 클래스명 변경

da1decf

#41

test: 불필요한 검증 제거

0cd66d0

#41

amaran-th reviewed Jul 22, 2023

View reviewed changes

hyeonjerry added 5 commits July 23, 2023 00:08

fix: default 생성자 오류 수정

c79cb57

#41

feat: githubId 컬럼 추가

39c97a4

#41

feat: githubId로 사용자를 조회하는 기능 구현

c003c2a

- githubId를 기준으로 사용자 조회 기능 구현 - 최초 사용자일 경우 회원가입 후 사용자 반환 기능 구현 #41

refactor: Authorization 추출 메서드 리팩터링

4d99f76

#41

refactor: 메서드 추출 및 예외처리 추가

1cc219d

#41

amaran-th requested changes Jul 23, 2023

View reviewed changes

java-saeng approved these changes Jul 23, 2023

View reviewed changes

hyeonjerry added 2 commits July 23, 2023 18:51

refactor: 패키지면 변경

b52fc15

#41

refactor: MemberOnly 어노테이션 제거

fac42f4

#41

hyeonjerry merged commit 679d18b into backend-main Jul 24, 2023
1 check passed

hyeonjerry deleted the Feature/#41-깃허브_로그인_기능_구현 branch July 31, 2023 04:30

Feature/#41 깃허브 로그인 기능 구현 #50

Feature/#41 깃허브 로그인 기능 구현 #50

Conversation

hyeonjerry commented Jul 18, 2023

#️⃣연관된 이슈

📝작업 내용

💬리뷰 요구사항(선택)

hong-sile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

java-saeng commented Jul 20, 2023

hong-sile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaran-th left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaran-th left a comment

Choose a reason for hiding this comment

java-saeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment