Skip to content
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

[jangbagoony팀 이노 & MJ] OAuth 구현, 로그인, 로그아웃 API 구현 #67

Open
wants to merge 43 commits into
base: jangbagoony
Choose a base branch
from

Conversation

eNoLJ
Copy link

@eNoLJ eNoLJ commented Apr 30, 2021

금요일이 되어서야 두번째 PR을 날립니다.

지난 PR과 크게 달라진 점은 2가지 입니다.

  1. 현재 dish 관련 테이블이 조인테이블까지 7개입니다. 지난주에는 각각의 테이블마다 Repository를 만들어 구현을 했습니다. 이 부분을 oneToMany 매핑을 통해 리팩토링을 진행하여 3개의 Repository를 줄일 수 있었습니다. 코드도 상당히 깔끔해졌어요.

  2. OAuth를 적용했습니다. 흐름은

  • 클라이언트에서 스코프를 지정해 code를 받습니다.
  • 코드를 저희 서버에 login API를 사용하여 넘겨줍니다.
  • 서버에서는 code값과 client_id, client_secret을 갖고 토큰 값을 받습니다.
  • 토큰값을 가지고 github에 필요한 정보를 가져옵니다.
  • 해당 정보를 db에 저장, 클라이언트로 유저 정보를 넘겨줍니다.

로그아웃은 해당 토큰을 파괴하는 흐름으로 만들었습니다. 현재 코드에 문제점은 깃헙에서 받아온 토큰을 그대로 클라이언트로 넘겨주고 있습니다. 원래는 세션이든 토큰이든 별개의 방법을 정해서 서버와 클라이언트가 소통해야한다는 점은 숙지하고 있습니다. 다만 시간이 없어서 그 부분까진 진행을 하지 못했네요... 그리고 cors부분도 작동을 잘 안해서 일단 주석처리 후 모든 접속을 허용했습니다. 이 부분도 좀 더 공부하여 수정하도록 하겠습니다. 너그러운 마음으로 이해부탁드리겠습니다!!

여기까지가 변경된 사항이고 중점적으로 리뷰해주시면 좋을 것 같은 부분을 말씀드리겠습니다.
UserService에서 깃헙 API를 사용하고 있습니다. 서버에서 서버로 요청을 한 경험은 처음이라 이 부분 어떻게하면 분리/관리를 할 수 있을까 고민을 했는데 뚜렷한 방법이 생각나지 않았습니다. 현재 하고 있는 방식이 맞는지도 모르겠어요. 보시고 많은 지적 부탁드리겠습니다.

리뷰어분께서 현재 pr을 리뷰하실 때 프로젝트가 오늘부로 끝나서 리뷰적용을 잘 할수 있을까 생각하실지도 모르겠습니다.
솔직히 저도 적용까진 확답을 드리진 못하겠지만 해주신 리뷰 하나하나 읽어보며 숙지하도록 하겠습니다. 그러기 위해 pr을 남겼습니다. 가능하면 적용까지 하도록 하겠습니다!!! 그러니 잘 부탁드릴게요ㅎㅎ

DishService 부분은 지난 리뷰로 꽤나 개선되었다고 생각하는데 UserService 부분은 항상 그런것처럼 맘에 들진 않는데 머가 문제인지 보이지가 않습니다. 도와주시면 정말 감사하겠습니다!!

eNoLJ and others added 30 commits April 27, 2021 17:47
Image, DetailImage, DishBadge 엔티티에 대해 oneToMany 관계 설정, 관련 메소드 생성
Dish 클래스에서 oneToMany 관계가 설정 되었기 때문에 dishId 멤버변수가 불필요해져 삭제
Dish 클래스에 oneToMany 매핑 설정이 되었기 때문에 불필요한 매개변수 삭제
기존에 repository를 사용하여 값을 조회하던 방식에서 oneToMany 매핑을 통해 조회하도록 변경, 관련 메소드 수정 및 생성
로그인 기능을 위한 user 테이블 생성 쿼리문 생성
로그인 기능을 위해 User 엔티티 클래스와 UserRepository 인터페이스 생성
로그인 기능을 위해 UserController 클래스 생성, login 메소드 생성 중
oauth 인증을 통해 토큰 값을 가져올 때 사용하기 위해 TokenDTO 생성
Spring jdbc의 관계매핑을 사용하여 리팩토링
UserController 클래스에 있던 비지니스 로직은 UserService 클래스를 만들어 이동, code를 통해 토큰을 얻어오는 tokenRequestApi 메소드, 토큰으로 유저 정보를 얻어오는 userInfoRequestApi 메소드, 토큰으로 유저 이멘일을 얻어오는 emailRequestApi 메소드 생성
TokenDTO 클래스에 getAccess_token 메소드, toString 메소드 생성,
유저 이메일을 전달하기 위해 EmailDTO 클래스 생성,
유저 정보를 전달하기 위해 UserInfoDTO 클래스 생성
@MJbae MJbae added the review-BE BE 리뷰 label Apr 30, 2021
@MJbae MJbae requested a review from ksundong April 30, 2021 10:27
@wheejuni wheejuni self-requested a review May 2, 2021 01:17
@wheejuni wheejuni self-assigned this May 2, 2021
Copy link

@wheejuni wheejuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우선 짧은 시간에 많은 스펙 구현하시느라 고생 많으셨습니다. 💯
유저 서비스도 살펴봤는데요, 우선 문제는 깃허브라는, 애플리케이션 외부 요소와 강하게 결합되어 있다는 점입니다.
이 부분을 개선할 수 있는 힌트(?) 를 드렸는데 반영하려면 아마 시간이 꽤 많이 필요할 겁니다.
우선 읽어보시고, 차분히 고민하는 시간 가져보시면 좋겠어요.

다시 한 번 수고 많으셨습니다. 🥇

Comment on lines +41 to +42
.collect(Collectors.toList())
.get(0);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.findFirst() 를 쓰면 예외 처리까지 잘 할 수 있겠네요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분 리스트에서는 어떻게 예외처리를 하지? 싶었다가 넘긴 부분이였는데 findFirst()라는 메소드가 있군요. 참고하겠습니다. 감사해요!!ㅎㅎ


public User() {}

public User(UserInfoDTO userInfoDTO, EmailDTO emailDTO, TokenDTO tokenDTO) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DTO에서 정보를 가져오는 구현이 꼭 필요하다면, 별도의 스태틱 메소드나 빌더를 활용해 주시고요,
가급적이면 생성자는 필드 그대로, 타입 그대로 받는 것이 좋다고 생각합니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오홍!! 그렇군요. 타입을 그대로 받는 생성자를 만들고 스태틱 메소드를 활용하도록 하겠습니다. 지적해 주신 부분 생각해보니까 타입을 그대로 받는 생성자가 좀 더 범용성?이 있겠네요!! 감사합니다.

Comment on lines +27 to +30
public static final String GITHUB_CLIENT_ID = "github.client.id";
public static final String GITHUB_SECRET = "github.secret";
public static final String CLIENT_ID = "client_id";
public static final String CLIENT_SECRET = "client_secret";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아마 이 피드백을 반영하기엔 작업의 규모가 너무 커지겠지만 UserService 는 깃허브의 존재 조차도 몰라야 합니다.
충분히 추상화된 외부 API 접속(?) 모듈의 메소드들을 호출하면서, 규격화된 유저 정보만 받아와야 합니다.
그러자면 아래와 같은 컴포넌트(인터페이스) 들이 필요할 겁니다.

  • 외부 OAuth 공급자와의 통신을 책임지는 컴포넌트, 높은 확률로 인터페이스
  • 외부 OAuth 공급자에게 받아와야 할 규격화된 유저 정보.
  • 외부 OAuth 공급자를 비롯한 서버 외부로 향하는 모든 통신을 책임지는 컴포넌트.

이렇게 컴포넌트와 모델 클래스 분리를 한 다음, UserService 에서는 구현체가 아닌 타입에 의존하게 하면 될 겁니다.
그러니까 `UserService는 아래와 같은 종속성을 갖게 됩니다.

public class UserService {
    //OAuthConnector는 인터페이스임, 깃허브인지 네이버인지 카카오인지...공급자는 알 수 없음
    private final OAuthConnector connector;

추후 설계에 참고가 되시면 좋겠습니다.

Copy link
Author

@eNoLJ eNoLJ May 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wheejuni 오호.... 아주 약간은 흐름에 대해 이해가 된 것 같은데 지금 당장 코드를 쳐보면서 구현할 것을 생각해보니 어떻게 해야할지 팍 떠오르지 않네요. 이 리뷰는 앞으로도 계속 보면서 구체화 시키도록 하겠습니다. 현재 상태에선 좀 더 질문을 하려해도 어떻게, 무엇을 질문 해야할지도 잘 모르겠네요. 하하핰ㅋㅋㅋㅋㅋ 비슷하게 구현 된 코드를 보면 이해하는데 좀 더 도움이 될 것 같은데 이건 너무 떠먹여달라는 식이겠죠...??ㅎㅎ

아무튼 진짜 전혀 감을 못잡고 있었는데 브라이언 덕분에 도움이 되었어요. 정말 감사해요. 브라이언!! 덕분에 나날이 발전해가는게 느껴집니다ㅎㅎ

Comment on lines +65 to +66
String id = environment.getProperty(GITHUB_CLIENT_ID);
String secret = environment.getProperty(GITHUB_SECRET);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

서비스 클래스 내부에서 특정 공급자와의 강결합이 발생되었습니다.
이로 인해 확장성은 거의 0에 수렴하게 되었고, 앞으로 깃허브의 API 연동 규격 변경에도 쉽게 대응할 수 없는 구조가 됐습니다.

Copy link
Author

@eNoLJ eNoLJ May 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wheejuni 음... 그럼 어떤식으로 변경을 해야할까요?? 전혀 감이 잡히지 않는데 좀 더 힌트를 주실 수 있으실까요!!??

Comment on lines +59 to +61
User user = findByToken(token);
user.removeToken();
userRepository.save(user);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로그아웃 할때마다 유저DB에 업데이트해야 하나요? 왜 그렇죠?
토큰 테이블을 따로 구현해서, 토큰 테이블에 대한 업데이트만 수행되면 될 것 같아요.

Copy link
Author

@eNoLJ eNoLJ May 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그 말씀은 토큰값과 user_id를 포린키로 갖는 테이블을 별도로 만들라는 말씀이시죠?? 생각도 못했는데 이 방법이 훨씬 좋겠네요!!ㅎㅎ

@eNoLJ
Copy link
Author

eNoLJ commented May 2, 2021

@wheejuni 일요일인데 이렇게 리뷰를 해주셔서 정말 감사드립니다. 새로운 기능(OAuth)이 생기니 역시 문제점들도 많이 생기네요. 문제들이 있다는건 인식이 되지만 막상 어떻게 접근을 해야하고 어떻게 관련 지식을 찾아 적용해야할지 감이 잡히지 않습니다ㅎㅎ;; 현재처럼 리뷰어분이 계시면 정 안될 때 물어보기라도 할텐데 없다면 어떻게 해야할까 고민이 되네요. 더군다나 이런 경험이 앞으로 새로운 것을 적용할 때마다 반복될텐데 말이죠. 물론 이런 경험이 기분이 나쁜건 아닙니다. 오히려 환영이죠. 해결하고 하다보면 분명 훨씬 발전해 있을테니까요. 다만 그 해결 방법에 대해 어떻게하면 좀 더 빠르고 효율적으로 할 수 있을지 궁금하네요ㅎㅎ
리뷰어분께서도 분명 이런 기분을 느끼셨을 때가 있을 것이기 때문에 여유가 되신다면 꼭 좀 답변 부탁드립니다. 하나의 문제에 대해 해결방법을 아는 것보다 이런한 문제들이 생겼을 때 어떻게 접근하면 좋을지가 더 궁금하네요!!!!

@eNoLJ eNoLJ requested a review from wheejuni May 2, 2021 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-BE BE 리뷰
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants