[고정완] OAuth 2.0 리뷰 요청드립니다. #8

ghojeong · 2025-02-19T13:00:11Z

OAuth2 의 학습에 목적을 두고 Spring Security 의 소스코드를 최대한 이해할 수 있도록 노력했습니다.
Value Object 로 다루어지는 객체들은 record 문법을 사용해서 표현을 해볼 수 있도록 도전했습니다.

기존의 SecurityConfig 와 별개로, OAuth 의 핵심이 되는 설정들은 OAuthConfig 에서 따로 설정을 하도록 하였습니다.

개인적인 감상으로 OAuth2 미션에서 가장 핵심이 되었던 개념들은,
ClientRegistrationRepository,
AuthorizationRequestRepository,
OAuth2ProfileUser,
OAuth2AuthorizationRequest,
OAuth2AuthorizationRequestResolver,
OAuth2UserService,
였던것 같습니다.

OAuth2LoginAuthenticationFilter 가 특히 어렵고 힘들었는데,
어떻게든 최대한 이해가능하게끔 단순화 하려 노력했습니다.

이전 인가 미션에서 진행되었던 AuthenticationProvider 이 핵심적으로 다시 재활용되는 것도 인상깊었습니다.
OAuth2LoginAuthenticationProvider
OAuth2AuthorizationCodeAuthenticationProvider
위 2개가 미션을 진행하며 OAuth2 인가의 핵심 로직을 담게 되었던 것 같습니다.

이 외에는 대부분 무지막지만 dto 객체들로, 코드 줄수와 클래스 숫자는 많지만 로직은 없는 객체들이었습니다.
로직이 없는 애들을 명시적으로 구분하고 싶은 마음에 record 문법을 활용했습니다.

…t 구현

boorownie

미션 구현하시느라 고생 많으셨습니다!
요구사항을 비롯하여 스프링 시큐리티 코드에서 언급되는 부분까지 모두 잘 구현되었네요 💯

추가적으로 살펴보면 좋을 부분에 대해서 코멘트를 남겨두었는데요!
코멘트 확인하시고 코드 한번 살펴보셔요~

boorownie · 2025-02-20T10:57:23Z

src/main/java/nextstep/oauth2/web/authorizationrequest/OAuth2AuthorizationRequestDao.java

+import nextstep.oauth2.exception.UnmatchedStateException;
+import nextstep.oauth2.web.OAuth2ParameterNames;
+
+public class OAuth2AuthorizationRequestDao implements OAuth2AuthorizationRequestRepository {


OAuth2AuthorizationRequestRepository의 구현체 네이밍을 Dao로 정한 이유가 있으신가요?

Dao 라는 네이밍은 개인적인 개발 습관에서 비롯된 것인데,
저는 Repository 라고 네이밍된 것은 구현체가 아닌 순수한 인터페이스로 선언해두는 것을 좋아하는 편입니다.

Repository 가 데이터 저장소의 인터페이스로 선언되었다면,
데이터를 접근해서 조작 및 조회하는 실구현체 객체들은 Dao (Data Access Object) 라고 이름을 짓습니다.

만약 AuthorizationRequest 데이터를 관리할 수 있는 다른 방법이 세션외에 존재한다면,
클래스명에서 데이터를 관리하는 방법이 들어날 수 있게끔 짓는 편입니다.
HttpSessionOAuth2AuthorizationRequestRepository

하지만 spring-security 전체 코드를 검색해본 결과 HttpSessionOAuth2AuthorizationRequestRepository 외에 OAuth2AuthorizationRequestRepository 를 implements 하고 있는 클래스는 없었고,
DefaultOAuth2AuthorizationRequestRepository 라고 네이밍을 하기 보다는,
그냥 OAuth2AuthorizationRequestDao 라고 하게 되었던 것 같습니다.

생각을 해보니 학습을 위해서는 원본처럼 HttpSessionOAuth2AuthorizationRequestRepository 라고 하는게 더 적절할 것 같습니다!

b676536 커밋에서 spring-security 소스코드를 참고하여 다음과 같이 Dao 를 사용한 객체들의 네이밍을 바꾸었습니다.

HttpSessionOAuth2AuthorizationRequestRepository

HttpSessionOAuth2AuthorizedClientRepository

InMemoryClientRegistrationRepository

boorownie · 2025-02-20T10:58:29Z

src/main/java/nextstep/oauth2/web/OAuth2AuthorizationRequestRedirectFilter.java

+            filterChain.doFilter(request, response);
+            return;
+        }
+        OAuth2AuthorizationRequestRepository.saveAuthorizationRequest(authorizationRequest, request, response);


대문자로 시작해서 static 메서드인 줄 알았네요 😅

앗! 😓 94b3a94 커밋에서 해당 오타 수정하였습니다.

boorownie · 2025-02-20T11:07:37Z

src/main/java/nextstep/oauth2/web/OAuth2LoginAuthenticationFilter.java

+    }
+
+    @Override
+    public Authentication attemptAuthentication(HttpServletRequest request, HttpServletResponse response) {


혹시 requestRepository에서 removeAuthorizationRequest 해주는 부분은 어디에있나요?

OAuth2LoginAuthenticationFilter 소스코드를 다시 읽으며 removeAuthorizationRequest 를 하는 부분을 확인했습니다.

AuthorizationRequest 를 Session 에 가져온다는 개념만 생각하다보니, getter 형식으로 메서드 이름을 지었는데,
그러다 보니 AuthorizationRequest 를 제거해주어야 한다는 추가 효과를 깜빡해버렸습니다.
메서드에서도 명시적으로 이를 표현할 수 있도록 get 이 아닌, pop 을 사용하도록 이름을 바꾸겠습니다.

어차피 세션이 종료되면 Session TTL 이후 휘발되는 데이터라 명시적으로 삭제할 필요가 없다고 착각을 해버렸는데,
OAuth2 인증시도를 할 때마다 OAuth2AuthorizationRequest 를 새로할 수 있도록 명시적으로 휘발할 필요가 있겠군요.

휘발하지 않았을 위험을 상상해보니, authentication 도중에 실패를 했을 때 세션이 유지된채로 재인증 시도시,
과거의 잘못된 AuthorizationRequest 가 재사용될 위험이 있을 것 같습니다.

3db0484 커밋에서 반영 완료했습니다.

boorownie · 2025-02-20T11:10:02Z

src/main/java/nextstep/oauth2/web/OAuth2LoginAuthenticationFilter.java

+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class OAuth2LoginAuthenticationFilter extends AbstractAuthenticationProcessingFilter {


AbstractAuthenticationProcessingFilter를 잘 활용하셨네요 👍

boorownie · 2025-02-20T11:12:19Z

src/main/java/nextstep/app/OAuth2Config.java

+    public OAuth2UserService oauth2UserService(
+            MemberRepository memberRepository
+    ) {
+        return new OAuth2UserService() {


커스텀을 하게 되겠지만 DefaultOAuth2UserService를 두어서 필요한 로직만 추가로 구현해보는 구조로 해보시면 어떨까요?

미션을 진행할 때, oauth2 패키지만 해도 다루어야할 게 무척 많다보니, app 패키지에서의 변경사항을 최소화하고,
만약 추가 클래스를 구현할게 있다면 최대한 OAuth2Config 내부에서 해결하려다보니 이렇게 Bean 등록을 하게 된 것 같습니다.

DefaultOAuth2UserService 를 별도 클래스로 분리시키도록 하겠습니다.

5d68d0d 커밋에서 반영 완료하였습니다.

boorownie · 2025-02-20T11:17:19Z

...in/java/nextstep/oauth2/authentication/token/OAuth2AuthorizationCodeAuthenticationToken.java

+
+import java.util.Set;
+
+public final class OAuth2AuthorizationCodeAuthenticationToken implements Authentication {


authorizationCodeAuthentication를 살펴보면 Authentication의 getCredentials나 getPrincipal를 별도로 사용하고 있지 않은데 왜 굳이 Authentication를 구현하게 하는 것일 지 생각해보셨나요? 정답이 있거나 무언가를 의도하는 질문은 아니고 생각해보시길 바라는 마음에서 남기는 코멘트 입니다!

spring-security 의 OAuth2AuthorizationCodeAuthenticationToken 소스코드를 보면
principal 로 clientId 값을,
credentials 로 accessToken 혹은 code 값을 사용하고 있습니다.
authenticated 값 또한 true 라고 설정되어 있고요.
하지만 저는 principal 과 credentials 를 null 값으로, isAuthenticated 를 false 로 설정하였습니다.

저는 맨 처음에 OAuth2AuthorizationCodeGrantFilter 처럼 OAuth2AuthorizationCodeAuthenticationToken 를 Authorization 으로 활용하는 필터가 존재하지만,
고급 기능의 필터라서 제가 미처 구현을 못한줄 알았습니다.
하지만 코드를 자세히 읽어보니, clientRegistration, accessToken 등의 값을 가져오는 자료구조로만 활용하고 있을 뿐이지, Authorization 으로서 활용을 하고 있다는 느낌을 받지 못했습니다.

spring-security 의 이슈 히스토리를 추적하다보니
OAuth2LoginAuthenticationProvider 과 OAuth2AuthorizationCodeAuthenticationProvider 의 로직이 비슷하다고 리팩토링을 요구하는 이슈가 있더군요.

처음에는 OAuth2LoginAuthentication 과 OAuth2AuthorizationCodeAuthentication 이 별개의 Authentication 으로 구현을 하다가, 막상 구현을 하다보니 이 2개의 Provider 로직이 비슷하다는 것을 깨닫게 되어 OAuth2LoginAuthenticationProvider 로직만을 남기고, OAuth2AuthorizationCodeAuthenticationToken 쪽의 존재감을 지워버린게 아닌가 하는 생각이 들었습니다.

그렇다고 이제와서 OAuth2AuthorizationCodeAuthenticationToken 를 Authentication 이 아니도록 다시 리팩토링을 하자니 귀찮아져서 그냥 남겨두었다는 생각도 들었고요.

저는 해당 부분을 고민하면서 들었던 생각은 인증 과정에서 필요한 객체임을 드러내기위해 Authentication을 구현한 것이라 생각했었습니다. 정완님의 히스토리 추적을 살펴보니 일리가 있어 보이네요 👍

- https://github.com/spring-projects/spring-security/blob/main/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/HttpSessionOAuth2AuthorizationRequestRepository.java - https://github.com/spring-projects/spring-security/blob/main/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/HttpSessionOAuth2AuthorizedClientRepository.java - https://github.com/spring-projects/spring-security/blob/main/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/registration/InMemoryClientRegistrationRepository.java

…구조로 변경

…tials 정의 https://github.com/spring-projects/spring-security/blob/7fc5d50adfe5546ca725f3259e4d65ee6083f7cd/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/authentication/OAuth2AuthorizationCodeAuthenticationToken.java#L112

boorownie

3주차 미션도 잘 마무리해주셨네요!
코드 분석과 깊이있는 이해가 느껴졌습니다!
남은 기간동안 시큐리티의 코드를 살펴보면서 적용해보면 좋을 부분을 추가로 살펴봐도 좋을 것 같아요!
수고 많으셨습니다~ 이만 머지할게요~

boorownie · 2025-02-21T03:45:24Z

...extstep/oauth2/web/authorizationrequest/HttpSessionOAuth2AuthorizationRequestRepository.java

-public class OAuth2AuthorizationRequestDao implements OAuth2AuthorizationRequestRepository {
+public class HttpSessionOAuth2AuthorizationRequestRepository implements OAuth2AuthorizationRequestRepository {


사실 정답이 있는 건 아니지만 학습의 목적으로서 생각해봤을 때 실제 시큐리티 코드와 유사하면 도움이 될 것 같아요!
요구사항을 넘어 추가 학습이 필요한 상황에서 추가적으로 기능 구현을 하는데 있어서도 이어서 진행하는데 좋을 것 같네요 :)

boorownie · 2025-02-21T03:46:01Z

src/main/java/nextstep/oauth2/web/OAuth2LoginAuthenticationFilter.java

+    public OAuth2AuthorizationRequest popAuthorizationRequest(
+            HttpServletRequest request, HttpServletResponse response
+    ) {
+        final OAuth2AuthorizationRequest authorizationRequest = requestRepository.removeAuthorizationRequest(request, response);


boorownie · 2025-02-21T03:47:43Z

src/main/java/nextstep/app/service/DefaultOAuth2UserService.java

+import java.util.Set;
+
+@Service
+public class DefaultOAuth2UserService implements OAuth2UserService {


반영 잘 해주셨습니다!
DefaultOAuth2UserService에서 제공해주는 것 외에 커스텀이 필요한 부분은 어떤 부분인지 확인해보고 구현을 해보면
추후 스프링 시큐리티를 사용할 때 조금 더 색다르게 다가올 것 같네요 :)
어떤 부분이 있을 지 고민만 해보셔요~

ghojeong added 20 commits February 15, 2025 19:33

feat: application.properties 를 통해 OAuth2Client 를 등록하는 Repository 를 구현

d6a4354

feat: endpoint 에서 사용되는 dto를 정의

dbbc497

feat: OAuth2AccessTokenResponse 를 가져오는 OAuth2AccessTokenResponseClien…

d65d82c

…t 구현

feat: OAuth2User 와 OAuth2UserService 구현

68604ac

feat: AuthenticationToken 과 AuthenticationProvider 를 구현

f3581d5

feat: ProfileUser 를 정의함

b494728

feat: State 를 위해 8자리 랜덤 문자열을 생성하는 Generator 구현

6e84278

feat: OAuth2AuthorizedClientRepository 를 구현

cde753e

feat: OAuth2AuthorizationRequestRepository 구현

c547968

feat: OAuth2AuthorizationRequestResolver 구현

a518b2d

feat: OAuth2AuthorizationRequestRedirectFilter 구현

f6203fb

refactor: SecurityContextRepository 인터페이스를 따로 선언

c31f070

feat: AbstractAuthenticationProcessingFilter 구현

a0b1a60

feat: OAuth2LoginAuthenticationFilter 구현

3654c25

feat: AuthenticationFilter 에 AuthenticationManager 를 활용하도록 Config 를 수정

34ad282

test: FormLoginTest 에서 로그인 성공시 OK 가 아니라 302 Redirect 가 되도록 테스트를 수정

fba333b

fix: 테스트 시 공통적으로 사용하는 Member 정보를 Fixture 로 분리

556d4c5

test: OAuth2 LoginRedirect 에 관한 테스트 작성

c0715a9

test: GitHub 와 Google 을 Stub 을 활용해 인증하는 테스트 작성

b1f894f

test: LoginToken 을 발급받을 시, user code 가 일치하는지 확인하는 테스트 추가

bae948c

boorownie requested changes Feb 20, 2025

View reviewed changes

ghojeong added 5 commits February 20, 2025 21:29

fix: 멤버의 네이밍을 대문자로 시작하게 한 오타를 수정

94b3a94

fix: attemptAuthentication 시 removeAuthorizationRequest 를 하도록 로직 수정

3db0484

refactor: OAuth2UserService 의 설정을 DefaultOAuth2UserService 를 통해 구현하는 …

5d68d0d

…구조로 변경

ghojeong requested a review from boorownie February 20, 2025 14:22

boorownie approved these changes Feb 21, 2025

View reviewed changes

boorownie merged commit 1130ddb into next-step:ghojeong Feb 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[고정완] OAuth 2.0 리뷰 요청드립니다. #8

[고정완] OAuth 2.0 리뷰 요청드립니다. #8

ghojeong commented Feb 19, 2025 •

edited

Loading

boorownie left a comment

boorownie Feb 20, 2025

ghojeong Feb 20, 2025

ghojeong Feb 20, 2025

boorownie Feb 20, 2025

ghojeong Feb 20, 2025

boorownie Feb 20, 2025

ghojeong Feb 20, 2025

ghojeong Feb 20, 2025

boorownie Feb 20, 2025

boorownie Feb 20, 2025

ghojeong Feb 20, 2025

ghojeong Feb 20, 2025

boorownie Feb 20, 2025

ghojeong Feb 20, 2025 •

edited

Loading

boorownie Feb 21, 2025

boorownie left a comment

boorownie Feb 21, 2025

boorownie Feb 21, 2025

boorownie Feb 21, 2025


		import java.util.Set;

		public final class OAuth2AuthorizationCodeAuthenticationToken implements Authentication {

		public class OAuth2AuthorizationRequestDao implements OAuth2AuthorizationRequestRepository {
		public class HttpSessionOAuth2AuthorizationRequestRepository implements OAuth2AuthorizationRequestRepository {

[고정완] OAuth 2.0 리뷰 요청드립니다. #8

[고정완] OAuth 2.0 리뷰 요청드립니다. #8

Conversation

ghojeong commented Feb 19, 2025 • edited Loading

boorownie 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

ghojeong Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boorownie 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

ghojeong commented Feb 19, 2025 •

edited

Loading

ghojeong Feb 20, 2025 •

edited

Loading