[임지영] 1단계 OAuth 2.0 Login, 리팩터링 & OAuth 2.0 Resource 연동 #12

sazzeo · 2025-02-22T16:52:15Z

리뷰어님, 안녕하세요. 해당 미션에서 고군분투 하느라 첫 리뷰요청이 늦어졌습니다...!

OAuth2 프로토콜에서 redirect 하는 부분과 access token을 가져오는 부분까지는 동일하지만 user 정보를 가져오는 부분은 상이하다고 생각해서 이 부분을 OAuth2UserDetailsService 인터페이스로 만들었습니다.

타입 캐스팅하는 부분이나 상수 하드코딩 같은 곳 에서 맘에 안드는 부분이 많은데.. 일단 구현이 우선이라고 생각해서 제출합니다..!

제가 commit 메세지 같은 게 정돈되지 않아서 ㅠㅠ 혹시 이 부분에 대해서 가이드 주시면 다음부터 적용하겠습니다.

잘부탁드립니다.

…트 통과

jaeyeonling

안녕하세요! 이번 미션을 함께하게 된 김재연입니다. 잘 부탁드립니다!
코멘트 남겨두었으니 확인 부탁드릴게요 ㅎㅎ
이번 미션 화이팅입니다!!

jaeyeonling · 2025-02-23T14:09:01Z

src/main/java/nextstep/app/application/MemberService.java

+    public void saveOauth2Member(Oauth2MemberSaveDto dto) {
+        var member = memberRepository.findByEmail(dto.email());
+        if (member.isPresent()) {
+            return;


member가 중복될 경우 무시하는 것이 의도한 구현이 맞을까요?

네! 이미 있는 경우에만 member에 저장하고 있는 경우에는 저장하지 않도록 했습니다

jaeyeonling · 2025-02-23T14:12:41Z

src/main/java/nextstep/app/oauth2/GithubOAuth2UserDetailService.java

+    private final RestTemplate restTemplate;
+    @Value("${app.oauth2.github.user-url}")
+    private String userUrl;
+
+    public GithubOAuth2UserDetailService(final RestTemplate restTemplate) {
+        this.restTemplate = restTemplate;
+    }
+


Suggested change

private final RestTemplate restTemplate;

@Value("${app.oauth2.github.user-url}")

private String userUrl;

public GithubOAuth2UserDetailService(final RestTemplate restTemplate) {

this.restTemplate = restTemplate;

}

private final RestTemplate restTemplate;

private final String userUrl;

public GithubOAuth2UserDetailService(

final RestTemplate restTemplate,

@Value("${app.oauth2.github.user-url}") final String userUrl

) {

this.restTemplate = restTemplate;

this.userUrl = userUrl;

}

통일성있게 userUrl도 final로 만들어보면 좋겠네요!

오 이 부분은 코틀린에서도 되는건데 자바에서도 된다는 생각을 못했네요! 감사합니다!!

jaeyeonling · 2025-02-23T14:18:08Z

src/main/java/nextstep/app/oauth2/GithubOAuth2UserDetailService.java

+import java.util.Set;
+
+@Component
+public class GithubOAuth2UserDetailService implements OAuth2UserDetailsService {


지금은 벤더사마다 OAuth2UserDetailsService의 구현체를 만드는 방식으로 구현해 주셨는데요!
각 코드마다 중복되는 부분이 많이 발생하고 있는데, 공통으로 묶어 추상화해보면 좋을 것 같습니다 ㅎㅎ

이부분을 설계할 때 userInfo 를 받아오는 부분은 인가서버마다 스펙이 다르기 때문에
사용자(시큐리티를 사용하는 클라이언트)가 커스텀해야 되는 부분이라고 생각했습니다.
이번 미션이 핵심은 시큐리티 패키지고 app 패키지는 사용자마다 달라지는 부분이라 딱히 힘을 주지 않고 중복코드 가득하게 코드를 짰습니다...💦💦

여기서 중복 코드 추상화는 security 단에서 제공을 해줘야하는 부분인지? 아니면 사용자 관점에서 해야하는 추상화의 관점에서 얘기해 주신 부분인지 궁금합니다!

저는 인가 서버마다 사용자 정보를 받아오는 방식이 크게 다르지 않기 때문에 security 단에서 충분히 추상화할 수 있을 거라고 생각합니다!
어느 부분까지 추상화할 수 있을지 고민해보시고, 공통 로직은 DefaultOAuth2UserService처럼 추상화하고, 벤더사마다 차이가 필요한 부분은 확장 포인트로 설계해보는 것도 좋은 방향일 것 같은데 어떻게 생각하시나요?

이부분은 템플릿 메소드 패턴으로 개선해보았습니다
d799324

jaeyeonling · 2025-02-23T14:24:39Z

src/main/java/nextstep/security/oauth2/OAuth2AuthenticationFilter.java

+                                      final List<OAuth2UserDetailsService> userDetailsServices
+    ) {
+        this.oAuth2RegistrationRepository = oAuth2RegistrationRepository;
+        this.oAuth2AccessTokenRequestProvider = new OAuth2AccessTokenRequestProviderImpl();


해당 클래스만 생성자 주입이 아니게 설계하신 이유가 있을까요!?

이부분은 경우에는 거의 커스텀이 없을것 같은 역할들은 Default 객체로 생성하게 만들었습니다
스프링에서 보통 Default 로 생성자 안에서 만드는 객체들은 setter나 또 다른 생성자를 통해 객체를 교체할 수 있게 설계해서 만드는 걸 많이 봤는데요.
현재 경우에는 쓰이지 않는 메소드가 되어버려서 따로 추가하지 않았습니다. 프레임워크를 만들 때는 쓰지 않는 메소드라도 setter 를 추가해두는게 좋을까요?

맞습니다! 구현 방식에 따라 확장할 필요가 없는 부분이라면 내부에서 직접 생성하는 것도 좋은 방법이 될 수 있는데요.
다만, 그렇게 설계할 경우 인터페이스를 유지하는 이유와 충돌할 가능성이 있어서, 오히려 인터페이스를 제거하는 것이 코드의 의도가더 명확하게 드러날 것 같습니다 ㅎㅎ

제가 시큐리티를 쓸 때 종종
이걸 왜 커스텀 불가능하게 만들었지? 라는 생각이 들때가 있었는데요.
요즘에는 애초에 커스텀 하려고 하는 게 잘못된 생각인 건 아닌지 하는 의심을 합니다 ㅎㅎ

다음에는 스프링에 해당 부분이 인터페이스인지 아닌지 유심히 봐봐야겠네요..

jaeyeonling · 2025-02-23T14:30:57Z

src/main/java/nextstep/security/oauth2/OAuth2RedirectFilter.java

+            filterChain.doFilter(servletRequest, servletResponse);
+            return;
+        }
+        StringBuffer url = httpServletRequest.getRequestURL();


Suggested change

StringBuffer url = httpServletRequest.getRequestURL();

String url = httpServletRequest.getRequestURI();

RequestURL로 가져올 경우 전체 URL을 반환하기 때문에 https://example.com/oauth2/authorization/google?state=some와 같은 쿼리 스트링이 있을 경우 올바르게 동작하지 않을 수 있겠네요.
getRequestURI로 변경해보면 어떨까요?

jaeyeonling · 2025-02-23T14:35:01Z

src/main/resources/application.yml

+        uri: https://github.com/login/oauth/authorize
+        authorize-uri: https://github.com/login/oauth/authorize


uri와 authorize-uri를 중복해서 만든 이유가 있을까요?

이 부분은 파라미터 이름 수정하면서 지워지지 않은 부분이 있었던것 같습니다!
꼼꼼한 확인 감사합니다 ㅎㅎㅎ

jaeyeonling · 2025-02-23T14:40:24Z

src/test/java/nextstep/app/OAuth2AuthenticationFilterTest.java

+@SpringBootTest
+@AutoConfigureMockMvc
+@AutoConfigureWireMock(port = 8089)
+class OAuth2AuthenticationFilterTest {


테스트 👍

jaeyeonling · 2025-02-23T14:43:58Z

src/main/java/nextstep/security/oauth2/OAuth2AuthenticationFilter.java

+import java.util.List;
+import java.util.Set;
+
+public class OAuth2AuthenticationFilter extends GenericFilterBean {


전체적으로 Spring Security의 구조와 유사하게 잘 구현해 주셨습니다. 확장성을 고려한 설계도 인상적이네요! 👍
다만, 코드의 확장성과 설정값 중 이번 미션에서 요구되지 않는 정보가 포함되면서 코드의 복잡도가 다소 높아진 것 같은데요!
구현 범위를 고려하여, 조금 더 간결한 형태로 정리해 보면 유지보수성이 더욱 향상될 것 같은데 어떻게 생각하시나요?

이 부분은 따로 디엠 드리겠습니다..!

jaeyeonling · 2025-02-23T14:46:32Z

src/main/java/nextstep/security/oauth2/OAuth2RedirectFilter.java

+        }
+        StringBuffer url = httpServletRequest.getRequestURL();
+        var vendor = url.substring(url.lastIndexOf("/") + 1);
+        var registration = oAuth2RegistrationRepository.getRegistration(vendor);


현재 oAuth2RegistrationRepository를 통해 registration을 가져오는 방식이 조금 어색하게 느껴지는데요.
이와 같은 설계를 선택하신 이유가 있을까요?

OAuth2RegistrationRepository 는 단순 데이터 객체인 OAuth2Properties를 랩핑해서 여러가지 Registration 정보를 가져오게 해주는 중간 객체 역할로 설계했습니다.
시큐리티의 ClientRegistrationRepository 랑 유사한 역할로 설계했습니다.
이 부분은 만들다보니 다음 단계 미션으로 있는 것 같아서 적당히 만들었던 것 같습니다

혹시 어색하게 느껴지는 부분이 OAuth2RegistrationRepository 이 app 패키지에 있어서일까요? 이부분은 제가 auto configuration 으로 만들고 싶었는데 시간 부족으로 bean 주입이 필요해서 어쩔 수 없이 패키지 의존성 구조가 꼬이게 되었습니다.

DM으로 나눈 내용과 조금 연관되는 것 같아요! oAuth2RegistrationRepository의 역할이 단순히 properties를 포장한 형태로 구현되어 있어서 불필요한 레이어가 있는 느낌이 들었습니다!
조금 더 의도가 명확한 코드를 생각해본다면

OAuth2RegistrationRepository를 제거하여 Properties 자체가 역할을 가지도록 한다.

불필요한 레이어를 줄여 코드가 단순해진다.

Properties의 역할을 OAuth2RegistrationRepository가 가지도록 한다.

OAuth2RegistrationRepository의 역할이 명확해진다.

Properties도 Properties 자체의 역할을 할 수 있다.

OAuth2RegistrationRepository를 저장소의 확장 포인트로 설계한다. (인터페이스 제공)

코드는 그대로 있더라도, 확장 포인트를 위해 Repository가 있다는 점에서 근거가 명확해진다.

와 같은 방식이 조금 더 코드에 의도를 담는 것 같은데 어떻게 생각하시나요?

OAuth2RegistrationRepository 를 인터페이스로 바꾸면서 ClientRegistrationRepository 로 이름을 변경했습니다

인터페이스로 바꾸고 구현체를 설계하니 제가 이전에 생각했던 의존성이 꼬이는 문제가 해결되더라구요.
사용하는 쪽에서 인터페이스를 바라보니
자연스럽게 Registration 속 requestMatcher를 사용하는 부분이 사라지면서 책임도 명확해지고 코드도 깔끔해진것 같습니다.

좋은 포인트를 짚어주셔서 감사합니다.

그리고 redirect url 을 공통으로 가져가면서 domain 부분이 필요하게 되었는데요.
domain 을 OAuth2Properties.java 에서 아래로 넘겨주는데 이건 어색하지 않은지가 궁금합니다.

감사합니다.

jaeyeonling

질문주신 내용에 코멘트 남겨두었습니다 ㅎㅎ

jaeyeonling · 2025-02-24T12:43:59Z

src/main/java/nextstep/app/oauth2/GithubOAuth2UserDetailService.java

+import java.util.Set;
+
+@Component
+public class GithubOAuth2UserDetailService implements OAuth2UserDetailsService {


저는 인가 서버마다 사용자 정보를 받아오는 방식이 크게 다르지 않기 때문에 security 단에서 충분히 추상화할 수 있을 거라고 생각합니다!
어느 부분까지 추상화할 수 있을지 고민해보시고, 공통 로직은 DefaultOAuth2UserService처럼 추상화하고, 벤더사마다 차이가 필요한 부분은 확장 포인트로 설계해보는 것도 좋은 방향일 것 같은데 어떻게 생각하시나요?

jaeyeonling · 2025-02-24T12:47:10Z

src/main/java/nextstep/security/oauth2/OAuth2AuthenticationFilter.java

+                                      final List<OAuth2UserDetailsService> userDetailsServices
+    ) {
+        this.oAuth2RegistrationRepository = oAuth2RegistrationRepository;
+        this.oAuth2AccessTokenRequestProvider = new OAuth2AccessTokenRequestProviderImpl();


맞습니다! 구현 방식에 따라 확장할 필요가 없는 부분이라면 내부에서 직접 생성하는 것도 좋은 방법이 될 수 있는데요.
다만, 그렇게 설계할 경우 인터페이스를 유지하는 이유와 충돌할 가능성이 있어서, 오히려 인터페이스를 제거하는 것이 코드의 의도가더 명확하게 드러날 것 같습니다 ㅎㅎ

jaeyeonling · 2025-02-24T12:58:05Z

src/main/java/nextstep/security/oauth2/OAuth2RedirectFilter.java

+        }
+        StringBuffer url = httpServletRequest.getRequestURL();
+        var vendor = url.substring(url.lastIndexOf("/") + 1);
+        var registration = oAuth2RegistrationRepository.getRegistration(vendor);


DM으로 나눈 내용과 조금 연관되는 것 같아요! oAuth2RegistrationRepository의 역할이 단순히 properties를 포장한 형태로 구현되어 있어서 불필요한 레이어가 있는 느낌이 들었습니다!
조금 더 의도가 명확한 코드를 생각해본다면

OAuth2RegistrationRepository를 제거하여 Properties 자체가 역할을 가지도록 한다.

불필요한 레이어를 줄여 코드가 단순해진다.

Properties의 역할을 OAuth2RegistrationRepository가 가지도록 한다.

OAuth2RegistrationRepository의 역할이 명확해진다.

Properties도 Properties 자체의 역할을 할 수 있다.

OAuth2RegistrationRepository를 저장소의 확장 포인트로 설계한다. (인터페이스 제공)

코드는 그대로 있더라도, 확장 포인트를 위해 Repository가 있다는 점에서 근거가 명확해진다.

와 같은 방식이 조금 더 코드에 의도를 담는 것 같은데 어떻게 생각하시나요?

# Conflicts: # src/main/java/nextstep/security/oauth2/OAuth2AccessTokenRequestProviderImpl.java # src/main/java/nextstep/security/properties/Registration.java # src/main/resources/application.yml # src/test/resources/application-test.yml

sazzeo and others added 30 commits February 18, 2025 23:10

feat: 1-1 미션 페어 프로그래밍

69c21c6

feat: antMatcher 클래스 추가

9c78de0

feat: OAuth2RedirectFilter 추가

f118c8b

feat: Oauth2 프로퍼티용 객체 추가

dd009d1

refactor: 클래스 이름 변경

ebf9fce

feat: OAuth2RegistrationRepository 생성

e4dcb8d

feat: EnableConfigurationProperties 설정 추가, test 이름 변경

cc6467e

feat: application-test.yml 작성 후 OAuth2PropertiesRedirectFilterTest 테스…

1bb97f6

…트 통과

feat: redirectFilter에 벤더사 설정이 없으면 404 반환한다

5683a2b

feat: google oauth2 테스트 코드 추가

06c8a35

feat: include 에 oauth2 추가

598ce74

feat: AntRequestMatcher 검증에 AntPathMatcher 사용하도록 변경

ac7228a

feat: Registration 프로퍼티에 token-uri 추가

ee66000

feat: matchRedirectUrl redirectUrl 판단하도록 메소드 추가

5e6ae1e

feat: /login/oauth/access_token WireMock 테스트 완료

ee82ae4

feat: OAuth2Parameter 클래스 추가

2a0a0ca

feat: accessToken 가져오는 부분 클래스로 변경

7531b46

feat: OAuth2 용 UserDetailsService 추가

2c8413b

feat: vendor 사 반환 코드 추가

1cb971d

feat: vendor 사 List로 받을 수 있도록 변경

3d20302

feat: OAuth2UserDetailsAuthenticationToken 추가

7a605f3

feat: oauth2UserDetailsServiceResolver 가 user 정보 가져오도록 변경

3c03fe7

feat: yml 파일 속성 추가

b48fe94

feat: config 수정

43f8553

refactor: test 클래스명 변경

4706fd2

feat: 인증 성공 처리용 OAuth2AuthenticationSuccessHandler 추가

44cccc1

feat: OAuth2UserDetail 에 getAuthorities 추가

f08eba8

feat: MemberService 추가

061824f

feat: OAuth2AuthenticationSuccessHandlerImpl에 member 저장하는 코드 추가

181522f

fix: OAuth2 인증후 로그인 유저처럼 동작하는지 확인하는 테스트 코드로 수정

160cdad

sazzeo added 3 commits February 23, 2025 01:37

feat: google 용 OAuth2 서비스 추가

85929ce

feat: google 용 설정 추가

0651813

fix: google 용 stub 객체 분리, 깨지는 테스트 수정

32aa5d8

jaeyeonling requested changes Feb 23, 2025

View reviewed changes

sazzeo and others added 3 commits February 24, 2025 00:07

refactor: @value 주입 생성자로 받도록 수정

2963af0

fix: 필요없는 코드 제거 및 수정

dc77dd0

feat: response-type 제거, OAuth2Parameter에 default 값 추가

144c8ac

jaeyeonling reviewed Feb 24, 2025

View reviewed changes

sazzeo added 7 commits February 25, 2025 22:38

fix: OAuth2AccessTokenRequestProvider 인터페이스 삭제 후 구현체만 남김

242ca18

feat: UrlUtils 클래스 추가

5b9e853

feat: ClientRegistrationRepository 인터페이스로 변경

8ab6d2f

feat: getRedirectUrl 가져오는 부분 기본 url 쓰도록 수정

8ad7794

feat: AbstractOAuth2UserDetailService 추가해 템플릿 메소드 패턴으로 중복 제거

d799324

refactor: 안쓰는 import문 정리

eb9f69d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[임지영] 1단계 OAuth 2.0 Login, 리팩터링 & OAuth 2.0 Resource 연동 #12

[임지영] 1단계 OAuth 2.0 Login, 리팩터링 & OAuth 2.0 Resource 연동 #12

sazzeo commented Feb 22, 2025

jaeyeonling left a comment

jaeyeonling Feb 23, 2025

sazzeo Feb 23, 2025

jaeyeonling Feb 23, 2025

sazzeo Feb 23, 2025

jaeyeonling Feb 23, 2025

sazzeo Feb 23, 2025

jaeyeonling Feb 24, 2025

sazzeo Feb 25, 2025

jaeyeonling Feb 23, 2025

sazzeo Feb 23, 2025

jaeyeonling Feb 24, 2025

sazzeo Feb 25, 2025

jaeyeonling Feb 23, 2025

jaeyeonling Feb 23, 2025

sazzeo Feb 23, 2025

jaeyeonling Feb 23, 2025

jaeyeonling Feb 23, 2025

sazzeo Feb 23, 2025

jaeyeonling Feb 23, 2025

sazzeo Feb 23, 2025

sazzeo Feb 23, 2025

jaeyeonling Feb 24, 2025

sazzeo Feb 25, 2025

jaeyeonling left a comment

jaeyeonling Feb 24, 2025

jaeyeonling Feb 24, 2025

jaeyeonling Feb 24, 2025

	StringBuffer url = httpServletRequest.getRequestURL();
	String url = httpServletRequest.getRequestURI();

		uri: https://github.com/login/oauth/authorize
		authorize-uri: https://github.com/login/oauth/authorize

[임지영] 1단계 OAuth 2.0 Login, 리팩터링 & OAuth 2.0 Resource 연동 #12

Are you sure you want to change the base?

[임지영] 1단계 OAuth 2.0 Login, 리팩터링 & OAuth 2.0 Resource 연동 #12

Conversation

sazzeo commented Feb 22, 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

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

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

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