feat: 로컬 환경에서 Oauth Mocking 및 인증 인가 작업 - [merged] #56

Closed halucinor closed 1 year ago

In GitLab by @haaeee on Feb 6, 2023, 13:30

Merges feature/hiworks-mock -> develop

작업 내용

Oauth Mocking 및 인증 인가 비즈니스 로직 추가

스크린샷

주의사항

Closes #19

[x] 💯 테스트는 잘 통과했나요?
[x] 🏗️ 빌드는 성공했나요?
[x] 🧹 불필요한 코드는 제거했나요?
[x] 💭 이슈는 등록했나요?
[x] 🏷️ 라벨은 등록했나요?
[x] 💻 git rebase를 사용했나요?

In GitLab by @haaeee on Feb 6, 2023, 13:30

requested review from @AnTaeWook

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/main/java/com/gabia/bshop/mapper/HiworksProfileMapper.java line 25

이 메서드는 MapStruct 기본 기능으로 해결해 볼 수 있어 보입니다!!
제가 매퍼에 대해 잘 모르지만 복잡한 로직이 없은이 디폴트 값에 주의하며 작업하면 될 것 같습니다

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/main/java/com/gabia/bshop/mapper/LoginResponseMapper.java line 23

member.getPhoneNumber()의 결과가 null 여부에 따라 분기를 수행하게 되는 것 같습니다.
매퍼에 비즈니스 로직 성격이 강한 코드가 들어가는 것이 적절할까에 대한 고민이 생기네요.
가입했느냐 안했느냐 여부는 서비스레이어에서 판단하는 건 어떨까요?

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/main/java/com/gabia/bshop/security/client/LocalHiworksOauthClient.java line 28

Local 개발용 코드긴 하지만, (6, 8), (5,7)의 의미를 코드를 읽는 입장에서 파악하기 힘듭니다.
의미를 갖는 네이밍된 변수로 변경하는 것은 어떤가요?

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/main/java/com/gabia/bshop/security/client/LocalHiworksOauthClient.java line 37

마찬가지로, "@gabia.com" 이라는 스트링이 반복되괴 있는데, 유지보수성와 변경성을 위해 변수로 빼는 것이 어떨까요?

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/main/java/com/gabia/bshop/security/client/ProdHiworksOauthClient.java line 41

ObjectMapper의 생성비용은 매우 큽니다. 스프링 팩토리에서 기본적으로 생성해 주는 객체인데, 주입받는 다른 방법을 고민해보시는 건 어떨까요

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/main/java/com/gabia/bshop/security/client/ProdHiworksOauthClient.java line 45

타임아웃을 10초로 설정하신 이유가 있을까요? 매직넘버는 빼주시면 좋을 것 같습니다.

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/main/java/com/gabia/bshop/security/support/AuthArgumentResolver.java line 28

확실하지 않게 드리는 질문인데요,
CurrentMember 라는 어노테이션이 Integer 클래스로 선언되면 어떻게 되나요?

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/main/java/com/gabia/bshop/security/support/AuthArgumentResolver.java line 37

요구사항에서, 사내 서비스인 만큼 외부 인원은 접근할 수 없어야 하는 것으로 이해했습니다.
auth가 null이라면, 더 이상 흐름을 진행하지 않고 오류를 뱉는 것은 어떨까요?
인증/인가에 대해 잘 몰라서 궁예식 코드리뷰 드리는점 양해 부탁드립니다..

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/main/java/com/gabia/bshop/service/AuthService.java line 46

개인적인 취향인데요, member.getRole() 은 따로 변수로 빼지 않고 member.getId() 만 변수로 빼서 사용하신 이유가 있을까요?

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/main/java/com/gabia/bshop/service/AuthService.java line 52

관리자로 등록되지 않으셨습니다. -> 회원으로 등록되지 않으셨습니다 로 바뀌면 좋을 것 같습니다

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/main/java/com/gabia/bshop/service/MemberService.java line 28

final 키워드 빠졌어요 제이미!

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/main/java/com/gabia/bshop/util/AuthTokenExtractor.java line 24

매직넘버의 의미를 확실히 네이밍해주면 좋을 것 같습니다!

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/test/java/com/gabia/bshop/security/client/LocalHiworksOauthClientTest.java line 41

when 스코프는 로직 하나만 작성해서 하나에 대한 검증만 실시하는 것이 좋습니다.
그게 불가능하다면 잘못 짜여진 코드라고 어디서 들은 것 같습니다. 테스트 코드를 분리하거나, 코드에 대한 고민을 해야할 것 같습니다.!

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/test/java/com/gabia/bshop/security/redis/RefreshTokenServiceImplTest.java line 55

given(refreshTokenRepository.findById(refreshToken.refreshToken())).willReturn( Optional.empty()); 절이,
//when의 행위를 검증하는 스코프에 있는 것이 맞을까요?
이 테스트에서 검증해야 하는 행동은 결국 refreshTokenRedisService.findToken(refreshToken.refreshToken())일 것 같습니다.

In GitLab by @AnTaeWook on Feb 7, 2023, 09:42

Commented on src/test/java/com/gabia/bshop/security/redis/RefreshTokenServiceImplTest.java line 70

void 저장된_리프레시_토큰이_없을_때_예외를_던진다() 와 동일한 리뷰입니다.

In GitLab by @haaeee on Feb 7, 2023, 10:02

Commented on src/main/java/com/gabia/bshop/security/client/LocalHiworksOauthClient.java line 28

이 부분은 Magic Number의 의미를 좀 더 구체화하겠습니다.

In GitLab by @haaeee on Feb 7, 2023, 10:02

Commented on src/main/java/com/gabia/bshop/security/client/LocalHiworksOauthClient.java line 37

변수로 슬쩍위로 빼겠습니다!

In GitLab by @haaeee on Feb 7, 2023, 10:02

Commented on src/main/java/com/gabia/bshop/mapper/HiworksProfileMapper.java line 25

Reference: https://mapstruct.org/documentation/stable/reference/html/#default-values-and-constants

이걸로 변경해보겠습니다.

In GitLab by @haaeee on Feb 7, 2023, 10:02

Commented on src/main/java/com/gabia/bshop/mapper/LoginResponseMapper.java line 23

저도 이 부분에 대해서 고민을 좀 더 해보고 추후 리팩토링하고 다시 커밋 올리겠습니다!

In GitLab by @haaeee on Feb 7, 2023, 10:03

Commented on src/main/java/com/gabia/bshop/security/client/ProdHiworksOauthClient.java line 41

Client가 한번 생성되도록 Config에서 Bean 으로 등록되어 mapper가 한번 생성된다고 생각하였습니다. 주입 받는 방식으로 코드를 다시 구성해보겠습니다.

In GitLab by @haaeee on Feb 7, 2023, 10:06

Commented on src/main/java/com/gabia/bshop/security/client/ProdHiworksOauthClient.java line 41

Config에서 Bean으로 Client를 등록하여 한번 생성되고 재사용한다 생각헀는데, 스프링에서 빈으로 mapper를 등록해주기에 이를 주입 받는 방식으로 변경하겠습니다.

In GitLab by @haaeee on Feb 7, 2023, 10:18

Commented on src/main/java/com/gabia/bshop/security/client/ProdHiworksOauthClient.java line 45

TimeOut을 설정하지 않을시에는 서버 연결에 문제가 있을 때 오랜 시간 대기할 수 있습니다.
TimeOut을 엄청 짧게 가져갈시에 `org.apache.http.conn.ConnectTimeoutException가 발생할 수 있습니다.
chrome 개발자 도구에서 네트워크 추적 개발자 도구 창이 10초를 감안하여 작성하였습니다.
최적의 타임아웃 시간을 찾거나 좀 더 이유를 찾아보겠습니다!

:)

In GitLab by @haaeee on Feb 7, 2023, 10:19

Commented on src/main/java/com/gabia/bshop/security/support/AuthArgumentResolver.java line 28

네 바로 수정 들어가겠습니다!

변경점은 추후 커멘트로 남기겠습니다.

In GitLab by @haaeee on Feb 7, 2023, 10:23

Commented on src/main/java/com/gabia/bshop/security/support/AuthArgumentResolver.java line 37

-> authorization Header에 담기는 것은 accessToken(저희 백엔드 서버에서 만든 Jwt Token)으로 사용자의 정보를 담고 있습니다. 즉, 사용자를 판별하는 용도로 사용되고 있습니다.

이에 따라 authorization Header가 없는 경우는 로그인 하기 전입니다.

AuthArgumentResolver는 MemberPayload를 controller에서 request가 들어올 떄 사용되는 데 이 떄 위에 언급하신 auth가 null이라면, 더 이상 흐름을 진행하지 않고 오류를 뱉는 것은 AuthInterceptor단에서 처리하였습니다.

In GitLab by @haaeee on Feb 7, 2023, 10:24

Commented on src/main/java/com/gabia/bshop/service/AuthService.java line 46

이 부분은 코드를 한번 더 읽어보고 커멘트 남기겠습니다!

In GitLab by @haaeee on Feb 7, 2023, 10:24

Commented on src/main/java/com/gabia/bshop/service/AuthService.java line 52

수정하도록 하겠습니다!

In GitLab by @haaeee on Feb 7, 2023, 10:26

Commented on src/main/java/com/gabia/bshop/service/MemberService.java line 28

final 키위드 붙이겠습니다!

In GitLab by @haaeee on Feb 7, 2023, 10:26

Commented on src/main/java/com/gabia/bshop/util/AuthTokenExtractor.java line 24

수정하도록하겠습니다!

In GitLab by @haaeee on Feb 7, 2023, 10:26

Commented on src/test/java/com/gabia/bshop/security/client/LocalHiworksOauthClientTest.java line 41

-> 제가 테스트 코드의 normal, Admin 을 두 가지를 동시에 테스트를 작성하는데 이는 분리를 잘 못한 것 같습니다. 분리한 후 추후 커밋 올리겠습니다.

In GitLab by @haaeee on Feb 7, 2023, 10:26

Commented on src/test/java/com/gabia/bshop/security/redis/RefreshTokenServiceImplTest.java line 55

수정하도록하겠습니다!

In GitLab by @haaeee on Feb 7, 2023, 10:26

Commented on src/test/java/com/gabia/bshop/security/redis/RefreshTokenServiceImplTest.java line 70

넵!

In GitLab by @halucinor on Feb 8, 2023, 10:24

Commented on src/main/java/com/gabia/bshop/controller/AuthController.java line 51

logout 시 main 페이지로 redirect 하는건 어떨까요?

In GitLab by @halucinor on Feb 8, 2023, 10:28

Commented on src/main/java/com/gabia/bshop/dto/request/HiworksTokenRequest.java line 9

@JsonProperty를 설정해주게되면 clientId가 client_id로 변경되어 response body에 담기나요?

In GitLab by @halucinor on Feb 8, 2023, 10:29

Commented on src/main/java/com/gabia/bshop/dto/response/AccessTokenResponse.java line 3

개행이 되어있지 않은거 같습니다

In GitLab by @halucinor on Feb 8, 2023, 10:31

Commented on src/main/java/com/gabia/bshop/dto/response/HiworksTokenResponse.java line 7

HiworksProfileResponse 에서는

@JsonIgnoreProperties(ignoreUnknown = true)

가 있는데 TokenResponse에는 없어도 되는건가요?

In GitLab by @halucinor on Feb 8, 2023, 10:32

Commented on src/main/java/com/gabia/bshop/dto/response/IssuedTokensResponse.java line 6

다른 record와 동일하게 개행이 있으면 좋을거 같습니다~

In GitLab by @halucinor on Feb 8, 2023, 10:34

Commented on src/main/java/com/gabia/bshop/dto/response/HiworksTokenResponse.java line 19

user_no -> userNo

In GitLab by @halucinor on Feb 8, 2023, 10:40

Commented on src/main/java/com/gabia/bshop/exception/CustomException.java line 8

CustomException을 만들 때 그냥 Exception 이 아닌 RuntimeException을 상속받은 이유가 있을까요?

In GitLab by @halucinor on Feb 8, 2023, 10:54

Commented on src/main/java/com/gabia/bshop/security/client/ProdHiworksOauthClient.java line 52

나중에 공통적으로 예외처리를 수행하는 ExceptionHandler를 만들어 중복되는 코드를 줄일 수 있을 거같아요. 어떻게 만들지 같이 고민해보면 좋을거 같습니다.

In GitLab by @halucinor on Feb 8, 2023, 10:56

Commented on src/main/java/com/gabia/bshop/security/provider/JwtProvider.java line 90

body와 id에도 final키워드를 붙하는게 맞을까요?

In GitLab by @haaeee on Feb 8, 2023, 11:06

Commented on src/main/java/com/gabia/bshop/security/client/ProdHiworksOauthClient.java line 41

changed this line in version 2 of the diff

In GitLab by @haaeee on Feb 8, 2023, 11:06

Commented on src/main/java/com/gabia/bshop/security/client/ProdHiworksOauthClient.java line 45

changed this line in version 2 of the diff

In GitLab by @haaeee on Feb 8, 2023, 11:06

Commented on src/main/java/com/gabia/bshop/dto/response/AccessTokenResponse.java line 3

changed this line in version 2 of the diff

In GitLab by @haaeee on Feb 8, 2023, 11:06

Commented on src/main/java/com/gabia/bshop/dto/response/IssuedTokensResponse.java line 6

changed this line in version 2 of the diff

In GitLab by @haaeee on Feb 8, 2023, 11:06

Commented on src/main/java/com/gabia/bshop/dto/response/HiworksTokenResponse.java line 19

changed this line in version 2 of the diff

In GitLab by @haaeee on Feb 8, 2023, 11:06

added 71 commits

be6754e0...b0164331 - 6 commits from branch develop
0beca88c - feat: hiworks oauth mock impl init
6d75ac7b - feat : spotless 적용 (mentee/mentee_2023.01/team/g-market/gabia_b_shop_backend!6)
9ed3224c - feat: docker-compose 환경 구성 (mentee/mentee_2023.01/team/g-market/gabia_b_shop_backend!8)
a1aa4698 - feat: 하이웍스 Oauth Client 추가
129f0d64 - feat: Local 하이웍스 Oauth Client 추가
f5a03339 - test: Local 하이웍스 Client 테스트
e00f88b0 - chore: json parser 의존성 추가
ba62bde2 - feat: Prod 하이웍스 Client 추가
fcdb8950 - test: JwtProvider memberId, role을 기준으로 토큰 생성
7b9ea214 - feat: memberId, role을 통해 accessToken 생성
90cdb571 - feat: refreshToken은 RedisHash로 저장 record 추가
d3c4c694 - test: refreshToken isExpired 테스트 작성
d31f8477 - test: 공통적으로 생성하는 Member Fixture 추가
2be5e0e4 - feat: RefreshTokenCookie 생성 클래스 추가
88fe7fae - test: 쿠키를 생성하고 만료하는 테스트 코드 작성
b00a3eaf - feat: RefreshToken Provider 추가
04597d63 - test: RefreshToken Provider 생성 테스트 코드 작성
63b147b5 - feat: RefreshToken Repo 및 Service save, find, delete 추가
8166bccb - feat: RefreshTokenServiceImpl save, delete, find 로직 추가
16c2ddf3 - feat: OauthClientConfig 추가
e6147cff - feat: CustomException 및 ErrorResponse 추가
e6fa7927 - rename: CustomException 및 ErrorResponse 에 따른 package import 변경
084a6f21 - remove: 기존 작업물 삭제
49da4e85 - rename: 패키지 이동 경로 변경
6f0765c0 - feat: Login, CurrentMember annotation 추가
fec60418 - feat: AuthInterceptor 추가
84d02d6d - feat: CurrentMember Argument Resolver 추가
1af905d5 - feat: Auth Token Extractor 추가
2b775286 - test: 헤더에서 "Bearer" 토큰을 추출 테스트 작성
ed4b1367 - feat: MemberPayload에서 id를 추출하는 support 추가
f6d07ade - feat: AuthController 인증/인가 controller 작성
b6b00c2f - feat: login, issueAccessToken, loginAdmin 메서드 추가
ff097374 - feat: Mapstruct를 활용한 mapper 추가
4e27b92a - feat: RECORD dto 추가
085b1833 - test: AuthService Test 코드 작성
027760ee - test: refreshTokenService 테스트 코드 작성
bbf76aac - feat: webConfig를 통해 Cors 및 interceptor, argument Resolver 등록
b1976f14 - test: webConfig테스트 코드 작성
eca79338 - feat: MemberController update 및 조회 쿼리 작성
d77e85f7 - feat: 사용자 update 메서드 추가
447e31a1 - chore: spring validation dependency 추가
76bef3cf - chore: application.yml admin hiworks Client 추가
6b6b0ad6 - test: 로그인된 회원 사용자 조회 및 업데이트 테스트 작성
ec5cbf5f - feat: 로그인된 회원 사용자 조회 및 업데이트 메서드 추가
2f972d2a - test: MemberPayloadSupport 테스트 코드 작성
b2c26c31 - feat: MemberPayloadSupport Util 의미에 맞게 private 생성자 추가
4153828a - chore: code CheckStyle 적용
a23ab3db - rename: order -> orders 로 rename 변경
3dbf5ba1 - style: checkStyle Warn equals o -> that으로 변경
936dee08 - chore: git rebase 작업물 추가
dc0e41c0 - rename: toURI checkStyle warning 으로 인한 변경
3cd3afd9 - chore: application.yml Oauth 하나로 통일로 변경
6f519b3a - refactor: Oauth 하나로 변경
50861047 - chore: application-prod.yml 추가
0cd28b74 - chore: line break formatter 변경
76b0c3d2 - refactor: supportsParameter 애노테이션과 클래스 체크 로직 추가
f723b946 - style: 개행 문자 추가
dd7edce7 - feat: CORS 정책에 해당하는 domain 추가
632be85d - refactor: 객체를 주입 받는 방식으로 변경 및 Hiworks Oauth API에 맞게 변경
a71be9c3 - refactor: Form 형식으로 보내기에 TokenRequest 삭제
e57e68ba - refactor: mapper 이름 더 직관적으로 변경
10fdc191 - refactor: record 변경 및 qualifer 제거
a527b376 - refactor: Oauth 요청에 따른 회원 이름 길이가 길어서 길이 변경
7084d9e3 - Merge remote-tracking branch 'origin/feature/hiworks-mock' into feature/hiworks-mock
9d80ad30 - refactor: git merge 충돌 제거

Compare with previous version

In GitLab by @halucinor on Feb 8, 2023, 11:10

Commented on src/main/java/com/gabia/bshop/security/provider/JwtProvider.java line 97

Exception 별로 정확하게 메시지를 나누는게 좋을거 같다고 생각합니다.

In GitLab by @halucinor on Feb 8, 2023, 11:11

Commented on src/main/java/com/gabia/bshop/security/redis/RefreshToken.java line 12

fianl 키워드가 빠진거 같아요

In GitLab by @haaeee on Feb 8, 2023, 11:11

added 1 commit

825063d1 - refactor: git merge 충돌 제거

Compare with previous version

g-market / b-shop-backend