[BE] 깃허브 인증 구현과 세션 권한 체계 설계

[nayonsoso]

• resolves [BE] 깃허브 로그인/회원가입을 구현하고, 세션의 권한 체계를 설계한다. #1059

---

🚀 어떤 기능을 구현했나요 ?

• 깃허브 인증을 구현했습니다.
  • Client에 대해서는 레벨2에서 지식이 머물러 있습니다.. 🤧
  • 일단 동작을 목표로 했습니다!
• 권한 체계를 설계했습니다.
  • 기존에는 reviewRequestCode 를 세션에 저장하고, argumentResolver 에서 이를 바탕으로 조회했습니다.
    그래서 인증을 위해서 요청마다 한번씩 DB를 조회했는데, 이 접근을 줄일 수 있을 것 같습니다.
  • 그래서 세션에 직접 객체를 넣는 방식을 생각하게 되었고, 우리는 세션에서 두가지 유형을 포괄해야 하니 추상화를 해봤습니다.
  • 이것들을 일종의 '권한에 대한 도메인' 으로 생각하니, 관련 기능을 수행하는 서비스 객체인 PrincipalService 까지 도출하게 되었습니다.
• 권한 체계에 대한 앞으로의 계획은 아래와 같습니다.
  • argumentResolver 를 사용해서, 컨트롤러에서 세션을 Principal 형식으로 받게 합니다.
    컨트롤러는 인증에 대해 아~무것도 모른 채로, 서비스에게 Principal 넘겨줍니다.
  • 서비스도 Principal 이 어떻게 구현되었는지 모른채로, PrincipalService에서 필요한 함수를 호출해서 기능을 분기합니다.

🔥 어떻게 해결했나요 ?

📝 어떤 부분에 집중해서 리뷰해야 할까요?

• GithubOauthClient가 깃허브 api 명세랑 똑같은데, 테스트를 짜야 할지? 고민입니다..
• GithubOauthClient 를 auth.infrastructure 패키지 아래에 두는건 어떨까요?
• Member 스키마 괜찮나요? 깃허브에서 프로필이랑 사용자 이름 받아오니까 식별자랑 이메일만 저장해도 되지 않을까? 싶었어요
• 회원가입/로그인을 한 api에서 진행하게 한 것 괜찮나요? 논의는 안되었던 부분이긴 한데 이게 편할 것 같아서 일단 해봤어요🥹

📚 참고 자료, 할 말

[github-actions]

Test Results

148 tests   145 ✅  4s ⏱️
 54 suites    3 💤
 54 files      0 ❌

Results for commit 29d8500.

♻️ This comment has been updated with latest results.

[donghoony]

1차 RC입니다~ 인증 Http는 다음리뷰에서 진행할게요~

[donghoony]

abstract class? interface로 하지 않을 이유가 있나요?

• Principal이라는 명명이 조금 어색하다고 생각해요. 인증/인가와 관련된 것 같은데 단순히 직역하면 '주요한'이라는 의미라서요.
• interface로 바꾼 뒤, 이를 구현하는 구현체에 accessReviewGroup과 같은 메서드를 준 다음 빈으로 등록하고, 현재 PrincipalService 쪽에서 주입받아 instanceof 없이 진행해보는 건 어떤가요? 이렇게 하면 다형성도 챙기고, Service 쪽에서 구현체 클래스를 몰라도 될 것 같아요.

[nayonsoso]

Principal 이라는 명명이 조금 어색하다고 생각해요.

사실 저도 네이밍에 고민이 많았는데..
Principal 은 스프링 시큐리티에서 신원을 나타낼 때 사용하더라고요!
거기에서 차용해봤어요.

다른 후보로는 이런 것들이 있었는데, 스프링 시큐리티에서 사용한다는걸 안 후로 Principal이 가장 좋아보아더라고요🤔

• SiteUser (우리 사이트의 사용자)
  • SignedinSiteUser
  • ReviewGroupOwner
• Subject (어떤 행위의 주체)
  • MemberSubject
  • ReviewGroupSubject
• Authority (말 그대로 인증 정보)
  • MemberAuthority
  • ReviewGroupAuthority

이 정보를 알고도 어색하게 느껴지나요? 아니면 후보 중에서 대체할만하다 싶은게 있나요?

[nayonsoso]

👮디스코드에서 논의된 내용 깃허브에도 남겨둡니다👮

interface로 바꾼 뒤, 이를 구현하는 구현체에 accessReviewGroup과 같은 메서드를 준 다음 빈으로 등록하고, 현재 PrincipalService 쪽에서 주입받아 instanceof 없이 진행해보는 건 어떤가요? 이렇게 하면 다형성도 챙기고, Service 쪽에서 구현체 클래스를 몰라도 될 것 같아요.

문제를 느낀 부분 :

• PrincipalService에서 Principal의 세부 구현체들을 알고 있는 것
• Auth가 ReviewGroup이나 Member 를 알고 있는 것

논의를 했지만 결론이 나지 않은 것 :

• Session 에서 어디까지를 저장해야 하나? 구체 클래스? 아니면 인증 정보?
  (e.g. Member 를 저장할 것인가 memberId 를 저장할 것인가)

더 알아볼 키워드 :

• Role-based, Attribute-based, Policy-based

[Kimprodp]

세션에 객체 자체를 저장하는게 데이터 동기화나 확장성 측면에서 좋진 않지만,
세션을 사용한다는거 자체가 이미 확장성이 떨어져서 뭐 괜찮다고 봅니다.
어차피 서버도 한개고, 만약 나중에 추가할 일이 생기면 그때 세션말고 다른 방식을 사용하는게 더 좋아보여요.

그리고 도메인간 참조 문제는, 지난번에 리팩터링 얘기할 때 나왔던 부분이지만 아예 완벽하게 끊을 거 아니면 불가능한걸로 이야기 한걸로 기억해서 상관 없다고 봅니다. (저거 신경 쓸거면 나머지 다른 패키지도 다 끊어야 함)

PrincipalService에서 Principal의 세부 구현체들을 알고 있는 것

이 부분은 따로 코멘트 남길게요.

[nayonsoso]

세션을 사용한다는거 자체가 이미 확장성이 떨어져서 뭐 괜찮다고 봅니다.
어차피 서버도 한개고, 만약 나중에 추가할 일이 생기면 그때 세션말고 다른 방식을 사용하는게 더 좋아보여요.

동의합니다👍

그리고 [Auth가 ReviewGroup이나 Member 를 알고 있는 것] 이 부분에 대해서 더 이게 맞다고 생각하게 된 일이 있었어요.
제가 하는 다른 프로젝트에서는 jwt 토큰으로 인증을 하는데,
모든 요청에 대해서 [jwt 파싱 → jwt에 있는 사용자 정보로 DB 조회] 를 하고 있습니다.
그런데 세션은 세션 스토리지라는 특장점이 있으니, 요청마다 DB를 조회하지 않아도 됩니다.
이를 잘 활용하기 위해서 '인증에 대한 객체 자체를 저장하고 있는 지금 방식이 맞구나!' 하는 교훈을 얻었습니다 ㅎㅎ

예를 들어, 의존을 끊기 위해서 세션에 memberId 나 reviewGroupId 만 저장했다면
jwt 방식과 마찬가지로 요청마다 DB를 조회해야 했을테니깐요.

[Kimprodp]

일단 1차 리뷰 남겨요~ 확인 후 다시 리뷰하겟슴다.

+) @EnableConfigurationProperties(GitHubOAuthProperties.class)가 중복되어있어서 config 파일을 못보고 몇개 잘못 리뷰달린게 있네요. 다시 코멘트 달아서 무시해도 됩니다!

[Kimprodp]

리반에 추가적인 의견과 코멘트 추가했습니다!

[Kimprodp]

다른 코멘트는 모두 반영 확인했어요!
다만, 처음 질문했던 부분 아직 해소되지 않아서😭 빠르게 답변 받고 나면 다음 부분들(domain) 리뷰 작성할게요.

[Kimprodp]

나머지 리뷰 남갸요~

[Kimprodp]

요건 사용되는 곳이 안보이는데 일단 받아두나요?

[nayonsoso]

사용자 닉네임/프로필을 보여줄 때 사용될 값입니다.
지금은 아직 api 구현을 안해서 사용되는 곳이 없어 보이는 것 뿐입니다~

[Kimprodp]

세션에 객체 자체를 저장하는게 데이터 동기화나 확장성 측면에서 좋진 않지만,
세션을 사용한다는거 자체가 이미 확장성이 떨어져서 뭐 괜찮다고 봅니다.
어차피 서버도 한개고, 만약 나중에 추가할 일이 생기면 그때 세션말고 다른 방식을 사용하는게 더 좋아보여요.

그리고 도메인간 참조 문제는, 지난번에 리팩터링 얘기할 때 나왔던 부분이지만 아예 완벽하게 끊을 거 아니면 불가능한걸로 이야기 한걸로 기억해서 상관 없다고 봅니다. (저거 신경 쓸거면 나머지 다른 패키지도 다 끊어야 함)

PrincipalService에서 Principal의 세부 구현체들을 알고 있는 것

이 부분은 따로 코멘트 남길게요.

[Kimprodp]

모든 타입에 대해서 다운캐스팅하면 추상화의 의미가 떨어진다고 생각해요.
타입이 증가하면 그만큼 또 수정해야할거고..

제네릭 방식 사용 제안합니다.

@Getter
public abstract class Principal<T> {
    private final T value;

쓰는 곳에서 getter에서 타입 지정해서 사용하면 PrincipalService 없어도 될 것 같아요.

[nayonsoso]

테드가 말한대로 정석적인 제네릭 방식으로 구현해봤는데,
'회원과 비회원 모두 호출할 수 있는 api' 부분에서 이슈가 있었습니다.
이런 api의 경우, 컨트롤러에서 어떤 타입인지 정하지 않은 상태에서 받게 되므로, 인자가 Principal<?> 가 됩니다.
즉, 제네릭을 적절히 사용할 수 없는 상황이 생겨서 이런 코드가 생기게됩니다.

public void 회원과_비회원_모두_사용_가능한_함수(제네릭_Principal<?> principal) {
    if (principal.getType() == Member.class) {
        Member member = (Member) principal.getPrincipal();
        // 회원 로직
    }

    if (principal.getType() == ReviewGroup.class) {
        ReviewGroup reviewGroup = (ReviewGroup) principal.getPrincipal();
        // 비회원 로직
    }
}

이렇게 타입 캐스팅이 일어나는 코드는 좋지 않다고 생각해서,
Principal 객체 안에 isMemberPrincipal, isMemberPrincipal, getMember, getReviewGroup 함수를 만들었는데,
이렇게 하다보니 제네릭이나 상속을 쓸 필요가 없어졌습니다.

제가 처음 사용했던 상속의 방식은 제네릭 하위호환이라 생각해서 제외하고..
두가지 중에서 선택하면 좋을 것 같습니다.
(1) 제네릭 + 와일드카드 + 타입 캐스팅 방식
(2) 하나의 클래스에서 다 처리하는 방식

제가 둘 다 코드로 구현해봤는데, 이걸 보고 선택해주세영~
cf42952
저는 구현하면서 생각한건데, 하나의 클래스에서 다 하는 방식이 은근 괜찮더라고요?!
클린 코드에서 퇴화하는 느낌이 들긴 하는데🥸 회원/비회원 방식 말고 다른게 추가될것 같진 않아서 괜찮아보여요!