Authorization header를 사용하는 인증 클래스 생성 #954

zangsu · 2024-12-06T19:04:52Z

⚡️ 관련 이슈

file changed가 많이 잡혀서 코드 리뷰를 하기 용이하도록 작업을 분리하였습니다.
다음 작업때 해당 클래스를 사용하도록 코드가 변경될 예정입니다.

📍주요 변경 사항

AuthorizationHeaderCredentialManager 생성
- Authorization 헤더를 사용하는 CredentialManager를 생성하였습니다.
Credential 클래스 생성
- 인증 타입, 값을 가지는 래퍼 클래스 Credential을 생성하였습니다.

🎸기타

기존의 코드에서는 CredentialProvider와 CredentialManager가 개별적으로 동작하고 있었습니다.
ex) service에서 CredentialProvider가 String credential을 생성하고, controller에서 CredentialManager가 응답에 credential 설정

다만, CredentialManager와 CredentialProvider는 어느 정도 함께 동작해야 하는 문제가 있었습니다. issue comment
이에 CredentialProvider의 외부 사용을 제거하고 CredentialManager에서 CredentialProvider의 의존성을 가지도록 변경하였습니다.

🍗 PR 첫 리뷰 마감 기한

12/11 00:00

Co-authored-by: jminkkk <[email protected]>

Co-authored-by: zangsu <[email protected]>

Co-authored-by: jminkkk <[email protected]>

zangsu · 2024-12-06T19:08:00Z

@jminkkk
마지막 커밋에서 기존의 HeaderCredential을 Credential로 조금 더 일반화 해봤습니다.
이 부분은 몰리도 한번 확인해 주시고 편하게 의견 남겨주세욥...!

zeus6768

테스트가 아주 꼼꼼하네요~~

면접으로 바쁜 와중에 고생했어요!!

나름대로 꼼꼼히 보려고 노력했습니다 🔥

backend/src/main/java/codezap/auth/configuration/AuthWebConfiguration.java

backend/src/main/java/codezap/auth/controller/AuthController.java

backend/src/main/java/codezap/auth/dto/LoginAndMemberDto.java

backend/src/main/java/codezap/auth/manager/CookieCredentialManager.java

zeus6768 · 2024-12-08T08:57:05Z

backend/src/main/java/codezap/auth/manager/CredentialManager.java


    boolean hasCredential(HttpServletRequest httpServletRequest);

-    void setCredential(HttpServletResponse httpServletResponse, String credential);
+    void setCredential(HttpServletResponse httpServletResponse, Member member);


HttpServletResponse와 Member가 맞닿아있어서 불안하네요.

우리가 지금까지 DTO를 사용해 도메인과 웹을 격리했는데, 이렇게 되면 일관성이 깨지는 것 같아요.

CredentialManager와 CredentialProvider는 우리의 계층 구조에서 어디에 위치한다고 생각하나요?

저는 presentation 또는 web이라고 일컫는 계층에 있다고 생각하는데, 얘기를 나눠보고 싶네요.

자바 기본 타입을 사용하거나, DTO를 만들어서 데이터를 주고 받는 게 어떨까요?

CredentialManager는 단순히 인증 정보를 응답에 설정해 주는 책임만 가지도록 개선하였습니다.
이 과정에서 다시 CredentialManager와 CredentialProvider 사이의 의존성이 제거되었으며 두 객체 모두 도메인 객체인 Member와 의존성을 가지지 않도록 LoginMember라는 dto를 추가하였습니다.

이미 논의가 끝난 후인 것 같은데 다시 이야기해서 미안해요.
저는 반대로 생각해요 🥲
저는 여기서 DTO를 사용하면 일관성이 깨지는 것 같아요. 이 부분이 DTO로 변경되면 우리 이전 논의를 통해 결정했던 AuthArgumentResolver에서 반환하는 것도 Member가 되면 안된다고 생각해요.

제우스 의견처럼 다른 부분에서는 view에 영향을 최소화하기 위해 DTO를 사용하였지만 이 부분의 역할은 view에서 사용하기 위함은 아니라고 생각해요. Http 등 특정 웹 계층 관련 로직이 Member에 영향을 주면 안되지만 여기서는 그 �목적이 반대를 위함이기도 하구요.
오히려 DTO 관리 리소스가 드는 것 같은데 흠 다른 분들은 어떻게 생각하시나요?

너무 어려운 주제군요 ㅠㅠ 고민을 더 해보느라 시간이 걸렸습니다...

결국 도메인과 web 계층을 얼마나 완벽하게 격리시키느냐의 문제인데요.
제가 느끼기에는 Member 도메인을 웹 계층과 완벽하게 분리하는 것은 이상적이지만 그만큼 또 어렵다고 느껴집니다. 몰리 말처럼 지금도 controller의 파라미터로 Member를 받고 있기도 하고요.

또한, 지금의 로그인이 두 가지 역할을 가지는 것도 어려움을 야기하는 또 다른 문제점인데요.

인증 정보를 응답에 포함한다. (Cookie / Authorization 헤더 설정)

로그인 정보를 바디에 포함한다. (LoginResponse를 반환)

이 때문에 authService.login() 메서드가 Member, 또는 해당 정보를 갖는 dto를 반환해야 하는 상황이에요.

이 문제에 대한 개인적인 선호는 도메인이 controller 영역까지 노출되거나 HttpServletResponse가 Service 영역까지 침투하는 것도 허용 가능한 범위에요.
특히 지금같은 경우 로그인이라는 기능이 "인증 정보 유지를 위한 설정"의 책임까지 가지고 있기 때문에 authService.login() 메서드의 파라미터로 response를 넘겨주는 것이 가장 마음이 편하기도 하고요.

다만, 이 부분에 대해선 각자 느끼는 기준, 허용치 등이 다를 것 같아서 의견을 더 주시면 감사하겠습니다.

backend/src/test/java/codezap/auth/manager/AuthorizationCredentialManagerTest.java

backend/src/test/java/codezap/auth/manager/CookieCredentialManagerTest.java

backend/src/test/java/codezap/auth/provider/PlainCredentialProvider.java

kyum-q

짱수 고생했어요! 전반적으로 깔끔한 것 같네요 ! 리뷰를 뭘 할까 열심히 보는데 제우스가 이미 남긴 리뷰 외에 남길 리뷰가 없네요 ..!
제우스 리뷰 중에 동의하는 부분이 많아서 일단 코멘트로 리뷰 남길게요~ 리뷰 반영되고 바로 어프루브 하겠습니다~

jminkkk

대부분 페어로 진행해서 따로 코멘트 남길 부분은 없네요!!
수고했어요 짱수 👍👍👍

HoeSeong123

으악 리뷰 마감기한이 어제까지였네요...
늦어서 죄송합니다ㅠㅠ
간단하게 확인해봤는데 켬미 말대로 제우스 코멘트 이외에 리뷰할만한 부분이 크게 없는 것 같네요.
너무 고생하셨습니다👍

zeus6768

고생많았어요 짱수~!

NPE 위험이 있어 코멘트 하나만 더 추가했어요 😭

zeus6768 · 2024-12-15T23:01:01Z

backend/src/main/java/codezap/auth/configuration/AuthArgumentResolver.java

@@ -37,7 +36,7 @@ public Member resolveArgument(
        if (!parameterAnnotation.required() && !credentialManager.hasCredential(request)) {


아래와 같이 테스트코드 추가해서 확인해보니 parameterAnnotation에서 NPE가 발생할 수 있네요.

@Nested @DisplayName("파라미터 반환 테스트") class ResolveArgument { static class ResolveTestController { public void notRequiredMethod(@AuthenticationPrinciple(required = false) Member member) ... public void requiredMethod(@AuthenticationPrinciple Member member) ... public void unsupportedMethod(Member member) {} } @Nested @DisplayName("required 값이 false 일 경우") class RequiredFalseTest { Method notRequiredMethod = ... Method unsupportedMethod = ReflectionSupport.findMethod( ResolveTestController.class, "unsupportedMethod", Member.class).orElseThrow(); @Test @DisplayName("NPE") void npeTest() { assertThatNullPointerException().isThrownBy(() -> resolveArgument(unsupportedMethod, nativeWebRequest)); } ...

그래서 아래와 같이 개선 제안해요~~

Suggested change

if (!parameterAnnotation.required() && !credentialManager.hasCredential(request)) {

@Override

public Member resolveArgument(

@NonNull MethodParameter parameter,

ModelAndViewContainer mavContainer,

NativeWebRequest webRequest,

WebDataBinderFactory binderFactory

) {

AuthenticationPrinciple parameterAnnotation = parameter.getParameterAnnotation(AuthenticationPrinciple.class);

boolean supported = Objects.nonNull(parameterAnnotation);

HttpServletRequest request = (HttpServletRequest) webRequest.getNativeRequest();

if (supported && !parameterAnnotation.required() && !credentialManager.hasCredential(request)) {

return null;

}

Credential credential = credentialManager.getCredential(request);

return credentialProvider.extractMember(credential);

}

zeus6768 · 2024-12-15T23:04:35Z

backend/src/main/java/codezap/auth/dto/response/LoginResponse.java

@@ -1,12 +1,12 @@
 package codezap.auth.dto.response;

-import codezap.member.domain.Member;
+import codezap.auth.dto.LoginMember;

 public record LoginResponse(
        Long memberId,


혹시 null이 들어올 수 있는 경우가 있나요?

없다면 primitive type long으로 바꾸는게 좋을 것 같아요!

jminkkk · 2024-12-16T06:21:55Z

backend/src/main/java/codezap/auth/controller/AuthController.java

+        credentialManager.setCredential(httpServletResponse, credential);
+        return ResponseEntity.ok(LoginResponse.from(loginMember));


제우스 리뷰에 코멘트 남긴 것에 이어서 의견 남길게요.
여기서도 LoginMember DTO를 다시 한번 감싼 것이 비용이라고 생각이 돼요 🥲

HoeSeong123

확인했습니다👍

kyum-q

수고하셨습니다~!!

zangsu and others added 10 commits December 6, 2024 13:56

refactor: CredentialManager 에서 Member를 반환하는 메서드 추가

f87be14

Co-authored-by: jminkkk <[email protected]>

refactor: AuthArgumentResolver에서 CredentialProvider 의존성 제거

88c93e6

Co-authored-by: jminkkk <[email protected]>

refactor: CredentialManager.getCredential 제거

2339fa0

Co-authored-by: jminkkk <[email protected]>

refactor: 사용되지 않는 메서드 제거

040e9bd

Co-authored-by: jminkkk <[email protected]>

refactor(manager): CredentialManager 내부에서 인증값을 생성하도록 변경

0a9fcd1

Co-authored-by: zangsu <[email protected]>

refactor(service): AuthService에서 CredentialProvider를 제거

82978a6

Co-authored-by: zangsu <[email protected]>

refactor(provider): provider에 인증값의 타입을 반환하는 책임 부여

a6ce5e7

Co-authored-by: zangsu <[email protected]>

refactor(header): AuthorizationHeader를 사용하는 CredentialManager 객체 구현

da7922c

Co-authored-by: zangsu <[email protected]>

refactor: HeaderCredential 테스트 및 예외 처리 추가

d6bd86f

Co-authored-by: jminkkk <[email protected]>

refactor: HeaderCredential 을 공용으로 사용하는 Credential 로 변경

29d9f97

Co-authored-by: jminkkk <[email protected]>

zangsu added refactor 요구사항이 바뀌지 않은 변경사항 BE 백엔드 labels Dec 6, 2024

zangsu requested review from zeus6768, jminkkk, kyum-q and HoeSeong123 December 6, 2024 19:04

zangsu self-assigned this Dec 6, 2024

zeus6768 requested changes Dec 10, 2024

View reviewed changes

kyum-q reviewed Dec 10, 2024

View reviewed changes

jminkkk approved these changes Dec 11, 2024

View reviewed changes

HoeSeong123 reviewed Dec 12, 2024

View reviewed changes

zangsu added 8 commits December 13, 2024 22:02

refactor: 불필요한 개행 제거

87e16c3

refactor: 일관성을 위한 ResponseEntity 사용

39227c4

refactor: 파라미터의 final 제거

fa52a9a

refactor: SameSite 설정을 스프링부터 상수 사용

b3e7cb2

refactor: 포맷 적용 및 상수화 적용

e7c68c0

refactor: getter 대신 필드를 직접 사용

e0c2d96

refactor: 테스트클래스 이름을 구현 클래스와 일치하도록 변경

7d7a017

refactor: 일관성을 위해 assertJ 사용하도록 변경

5f18590

zangsu added 5 commits December 13, 2024 22:23

refactor: 사용하지 않는 코드 정리

dda3ef4

refactor: Member 도메인을 presentation 계층과 격리

6c60e01

refactor: CredentialProvider 와 CredentialManager 를 분리

8911a1e

refactor: 필요 없는 import 제거

b04d4dd

docs: Javadoc 추가

4fc9b5a

zangsu requested review from HoeSeong123, kyum-q, zeus6768 and jminkkk December 15, 2024 13:47

zeus6768 requested changes Dec 15, 2024

View reviewed changes

jminkkk reviewed Dec 16, 2024

View reviewed changes

HoeSeong123 approved these changes Dec 16, 2024

View reviewed changes

kyum-q approved these changes Dec 16, 2024

View reviewed changes

zangsu added 2 commits December 17, 2024 17:32

refactor: 기본 타입으로 변경

018ebab

refactor: NPE 발생 방지

80b40e3

zangsu requested a review from zeus6768 December 17, 2024 11:39

zeus6768 approved these changes Dec 18, 2024

View reviewed changes

zeus6768 merged commit b274f8d into dev/be Dec 18, 2024
3 checks passed

zeus6768 deleted the refactor/908-use-authorization-header branch December 18, 2024 00:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authorization header를 사용하는 인증 클래스 생성 #954

Authorization header를 사용하는 인증 클래스 생성 #954

zangsu commented Dec 6, 2024 •

edited

Loading

zangsu commented Dec 6, 2024 •

edited

Loading

zeus6768 left a comment

zeus6768 Dec 8, 2024

zangsu Dec 15, 2024

jminkkk Dec 16, 2024 •

edited

Loading

zangsu Dec 17, 2024

kyum-q left a comment •

edited

Loading

jminkkk left a comment

HoeSeong123 left a comment

zeus6768 left a comment

zeus6768 Dec 15, 2024

zeus6768 Dec 15, 2024

jminkkk Dec 16, 2024

HoeSeong123 left a comment

kyum-q left a comment

		@@ -37,7 +36,7 @@ public Member resolveArgument(
		if (!parameterAnnotation.required() && !credentialManager.hasCredential(request)) {

-        if (!parameterAnnotation.required() && !credentialManager.hasCredential(request)) {
+    @Override
+    public Member resolveArgument(
+            @NonNull MethodParameter parameter,
+            ModelAndViewContainer mavContainer,
+            NativeWebRequest webRequest,
+            WebDataBinderFactory binderFactory
+    ) {
+        AuthenticationPrinciple parameterAnnotation = parameter.getParameterAnnotation(AuthenticationPrinciple.class);
+        boolean supported = Objects.nonNull(parameterAnnotation);
+        HttpServletRequest request = (HttpServletRequest) webRequest.getNativeRequest();
+        if (supported && !parameterAnnotation.required() && !credentialManager.hasCredential(request)) {
+            return null;
+        }
+        Credential credential = credentialManager.getCredential(request);
+        return credentialProvider.extractMember(credential);
+    }

		credentialManager.setCredential(httpServletResponse, credential);
		return ResponseEntity.ok(LoginResponse.from(loginMember));

Authorization header를 사용하는 인증 클래스 생성 #954

Authorization header를 사용하는 인증 클래스 생성 #954

Conversation

zangsu commented Dec 6, 2024 • edited Loading

⚡️ 관련 이슈

📍주요 변경 사항

🎸기타

🍗 PR 첫 리뷰 마감 기한

zangsu commented Dec 6, 2024 • edited Loading

zeus6768 left a comment

Choose a reason for hiding this comment

zeus6768 Dec 8, 2024

Choose a reason for hiding this comment

zangsu Dec 15, 2024

Choose a reason for hiding this comment

jminkkk Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

zangsu Dec 17, 2024

Choose a reason for hiding this comment

kyum-q left a comment • edited Loading

Choose a reason for hiding this comment

jminkkk left a comment

Choose a reason for hiding this comment

HoeSeong123 left a comment

Choose a reason for hiding this comment

zeus6768 left a comment

Choose a reason for hiding this comment

zeus6768 Dec 15, 2024

Choose a reason for hiding this comment

zeus6768 Dec 15, 2024

Choose a reason for hiding this comment

jminkkk Dec 16, 2024

Choose a reason for hiding this comment

HoeSeong123 left a comment

Choose a reason for hiding this comment

kyum-q left a comment

Choose a reason for hiding this comment

zangsu commented Dec 6, 2024 •

edited

Loading

zangsu commented Dec 6, 2024 •

edited

Loading

jminkkk Dec 16, 2024 •

edited

Loading

kyum-q left a comment •

edited

Loading