Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEAT] 소셜 로그인(카카오 로그인 구현) #17

Merged
merged 11 commits into from
Jan 9, 2024

Conversation

hyeyeonnnnn
Copy link
Member

@hyeyeonnnnn hyeyeonnnnn commented Jan 8, 2024

📌 관련 이슈

closed #2

✨ 어떤 이유로 변경된 내용인지

  • OAuth 2.0을 활용해 카카오 회원가입/로그인/로그아웃 구현했습니다.

  • Authorization Code Grant 방식으로 authorization code와 access token을 발급받은 후에,
    login controller에서 access token으로 유저 정보를 받아옵니다. 로그인에 성공한 경우 access token과 refresh token을 반환하도록 처리하였습니다.

  • Redis를 통한 JWT Blacklist을 활용하여 refresh token과 access token을 관리합니다. 로그아웃 되는 경우에 Blacklist에 access token을 blocked 시키게 구현하였습니다. (access token의 유효기간이 남아있는 경우를 고려했을 때 보안상의 이유로 access token을 Blacklist로 저장하여 만료시키고자 하였습니다)

  • 로그아웃시 redis에 저장된 refresh token을 삭제하고 Blacklist에 access token을 등록하게 됩니다.
    access token의 남은 유효기간만큼 Redis에 유효기간을 설정하여 블랙리스트로 등록하여 유효기간이 끝나면 자연스럽게 Redis에서 삭제시켜 메모리에 남아있지 않도록 합니다.

🙏 검토 혹은 리뷰어에게 남기고 싶은 말

  • JWT Blacklist 방식을 처음 사용해보는데, 로그아웃시 redis에서 access token이 잘 blocked되는지 확인해주시면 감사하겠습니다!
  • 시큐리티 설정 에러/소셜 로그인 초보자의 뚝딱거림으로 인해서(ㅠㅠ) 작업이 많이 늦어져서 죄송합니다!!
    피드백 마구마구 부탁드려요 ~~~

@hyeyeonnnnn hyeyeonnnnn added 🪄API 서버 API 작업 ✨FEAT 새로운 기능 구현 🐰혜연🐰 labels Jan 8, 2024
@hyeyeonnnnn hyeyeonnnnn self-assigned this Jan 8, 2024
@hyeyeonnnnn hyeyeonnnnn changed the title [FEAT] : 소셜 로그인(카카오 로그인 구현) [FEAT] 소셜 로그인(카카오 로그인 구현) Jan 8, 2024
@jun02160 jun02160 linked an issue Jan 8, 2024 that may be closed by this pull request
2 tasks
Copy link
Member

@jun02160 jun02160 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

구현하시느라 정말 수고많으셨습니다 !!! 짱짱 👍 👍 😆

build.gradle Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기 develop 내용 반영이 안 된 것 같네요 !!! pull 받으시고 develop에 있는 코드 모두 반영해주세요 :)

Comment on lines 22 to 23
@GetMapping("/oauth/login/{provider}")
public ResponseEntity<ApiResponse<LoginResponse>> login(@PathVariable String provider, @RequestBody OauthTokenResponse tokenResponse){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2

컨트롤러의 인자가 요청바디를 받고 있는데 클래스명이 OauthTokenResponse인 이유가 있을까요 ?!
요청 바디로 넘어오는 값이니까 Request로 통일하는 게 좋을 것 같아요 !!

public class OauthController {
private final OauthService oauthService;

@GetMapping("/oauth/login/{provider}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2

provider는 kakao 또는 apple로 넘어오는 값이 맞을까요? 그렇다면 그 외의 값으로 요청이 들어왔을 때 예외처리도 해주시면 좋을 것 같습니다 !!

package sopt.org.motivooServer.domain.user.dto.oauth;

public interface OAuth2UserInfo {
String getProviderId();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서 providerId 필드가 User의 socialId 필드와 대응되는 게 맞을까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네!!! 맞습니다 ~~

Comment on lines 22 to 24
public String getProvider() {
return "kakao";
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P4

여기는 애플로그인이 추가된다면 분기 처리되는 부분인가요?!
나중에 변경될 여지가 있는 코드라면 놓치는 부분이 없도록 //TODO로 미리 작성해두시는 거 추천드립니다 ㅎㅎ

Comment on lines 66 to 100
@Transactional
public User getUserProfile(String providerName, OauthTokenResponse tokenResponse, ClientRegistration provider, String refreshToken){
Map<String, Object> userAttributes = getUserAttributes(provider, tokenResponse);
OAuth2UserInfo oAuth2UserInfo = null;
SocialPlatform socialPlatform = null;
if(providerName.equals("kakao")){
oAuth2UserInfo = new UserProfile(userAttributes);
socialPlatform = SocialPlatform.KAKAO;
} else {
throw new UserException(UserExceptionType.INVALID_SOCIAL_PLATFORM);
}

String providerId = oAuth2UserInfo.getProviderId();
String nickName = oAuth2UserInfo.getNickName();

User userEntity = userRepository.findBySocialId(providerId);


if(userEntity == null){
userEntity = User.builder()
.nickname(nickName)
.socialId(providerId)
.socialPlatform(socialPlatform)
.socialAccessToken(tokenResponse.getAccessToken())
.socialRefreshToken(refreshToken)
.type(UserType.NONE)
.deleted(Boolean.FALSE)
.build();
userRepository.save(userEntity);
}
else{
userEntity.updateRefreshToken(refreshToken);
}
return userEntity;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2

User 엔티티를 생성하여 DB에 저장하는 부분과 Refresh Token을 업데이트하는 로직은 별도의 메서드로 분리하는 것이 좋을 것 같습니다!
자바 클린코드 규칙 중 else 예약어의 사용을 지양한다는 내용이 있는데요!
if (userEntity == null) 보다 if (userEntity != null) 로 RefreshToken을 업데이트하는 메서드 하나, userEntity를 새로 DB에 저장하는 메서드 하나로 분리하면 좋을 것 같네요 ㅎㅎ

*else 예약어에 대한 참고자료
https://limdingdong.tistory.com/8

ClientRegistration provider = inMemoryRepository.findByRegistrationId(providerName);

String refreshToken = jwtTokenProvider.createRefreshToken();
User user = getUserProfile(providerName, tokenResponse, provider, refreshToken);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2

getUserProfile()@transactional 로 선언적 트랜잭션을 사용하신 것 같은데 그 이유가 있을까요 ?!
여기서만 해당 메서드가 사용된다면 private을 바꿔도 될 것 같구, 여기서 이미 트랜잭션 처리를 하고 있기 때문에 해당 메서드까지 @transactional 어노테이션을 사용하지 않아도 될 것 같습니다 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P4

스프링부트 3.X 버전 이후로는 이와 같은 방식으로 SecurityConfig를 구성하는 게 deprecated 되고, 람다식으로 구성하는 걸 권장하는데 다음에 시간 되시면 바꿔보시는 것도 좋을 것 같습니당 :>

Comment on lines 37 to 43
} catch (Exception exception) {
try {
exception.printStackTrace();
} catch (Exception e) {
throw new ServletException("Authentication failed: " + exception.getMessage(), exception);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기 이렇게 처리해주신 이유가 궁금합니당 !!

Comment on lines 61 to 64
} catch (JwtException e){
throw new RuntimeException("유효하지 않은 토큰 입니다");
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2

여기 부분 BusinessException과 JwtValidationType ENUM으로 처리해줘도 좋을 것 같아요 !!

Copy link
Contributor

@oownahcohc oownahcohc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혜연님 고생하셨어요~!!🙂 여러가지 기술들이 많이 들어가 있어서 힘드셨을 것 같아요😵‍💫
리뷰 남겨둔 것들 읽어보시고 반영해주시면 좋을 것 같아요.

전체적으로 리뷰 한번 드리고 싶은 부분이 있어서 남겨요.

스프링 시큐리티 적용에 대해

먼저 말씀드리고 싶은건 적용해주신 스프링 시큐리티인데요. 강력한 라이브러리임은 분명합니다.
다만 스프링 시큐리티는 내용이 방대하고 난이도도 어려워서 쉽게 적용을 결정할 문제는 아닌 것 같아요. 실제 현업에서도 스프링 시큐리티는 팀 바이 팀으로, 적용하는 팀도 있고, 굳이 적용하지 않는 팀도 있습니다.
그렇기 때문에 지금 저희 서비스 단계에서는 굳이 스프링 시큐리티까지 적용하지 않아도 된다고 생각해요. 물론 공부하는 느낌으로 적용해볼 수 있겠지만, 기술의 적용보다 더 중요한 것들이 많거든요.

제가 생각하는 더 중요한 것들은 "스프링의 환경을 제대로 알고, 부트가 제공하는 편리한 기능들을 이용해서 좀 더 객체 지향적이고 깔끔한 코드를 작성하는 것"입니다.
그래서 지금은 다양한 기술들을 적용해보는 것 보다 위에 말한 중요한 것들을 지켜가면서 코드를 짜는 게 좋을 것 같다는 생각이 들어요.

이런 이유로, 저는 개인적으로 스프링 시큐리티 없이 구현하는게 더 도움이 될 거라고 생각하는데요, 이 부분 고민한번 해보시고 알려주세요!!

패키지 관련

지금 혜연님이 구성하신 패키지를 살펴보면, oauth 관련된 로직들을 global.config.oauth 패키지 안에, 그리고 로그인/회원가입 기능들은 user 패키지에 구성해두셨는데요. 이렇게 "인증" 이라는 하나의 도메인에 특화된 config 클래스와 구현 클래스들은 domaim.auth 라는 패키지 안에 구성해두는 것이 응집도 측면에서 더 좋습니다.
정말 user 와 연관성이 깊은 UserRepository 클래스나 UserException 등을 제외하고는 모두 domain.auth 패키지를 새로 만들어 옮기는게 더 좋을 것 같아요~!! ㅎㅎ

OauthService 관련

로그인 플로우

일단 로그인 플로우가 살짝 애매한 것 같아요.
보통 소셜 로그인을 진행하면

  1. 클라이언트로부터 각 소셜 계정의 token 값을 받아오고
  2. 해당 token 값으로 kakao 또는 apple 등의 인증 서버 api 를 호출
  3. 사용자의 정보를 가져와서
  4. 해당 정보를 바탕으로 로그인 및 회원가입 처리

를 하는 형식인데요.

OauthService 를 보면 1~3 과정 이후에 토큰 재발급(reissue) 로직을 호출하고 있어요.
토큰 재발급은 보통 자동 로그인 상태를 유지하기 위해서 accessToken 만료 시, 저장된 refreshToken 을 활용해 accessToken 과 refreshToken 을 재발급 해주는 역할을 합니다.
때문에 login 메서드 내에서 호출하고 있는 reissue 메서드는 제거하고, 토큰 재발급 api 를 따로 제공하는게 좋습니다!
(추가적으로 @Transactional 은 스프링 AOP 기반으로 Proxy 로 동작하는데요, 이 때문에 동일한 클래스 내에서 트랜잭션 어노테이션이 붙어있는 메서드에는 트랜잭션이 적용되지 않습니다. 이 부분은 나중에 찾아보시면 좋을 것 같아요)

캡슐화

객체지향의 핵심은 캡슐화라고 할 수 있습니다. 지금 이 리뷰에서 자세히 설명드리기는 쉽지 않겠지만, 객체들 하나하나를 그냥 소심한 사람 이라고 생각하시면 조금 이해가 쉬우실 것 같아요.
그렇기 때문에 객체는 자신 내부의 상태(필드)를 외부에 공개하는걸 꺼려하고, 오직 인터페이스(메서드) 만을 통해 외부와 메시지를 주고 받고자 해요.
이런 관점에서 모든 클래스들을 한번 다시 보시면, public 접근 제한자들을 아마 감추고 싶어지실 거예요.
OauthService 객체에서도 마찬가지입니다.
이런 부분들 신경 써주시면서 한번 리팩토링 해주시면 좋을 것 같아요 ㅎㅎ

코드 정리 관련

개발 완료하시고 커밋 하시기 전에, 혹시 사용되지 않는 import 된 패키지들이 있는지, 사용되지 않는 변수들이 있는지 확인해주세요!
그리고 인텔리제이에서 "커멘드 + 옵션 + L" 을 눌러주시면 코드가 정돈됩니다! 이것도 꼭 해주시면 좋을 것 같아요~!!



@RequiredArgsConstructor
@Slf4j
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 클래스에서 로깅 하지 않으실 거라면@Slf4j 에 대한 의존성은 필요 없어 보여요!

public class OauthController {
private final OauthService oauthService;

@GetMapping("/oauth/login/{provider}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로그인에 대한 요청은 GET 보다는 POST 가 적합합니다~!
그리고 provider 로 바인딩 되는 값은 "kakao" 또는 "apple"인가요? RequestBody 로 받아와도 될 것 같아요

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리고 @RequestBody 로 바인딩 되는 클래스 이름이 OauthTokenResponse 로 되어있네요!! OauthTokenRequest 로 바꿔주세요!! ㅎㅎ

Comment on lines 25 to 26


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기 두 칸 띄우신 이유가 있나요?

import sopt.org.motivooServer.domain.common.BaseTimeEntity;
import sopt.org.motivooServer.domain.mission.entity.UserMission;
import sopt.org.motivooServer.domain.parentchild.entity.Parentchild;

@Getter
@Entity
@Table(name = "`user`")
@Builder
@AllArgsConstructor(access = AccessLevel.PROTECTED)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AllArgsConstructor(access = AccessLevel.PROTECTED) 를 붙여주신 이유가 있을까요?

JPA 의 엔티티로 매핑되는 클래스는 기본 생성자가 필요한데요.
이는 JPA 가 데이터베이스에서 읽어온 값을 객체로 매핑할 때 리플랙션이란 기능을 이용해 필드 값들을 주입해주기 때문입니다. (프록시 기능을 사용할 수 있도록 해주기 위함도 있습니다.)
따라서 접근 제한을 private 으로 가져갈 수 없다는 단점이 있고, 최대한의 캡슐화를 지키기 위해 protected 의 레벨을 가져가는 거죠!

따라서 @AllArgsConstructor(access = AccessLevel.PROTECTED) 부분은 사실상 불필요하고, 아래에 작성해두신 public User(){} 기본 생성자는 protected 로 바꿔주시면 좀 더 안정성 있는 코드가 됩니다~!

Comment on lines 25 to 26
private final static String PREFIX_REFRESH = "REFRESH:";
private final static String PREFIX_BLOCKED = "BLOCKED:";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private static final 순서가 바뀌어 있네요!! ㅎㅎ

Comment on lines 65 to 84

public JwtValidationType validateToken(String token) {
try {
final Claims claimsJws = Jwts.parserBuilder()
.setSigningKey(secretKey)
.build()
.parseClaimsJws(token)
.getBody();
return JwtValidationType.VALID_JWT;
} catch (MalformedJwtException ex){
return JwtValidationType.INVALID_JWT_TOKEN;
} catch (ExpiredJwtException ex) {
return JwtValidationType.EXPIRED_JWT_TOKEN;
} catch (UnsupportedJwtException ex) {
return JwtValidationType.UNSUPPORTED_JWT_TOKEN;
} catch (IllegalArgumentException ex) {
return JwtValidationType.EMPTY_JWT;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1]

validate 하는 메서드에서 getBody 로 Claim 값을 가져오지 않아도 될 것 같아요~!
전반적으로 사용되지 않는 변수는 제거해주는게 좋습니다 ㅎㅎ
parseClaimsJws 까지만 체이닝 해줘도, 해당 메서드 내에서 발생한 Exception들을 처리해줄 수 있습니다.

추가적으로, JwtValidationType 으로 정의해주신건 좋은 방법인 것 같은데요, 사실 이 메서드는 제대로 된 역할을 못하고 있는 것 같아 보여요.
인자로 받은 token 에 대해서 유효성 검사를 진행하고 해당 토큰이 유효하지 않다는 Exception이 발생하면 JwtValidationType 의 값들 중 하나를 반환해주고, 검사 결과에 대한 처리를 이 메서드를 호출한 외부 클래스에서 해주고 있어요.

이렇게 되면 외부에서 이 메서드를 호출한 로직은 아마 if-else 와 같은 분기처리가 필수적일 것으로 예상되는데, Jwt 에 대한 validate 의 책임을 떠넘기고 있는 거죠!

따라서 다음과 같은 형식으로 수정해주는게 좋을 것 같아요!

Suggested change
public JwtValidationType validateToken(String token) {
try {
final Claims claimsJws = Jwts.parserBuilder()
.setSigningKey(secretKey)
.build()
.parseClaimsJws(token)
.getBody();
return JwtValidationType.VALID_JWT;
} catch (MalformedJwtException ex){
return JwtValidationType.INVALID_JWT_TOKEN;
} catch (ExpiredJwtException ex) {
return JwtValidationType.EXPIRED_JWT_TOKEN;
} catch (UnsupportedJwtException ex) {
return JwtValidationType.UNSUPPORTED_JWT_TOKEN;
} catch (IllegalArgumentException ex) {
return JwtValidationType.EMPTY_JWT;
}
}
public void validateToken(String token) {
try {
Jwts.parserBuilder()
.setSigningKey(secretKey)
.build()
.parseClaimsJws(token);
} catch (MalformedJwtException ex){
throw new ~Exception();
} catch (ExpiredJwtException ex) {
throw new ~Exception();
} catch (UnsupportedJwtException ex) {
throw new ~Exception();
} catch (IllegalArgumentException ex) {
throw new ~Exception();
}
}

} catch (ExpiredJwtException e) {
return e.getClaims().getSubject();
} catch (JwtException e){
throw new RuntimeException("유효하지 않은 토큰 입니다");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RuntimeException 으로 예외처리 해주는 것 보다 저희가 전역적으로 사용하는 CustomExcpetion 형태로 바꿔서 처리해주는게 좋아요🙂

Comment on lines 45 to 56
@Column(nullable = false)
private String socialId;

private String socialNickname;
private String nickname;

private String socialAccessToken;

private String socialRefreshToken;

@Enumerated(EnumType.STRING)
@Column(nullable = false)
private SocialPlatform socialPlatform;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Social 계정과 관련되어 있는 필드들은JPA 의 Embedded 를 활용해서 관리해주는게 객체 간 책임 분리에 좋을 것 같아요

.getExpiration();

} catch (JwtException e){
throw new RuntimeException("유효하지 않은 토큰 입니다");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RuntimeException 으로 예외처리 해주는 것 보다 저희가 전역적으로 사용하는 CustomExcpetion 형태로 바꿔서 처리해주는게 좋아요🙂

Comment on lines 112 to 114
User user = userRepository.findById(userId)
.orElseThrow(
() -> new UserException(UserExceptionType.INVALID_USER_TYPE));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서 이 user 는 사용되지 않는 값인 것 같은데요! 단순히 존재하는지 확인하는 용도로 사용하신거라면 existById 라는 메서드를 사용하면 될 것 같아요~!

제가 봤을때는 이 메서드는 따로 토큰 재발급 API 를 제공할 때 사용될 것 같아서, 만약 redis 에서 바로 refreshToken 을 가져온다면, 아예 제거해도 될 것으로 보입니다~!!

@hyeyeonnnnn hyeyeonnnnn merged commit 478659e into develop Jan 9, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪄API 서버 API 작업 ✨FEAT 새로운 기능 구현 🐰혜연🐰
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] 소셜 로그인 기능 구현
3 participants