소셜 로그인 템플릿 적용 by U-Keun · Pull Request #1 · DoRun-DoRun/routine-app-BE

U-Keun · 2024-04-30T12:56:35Z

수정을 해서 올릴까 하다가 그냥 통째로 올립니다.

PR 양식은 올려주시면 이후부터 적용해서 작성하겠습니다.

qjatjr29

LGTM 👍

qjatjr29 · 2024-05-01T03:48:24Z

+
+@Getter
+@AllArgsConstructor
+public enum RoleType {


RoleType 이라는 네이밍 보다 Role 만 해도 되지 않을까요?

javax.management.relation 패키지에 Role 클래스가 존재하는 것으로 알고 있어서 구분을 위해 넣은 것이긴 한데, 그것이 더 자연스러워보인다면 바꿔도 괜찮습니다.

qjatjr29 · 2024-05-01T03:48:51Z

+@Setter
+@RequiredArgsConstructor
+@AllArgsConstructor
+public class UserPrincipal implements OAuth2User, UserDetails, OidcUser {


Setter 가 필요한 경우가 있는지 궁금합니다!

어떤 근거를 가지고 추가한 코드는 아닙니다. 쓰이는 곳이 없다는 것이 명확해지면 삭제하는 것이 좋아보입니다.

qjatjr29 · 2024-05-01T03:56:25Z

+        this.key = key;
+    }
+
+    private String createToken(String id, Date validity) {


Date 타입을 파라미터로 받을 필요가 없어 보입니다!

어떻게 구현하는 것이 좋을까요? 토큰의 만료 기간을 설정하는데 사용되긴 하는데 어떤 방식을 생각하시는지 궁금합니다.

결국 토큰 생성에 대한 책임은 Token이 가지고 있다고 생각합니다.
근데 AuthController에서 부터 토큰의 만료 기간 + 현재 시간 에 대한 정보를 인자로 넘겨주면서 Token 을 생성하는 것으로 보입니다.

Token 클래스에서 토큰의 만료기간에 대한 정보를 가지고 있고 이를 통해서 createToken 메서드에서 사용하면 되지 않을까라는 생각이 있습니다. 어떻게 생각하시는지 궁금합니다!!

qjatjr29 · 2024-05-01T03:57:00Z

+                .compact();
+    }
+
+    private String createToken(String id, String role, Date validity) {


오버로딩으로 처리한 이유가 있을까요?

토큰을 생성하는 방식을 어떻게 정해야 할 지 고민하다가 추가해놓은 것입니다. 한 가지 방식이 정해지면 나머지는 지우는 것이 맞습니다.

qjatjr29 · 2024-05-08T05:01:53Z

+    private final TokenProvider tokenProvider;
+    private final RefreshTokenRepository refreshTokenRepository;
+
+    private static final long UPDATE_TOKEN_STRATEGY = 259200000; // 3일


해당 변수를 yaml 파일로 빼는게 좋아보입니다.

넵, 동의합니다. 그리고 리프레시 토큰을 레디스에 얼마나 저장할 지도 정해야 합니다. 임의로 3일로 해두었는데, 적당한 기간을 정해봐요.

qjatjr29 · 2024-05-08T05:42:21Z

+            User savedUser = userRepository.findByUserId(userInfo.getId());
+
+            if (savedUser != null) {
+                if (providerType != savedUser.getProviderType()) {


if 문이 중첩되는 것이 별로 좋아보이지 않습니다.

(savedUser != null) 은 findByUserId 를 Optional로 감싸면 해결될것 같습니다.

providerType을 비교하는 것은 findByUserIdAndProviderType() 이런 식으로 처리해 데이터베이스에 한 번 쿼리를 보내는 것을 통해 불필요한 데이터를 가져오는 상황도 방지하고 조건을 확인하는데 있어서 코드상의 실수(문제)도 방지할 수 있을 것이라고 생각합니다.

따라서, 코드를 수정해보자면 아래와 같이 변경할 수도 있을 것 같습니다.
User savedUser = userRepository.findByUserIdAndProviderType(userInfo.getId(), providerType) .orElseGet(() -> createUser(userInfo, providerType));

https://github.com/DoRun-DoRun/routine-app-BE/pull/1/files#r1593407207

이 리뷰와 연관되어 수정해야 하는 부분인 것 같네요. 알겠습니다!

qjatjr29 · 2024-05-08T05:45:18Z

+    }
+
+    private User updateUser(User user, OAuth2UserInfo userInfo) {
+        if (userInfo.getName() != null && !user.getUsername().equals(userInfo.getName())) {


name이 null인지 확인하고 같은 이름인지 확인하는 코드는 updateUser 라는 메소드의 책임이 아닌것 같습니다.

맞습니다. 메서드를 따로 만들어서 나눠야 겠네요.

qjatjr29 · 2024-05-08T05:49:33Z

+
+    @Override
+    public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException {
+        User user = userRepository.findByUserId(username);


메소드는 userId를 통해서 user를 조회하는 것 같은데 username이 인자로 사용되고 있습니다.
이유가 있는 걸까요?

저희 서비스에서 CustomUserDetailsService 클래스가 없어도 될 것 같긴 한데, 아마 이 코드는 username과 userId를 동일시하는 것을 가정하고 있어서 이렇게 작성된 것 같습니다. 아무튼 좋은 코멘트인 것 같습니다. 도메인이 좀 애매하게 정의되어 있는 것 같네요.

qjatjr29 · 2024-05-08T05:49:51Z

+    @Override
+    public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException {
+        User user = userRepository.findByUserId(username);
+        if (user == null) {


Optional 사용하면 null 처리 할 수 있을거 같습니다

qjatjr29 · 2024-05-08T05:50:22Z

+    public static String getToken(HttpServletRequest request, String division) {
+        String header = request.getHeader(division);
+
+        if (header != null && header.startsWith(TOKEN_PREFIX)) {


위 조건절도 따로 메소드를 분리해 주는 것이 좋아보입니다.

https://github.com/DoRun-DoRun/routine-app-BE/pull/1/files#r1593414205

이것과 비슷하게 책임을 제대로 나눠줘야겠네요. 알겠습니다!

qjatjr29

👍 LGTM

qjatjr29 · 2024-05-09T06:41:55Z

+@MappedSuperclass
+@Getter
+@EntityListeners(AuditingEntityListener.class)
+public abstract class BaseEntity {


오! 그렇군요.
추상클래스로 만든다는 생각을 못해봤는데 생각해보니 사용할 이유가 있는것 같습니다.

추상 클래스로 선언함으로써 BaseEntity를 직접 생성하는 것을 방지

추상 클래스로 만듬으로 해당 클래스의 의미를 명확하게 나타낼 수 있을 것 같다

배워갑니다...

qjatjr29 · 2024-05-09T06:43:25Z

+    @Column(name = "EMAIL", length = 512, unique = true)
+    @NotNull
+    @Size(max = 512)
+    private String email;


저의 경우 여러 소셜로그인을 사용하는데 다른 provider 같은 이메일을 사용하곤하는데
만약 email이 unique 하다면 한 사용자가 여러 계정을 만드는 상황은 힘들것 같다는 생각입니다.

의도적으로 여러 계정을 만들지 못한다면 unique = true가 적절하다고 생각합니다

qjatjr29 · 2024-05-09T06:50:52Z

+import org.springframework.context.annotation.Configuration;
+
+@Configuration
+public class JwtConfig {


제가 잘 몰라서 그러는데 yml 파일에서 가져오는 환경변수를 TokenProvider 클래스에서 직접 바인딩해서 사용하면 안되는 이유가 있을 까요?

qjatjr29 · 2024-05-09T07:00:47Z

+        this.key = key;
+    }
+
+    private String createToken(String id, Date validity) {


결국 토큰 생성에 대한 책임은 Token이 가지고 있다고 생각합니다.
근데 AuthController에서 부터 토큰의 만료 기간 + 현재 시간 에 대한 정보를 인자로 넘겨주면서 Token 을 생성하는 것으로 보입니다.

Token 클래스에서 토큰의 만료기간에 대한 정보를 가지고 있고 이를 통해서 createToken 메서드에서 사용하면 되지 않을까라는 생각이 있습니다. 어떻게 생각하시는지 궁금합니다!!

qjatjr29

혹시 코드 수정된게 있다면 push 해주시고 코멘트 주시면 감사하겠습니다.

수정된 코드를 보고 리뷰할것이 있다면 하겠습니다.
👍

qjatjr29 · 2024-05-10T04:44:50Z

+import org.springframework.context.annotation.Configuration;
+
+@Configuration
+public class JwtConfig {


TokenProvider에서 결국 Secret와 같은 환경변수 값을 주입받아서 사용하는 형태이기 때문에 유지보수면에서 이를 분리해 환경변수 값에 변경이 일어났을 경우 해당 config 파일만을 수정하도록 해 유연하게 대응하도록 한 것이군요!

그렇다면, authController에서 사용중인 UPDATE_TOKEN_STRATEGY 같은 값도 환경변수로 사용하고, 이를 해당 config 파일에 추가하여 TokenProvider가 사용하도록 하는 것도 좋을 것 같습니다.

Token 생성 / 유지 에 대해 필요한 값들이 약간 흩어져 있다고 생각합니다.

소셜 로그인 템플릿 적용

87d2007

qjatjr29 self-requested a review May 1, 2024 03:47

qjatjr29 reviewed May 8, 2024

View reviewed changes

qjatjr29 reviewed May 9, 2024

View reviewed changes

qjatjr29 reviewed May 10, 2024

View reviewed changes

Conversation

U-Keun commented Apr 30, 2024

Uh oh!

qjatjr29 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!

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!

qjatjr29 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!

qjatjr29 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants