feature: auth, user #46

ozzing · 2023-03-28T16:50:36Z

📝 PR Summary

유저 인증 및 유저에 관한 API 구현했습니다.
대부분 순차적으로 작성해야 하는 코드들이었는데요, 부득이하게 제가 팀에 혼자라...
PR을 한 번 올리면 다른 분들 확인하시는데 시간이 좀 걸리는 관계로 이렇게 한 PR에 변동사항이 우르르인점 굉장히 죄송하게 생각하고 있습니다...

🌵 Working Branch

feature/auth

🌴 Works

JWT Token 관련 메소드 구현
Security 설정
모든 API 헤더 Authorization으로 변경
닉네임 변경, 탈퇴하기 -> UserController
불필요 코드 리팩
한마디, 닉네임 조회 / 유저 정보 조회
한마디 변경 이사

🌱 Related Issue

#45

bluayer · 2023-03-29T03:55:47Z

@ozzing 오... 어진님 해당 PR 보려면 최소 순수 시간으로 1~2시간은 필요할 거 같은데 혹시 due date가 언제까지일까요?

ozzing · 2023-03-29T04:02:29Z

@bluayer 음 목요일 저녁까지 시간되시면 그 안에 되면 더 좋을 것 같아요!
구현해야 할 것이 더 있긴한데, 간단한 중복 검사라 해당 PR에 추가해도 괜찮을까요? => 했습니다!

코드 봐주실 때 혼란을 덜기 위해 설명을 더 적자면, 저희 앱팀 개발자 회의를 통해 레거시 제거를 위해 v1 -> v2로 올리기로 했고, 그 과정에서 불필요한 코드(플레이그라운드 소셜 로그인 구현에 따른 기존 인증 로직 등)을 전부 삭제, 이동, 리네이밍해서 pr이 매우매우 커졌습니다.
삭제된 코드는 앞으로 사용하지 않을 내용이기에 크게 신경쓰지 않고 보셔도 될 것 같습니다...!

bluayer

개인적으로는 PR이 길다 보니깐 제가 최대한 집중력을 유지해서 보려고 했음에도 불구하고, 좀 놓치는 부분이 많을 거 같아 아쉽습니다 😥

전반적으로 이 정도 사이즈의 기능을 빠르게 개발하셨다는 점에서 👍 를 드리고 싶은데요, 제가 생각했을 때 조금 아쉬운 점들이 있었습니다.

다음은 제가 코드를 읽으면서 느낀 아쉬운 점입니다.

저번 PR 리뷰 때 val 사용을 권장드렸었고 제 기억으로는 굉장히 잘 지키셨던 것으로 기억하는데, 본 리뷰에서는 auth 관련된 부분이나 user 관련 부분에서 거의 다 해당 부분을 지키지 못한 점이 아쉽습니다...
물론 일정과 시간에 쫓기면서 작성하셨고 PR 사이즈가 커서 그런 것이라고 생각되지만, 저번 코드리뷰 요청 때에 비해 상대적으로 코드 퀄리티가 많이 하락한 부분이 아쉽습니다. 제가 리뷰했던 어진님은 지금보다 더 좋은 코드를 작성하실 수 있는 분이라고 생각되어 특히 더 이런 느낌을 받았던 것 같습니다.
major 버전이 한 단계 올라간 것으로 보이는데요, 기존 v1에 대한 지원은 어떻게 되는지(하위호환성 등)에 대해서 PR 관련 설명에 없는 점이 아쉬웠습니다.

분명 일정과 시간에 쫓기는 것은 맞지만, 약간의 시간을 더 들여서 지속될 수 있는 코드를 작성하는 것도 중요하다고 생각합니다. 일반적으로 제가 드리는 리뷰는 권장이고 제가 꼭 다 맞는 것은 아니지만, 이번 리뷰에서는 특히 유의미한 내용들이 있어서 Request Changes를 하겠습니다!

src/main/java/org/sopt/app/application/auth/JwtTokenService.java

src/main/java/org/sopt/app/application/auth/PlaygroundAuthService.java

src/main/java/org/sopt/app/common/config/OpenApiConfig.java

bluayer · 2023-03-30T14:32:48Z

src/main/java/org/sopt/app/common/config/WebSecurityConfig.java

+
+@RequiredArgsConstructor
+@EnableWebSecurity
+public class WebSecurityConfig extends WebSecurityConfigurerAdapter {


제가 알기로는 WebSecurityConfigurerAdapter가 deprecated되어 있다고 알고 있는데 아닐까요??

이건 더 찾아보겠습니다!

https://spring.io/blog/2022/02/21/spring-security-without-the-websecurityconfigureradapter
👍

감사합니당!

아 혹시 해당 PR에 위 내용 수정까지 요청하신걸까요??

이거까지 하면 PR 사이즈가 너무 커질 거 같은데, 따로 PR 하나 열어주시죠!
리뷰가 가장 큰 blocker가 되면 안될 거 같습니다!

넵 늦은 시간까지 꼼꼼한 리뷰 정말 감사드립니다! 좋은 밤 되세요 😀

src/main/java/org/sopt/app/presentation/auth/AuthController.java

src/main/java/org/sopt/app/presentation/user/UserController.java

src/main/java/org/sopt/app/presentation/user/UserResponse.java

ozzing · 2023-03-30T15:03:34Z

먼저 엄청난 사이즈에도 정성스레 리뷰해주신 점 정말 감사드립니다!
v2로 올리면서 기존에 있던 여러가지 문제 해결을 위해 팀 논의를 통해 강제업데이트를 진행하기로 결정하였습니다. 변동사항이 많아지는만큼 먼저 클라이언트측에 통신 가능한 api를 dev 서버에 제공해주는 것이 중요하다고 판단했습니다.
그래서 말씀해주신 것처럼 시간에 좀 쫓기느라 저도 마음에 들지 않았거나 놓친 부분이 많았는데, 지속될 수 있는 코드를 작성하는 편이 더 좋다는 정우님의 말씀 뒤늦게나마 굉장히 공감되었습니다! 다시 꼼꼼히 확인해보도록 하겠습니다 :)

bluayer

하하... 어진님 PR을 빨리 닫고 싶으신 마음은 이해하지만...
보통 Request Changes라면 리뷰를 재요청하는 것이 일반적입니다.
리뷰가 잘 반영되었는지에 대한 책임은 코드 작성자와 리뷰어가 함께 가지고 있기 때문입니다.
또한 리뷰를 드렸지만, 컨텍스트가 잘 이해가 되신건지 혹은 더 나은 코드를 작성하셨는지 확인하는 것도 리뷰어가 하는 일입니다.

제가 남긴 리뷰는 다른 PR에서 추가적으로 작업해주시길 바랍니다.

bluayer · 2023-03-30T16:30:04Z

src/main/java/org/sopt/app/common/config/JwtExceptionFilter.java

 public class JwtExceptionFilter extends OncePerRequestFilter {

-    @Autowired
    private ObjectMapper objectMapper;


이렇게 그냥 두면 null 객체일거 같은데 잘 작동할까요...?
@RequiredArgsConstructor 를 통해 작동하지 않을 것 같습니다.

bluayer · 2023-03-30T16:41:24Z

src/main/java/org/sopt/app/application/user/UserService.java

+    private String generateNickname(String username) {
+        return new String(username + (int) (Math.random() * 10000));
+    }


해당 코드에는 두 가지 문제점이 있습니다.

int로 타입 강제를 함. -> 반올림을 하고 싶으셨던 거라면 round와 같은 메소드를 사용하여 의미를 선사하는 것이 맞습니다. 타입 강제로 인한 이슈는 부동 소수점을 다룰 때 늘 이슈가 될 수 있으므로 좋지 못합니다.

new String을 사용함. 차라리 +를 사용하는 것이 나은데요, 이 이유는 찾아보고 어진님을 위해 꼭 댓글로 달아주시길 바랍니다.

넵 변경하겠습니다.

new String 사용 시 아예 새로운 메모리에 할당되고 연산 과정에서 속도 저하가 있는 것으로 알고 있습니다. 유저 가입시에만 생성되는 간단한 문자열이라 신경쓰지 못했는데, +를 사용하면 좋을 것 같네요! 감사합니다.

ozzing · 2023-03-30T16:46:57Z

앗, 다른 리뷰를 보지 못했습니다..ㅠ 재요청 드렸는데 추후 다른 PR을 열어달라 코멘트 남겨주셔서 착각했습니다!
추가 리뷰 주신 내용 꼭 반영하겠습니다.

dragontaek-lee · 2023-03-31T19:20:55Z

src/main/java/org/sopt/app/application/auth/JwtTokenService.java

+                .setExpiration(now.plusDays(14).toDate())
+                .claim("id", userId.getId())
+                .claim("roles", "USER")
+                .signWith(SignatureAlgorithm.HS256,


제가 알기로는 .signWith()가 deprecated 된거로 알고 있습니당!

public abstract JwtBuilder signWith( java.security.Key key, io.jsonwebtoken.SignatureAlgorithm alg )

다음과 같이 인자가 변경되어 있어요.

오 감사합니다!

주어진사랑(eojinjoo) added 20 commits March 23, 2023 02:32

feature: playground server 연결, 프로필 조회 #45

9b2f9fa

refactor: RankController 전반 리팩, Mapper 활용 #43

e20ea56

refactor: RankService 전반 리팩 #43

6f97456

refactor: controller 에서 missionList 조회 #43

4688890

refactor: 랭킹 -> 미션 조회 시 정렬 #43

620b995

refactor: 유저 프로필 메시지 변경 후 리스폰스 수정 #43

8782d14

add: template

608fbd7

feat: playground member 기본 정보 get #45

d6d2cc5

feat: jwt custom token #45

25a9ef6

feat: 자체 jwt token 발급 #45

16f513b

feat: nickname random 값, 필요 플그 필드 저장 #45

f1ca1ab

feat: playground 로그인 swagger #45

7c695e2

feat: StampController Authorization으로 변경 #45

66ac9cb

del: Auth 불필요 코드 삭제 #45

73337ee

refactor: 불필요 코드 정리, 리네이밍 #45

26f9f49

feat: Health Check #45

caf5cee

feat: security 구현 #45

2c7ee94

feat: 닉네임 변경, 탈퇴하기 API #45

02365fa

chore: security 경로 #45

95d6488

feat: 유저 정보 조회, 솝탬프 정보 조회, 한마디 편집 #45

2566bd5

ozzing added ✨ Feat 새로운 피쳐 생성 🔨 Refactor 코드 개선 labels Mar 28, 2023

ozzing requested review from bluayer, dragontaek-lee and thguss March 28, 2023 16:50

ozzing self-assigned this Mar 28, 2023

pull-request-size bot added the size/XXL label Mar 28, 2023

ozzing and others added 2 commits March 29, 2023 01:53

Merge branch 'develop' into feature/auth

a68c942

add: swagger security config #45

4e38c98

ozzing changed the title ~~Feature/auth~~ feature: auth, user Mar 28, 2023

chore: requestmapping #45

a6e658a

주어진사랑(eojinjoo) added 5 commits March 29, 2023 13:28

feat: 유저 닉네임 변경 시 중복 검사 #45

4ddfbbb

fix: 상세 조회 nickname으로 변경 #45

02ba8e3

del: Stamp 조회 시 userId 제거 #45

58f618c

del: user email, password nullable #45

0d74fc1

add: 가입 시 points 0으로 초기화 #45

8837ce6

bluayer requested changes Mar 30, 2023

View reviewed changes

주어진사랑(eojinjoo) added 3 commits March 31, 2023 00:25

refactor: 불필요 코드 정리, 간단한 변경사항 #45

e03ff35

refactor: 메소드 변동사항 #45

90cc0b7

refactor: date, restTemplate 수정 #45

7190634

ozzing requested a review from bluayer March 30, 2023 16:08

ozzing merged commit b823420 into develop Mar 30, 2023

ozzing deleted the feature/auth branch March 30, 2023 16:34

bluayer reviewed Mar 30, 2023

View reviewed changes

dragontaek-lee reviewed Mar 31, 2023

View reviewed changes

feature: auth, user #46

feature: auth, user #46

Uh oh!

Conversation

ozzing commented Mar 28, 2023

📝 PR Summary

🌵 Working Branch

🌴 Works

🌱 Related Issue

Uh oh!

bluayer commented Mar 29, 2023

Uh oh!

ozzing commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluayer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

ozzing commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluayer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bluayer Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozzing Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozzing commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

4 participants

ozzing commented Mar 29, 2023 •

edited

Loading

bluayer left a comment •

edited

Loading

ozzing commented Mar 30, 2023 •

edited

Loading

bluayer left a comment •

edited

Loading

bluayer Mar 30, 2023 •

edited

Loading

ozzing Mar 30, 2023 •

edited

Loading

ozzing commented Mar 30, 2023 •

edited

Loading