refactor: memberService에서 인증관련 로직 분리 #878

coli-geonwoo · 2024-11-02T10:54:03Z

🚩 연관 이슈

close #877

📝 작업 내용

refactor: Auth 인증 로직을 memberService로부터 분리 #877

🏞️ 스크린샷 (선택)

🗣️ 리뷰 요구사항 (선택)

노션 페이지에서도 볼 수 있습니다.

우선 서로 소통적인 부분에 있어 미리 리팩터링할 부분들을 공유하지 않고 진행했다는 점에 대해서는 양해 부탁드립니다 🙇 다만 제 입장에서는 다들 DH, 우형 채용일정으로 더 큰 태스크를 앞두고 있어 이와 관련한 협의 시간을 물어보기가 쉽지 않은 상황이었습니다.

개선하고 싶었던 부분 : Auth 패키지

문제1. Token validate 과정에서 중복 로직이 존재합니다.

토큰이 만료되었는지 확인하는 과정에서 중복 코드가 존재합니다.

JwtTokenProvider 코드

   public void validate(AccessToken accessToken) {
        if (!isUnexpired(accessToken)) {
            throw new OdyUnauthorizedException("만료된 액세스 토큰입니다.");
        }
    }

    public void validate(RefreshToken refreshToken) {
        if (!isUnexpired(refreshToken)) {
            throw new OdyUnauthorizedException("만료된 리프레시 토큰입니다.");
        }
    }

    public boolean isUnexpired(AccessToken accessToken) {
        ... 중략...
    }

    public boolean isUnexpired(RefreshToken refreshToken) {
        ... 중략...
    }

문제2. 인증 관련 정책이 MemberService 에 깊게 침투하여 있음

현재 인증 상황은 5가지 경우의 수로 나뉘어 지게 되는데요.

상황1.  신규 유저가 새로운 기기로 로그인
상황2. 신규 유저가 로그인 이력이 있는 기기로 로그인
상황3. 기존 유저가 본인이 사용하던 기기로 재로그인
상황4.  기존 유저가 새로운 기기로 로그인
상황5.  기존 유저가 다른 유저의 기기로 로그인

현재 코드는 다음과 같습니다.

   @Transactional
    public Member save(Member requestMember) {
        Optional<Member> findMember = memberRepository.findByDeviceToken(requestMember.getDeviceToken());

        if (findMember.isPresent()) {
            Member sameDeviceTokenMember = findMember.get();
            if (sameDeviceTokenMember.isSame(requestMember.getAuthProvider())) {
                return sameDeviceTokenMember;
            }
            sameDeviceTokenMember.updateDeviceTokenNull();
        }
        return saveOrUpdateByAuthProvider(requestMember);
    }
    
     private Member saveOrUpdateByAuthProvider(Member requestMember) {
        Optional<Member> findMember = memberRepository.findByAuthProvider(requestMember.getAuthProvider());
        if (findMember.isPresent()) {
            Member sameAuthProviderMember = findMember.get();
            sameAuthProviderMember.updateDeviceToken(requestMember.getDeviceToken());
            return sameAuthProviderMember;
        }
        return memberRepository.save(requestMember);
    }

제가 생각한 문제는 다음과 같아요.

상황 파악이 잘 안됨

각 상황이 어떤 상황인지 트래킹이 쉽지가 않았어요
예를 들어서 이제 부터 1인당 기기를 두개 이상 로그인할 수 있도록 변경해주세요! 라는 요구사항이 오면 기존 유저가 새로운 기기/ 타 유저 기기로 로그인 하는 상황을 특정해야 하는데 분기문을 계속 따라가야 하는 어려움이 있습니다.
또한 최종 데모데이 때 4기 선배님께서 인증 관련 코드가 코드 상으로 명확히 이해되지 않는다는 피드백을 받았습니다.

순서에 의존한 로직 처리

private method로 분리해놓아서 그렇지 사실상 순차적인 분기처리를 4번 해준 구조이고, 순서가 의존되어 있어 두 분기의 순서를 바꾼다면 처리가 다르게 됩니다.

MemberService에서 인증 정책을 분리할 필요성

각 인증 상황에 따라 deviceToken을 업데이트해주거나 이전하는 것은 인증 정책이라 생각하는데요.
현재는 MemberService내에 인증 정책이 깊게 침투해 있어 순수한 save가 안됩니다.
이는 save에 Member를 넣으면 진짜 내가 넘긴 Member가 저장될지 안될지를 외부에서 모르는 상황이라고도 볼 수 있을 것 같아요. 따라서 인증 관련 로직은 AuthService 내에만 있도록 리팩터링하고 싶었어요.

어떻게 개선했는가?

개선1) 인증 상황별 후속 처리를 다형성으로 처리

5가지로 구분된 각 인증 상황을 AuthorizationType이라는 인터페이스로 만들었습니다.

public interface AuthorizationType {

    boolean match(
            Optional<Member> sameDeviceMember,
            Optional<Member> samePidMember,
            Member requestMember
    );

    Member authorize(
            Optional<Member> sameDeviceMember,
            Optional<Member> samePidMember,
            Member requestMember
    );
}

match : 각 인증 상황인지 판단
authorize : 인증 후속 처리 후, 인증 대상 반환

그리고 각 상황 별로 객체를 만들어주었어요.

예를 들어 기존 유저가 기존 로그인 기기로 로그인 시도를 한 경우는 ExistingUserForExisintgDevice로 만들어주었습니다.

@Component
public class ExistingUserForExistingDevice implements AuthorizationType {

    @Override
    public boolean match(Optional<Member> sameDeviceMember, Optional<Member> samePidMember, Member requestMember) {
        return sameDeviceMember.isPresent()
                && samePidMember.isPresent()
                && requestMember.isSame(sameDeviceMember.get());
    }

    @Override
    public Member authorize(Optional<Member> sameDeviceMember, Optional<Member> samePidMember, Member requestMember) {
        return samePidMember.get();
    }
}

그리고 Authorizer가 각 AuthorizationType을 List 형태로 빈으로 주입받고 인증 상황을 판단하여 인증 후속 처리를 하도록 하였어요

@Component
@RequiredArgsConstructor
public class Authorizer {

    private final List<AuthorizationType> authorizationTypes;

    public Member authorize(
            Optional<Member> sameDeviceMember,
            Optional<Member> samePidMember,
            Member requestMember
    ) {
        return authorizationTypes.stream()
                .filter(type -> type.match(sameDeviceMember, samePidMember, requestMember))
                .findAny()
                .orElseThrow(() -> new OdyUnauthorizedException("잘못된 인증 요청입니다."))
                .authorize(sameDeviceMember, samePidMember, requestMember);
    }
}

이를 통해 얻고자 한 가치는 다음과 같아요.

관리의 응집성 : 각 상황별 관리가 가능합니다. 예를 들어 이제부터 1인당 2개 기기 접속 가능 이라고 한다면 해당되는 AuthorizationType에 들어가 기존 기기 사용자의 디바이스 토큰을 널처리하는 로직만 빼면 됩니다.
확장성 : 새로 추가되는 인증 상황은 스펙에 맞추어 인터페이스를 구현해주기만 하면됩니다. 반대로 막고자 하는 인증 상황을 뺄 수도 있어요.
- 예를 들어 현재는 타 유저의 기기로 이전 시 알람관련 후속 처리를 안해놓아서 이전 사용자의 알람을 그대로 받아요. 이 상황을 우리 서비스의 인증 정책에서 제외하려한다면, authorize 메서드에서 에러를 터트리면 됩니다.

단점이라 생각되는 것

각 인증 상황 처리 로직이 파편화되어 있다고 느낄 수 있다. : 객체들을 각 상황별로 처리하다 보니 상황별 인증 후속 처리를 한눈에 파악하기 힘들다고 느낄 수 있을 것 같아요.
확장성이 없다고 느낄 수 있다 : 5가지 상황 이상으로 인증 정책의 변화가 없다고 생각할 수 있을 것 같아요.
Optional로 인해 로직의 가정이 많다. : Optional의 경우 계속 get() 하고 써야 하는 경우가 많다보니 이런 로직이 불편하다고 느낄 수 있을 것 같아요.

개선2) Token으로 추상화하기

액세스 토큰과 리프레시 토큰을 token이라는 인터페이스로 추상화했어요.

package com.ody.auth.token;

import com.ody.auth.AuthProperties;

public interface Token {

    String getSecretKey(AuthProperties authProperties);

    String getValue();
}

그리고 각 token 처리를 추상화한 Token 인터페이스로 다루어주었습니다.

여기서 getSecretKey를 만들어 주고 AuthProperties를 넘겨준 이유는 각 Token에 AuthProperties를 인스턴스 필드로 가지고 있는 이유가 어색하게 느껴지고 관리포인트를 AuthService 하나로 축약하고 싶었기 때문이에요.

리팩터링이 더 필요하다 생각되는 부분

하나의 PR이 너무 커질 것 같아 생략한 부분들입니다. 추후 시간이 되면 다듬어야 할 것 같아요.

기존 유저가 타 유저로 기기 이전시 알람 처리

A유저가 B유저 핸드폰으로 이전하는 경우 인증과정 상황에서 그 어떤 알람 해제 처리도 없어서 B유저가 받아야 했던 알람을 그대로 A유저가 받게 됩니다.

validate를 parseAccessToken 내부로 옮겨주기

현재 validate는 토큰의 유효기간이 지났는지 확인하는 역할을 맡고 있어요. 그런데 현재 validate가 필요한 경우는 파싱하여 memberId를 추출하기 전에 항상 붙어다닙니다.

public Member parseAccessToken(String rawAccessToken) {
        AccessToken accessToken = new AccessToken(rawAccessToken);
        **jwtTokenProvider.validate(accessToken);
        long memberId = jwtTokenProvider.parseAccessToken(accessToken);**
        return memberService.findById(memberId);
}

@Transactional
public AuthResponse renewTokens(String rawAuthorizationHeader) {
        .... 중략 ...

        **jwtTokenProvider.validate(refreshToken);
        long memberId = jwtTokenProvider.parseAccessToken(accessToken);**
        checkAlreadyLogout(memberId);
        return issueNewTokens(memberId);
}

@Transactional
public void logout(String rawAccessTokenValue) {
        AccessToken accessToken = new AccessToken(rawAccessTokenValue);
        **jwtTokenProvider.validate(accessToken);
        long memberId = jwtTokenProvider.parseAccessToken(accessToken);**
        memberService.updateRefreshToken(memberId, null);
 }

그럼 validate를 pulbic으로 열어두지 말고 parseAccessToken 로직 안으로 넣는 것이 자연스럽지 않은가 생각했어요. 테스트 코드를 보면 만료된 토큰이더라도 파싱이 가능하도록 각 메서드별로 순수함을 유지하려 했던 것 같은데 현재는 만료된 토큰을 파싱하는 로직이 전무하고, 이렇게 될 경우 파싱을 할 때마다 외부에서 validate를 선제적으로 호출해야 하는 규칙을 지켜주어야 한다는 단점이 있을 것 같아요.

github-actions · 2024-11-02T10:56:01Z

Test Results

184 tests 184 ✅ 5s ⏱️
52 suites 0 💤
52 files 0 ❌

Results for commit a4e1e28.

♻️ This comment has been updated with latest results.

github-actions · 2024-11-02T10:59:18Z

📝 Test Coverage Report

Overall Project	81.49% `-0.09%`	🍏
Files changed	97.55%	🍏

File	Coverage
ExistingUserForExistingDevice.java	100%	🍏
NewUserForNewDevice.java	100%	🍏
ExistingUserForNewDevice.java	100%	🍏
NewUserForExistingDevice.java	100%	🍏
MemberService.java	100%	🍏
Member.java	97.67%	🍏
AuthService.java	96.99%	🍏
OtherUserForExistingDevice.java	96.97% `-3.03%`	🍏
JwtTokenProvider.java	96.34%	🍏
AccessToken.java	91.23%	🍏
Authorizer.java	85.29% `-14.71%`	🍏
RefreshToken.java	58.43%	🍏

coli-geonwoo · 2024-11-02T11:00:03Z

backend/src/test/java/com/ody/route/repository/ApiCallRepositoryTest.java

@@ -47,7 +47,7 @@ void findFirstByDateBetweenAndClientType() {
        apiCallRepository.save(secondDateApiCall);
        apiCallRepository.save(firstDateApiCall);

-        Optional<ApiCall> actual = apiCallRepository.findFirstByDateBetweenAndClientType(firstDay, now, clientType);
+        Optional<ApiCall> actual = apiCallRepository.findFirstByDateBetweenAndClientType(firstDay, firstDay.plusDays(10), clientType);


이 테스트에서 에러가 터져서 수정했어요.

조회 기간인 now가 첫 호출 일자인 firstDay + 3일보다 앞에 있는 경우 에러가 터집니다.

ex)
now : 11월 2일
첫 호출일 : 11월 1일 +3 = 11월 4일
=> 11월 1일부터 2일까지 조회하면 아무런 조회결과가 안 나옴!

kimhm0728

와 진짜 야무지게 정리 잘하시네요 콜리 ㄷㄷ
approve 드립니다 ㅋ

hyeon0208

PR로 상황을 잘 설명해주셔서 더 이해가 빨랐어요 고마워요 코로로로롤리 ~~ 👍
깔끔하게 개선해주신 것 같아요 👍
조금 의문이 생기는 부분들이 있어 몇 가지 의견 남겨봅니다 😄

hyeon0208 · 2024-11-04T09:17:20Z

backend/src/main/java/com/ody/auth/token/Token.java

+
+import com.ody.auth.AuthProperties;
+
+public interface Token {


[제안]
현재 네이밍으로 봐서는 Device 토큰도 이를 구현할 수 있을 것 처럼 보이는데
ISP 원칙을 적용해서 좀 더 구체적이게 JwtToken 으로 네이밍 해주는 건 어떤가요 ?

동!의!합니다

✅ 반영완료

deviceToken의 맥락을 생각치 못했네요! 저도 동의합니다~

hyeon0208 · 2024-11-04T09:35:11Z

backend/src/main/java/com/ody/auth/token/Token.java

+    String getSecretKey(AuthProperties authProperties);
+
+    String getValue();


[제안]
Token의 구현체마다 해당 메서드 로직이 완전 동일한데 default 메소드로 중복 구현을 제거해도 좋을 것 같아요
아니면 AuthProperties를 내부 필드로 갖고 있게해 공통 메서드를 제공하는 추상 클래스로 만들어도 좋을 것 같은데
어떻게 생각하시나요??

Token의 구현체마다 해당 메서드 로직이 완전 동일한데

getSecretKey()는 구현 로직이 달라요!

+)
[질문]

Token에 AuthProperties를 인스턴스 필드로 가지고 있는 이유가 어색하게 느껴지고

AuthProperties를 내부 필드로 갖는게 전 어색하지 않아요. AccessToken이 자신의 refreshKey와 refreshExpiration를 알고 있는게 더 자연스럽지 않나요? 다른 분들은 어떻게 생각하시나요?

도메인 객체 관점에서는 자연스러운 것 같아요. 또 내부 필드값으로 가지는 것을 고려하지 않은 것은 아닙니다. 다만 2가지 우려가 있었어요.

1. refreshToken은 member가 가진 embedded 필드값입니다.

그래서 authProperties를 가진 경우 @transient를 통해 임의값 처리를 추가해주어야 하고 db 관점으로 다룰 때도 계속 주의해주어야 해요.

2. authProperties가 null처리되어야 하는 생성자들이 있었어요.
현재 AuthorizationHeader 객체에서 accessToken을 생성하여 넘겨주는데요.

이 경우 accessToken을 클라이언트가 넘긴 값 그대로 파싱하여 초기화하기 때문에 controller단에서 accessToken이 생성되어 넘어와요. 이 경우에는 authProperties 필드가 null로 초기화될 수 밖에 없었습니다.

public AccessToken(String rawValue) { validate(rawValue); this.value = parseAccessToken(rawValue); this.authProperties = ???? <- 이 때는 null로 초기화될 수 밖에 없음.. } private void validate(String value) { if (!value.startsWith(ACCESS_TOKEN_PREFIX)) { throw new OdyBadRequestException("잘못된 액세스 토큰 형식입니다."); } } private String parseAccessToken(String rawValue) { return rawValue.substring(ACCESS_TOKEN_PREFIX.length()).trim(); }

필드값으로 넣는 것은 자연스러워 보이지만 이런 부분들에서 조금은 어색한 부분들이 있는 것 같아서 의견을 조금 묻고 싶어요.

hyeon0208 · 2024-11-04T09:38:25Z

backend/src/main/java/com/ody/auth/domain/authorizeType/AuthorizationType.java

+import com.ody.member.domain.Member;
+import java.util.Optional;
+
+public interface AuthorizationType {


[제안]
AuthorizationType 네이밍이 어떤 식으로 활용가능한 인터페이스인지 잘 그려지지 않는 것 같은데
'인증 방법', '인증 정책' 같은 네이밍은 어떨까요 ??
변경한다면 구현체 네이밍들도 조건 분기 처리를 담은 private 메서처럼 느껴져서 같이 변경하면 좋을 것 같습니다 😄

[제안]
네이밍 변경에 동의해요. Type 접미사가 붙으면 enum 같이 느껴지는 것 같네요.
AuthorizationPolicy, AuthPolicy, AuthStrategy 이런 네이밍은 어떤가요?

✅ 반영 완료

AuthPolicy가 마음에 들어요 😄

hyeon0208 · 2024-11-04T09:44:42Z

backend/src/main/java/com/ody/auth/domain/Authorizer.java

+@RequiredArgsConstructor
+public class Authorizer {
+
+    private final List<AuthorizationType> authorizationTypes;


[제안]
authorize()에서 AuthorizationType 구현체들을 하나씩 사용하고 DI를 편리하게 적용하기 위해 @component를 사용하신 것 같은데
개인적으로는 조건식을 담는 private 메서드들이 별도의 클래스로 분리된 것 처럼 느껴져서 응집도가 떨어진 느낌을 받았어요

두 방법 모두 고민한 것 같은데 한가지 제안 드려보면
인증의 경우 한 번에 하나의 인증 로직을 태울꺼 같아서
전략 패턴을 적용해 현재 분산되어 있는 인증로직들을 하나의 전략 구현체안에 두고
추후 새로운 인증 방식이 생길 경우 새로운 전략 구현체를 추가해 유연하게 바꿔끼게 하는 방식은 어떻게 생각하실까요 ?

카키.. 이 부분은 제가 명확하게 이해하지 못한 것 같아서 조금 더 풀어서 설명 부탁드려도 될까요? 😭

그런데 몇가지 부분에 대해서 제가 이해한 대로 답변을 남기면, 카키가 말한대로 각 인증 로직이 파편화되어 흩어져있다고 느낄 수 있을 것 같아요.

처음으로 시도했던 구현은 카키가 제안한대로 현재 분산되어 있는 인증로직들을 하나의 전략 구현체안에 두고 였는데요. EtaStatus mapping 처럼 mapper를 두고 각 인증을 하나의 구현체에서 모두 처리하도록 했어요.

그런데 현재 5가지 분기를 계속 if문을 써가면서 분기해주어야 했고 인증의 5가지 상황 판단 > 상황별 인증 과정 로직이 너무 두꺼워서 하나의 전략 구현체 안에 담기가 힘들었습니다..

또, 각 상황에 대한 트래킹을 하고 싶다는 수요가 컸어요. 예를 들어 새로운 유저가 새로운 기기로 로그인 하는 상황 에서의 인증 처리를 바로 찾고 싶었습니다. 그런데 하나의 구현체에서 분기를 통해 처리하게 되면 분기문을 계속 트래킹 해야하는 부담이 있어서 최대한 나누어 보았습니다.

제가 카키 리뷰의 의도를 정확히 이해하지 못한 것 같은데 의견 남겨주시면 합리적이라 생각하는 방향으로 최대한 반영해보도록 할게요!

제가 말한 내용을 잘 이해해서 설명해주신 것 같아요 👍

결론부터 말하면 절차에 대한 처리인지 인증 방식에 대한 처리인지 헷갈렸던 것 같아요
위와 같은 생각을 했던 이유는
Authorizer에서 List로 관리해 각 구현체들을 순회하고 구현체 명들이 분기처리 메서드 같이 느껴져 책임 연쇄 패턴인가 ?,
구현체의 조건만 만족하면 바로 인증을 해주니 전략 패턴이네?와 같이 조금 햇갈렸던 것 같아요

여기서 콜리가 얻고자한 확장성이 '인증 절차에 대한 확장성', '인증 방식에 대한 확장성' 두가지로 나뉠 것 같은데
각각의 구현체 조건을 만족하면 인증을 해주므로
콜리가 얻고자한 부분은 '인증 방식에 대한 확장성'인 것 같으니 지금의 구현이 적절할 것 같습니다 👍

eun-byeol

콜리 고생하셨어요!

전략 패턴을 잘 적용해주셨네요! 객체지향에 진심인 콜리.. 인증쪽 로직을 잘 몰랐는데, 덕분에 더 명확하게 알 수 있었어요. 물론, Authorizer의 authorize() 메서드에서 match 내부 검증 로직이 중복(isPresent(), isSame() 과 같은 로직 중복 호출하는 것)되지만, 어떤 정책을 검증하는지 분명하고 순서에 의존하지 않아서 좋아요. 결론 = 합리적이다

논외로, 한 가지 물어보고 싶은 점은
이제부터 1인당 2개 기기 접속 가능 이라고 한다면 해당되는 AuthorizationType에 들어가 기존 기기 사용자의 디바이스 토큰을 널처리하는 로직만 빼면 됩니다. 확장된 요구사항에 대해 언급해주셨는데, 실제 고려하고 언급한 부분은 아니겠죠?
AuthorizationType 로직만 보면 간단하지만, 관련된 조회 로직 다수가 함께 변경(디바이스 토큰 조회시 회원 1개만 조회되는 것이 보장X)되어야 하는 부분이라 저는 쉽지 않은 정책이라 보았어요.

답변 확인하고싶어서 RC 남길게요~

eun-byeol · 2024-11-04T09:58:46Z

backend/src/main/java/com/ody/auth/domain/Authorizer.java

+
+    public Member authorize(
+            Optional<Member> sameDeviceMember,
+            Optional<Member> samePidMember,


[제안]
Pid가 통용되는 표현이면 무시하셔도 됩니다.
sameProviderIdMember 축약어 사용 안 하는 건 어떤가요? 길어지긴 하지만, 처음 봤을때 이해가 한 번에 되지 않았어요.

✅ 반영 완료!

Authorizer의 authorize() 인자에는 반영이 안 되어 있어요!

eun-byeol · 2024-11-04T10:05:12Z

backend/src/main/java/com/ody/auth/domain/authorizeType/AuthorizationType.java

+import com.ody.member.domain.Member;
+import java.util.Optional;
+
+public interface AuthorizationType {


[제안]
네이밍 변경에 동의해요. Type 접미사가 붙으면 enum 같이 느껴지는 것 같네요.
AuthorizationPolicy, AuthPolicy, AuthStrategy 이런 네이밍은 어떤가요?

eun-byeol · 2024-11-04T10:29:25Z

backend/src/main/java/com/ody/auth/domain/authorizeType/ExistingUserForNewDevice.java

+    @Override
+    public Member authorize(Optional<Member> sameDeviceMember, Optional<Member> samePidMember, Member requestMember) {
+        Member sameAuthProviderMember = samePidMember.get();
+        sameAuthProviderMember.updateDeviceToken(requestMember.getDeviceToken());


[의견]
Authorizer에서 @Transactional 명시해서 트랜잭션을 명확히 해주면 어떤가요? 단, 외부에서 readOnly=true로 열리는 경우를 유의해야 할 것 같긴 합니다.(REQUIRED_NEW 방법도 있지만, 롤백 범위가 달라진다는게 단점이네요)

트랜잭션이 열려있다는 가정하에, update 쿼리가 날아가요. 변경감지를 사용하는 쪽과 트랜잭션을 여는 부분이 멀어질수록 실수 여지가 있을 것 같아요.
지금은 AuthService에서 트랜잭션을 열고 호출하고 있어 문제가 안 되지만, 다른 쪽에서 얼마든지 Authorizer를 사용할 수도 있을 것 같아요. Authorizer에 변경감지를 사용하는 로직이 있다는 것을 모르면, @Transactional을 안 열 수도 있을 것 같아요.

✅ 반영 완료

좋은 포인트의 리뷰네요 조조! 트랜잭션 문제로 겪은 여러 문제들도 있고요!

반영해주었습니다.

eun-byeol · 2024-11-04T10:30:46Z

backend/src/main/java/com/ody/auth/domain/authorizeType/ExistingUserForNewDevice.java

+            Optional<Member> samePidMember,
+            Member requestMember
+    ) {
+        return sameDeviceMember.isEmpty() && samePidMember.isPresent();


[필수] 아래 로직들과 통일성 맞춰주면 좋을것 같아요ㅎㅎ

Suggested change

return sameDeviceMember.isEmpty() && samePidMember.isPresent();

return sameDeviceMember.isEmpty()

&& samePidMember.isPresent();

✅ 반영 완료

eun-byeol · 2024-11-04T10:37:47Z

backend/src/main/java/com/ody/auth/token/Token.java

+
+import com.ody.auth.AuthProperties;
+
+public interface Token {


동!의!합니다

eun-byeol · 2024-11-04T10:47:29Z

backend/src/main/java/com/ody/member/service/MemberService.java

+    public Member save(Member member) {
+        return memberRepository.save(member);


👍 이 한줄을 위해 달려왔 ㄸ ㅏ..

eun-byeol · 2024-11-04T10:48:08Z

backend/src/test/java/com/ody/auth/JwtTokenProviderTest.java

@@ -134,6 +134,7 @@ void checkRefreshTokenUnexpiredSuccess() {
        @DisplayName("만료된 리프레시 토큰이면 False를 반환한다.")
        @Test
        void checkRefreshTokenExpired() {
+


✅

Suggested change

✅ 반영 완료

에러가 나서 logging했다가 지웠는데 개행은 신경쓰지 못했네요 😓

eun-byeol · 2024-11-04T10:52:08Z

backend/src/test/java/com/ody/auth/domain/authorizeType/ExistingUserForNewDeviceTest.java

+                        .usingRecursiveComparison()
+                        .ignoringFields("deviceToken")


[질문]
deviceToken은 왜 제외하나요? other_dt로 업데이트 된거면 requestMember의 디바이스 토큰과 일치하지 않나요?

있어도 상관없지만 두 검증 메서드의 목적이 다르다 생각해서 제외해주었던 것 같아요. 디바이스 토큰 업데이트의 경우 위의 검증부에서 증명이 되었다 생각했고, 해당 검증에서는 deviceToken을 제외한 나머지 필드값을 비교하고 싶었어요. 그런데 오히려 혼란만 가중되는 느낌이라 제거해주었습니다 😄

eun-byeol · 2024-11-04T10:52:20Z

backend/src/test/java/com/ody/auth/domain/authorizeType/NewUserForExistingDeviceTest.java

+    void match() {
+        NewUserForExistingDevice authorizationType = new NewUserForExistingDevice();
+        Member originalMember = new Member("pid", new Nickname("콜리"), "imgUrl", new DeviceToken("dt"));
+        Member requestMember = new Member("pid2", new Nickname("조조"), "imgUrl2", new DeviceToken("dt"));


eun-byeol · 2024-11-04T10:56:59Z

backend/src/test/java/com/ody/auth/service/AuthServiceTest.java

@@ -25,6 +28,86 @@ class AuthServiceTest extends BaseServiceTest {
    @Autowired
    private MemberRepository memberRepository;

+


✅ 아직 린트 사용 안 하시나요?

Suggested change

네...ㅋㅋㅋㅋ 호다닥 사용해보겠습니다... 하하 😓

samePidMember > sameProviderIdMember

mzeong

description을 꼼꼼하게 작성해주셔서 코드 리뷰 하는데 도움이 많이 되었습니다~ 좀 더 객체지향에 가까운 코드가 된 것 같네요

추가로, description에 언급해주셨던 부분에 대해 코멘트 남깁니다.

validate는 토큰의 유효기간이 지났는지 확인하고 지났으면 에러를 발생시킵니다. renewTokens는 만료된 access token을 대상으로 갱신을 진행해요. 따라서 parseAccessToken 내로 validate를 넣게 되면 토큰이 갱신되어야 하는 상황에서 에러가 발생해 동작하지 않습니다. (예시 코드를 주셨는데, 다른 코드들과 다르게 renewTokens는 refresh token에 대한 validate를 진행하고, access token에 대한 parse를 진행하고 있습니다)

isUnexpired = 만료 확인, 잘못되면 에러 발생
validate = 만료되면 에러 발생, 잘못되면 에러 발생
parseXXXToken = 만료된 경우에도 파싱 가능

mzeong · 2024-11-05T02:03:22Z

backend/src/main/java/com/ody/auth/domain/authpolicy/AuthPolicy.java

+    boolean match(
+            Optional<Member> sameDeviceMember,
+            Optional<Member> sameProviderIdMember,
+            Member requestMember
+    );


[제안] 인터페이스랑 구현체에서 개행이 되었다 안 되었다 하네요?? 하나로 통일하면 좋을 것 같아요

mzeong · 2024-11-05T02:31:15Z

backend/src/main/java/com/ody/auth/domain/Authorizer.java

+
+    private final List<AuthPolicy> authPolicies;
+
+    @Transactional


이곳에 Transactional이 있는 게 바로 이해가 안 가긴 하네요 🤔🤔

mzeong · 2024-11-05T02:47:42Z

backend/src/main/java/com/ody/auth/domain/authpolicy/AuthPolicy.java

+    Member authorize(
+            Optional<Member> sameDeviceMember,


[제안] 현재 authorize 메서드가 수행하고 있는 동작이 개인적으론 권한 부여로 보이지 않는데, 아래와 같은 네이밍은 어떤가요?

applyDevicePolicy

syncDeviceTokenWithLoginState

updateDeviceTokenBasedOnLoginContext

manageDeviceTokenOnLogin

eun-byeol

간단한 코멘트 하나 남겼어요!
반영해주어 감사합니다. 고생했어요!

eun-byeol · 2024-11-05T03:13:15Z

backend/src/main/java/com/ody/auth/domain/Authorizer.java

+
+    public Member authorize(
+            Optional<Member> sameDeviceMember,
+            Optional<Member> samePidMember,


Authorizer의 authorize() 인자에는 반영이 안 되어 있어요!

coli-geonwoo added 6 commits November 2, 2024 19:14

refactor: token tagging interface로 로직 통일

d4b594f

refactor: AuthorizationType 구현체에 따른 분기 구현

955b5bc

refactor: 다형성을 통해 인증로직 구성

639d456

refactor: 불필요한 delimiter 삭제

a4a76cc

refactor: 분기된 메서드 다시 생성

52faa43

refactor: token으로 추상화

f847a27

coli-geonwoo added BE REFACTOR labels Nov 2, 2024

coli-geonwoo requested review from eun-byeol, mzeong and hyeon0208 November 2, 2024 10:54

coli-geonwoo self-assigned this Nov 2, 2024

coli-geonwoo linked an issue Nov 2, 2024 that may be closed by this pull request

refactor: Auth 인증 로직을 memberService로부터 분리 #877

Open

2 tasks

test: apiCall 테스트 조회 기간 수정

214fe46

coli-geonwoo commented Nov 2, 2024

View reviewed changes

kimhm0728 approved these changes Nov 3, 2024

View reviewed changes

hyeon0208 requested changes Nov 4, 2024

View reviewed changes

eun-byeol requested changes Nov 4, 2024

View reviewed changes

coli-geonwoo added 7 commits November 5, 2024 01:41

chore: token을 jwtToken으로 rename

71fe61a

chore: authPolicy로 rename

3c1e4e5

chore: 파라미터명 rename

9f953b6

samePidMember > sameProviderIdMember

refactor: authorize에 트랜잭션 추가

607517a

style: 개행 추가

4071ef3

style: 컨벤션 준수

8bdf327

test: 불필요한 필드 제외 삭제

a4e1e28

mzeong approved these changes Nov 5, 2024

View reviewed changes

eun-byeol approved these changes Nov 5, 2024

View reviewed changes

		String getSecretKey(AuthProperties authProperties);

		String getValue();

	return sameDeviceMember.isEmpty() && samePidMember.isPresent();
	return sameDeviceMember.isEmpty()
	&& samePidMember.isPresent();

		public Member save(Member member) {
		return memberRepository.save(member);

		@@ -25,6 +28,86 @@ class AuthServiceTest extends BaseServiceTest {
		@Autowired
		private MemberRepository memberRepository;

refactor: memberService에서 인증관련 로직 분리 #878

Are you sure you want to change the base?

refactor: memberService에서 인증관련 로직 분리 #878

Conversation

coli-geonwoo commented Nov 2, 2024

🚩 연관 이슈

📝 작업 내용

🏞️ 스크린샷 (선택)

🗣️ 리뷰 요구사항 (선택)

개선하고 싶었던 부분 : Auth 패키지

문제1. Token validate 과정에서 중복 로직이 존재합니다.

어떻게 개선했는가?

개선1) 인증 상황별 후속 처리를 다형성으로 처리

단점이라 생각되는 것

개선2) Token으로 추상화하기

리팩터링이 더 필요하다 생각되는 부분

github-actions bot commented Nov 2, 2024 • edited Loading

Test Results

github-actions bot commented Nov 2, 2024 • edited Loading

📝 Test Coverage Report

coli-geonwoo Nov 2, 2024 • edited Loading

Choose a reason for hiding this comment

kimhm0728 left a comment

Choose a reason for hiding this comment

hyeon0208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyeon0208 Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eun-byeol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzeong left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eun-byeol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 2, 2024 •

edited

Loading

github-actions bot commented Nov 2, 2024 •

edited

Loading

coli-geonwoo Nov 2, 2024 •

edited

Loading

hyeon0208 Nov 4, 2024 •

edited

Loading

mzeong left a comment •

edited

Loading