[REFACTOR] 카카오 로그인 피드백 반영 #20
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jun02160
reviewed
Jan 9, 2024
Comment on lines
+12
to
+18
| public void commence(HttpServletRequest request, HttpServletResponse response, AuthenticationException authException) { | ||
| setResponse(response); | ||
| } | ||
|
|
||
| private void setResponse(HttpServletResponse response) { | ||
| response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); | ||
| } |
Comment on lines
+27
to
+49
| public String createAccessToken(String payload) { | ||
| return createToken(payload, accessTokenValidityInMilliseconds); | ||
| } | ||
|
|
||
| public String createRefreshToken() { | ||
| byte[] array = new byte[7]; | ||
| new Random().nextBytes(array); | ||
| String generatedString = Base64.getEncoder().encodeToString(array); | ||
| return createToken(generatedString, refreshTokenValidityInMilliseconds); | ||
| } | ||
|
|
||
| public String createToken(String payload, long expireLength) { | ||
| Map<String, Object> claims = new HashMap<>(); | ||
| claims.put("payload", payload); | ||
| Date now = new Date(); | ||
| Date validity = new Date(now.getTime() + expireLength); | ||
| return Jwts.builder() | ||
| .setClaims(claims) | ||
| .setIssuedAt(now) | ||
| .setExpiration(validity) | ||
| .signWith(SignatureAlgorithm.HS256,secretKey) | ||
| .compact(); | ||
| } |
There was a problem hiding this comment.
P4
Access Token과 Refresh Token을 발급받는 메서드가 동일해보이는데, 두 토큰의 구성을 같이 가져가는 이유가 있을까요?
Refresh Token은 Access Token 만료 시에 재발급을 하기 위한 용도라 굳이 userId를 담지 않아도 될 것 같아요!!
There was a problem hiding this comment.
Access Token에만 userId를 담고 Refresh Token은 String generatedString = Base64.getEncoder().encodeToString(array); 으로 인코딩한 데이터를 담고 있습니다!
Comment on lines
+38
to
+40
| public String createToken(String payload, long expireLength) { | ||
| Map<String, Object> claims = new HashMap<>(); | ||
| claims.put("payload", payload); |
There was a problem hiding this comment.
Access Token에 userId를 담으려 했고, Refresh Token은 랜덤값을 인코딩하여 전달하고자 하였습니다.
Comment on lines
65
to
87
| } catch (JwtException e){ | ||
| throw new RuntimeException(String.valueOf(JwtValidationType.INVALID_JWT_TOKEN)); | ||
| } | ||
| } | ||
|
|
||
| public void validateToken(String token) { | ||
| try { | ||
| token = token.replaceAll("\\s+", ""); | ||
| token = token.replace("Bearer", ""); | ||
| Jwts.parserBuilder() | ||
| .setSigningKey(secretKey) | ||
| .build() | ||
| .parseClaimsJws(token); | ||
| } catch (MalformedJwtException ex){ | ||
| throw new UserException(TOKEN_NOT_FOUND); | ||
| } catch (ExpiredJwtException ex) { | ||
| throw new UserException(TOKEN_EXPIRED); | ||
| } catch (UnsupportedJwtException ex) { | ||
| throw new UserException(TOKEN_UNSUPPORTED); | ||
| } catch (IllegalArgumentException ex) { | ||
| throw new UserException(TOKEN_NOT_FOUND); | ||
| } | ||
| } |
There was a problem hiding this comment.
P5
여기서 JwtException과 UserException으로 서로 다른 예외로 처리하신 이유가 무엇인가요?! 큰 이유가 없다면 둘 중 하나로 통일하는 방식이 더 좋을 것 같습니다 :)
There was a problem hiding this comment.
JwtException은 security oauth2에서 제공하는 예외 처리 클래스이고, jwt 관련 커스텀 예외 처리는 UserException 안에서 처리해주었습니다! jwt 관련 예외 처리를 별도의 클래스로 분리하는게 맞을까요??
src/main/java/sopt/org/motivooServer/domain/auth/controller/OauthController.java
Outdated
Show resolved
Hide resolved
src/main/java/sopt/org/motivooServer/domain/auth/service/OauthService.java
Outdated
Show resolved
Hide resolved
src/main/java/sopt/org/motivooServer/domain/auth/service/OauthService.java
Outdated
Show resolved
Hide resolved
src/main/java/sopt/org/motivooServer/domain/auth/service/OauthService.java
Outdated
Show resolved
Hide resolved
src/main/java/sopt/org/motivooServer/domain/auth/service/OauthService.java
Outdated
Show resolved
Hide resolved
src/main/java/sopt/org/motivooServer/domain/auth/service/OauthService.java
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📌 관련 이슈
closed #2
✨ 어떤 이유로 변경된 내용인지
객체지향(캡슐화) 고려하여 코드 수정
하나의 메서드에 합쳐져있던 로직들을 나누고,
메서드 간의 역할을 분리하여 클래스의 응집성을 높이는 방향으로 수정하였습니다.
도메인 기반 패키지 분리
기존에 global.config.oauth 패키지 아래에 있었던 oauth 관련 설정 파일과 user 패키지 아래에 있던 oauth 관련 구현 클래스 파일들을 domain.auth 패키지의 하위로 수정하였습니다.
토큰 재발행 API 추가
로그인의 전체적인 플로우에 맞게 access token과 refresh token을 재발행하는 메서드를 삭제하고 API를 추가하였습니다.
기타 수정 코드
객체지향적인 관점에서 봤을 때 분기문을 최소해야되는데, 찬우님 말씀처럼 기존에 작성했던 코드는 JwtValidationType의 반환값에 따라 외부 클래스에서 다시 분기문으로 처리해주는 코드가 비효율적이라고 생각해서 위와 같이 한번에 예외 처리를 해주는 방식으로 수정하였습니다!
🙏 검토 혹은 리뷰어에게 남기고 싶은 말
스프링 시큐리티 적용 관련해서
실제로 이번에 기능 구현을 하면서 Spring security 설정 관련된 다양한 에러를 접했기 때문에 찬우님께서 말씀해주신 의견에 대해서 전적으로 동의합니다!
하지만 인증, 권한 부여, 액세스 제어 같은 보안적인 이유와 이미 시큐리티를 활용해서 기능 구현이 이미 된 상태라서 시큐리티를 빼기에는 다소 시간적인 여유가 없을 것 같다고 생각해서 그대로 적용하기로 결정했습니다.
@AllArgsConstructor(access = AccessLevel.PROTECTED)
@builder와 @NoArgsConstructor를 함께 사용하려면, @AllArgsConstructor도 함께 사용하거나 모든 필드를 가지는 생성자를 직접 만들어 줘야 한다고 해서 @AllArgsConstructor를 추가했습니다 !
https://blog.leocat.kr/notes/2018/09/02/lombok-using-builder-and-noargsconstructor-together
SecurityConfig에서 람다식으로 작성시 발생하는 오류
람다식으로 변환했을 경우에 에러가 발생하는데,, 원인을 모르겠습니다! 혹시 아시는 분 계실까요?? (추가예정)