1-1단계 - OAuth 2.0 Login, 1-2단계 - 리팩터링 & OAuth 2.0 Resource 연동 리뷰 요청드립니다. #13

jukekxm · 2025-02-23T10:38:21Z

Github 로그인을 할 때, user profile을 조회 시 이메일 정보가 안 내려옵니다.. (email이 null로 내려오네요)
혹시 계정 문제일까요?
OAuth2 로그인을 요청할 때 적절한 OAUth2 Client로 프로퍼티 값을 불러오기 위해서, oAuth2ClientFactory 라는 Bean을 추가했습니다.
더 좋은 방식이 있을 것 같은데 떠오르지 않네요..! 의견 부탁드립니다.
패키지 간 양방향 참조를 방지하기 위해, OAuth2Property 클래스와 OAuth2Client 클래스를 따로 분리했는데, 더 나은 방식이 있는지 궁금합니다.

리뷰 부탁드립니다!

jinyoungchoi95

안녕하세요 주현님 😄

OAuth2 미션을 함께하게 된 최진영입니다.

미션 잘 진행해주셨네요 👍 몇가지 코멘트 남겨두었으니 확인부탁드려요 😄

궁금하거나 고민이 되는 부분이 있으시다면 언제든 pr 코멘트 또는 dm으로 요청 부탁드립니다.
감사합니다 🙇‍♂️

jinyoungchoi95 · 2025-02-24T12:25:45Z

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

Github 로그인을 할 때, user profile을 조회 시 이메일 정보가 안 내려옵니다.. (email이 null로 내려오네요) 혹시 계정 문제일까요?

nextauthjs/next-auth#374 라는 이슈가 있습니다.

jinyoungchoi95 · 2025-02-24T12:27:11Z

src/main/java/nextstep/security/userdetails/UserDetailsService.java

@@ -2,4 +2,5 @@

 public interface UserDetailsService {
    UserDetails loadUserByUsername(String username);
+    UserDetails signUpUser(UserDetails userDetails);


실제 UserDetailsService는 해당 메소드를 가지고 있지 않아요. UserDetailsService는 오로지 username에 대한 실제 유저 검증용으로 고정되어 사용되기 때문에 가입한다거나 하는 일반적으로 저희가 아는 서비스의 역할과는 거리가 멉니다.

https://github.com/spring-projects/spring-security/blob/main/core/src/main/java/org/springframework/security/core/userdetails/UserDetailsService.java

jinyoungchoi95 · 2025-02-24T12:27:40Z

src/main/resources/application.yml

@@ -0,0 +1,15 @@
+oauth2:
+  google:
+    clientId: CLIENT_ID


clientId는 식별값이어서 굳이 막지 않아도 되지 않을까요?

jinyoungchoi95 · 2025-02-24T12:28:12Z

src/main/java/nextstep/app/OAuth2Property.java

+        this.github = github;
+    }
+
+//    public static class OAuth2Client {


주석은 제거해주시면 좋을 것 같아요

jinyoungchoi95 · 2025-02-24T12:32:40Z

src/main/java/nextstep/app/OAuth2Property.java

+    private OAuth2Client google;
+    private OAuth2Client github;


변수로 각각 들고 있게 되는 경우, 인증방식(ex. 카카오 oauth)가 늘어나게 되는 경우 계속해서 여기 코드를 변경해야하는 문제가 있어요.

yml만으로 여러 인증방식을 추가하게 하기 위해서 Map<String, ?>으로 받아보는 방식이 있을 것 같네요. key는 "google", "github"와 같은 값으로 들어가구요.

Map으로만 받을 수 있도록 수정했습니다. 수정한 방식대로면 인증방식이 늘어날 때마다 oAuth2ClientRepository Bean에 인증방식이 추가되어야 하는데, 더 좋은 방법이 있을까요?

oAuth2Property.getClients() .entrySet() .stream() .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue))

로 clients 자체를 활용하면 따로 하드코딩안해도 될 것 같아요 :)

jinyoungchoi95 · 2025-02-24T12:46:26Z

src/main/java/nextstep/security/authentication/OAuth2ClientFactory.java

+
+import java.util.Map;
+
+public class OAuth2ClientFactory {


Factory는 생성을 담당하는 객체이기 때문에 현재 객체는 요청정보에 맞는 ClientRegistration을 가져다주는 repository가 조금 더 적절한 네이밍이 되겠네요.

그래서 현재 객체의 역할은 사실 RequestMatcher로 복잡할 필요없이 registrationId에 맞는 ClientRegistration을 가져오는 역할로만 구성해주시면 됩니다.

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

RequestMatcher의 역할은 필터에서 수행하는 게 더 적절해보여서 수정했고, 네이밍도 수정했습니다!

jinyoungchoi95 · 2025-02-24T12:53:25Z

src/main/java/nextstep/security/authentication/OAuth2AuthenticationProvider.java

+                }
+            };
+
+            UserDetails newUser = userDetailsService.signUpUser(newUserDetails);


위에서 말씀드렸듯 그래서 유저가 들어온 것에 대해서 회원가입을 시켜주는 것은 현재 spring security의 관심사는 아닙니다.

jinyoungchoi95 · 2025-02-24T12:53:57Z

src/main/java/nextstep/security/authentication/OAuth2AuthenticationProvider.java

+        try {
+            userDetails = userDetailsService.loadUserByUsername(authentication.getPrincipal().toString());
+            return UsernamePasswordAuthenticationToken.authenticated(userDetails.getUsername(), userDetails.getPassword(), userDetails.getAuthorities());
+        } catch (AuthenticationException e) {


인가 에러가 나면 유저를 던져주기보다는 401에러가 나야겠네요 :)

'새로운 멤버인 경우 회원 가입 후 세션에 로그인 정보를 저장한 뒤 "/"으로 리다이렉트'
위 요구사항을 구현하기 위해서 이렇게 구현했었습니다..!
예외 던지도록 수정했습니다!

jinyoungchoi95 · 2025-02-24T12:55:37Z

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

+        OAuth2Client oAuth2Client = oAuth2ClientFactory.getOAuth2Client(request);
+        if (oAuth2Client == null) {
+            filterChain.doFilter(request, response);
+            return;
+        }


oauth2 설정 가져온 값이 없는 경우에는 현재 서버가 해당 플랫폼을 지원하지 않기 때문에 에러가 나는 것이 맞습니다 :)

jinyoungchoi95 · 2025-02-24T12:59:39Z

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

+        SecurityContext context = SecurityContextHolder.createEmptyContext();
+        context.setAuthentication(authenticated);
+        SecurityContextHolder.setContext(context);
+        securityContextRepository.saveContext(context, request, response);


oauth2에 대해서 사실 spring security의 역할은 여기서 끝이기 때문에 SecurityContext를 관리하는 것은 관심사가 아닐 수 있어요.

oauth2 플로우에서 중요한 것은 유저에게 oauth2로 가는 길을 열어주고, 유저가 oauth2를 통해서 가져온 토큰이 정상적인지 판단해주는 것까지가 그 역할이라고 봐주시면 좋을 것 같습니다.

* OAuth2ClientRepository 클래스는 클라이언트 정보만 가져오도록 수정 * RequestMatcher는 필터에서 사용하도록 수정

jinyoungchoi95

안녕하세요 주현님 😄

피드백 잘 반영해주셨네요 👍 다음단계 진행에 도움될만한 코멘트 몇가지 드렸습니다
다음 단계는 실제 구현체에 대한 내용을 반영하는 리팩터링인데요. 실제 spring security 구현체 코드는 많이 복잡하기 때문에 sample 코드를 먼저 보시고 전체적인 흐름을 파악해보시면 이해하는데 도움이 되실 것 같아요 😄

궁금하거나 고민이 되는 부분이 있으시다면 언제든 pr 코멘트 또는 dm으로 요청 부탁드립니다.
감사합니다 🙇‍♂️

jinyoungchoi95 · 2025-02-26T14:54:46Z

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

+public class OAuth2AuthenticationFilter extends OncePerRequestFilter {
+
+    private final RestTemplate restTemplate = new RestTemplate();
+    private static final RegexRequestMatcher OAUTH2_AUTHENTICATION_REQUEST_MATCHER = new RegexRequestMatcher(HttpMethod.GET, "/login/oauth2/code/.*");


사실 상수는 "/login/oauth2/code/.*"까지만 상수로 처리하고 RegexRequestMatcher는 주입받아 사용하는 형태로 구성되어요. 그리고 RequestMatcher로 추상화하여 가짐으로써 테스트하기 쉬운 구조로 가져가게됩니다 :)

jinyoungchoi95 · 2025-02-26T14:56:09Z

src/main/java/nextstep/security/authentication/OAuth2LoginRedirectFilter.java

+        if (!OAUTH2_LOGIN_REQUEST_MATCHER.matches(request)) {
+            filterChain.doFilter(request, response);
+            return;
+        }
+
+        String clientRegistrationId = extractRegistrationId(request);
+        ClientRegistration clientRegistration = oAuth2ClientRepository.findByRegistrationId(clientRegistrationId);


다음 단계 진행하실때는 요런 로직들이 모두 OAuth2AuthorizationRequestResolver안에 들어간다고 봐주시면 됩니다 :)

jinyoungchoi95 · 2025-02-26T14:58:58Z

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

+    private static String extractRegistrationId(HttpServletRequest request) {
+        return request.getRequestURI().substring("/login/oauth2/code/".length());
+    }


아마 sample 코드에선 이렇게 나와있을건데요. 실제 구현체를 따라가보면 OAuth2AuthenticationFilter는 path에서 registrationId를 추출하지는 않아요.

요청 흐름을 살펴보면,

RedirectFilter에서 registrationId 추출하여 request의 session에 저장 후 리다이랙

유저는 이 session을 가진채로 oauth 서버에 요청하여 받은 토큰을 AuthenticationFilter로 전달

Authentication은 path에 있는 값을 추출할 필요없이 session에 있는 값을 꺼내옴

으로 구성되어있습니다.

jukekxm added 7 commits February 19, 2025 22:09

feat: GithubLoginRedirectFilter 추가

f373dc7

feat: GithubAuthenticationFilter 추가

a536536

feat: 새로운 멤버인 경우 회원가입 로직 추가

00a57a6

feat: 두 AuthentiactionFilter를 OAuth2AuthenticationFilter로 통합

3f44d50

feat: 두 LoginRedirectFilter를 OAuth2LoginRedirectFilter로 통합

8dcd4bb

feat: OAuth2AuthenticationFilter 로직 메서드로 추출

eae4333

feat: 클라이언트 관련 값 삭제

c50cffa

jinyoungchoi95 requested changes Feb 24, 2025

View reviewed changes

jukekxm added 8 commits February 25, 2025 23:40

fix: 불필요 주석 제거

8492166

fix: 프로퍼티 값을 Map으로 받도록 수정

ff7d2d2

fix: 네이밍 변경 (OAuth2Client -> ClientRegistration)

0487f64

fix: Factory 클래스를 OAuth2ClientRepository 클래스로 변경

9da8016

* OAuth2ClientRepository 클래스는 클라이언트 정보만 가져오도록 수정 * RequestMatcher는 필터에서 사용하도록 수정

fix: OAuth2AuthenticationProvider에서 회원가입 로직 제거

135f116

fix: CLIENT_ID 수정

d8baba0

fix: 회원가입 로직 제외

c610bc9

fix: 불필요 주석 제거

0e94150

jinyoungchoi95 approved these changes Feb 26, 2025

View reviewed changes

jinyoungchoi95 merged commit 1600e1c into next-step:jukekxm Feb 26, 2025

1-1단계 - OAuth 2.0 Login, 1-2단계 - 리팩터링 & OAuth 2.0 Resource 연동 리뷰 요청드립니다. #13

1-1단계 - OAuth 2.0 Login, 1-2단계 - 리팩터링 & OAuth 2.0 Resource 연동 리뷰 요청드립니다. #13

Uh oh!

Conversation

jukekxm commented Feb 23, 2025

Uh oh!

jinyoungchoi95 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinyoungchoi95 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!