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

Security 관련 코드 리팩토링 & 카카오 로그인 방식 제거 #20

Merged
merged 13 commits into from
Jul 10, 2022

Conversation

seonpilKim
Copy link
Member

@seonpilKim seonpilKim commented Jul 1, 2022

📌 Linked Issues

🔎 Change Details

카카오 로그인 방식 제거로 인한 코드 수정

카카오는 개인정보 조회 시, 휴대폰 번호와 이메일을 필수로 가져올 수 없기 때문에, MVP 단계에서는 제외하자는 의견이 회의에서나옴에 따라, 해당 방식은 지원하지 않기로 하였습니다.

네이버는 휴대폰 번호와 이메일을 필수로 가져올 수 있어서, 회원가입 시 해당 정보를 가져와서 DB에 저장하도록 코드를 수정하였습니다.

사용자 인증 과정에서 발생하는 예외 리팩토링

지원하지 않는 플랫폼(Naver 외) 로그인 요청의 경우 UnsupportedPlatformSignInException을 발생시키도록 코드를 추가하였습니다.
현재 인증 과정에서 발생할 수 있는 예외는 위와 JwtAuthenticationException이 있는데, 이들은 예외 상황에 대한 추가적인 정보(ErrorCode, ErrorResponse)를 가집니다.

  • 해당 예외들을 다형성을 이용하여 처리하기 위해 CustomAuthenticationException를 추가하였고, 구조는 아래와 같습니다.
    • image
    • image
  • CustomAuthenticationException이 발생하면, 아래의 doFilter 메소드에서 catch되어 예외처리를 수행하게 됩니다.
    public abstract class AbstractAuthenticationProcessingFilter {
    ...
        private void doFilter(HttpServletRequest request, HttpServletResponse response, FilterChain chain)
    		throws IOException, ServletException {
    	...
    	try {
    		Authentication authenticationResult = attemptAuthentication(request, response);
    		...
    	}
    	...
    	✔catch (AuthenticationException ex) {
    		// Authentication failedunsuccessfulAuthentication(request, response, ex);
    	}
        }
    ... 
        protected void unsuccessfulAuthentication(HttpServletRequest request, HttpServletResponse response,
    		AuthenticationException failed) throws IOException, ServletException {
    	SecurityContextHolder.clearContext();
    	...
    	✔this.failureHandler.onAuthenticationFailure(request, response, failed);
        }
    ...
    }

추가로 CustomAuthenticationExceptionBusinessException은 다형성을 이용하려는 목적으로 사용하고, 인스턴스를 생성하지 않기 때문에, 추상 클래스로 변경하였습니다.

인증 필터의 AuthenticationFailureHandler 변경

public abstract class AbstractAuthenticationProcessingFilter {
...
    private AuthenticationSuccessHandler successHandler = new SavedRequestAwareAuthenticationSuccessHandler();
    private AuthenticationFailureHandler failureHandler = new SimpleUrlAuthenticationFailureHandler();
...
}

위와 같이, AbstractAuthenticationProcessingFilter는 인증 성공/실패에 대한 핸들러가 디폴트로 SavedRequestAwareAuthenticationSuccessHandlerSimpleUrlAuthenticationFailureHandler를 사용하도록 구현되어 있습니다.
내부 코드를 살펴보면서 저는 SimpleUrlAuthenticationFailureHandler를 인증 실패 시 지정한 url으로 리다이렉트 및 예외를 세션에 저장하거나, 단순히 http 상태 코드와 메시지만 전달하기 위한 목적으로 사용하는 것으로 이해하였습니다.
따라서 저는 세션을 사용하지 않고, 예외에 대한 구체적인 정보를 JSON 형식으로 담아 클라이언트측에 전달하기 위해, AuthenticationEntryPointFailureHandler을 사용하도록 WebSecurityConfig에 핸들러를 변경하는 코드를 추가하였습니다.

public class WebSecurityConfig {
...
    	private final CustomAuthenticationEntryPoint authenticationEntryPoint;
...
	@Bean
	public AuthenticationEntryPointFailureHandler authenticationEntryPointFailureHandler() {
		return new AuthenticationEntryPointFailureHandler(authenticationEntryPoint);
	}

	@Bean
	public JwtAuthenticationFilter jwtAuthenticationFilter() throws Exception {
		...
		filter.setAuthenticationFailureHandler(authenticationEntryPointFailureHandler());
		return filter;
	}

	@Override
	protected void configure(HttpSecurity http) throws Exception {
		http
			...
			.oauth2Login()
			...
			.failureHandler(authenticationEntryPointFailureHandler())
			...
		;

		http.addFilterBefore(jwtAuthenticationFilter(), UsernamePasswordAuthenticationFilter.class);
	}
...
}
public class CustomAuthenticationEntryPoint implements AuthenticationEntryPoint {

	@Override
	public void commence(HttpServletRequest request, HttpServletResponse response,
		AuthenticationException authException) {
		final CustomAuthenticationException exception = (CustomAuthenticationException)authException;
		...
	}

}

JwtAuthenticationFilter 리팩토링

AbstractAuthenticationProcessingFilter 구현 메서드에 관한 질문에서 논의하였던 내용을 적용하였습니다.

  • 다른 부모 생성자를 호출함으로써 불필요한 코드를 제거하였습니다.
  • 인증 성공 시에는 SavedRequestAwareAuthenticationSuccessHandler가 호출되지 않도록, 인증 객체를 SecurityContext에 저장하고, 바로 다음 필터를 호출하도록 수정하였습니다.
  • 인증 실패 시에는 WebSecurityConfig에서 FailureHandler를 AuthenticationEntryPointFailureHandler으로 설정하였기 때문에, 부모의 FailureHandler를 가져와서 onAuthenticationFailure 함수를 호출하도록 수정하였습니다.

💬 Comment

📑 References

✅ Check Lists

  • 추가한 기능에 대한 테스트는 모두 완료하셨나요?
  • 코드 정렬(Ctrl + Alt + L), 불필요한 코드나 오타는 없는지 확인하셨나요?
  • merge할 base branch가 올바른지 확인하셨나요?

회원 가입 시, 프로필 정보에서 가져올 데이터 목록 변경
- nickname -> 적용 x
- phone -> 적용 o

지원하지 않는 플랫폼 로그인의 경우 예외 처리

Resolve: #18
1. AuthenticationException을 상속 받는 CustomAuthenticationException 추가
2. 사용자 인증 과정에서 발생시키는 예외들은 CustomAuthenticationException을 상속 받음
3. CustomAuthenticationEntryPoint 에서 다형성을 이용하여 위의 예외들을 처리하도록 수정 함
BusinessException, CustomAuthenticationException은 인스턴스를 생성하지 않고, 단순히 다형성 처리를 위해 만든 클래스이므로, 추상 클래스로 변경
AbstractAuthenticationProcessingFilter 클래스의 AuthenticationFailureHandler는 디폴트로 SimpleUrlAuthenticationFailureHandler을 사용한다.

AuthenticationEntryPointFailureHandler을 사용하도록 코드를 추가하였다.

Resolve: #19
다른 부모 생성자를 호출함으로써, 불필요한 재정의 코드 삭제

인증 성공 시, 디폴트 인증 성공 핸들러인 SavedRequestAwareAuthenticationSuccessHandler을 호출하지 않고, 단순히 인증 객체를 SecurityContextHolder에 저장한 후 다음 필터를 호출하는 로직으로 변경

인증 실패 시, AuthenticationEntryPointFailureHandler를 사용하기 위해 부모의 FailureHandler를 가져와서 onAuthenticationFailure 메소드를 호출하는 로직으로 변경

Resolve: #19
@seonpilKim seonpilKim added ✈ Feature 새로운 기능 추가/변경/삭제 ☄ Refactor 코드 리팩토링 ✏ Review request 코드 리뷰 요청 labels Jul 1, 2022
@seonpilKim seonpilKim requested a review from a team July 1, 2022 03:52
@seonpilKim seonpilKim self-assigned this Jul 1, 2022
@@ -54,14 +47,14 @@ public Authentication attemptAuthentication(HttpServletRequest request, HttpServ
protected void successfulAuthentication(HttpServletRequest request, HttpServletResponse response, FilterChain chain,
Authentication authResult) throws IOException, ServletException {
SecurityContextHolder.getContext().setAuthentication(authResult);
super.successfulAuthentication(request, response, chain, authResult);
chain.doFilter(request, response);
Copy link
Member

Choose a reason for hiding this comment

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

👍

@seonpilKim seonpilKim changed the title Feat/18 Security 관련 코드 리팩토링 & 카카오 로그인 방식 제거 Jul 1, 2022
final ErrorCode errorCode = exception.getErrorCode();
final int status = errorCode.getStatus();
final String code = errorCode.getCode();
final String message = errorCode.getMessage();
final List<ErrorResponse.FieldError> errors = exception.getErrors();
final ErrorResponse errorResponse = ErrorResponse.of(status, code, message, errors);
Copy link
Contributor

Choose a reason for hiding this comment

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

errorCode 의 필드를 다 꺼내서 생성하는 것보다 아래 메소드를 사용해서 ErrorResponse 객체를 생성하는건 어떨까요??

public static ErrorResponse of(final ErrorCode code, final List<FieldError> errors) {
    return new ErrorResponse(code, errors);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

해당 클래스에서 객체로 생성해서 사용하는 ObjectMapper 클래스는 Bean 으로 생성해 중앙에서 관리해도 좋을 것 같습니다!
ObjectMapper 는 다른 부분에서도 자주 사용될 것이라 예상됩니다!! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

감사합니다🙏 마침 저도 작성하면서 보기 안 좋다는 생각이 들었었는데, static 메소드로 구현하는 게 훨씬 깔끔하겠네요!
음 ObjectMapper는 어디서 Bean으로 생성하는 게 좋을까요? 도메인 모듈의 config 패키지에 ObjectMapperConfig 클래스를 만들어서 등록하는 건 어떨까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

좋을 것 같습니다! 그리고 해당 static 메소드는 이미 구현되어 있는걸로 봤습니다 ㅎㅎㅎ 👍

import com.zoopi.exception.response.ErrorCode;
import com.zoopi.exception.response.ErrorResponse;

import lombok.Getter;

@Getter
public class JwtAuthenticationException extends AuthenticationException {
public class JwtAuthenticationException extends CustomAuthenticationException {
Copy link
Contributor

Choose a reason for hiding this comment

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

이런식으로 인증쪽 관련된 별도의 클래스를 만들어 사용하는 것도 굉장히 깔끔하고 좋은 방법인 것 같네요 👍

다만, 제가 Spring Security 를 사용하면서 느낀 부분이 굉장히 Spring Security 의존적으로 개발하게 되는 것 같다는 것인데요,
만약 기존에 활용되고 있는 최상위 Exception 클래스인 BusinessException 을 활용하는 방식에 대해서는 어떻게 생각하시는지 의견이 궁금합니다!!

제가 생각한 방법은 따로 Spring Security 에서 제공해주는 CustomAuthenticationEntryPoint, CustomAccessDeniedHandler 를 사용하지 않고, JwtAuthenticationFilter.unsuccessfulAuthentication() 메소드에서 BusinessException 을 상속한 별도의 Exception 으로 throw 하게 되면 에러 처리에 대한 포인트를 하나로 모을 수 있을것 같다는 생각이 문득 들어서요!!

@seonpilKim @dongkyunkimdev 이 부분에 대해서 함께 논의해보고 싶습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

엇 혹시 filter에서 발생한 예외도 Advice가 처리할 수 있을까요?
저는 Advice를 이용한 예외 처리는 DispatcherServlet에서 담당하는 것으로 알고 있었는데, 제가 잘못 알고 있는 부분이 있다면 지적 부탁드려도 괜찮을까요?!

Copy link
Member

@dongkyunkimdev dongkyunkimdev Jul 2, 2022

Choose a reason for hiding this comment

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

제가 알기로 Filter는 Web Application Context 상에서 동작해서 선필님이 말씀해주신 것처럼 context가 달라 advice에서 처리하는 에러 핸들링이 불가능한 것으로 알고 있습니다!
찬준님이 말씀해주신 것처럼 unsuccessfulAuthentication 에서 CustomException을 던져보았는데, advice에서 받지 못하고 기본으로 제공되는 error response가 발생하는 것 같더라구요
filter에서 발생한 예외를 advice에서 받을 수 있게 하는 어떤 방법이 있는걸까요??
image

Copy link
Contributor

Choose a reason for hiding this comment

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

훔... 그렇군요!! 왜 된다고 생각했는지 모르겠는데 좀더 알아볼걸 그랬네요 ㅠ__ㅠ
감사합니다 ㅎㅎㅎ

Copy link
Member

Choose a reason for hiding this comment

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

저도 개념, 기술적으로 모르거나 애매하게 아는 부분이 엄청 많아서, 이런 논의들을 하면서 모르는 것도 알아가고, 애매하게 아는 것은 더 확실하게 알게 되니까 되게 좋은 것 같습니다!!
감사합니다 찬준님 선필님 👍👍

if (registrationId.equals(OAuth2Attributes.KAKAO)) {
isFirstLogin = kakaoAccountRepository.findById(id).isEmpty();
} else {
if (registrationId.equals(OAuth2Attributes.NAVER)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

registrationId 에 대한 부분이 OAuth2Attributes 클래스로 추상화 되는 방향도 괜찮을듯 합니다!
그렇게 되면 Provider 가 추가되거나, 삭제되었을 때 변경점이 OAuth2Attributes 클래스 하나로 집중되어서 좀 더 결합도를 낮추고 응집도가 높은 설계일듯 합니다 :)

코틀린으로 작성된 코드긴하지만 참고할만한 코드를 첨부해봅니다!

fun of(userRequest : OAuth2UserRequest, oAuth2User : OAuth2User) : OAuth2Attributes {
      val provider = userRequest.clientRegistration.registrationId
      val userNameAttributeName = userRequest.clientRegistration.providerDetails.userInfoEndpoint.userNameAttributeName

      return when(provider) {
           KAKAO -> ofKakao(provider, userNameAttributeName, oAuth2User.attributes,
               oAuth2User.attributes[userNameAttributeName] as Long
           )
           else -> throw java.lang.IllegalArgumentException()
      }
}
val oAuth2User = defaultUserService.loadUser(userRequest)
val oAuth2Attributes = OAuth2Attributes.of(userRequest, oAuth2User)

val member: Member = if (!oAuth2Repository.existsById(oAuth2Attributes.oauthId)) {
      oAuth2Repository.save(oAuth2Attributes)
      memberService.signupWithOAuth2(oAuth2Attributes.email, oAuth2Attributes.nickname, "", oAuth2Attributes.provider)
} else {
      memberRepository.findByEmail(oAuth2Attributes.email)!!
}

Copy link
Member Author

Choose a reason for hiding this comment

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

사실 저도 카카오 부분을 빼는 과정에서 느꼈는데, 변경점이 여러 군데 있으니까 확실히 불편하더라구요!
변경점을 한 곳으로 옮기는 것도 좋은 방법인 것 같습니다!

다만 DB에 접근하는 코드가 OAuth2Attributes 클래스에 존재한다는 게 조금 걸리기도 하고,
현재 코드가 Provider가 추가/삭제 될 때마다 DB의 테이블을 추가/삭제해야 하는 방식이라, 조금 비효율적인 것 같기도 해요!
그래서 생각난 게, 하나의 테이블(ex: oauth2_accounts)로 통합하고, provider 필드를 추가하는 건 어떨까요?
이렇게 변경하면 코드를 다음과 같이 수정할 수 있을 것 같습니다!

+ final boolean isFirstLogin = oauth2AccountRepository.findByIdAndProvider(id, registrationId).isEmpty();
- final boolean isFirstLogin;
- if (registrationId.equals(OAuth2Attributes.NAVER)) {
-     isFirstLogin = naverAccountRepository.findById(id).isEmpty();
- } else {
-     throw new UnsupportedPlatformSignInException();
- }

private Member signup(String provider, Long id, String email, String phone) {
-    final Member member;
    final String password = passwordEncoder.encode(UUID.randomUUID().toString());
    final Member member = memberService.createMember(email, phone, "", password, providerId)
-    if (provider.equals(OAuth2Attributes.NAVER)) {
-       member = memberService.createMember(email, phone, "", password, JoinType.NAVER);
+       member = memberService.createMember(email, phone, "", password, JoinType.valueOf(provider));
-        naverAccountRepository.save(new NaverAccount(id, member));
+       oauth2AccountRepository.save(new Oauth2Account(id, provider, member));
-    } else {
-        throw new UnsupportedPlatformSignInException();
-    }

    return member;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

음 DB 에 대한 부분은 Service 에 둔 상태의 코드를 말씀드린건데, 제가 개인적으로 작성했던 프로젝트 코드 한번 확인 부탁드립니다!!
https://github.com/Free4Developers/WebSocket_Backend/tree/main/src/main/kotlin/com/free4developer/sampleserver/domain/oauth2

Copy link
Contributor

Choose a reason for hiding this comment

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

아 그리고 oauth2_accounts 이런식으로 합치는건 좋은 방법인것 같아요!! 👍 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

아하 확인했습니다! 감사합니다😸

@seonpilKim
Copy link
Member Author

@slolee @dongkyunkimdev

소셜 계정 연동 관련 의견

일반 회원 가입을 한 사용자가 나중에 소셜 계정을 연동해서 간편 로그인을 할 수 있도록 설계하면 좋을 것 같다는 생각이 들었습니다!

image

  • 소셜 계정 테이블을 하나로 합침에 따라, members 테이블에서 join_type 컬럼 삭제
  • (sns_account_id, sns_type) 기본 키를 복합 키로 사용
    • sns_account_id: 소셜 로그인 시 얻을 수 있는 기본 키
  • 소셜 계정으로 회원 가입하는 경우 username은 user_System.currentTimeMillis() 으로 설정

    ex) user_1641881398379

@slolee
Copy link
Contributor

slolee commented Jul 3, 2022

@seonpilKim 좋은 의견인것 같습니다! 동의합니다 👍 👍 👍
sns_type 컬럼명을 sns_provider 혹은 sns_provider_type 과 같은 이름으로 바꾸는건 어떻게 생각하시나요!!
개인적으로 oauth 개념상 provider 라는 이름이 좀더 명확하게 느껴지는것 같아서요!!

@seonpilKim
Copy link
Member Author

provider 좋습니다👍

대부분 Entity들이 공통으로 사용하는 createdAt, updatedAt을 MappedSuperclass으로 묶어서 상속 받는 방식으로 리팩토링
Member 엔티티 수정 사항
- JoinType 제거

See Also: #20
@slolee slolee merged commit ca4b000 into develop Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✈ Feature 새로운 기능 추가/변경/삭제 ☄ Refactor 코드 리팩토링 ✏ Review request 코드 리뷰 요청
Projects
None yet
Development

Successfully merging this pull request may close these issues.

시큐리티 관련 코드 리팩토링 카카오 로그인 방식 제거
3 participants