OAuth 미션 2단계 리뷰 요청 드립니다. #14

ParkSeryu · 2025-02-24T11:41:15Z

안녕하세요 재연님!
OAuth2 2단계 미션 리뷰요청드립니다.

스프링 시큐리티 실제 코드를 참조하면서 어떤 흐름으로 이루어지는지 파악하고,
필요없는 소스는 제거하는 방식으로 진행했습니다.
물론 전부 다 이해하진 못했지만 (너무 어려웠습니다,,)😂,
기존 방식에 비해 프레임워크에서는 어떻게 확장성을 가져가는지 많이 엿보고 배울 수 있어서 유익했던 것 같습니다!

자고로 state는 구현하지 않았는데요
역할은 CSRF 공격을 방지하고, 인증 후 클라이언트의 상태를 유지하는 역할을 한다고 이해했는데, 맞을까요?

모쪼록, 이번 단계도 잘 부탁 드리겠습니다!

- OAuth2AuthorizationRequestResolver 구현 - AuthorizationRequestRepository 구현 - OAuth2AuthorizationRequest 구현

- 빈 등록

jaeyeonling

미션을 잘 진행해 주셨네요!
질문의 답변과 코멘트 남겨두었으니 확인 부탁드릴게요~

jaeyeonling · 2025-02-25T05:35:43Z

src/main/java/nextstep/app/SecurityConfig.java

-import java.util.ArrayList;
-import java.util.List;
-import java.util.Set;
-
 @Configuration
 @EnableAspectJAutoProxy
 @EnableConfigurationProperties(OAuth2ClientProperties.class)
 public class SecurityConfig {


자고로 state는 구현하지 않았는데요
역할은 CSRF 공격을 방지하고, 인증 후 클라이언트의 상태를 유지하는 역할을 한다고 이해했는데, 맞을까요?

state는 CSRF 공격을 방지하는 것이 맞지만 로그인 상태를 유지하는 역할은 하고 있지 않습니다! 로그인 상태를 유지하는 역할은 토큰의 역할이라고 보시면 됩니다 ㅎㅎ

jaeyeonling · 2025-02-25T05:44:14Z

src/main/java/nextstep/security/oauth2/client/DefaultOAuth2AuthorizationRequestResolver.java

+        String paramsQuery = UriComponentsBuilder.newInstance()
+                .queryParam("client_id", clientRegistration.clientId())
+                .queryParam("response_type", "code")
+                .queryParam("scope", clientRegistration.scope())


OAuth2 스펙에서는 scope를 공백으로 구분하는 것이 표준입니다.
지금은 쉼표로 구분하고 있는데, 표준에 맞게 수정해보면 어떨까요?

참고: https://datatracker.ietf.org/doc/html/rfc6749#section-3.3

jaeyeonling · 2025-02-25T05:47:00Z

src/main/java/nextstep/security/oauth2/client/DefaultOAuth2AuthorizationRequestResolver.java

+        String paramsQuery = UriComponentsBuilder.newInstance()
+                .queryParam("client_id", clientRegistration.clientId())
+                .queryParam("response_type", "code")
+                .queryParam("scope", clientRegistration.scope())
+                .queryParam("redirect_uri", clientRegistration.redirectUri())
+                .build()
+                .toUri()
+                .getQuery();
+
+        String authorizationRequestUri =
+                clientRegistration.providerDetails().authorizationUri() + "?" + paramsQuery;


UriComponentsBuilder.fromHttpUrl를 통해 생성한다면 아래 authorizationRequestUri도 합쳐서 생성할 수 있겠네요!

jaeyeonling · 2025-02-25T08:46:26Z

src/main/java/nextstep/security/oauth2/client/OAuth2AuthorizationRequestRedirectFilter.java

+public class OAuth2AuthorizationRequestRedirectFilter extends OncePerRequestFilter {
+    public static final String DEFAULT_AUTHORIZATION_REQUEST_BASE_URI = "/oauth2/authorization/";
+    private final OAuth2AuthorizationRequestResolver authorizationRequestResolver;
+    private final AuthorizationRequestRepository authorizationRequestRepository = new HttpSessionOAuth2AuthorizationRequestRepository();


해당 필드를 직접 인스턴스화 한다면 인터페이스로 분리한 장점을 얻지 못할 것 같은데 어떻게 생각하시나요?

말씀해주신 DI받는 부분에 대해서는 공감합니다! (5bbe21e 로 변경했습니다)

다만 여쭤보고 싶은게 실제 시큐리티 코드를 보았을 때도 마찬가지로 직접 생성해 주고,
setXXX (setter)로 DI 받는 구조가 흔하게 보이던데, 이러한 구조는 어떠한 이점이 있길래 가져가는 걸까 궁금합니다!

jaeyeonling · 2025-02-25T08:51:03Z

src/main/java/nextstep/security/oauth2/client/OAuth2AuthorizationRequestRedirectFilter.java

+        } catch (Exception e) {
+            throw new OAuth2AuthenticationException();
+        }


현재 catch 블록에서 예외를 처리할 때, 예외 메시지를 포함하지 않고 있어 의도 파악을 하기 힘드네요!
코드에 의미를 담아보면 어떨까요?

이부분은 try 부분 구현에 집중하다보니 뭉뚱그려서 한 경향이 있네요..! 반영해보겠습니다!

jaeyeonling · 2025-02-25T09:01:04Z

...main/java/nextstep/security/oauth2/client/authentication/OAuth2LoginAuthenticationToken.java

+import nextstep.security.oauth2.core.user.OAuth2User;
+import org.springframework.util.Assert;
+
+public class OAuth2LoginAuthenticationToken implements Authentication {


전체적인 설계를 봤을 때 인증과 권한에 대한 역할이 혼재되어 있다는 느낌이 있는데요.
예를 들어 OAuth2LoginAuthenticationToken에서 권한 정보를 가지고 있는데, 인증 객체가 사용자 권한을 관리하는 역할까지 수행하는 것은 Spring Security의 역할 분리 원칙에 맞지 않을 것 같아요.
OAuth2LoginAuthenticationToken은 단순 인증된 사용자 정보만 유지하고, 권한에 대한 처리는 OAuth2AuthenticationToken에서 처리하는게 좋을 것 같은데 어떻게 생각하시나요?

추가로 구조를 단순화시키면 principal, accessToken, authenticated 3개의 값으로 충분할 것 같습니다!

말씀해 주신 내용에 동감합니다!

다만 이렇게 혼재되어 있는 구조를 가지게 된 이유중에 하나가
실제 시큐리티 코드에서는 AbstractAuthenticationToken 으로 한 단계 추상 단계를 거치지만,
저희 요구사항에 있어서는 조금 과하다고 생각 해 바로 Authentication를 상속받기 때문에
getAuthorities를 오버라이딩 할 수 밖에 없는 구조가 되어 사용하지 않는 부분까지 포함이 되어버린 것 같습니다!

따라서 54cc2f5 정도로 줄이는 방법밖에 생각이 나지 않는데, 다르게 이해하고 있다면 말씀주시면 더 고민해보겠습니다!

jaeyeonling · 2025-02-25T09:01:46Z

src/main/java/nextstep/security/oauth2/client/registration/ClientRegistrationRepository.java

+
+    ClientRegistration findByRegistrationId(String registrationId);
+
+    class InMemoryClientRegistrationRepository implements ClientRegistrationRepository {


해당 클래스를 인터페이스 내부에 선언해주신 이유가 있을까요?

요구사항 중에

- ClientRegistrationRepository 코드를 참고하고 구현체인 InMemoryClientRegistrationRepository를 확인한다 - 다른 구현체를 사용하지 않을 계획이라면 ClientRegistrationRepository에 InMemoryClientRegistrationRepository를 구현해도 무방하다.

라고 기재되어 있어, 내부에 선언했습니다.
제가 잘못 이해 한 걸까요?

jaeyeonling · 2025-02-25T09:06:57Z

src/main/java/nextstep/security/oauth2/core/OAuth2ParameterNames.java

@@ -0,0 +1,15 @@
+package nextstep.security.oauth2.core;
+
+public class OAuth2ParameterNames {


상수를 위한 클래스라면 enum으로 분리해봐도 좋겠네요!

jaeyeonling · 2025-02-25T09:22:02Z

...xtstep/security/oauth2/client/authentication/OAuth2AuthorizationCodeAuthenticationToken.java

+    @Override
+    public Set<String> getAuthorities() {
+        return Set.of();
+    }
+
+    @Override
+    public Object getCredentials() {
+        return null;
+    }
+
+    @Override
+    public Object getPrincipal() {
+        return null;
+    }
+
+    @Override
+    public boolean isAuthenticated() {
+        return false;
+    }
+}


해당 부분은 놓치신걸까요?

맞습니다🥲 a1aaf2a 로 수정했습니다!

jaeyeonling · 2025-02-25T09:29:06Z

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

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


OAuth2LoginAuthenticationFilter가 직접 OAuth2LoginAuthenticationToken를 생성 후 AuthenticationManager로 전달하고 있네요!
스프링의 기본 설계 원칙은 Filter → AuthenticationManager → AuthenticationProvider인데요.
대략적으로 역할을 생각해보면

OAuth2LoginAuthenticationFilter: OAuth2 요청을 파싱하고, Authentication 객체를 생성하여 AuthenticationManager로 전달
OAuth2LoginAuthenticationProvider: OAuth2 코드 교환, Access Token 요청, User 정보 가져오기

와 같은 형태가 될 수 있을 것 같아요.
구조를 다시 한번 고려해보면 어떨까요?

ParkSeryu added 9 commits February 23, 2025 10:45

feat: OAuth2AuthorizationRequestRedirectFilter 구현

57f68fd

- OAuth2AuthorizationRequestResolver 구현 - AuthorizationRequestRepository 구현 - OAuth2AuthorizationRequest 구현

feat: ClientRegistrationRepository 구현

e41136a

- 빈 등록

feat: OAuth2UserService 구현

2584da7

chore: yml 수정

b394ce6

feat: OAuth2LoginAuthenticationFilter 구현

67da26b

feat: OAuth2ParameterNames 추가

13c80cb

refactor: OAuth2LoginAuthenticationFilter 빈으로 등록

eeed97c

test: 테스트 코드 수정

706e8c9

remove: 사용하지 않는 클래스 제거

9ff198d

jaeyeonling requested changes Feb 25, 2025

View reviewed changes

ParkSeryu added 8 commits February 25, 2025 22:41

refactor: Resolver의 authorizationRequestUri를 하나로 합치고, scope를 공백으로 조정

aded1bc

refactor: 필드를 DI 받도록 변경

5bbe21e

remove: 사용되지 않는 import 제거

21780ba

refactor: catch 블록에 예외 메시지 포함

601f3a6

refactor: 주석 대신 @nullable 애노테이션을 사용

dc6e13a

refactor: OAuth2LoginAuthenticationToken에서 권한 정보 제거

54cc2f5

refactor: 상수 enum으로 분리

5a9fe06

refactor: 놓친 리턴값 수정

a1aaf2a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OAuth 미션 2단계 리뷰 요청 드립니다. #14

OAuth 미션 2단계 리뷰 요청 드립니다. #14

ParkSeryu commented Feb 24, 2025

jaeyeonling left a comment

jaeyeonling Feb 25, 2025

jaeyeonling Feb 25, 2025

jaeyeonling Feb 25, 2025

jaeyeonling Feb 25, 2025

ParkSeryu Feb 25, 2025 •

edited

Loading

jaeyeonling Feb 25, 2025

ParkSeryu Feb 25, 2025

jaeyeonling Feb 25, 2025

jaeyeonling Feb 25, 2025

ParkSeryu Feb 25, 2025 •

edited

Loading

jaeyeonling Feb 25, 2025

ParkSeryu Feb 25, 2025 •

edited

Loading

jaeyeonling Feb 25, 2025

jaeyeonling Feb 25, 2025

ParkSeryu Feb 25, 2025

jaeyeonling Feb 25, 2025


		ClientRegistration findByRegistrationId(String registrationId);

		class InMemoryClientRegistrationRepository implements ClientRegistrationRepository {

		@@ -0,0 +1,15 @@
		package nextstep.security.oauth2.core;

		public class OAuth2ParameterNames {

OAuth 미션 2단계 리뷰 요청 드립니다. #14

Are you sure you want to change the base?

OAuth 미션 2단계 리뷰 요청 드립니다. #14

Conversation

ParkSeryu commented Feb 24, 2025

jaeyeonling 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

ParkSeryu Feb 25, 2025 • edited Loading

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

ParkSeryu Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParkSeryu Feb 25, 2025 • edited Loading

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

ParkSeryu Feb 25, 2025 •

edited

Loading

ParkSeryu Feb 25, 2025 •

edited

Loading

ParkSeryu Feb 25, 2025 •

edited

Loading