-
Notifications
You must be signed in to change notification settings - Fork 8
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
[BE] refactor: JWT에 권한에 따른 클레임을 담기 위해 인증 기능 개편 (#982) #985
Conversation
Test Results244 files 244 suites 29s ⏱️ Results for commit ebd3ce7. ♻️ This comment has been updated with latest results. |
@Override | ||
public boolean supportsParameter(MethodParameter parameter) { | ||
return parameter.getParameterType().equals(AdminAuthentication.class) && parameter.hasParameterAnnotation( | ||
Admin.class); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
굳이 parameter.hasParameterAnnotation(Admin.class)
으로 검사할 필요가 있을까 싶네요. 😂
애초에 권한이 맞지 않는 경우가 개발자가 실수로 핸들러 메서드에 Authorization(role = ADMIN)
을 해두고, 인자에 MemberAuhentication
을 받는 경우일텐데..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameter가 ~Authentication 으로 바뀌었으니 @Admin 과 같은 어노테이션은 없애도 되지 않을까요?
import org.springframework.util.Assert; | ||
import org.springframework.web.servlet.HandlerInterceptor; | ||
|
||
public class FixedAuthorizationInterceptor implements HandlerInterceptor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존 경로로 권한을 검증하는 기능을 위해 만들어진 인터셉터 입니다.
(어드민, 레거시)
public class AdminAuthentication implements Authentication { | ||
|
||
private final Long id; | ||
|
||
public AdminAuthentication(Long id) { | ||
if (id == null) { | ||
throw new UnexpectedException("id는 null이 될 수 없습니다."); | ||
} | ||
this.id = id; | ||
} | ||
|
||
@Override | ||
public Long getId() { | ||
return id; | ||
} | ||
|
||
@Override | ||
public Role getRole() { | ||
return Role.ADMIN; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어드민에도 역할이 있는데, ROOT와 일반 어드민의 구분입니다.
새로운 어드민 계정을 생성하는 것은 ROOT 어드민만 가능한데, 해당 Authentication
에 isRoot
라는 boolean 필드를 사용해서 ROOT 어드민 유무를 DB를 거치지 않고 확인할 수 있을 것 같네요.
private static final String ADMIN_ID_KEY = "adminId"; | ||
private static final long EXPIRATION_MINUTES = 60L * 24L; | ||
|
||
private final TokenProviderTemplate tokenProviderTemplate; | ||
|
||
public TokenResponse provide(AdminAuthentication adminAuthentication) { | ||
return tokenProviderTemplate.provide(EXPIRATION_MINUTES, | ||
jwtBuilder -> jwtBuilder | ||
.claim(ADMIN_ID_KEY, adminAuthentication.getId()) | ||
.audience().add(Role.ADMIN.name()).and() | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
식별자를 설정하는 클레임으로 memberId
, adminId
와 같이 key를 사용하고 있습니다.
해당 식별자를 OpenID와 같이 subject
에 할당하는게 어떤가 하네요.
(기능 머지되면 기존 사용자의 토큰에 null 값이 나오므로, 해당 값이 없을 경우 401 응답을 던져주는 임시 패치가 필요함)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매번 memberId, adminId 와 같이 저희가 설정한 값들에서 추출하는 것 보다는 일괄적으로 subject에 있는 것이 훨씬 직관적일 것 같네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우선 하위 호환을 보장하기 위해, Provider에서 Subject에도 식별자를 넣어주고, 머지가 끝나고 운영 서버에 올라간 뒤 엑세스 토큰이 만료되는 시간(360분)이 끝나면 Extractor에서 Subject로 Authentication 객체를 반환해주면 될 것 같네요!
@Component | ||
@RequiredArgsConstructor | ||
public class CompositeAuthenticationTokenExtractor implements AuthenticationTokenExtractor { | ||
|
||
private final JwtTokenParser jwtTokenParser; | ||
private final List<AuthenticationClaimsExtractor> authenticationClaimsExtractors; | ||
|
||
@Override | ||
public Authentication extract(String token) { | ||
Claims claims = jwtTokenParser.getClaims(token); | ||
for (AuthenticationClaimsExtractor claimsExtractor : authenticationClaimsExtractors) { | ||
Authentication authentication = claimsExtractor.extract(claims); | ||
if (authentication.getRole() != Role.ANONYMOUS) { | ||
return authentication; | ||
} | ||
} | ||
return AnonymousAuthentication.getInstance(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Composite
패턴을 사용한 AuthenticationTokenExtractor
입니다.
AuthenticationTokenExtractor
에서 토큰을 Authentication
객체로 바로 반환하지 않고, AuthenticationClaimsExtractor
을 사용한 이유이기도 한데, 전자로 구현하면 매 시도마다 불필요하게 토큰을 파싱하기 때문입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claims claims = jwtTokenParser.getClaims(token);
이 부분의 중복을 제거하기 위해서 claim 추출과 AuthenticationClaims 추출을 분리했다는 뜻 맞을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 그렇습니다!
@Component | ||
public class TokenProviderTemplate { | ||
|
||
private final SecretKey secretKey; | ||
private final Clock clock; | ||
|
||
public TokenProviderTemplate( | ||
@Value("${festago.auth-secret-key}") String secretKey, | ||
Clock clock | ||
) { | ||
this.secretKey = Keys.hmacShaKeyFor(secretKey.getBytes(StandardCharsets.UTF_8)); | ||
this.clock = clock; | ||
} | ||
|
||
public TokenResponse provide(long expirationMinutes, UnaryOperator<JwtBuilder> template) { | ||
Instant now = clock.instant(); | ||
Instant expiredAt = now.plus(expirationMinutes, ChronoUnit.MINUTES); | ||
JwtBuilder builder = Jwts.builder() | ||
.expiration(Date.from(expiredAt)) | ||
.issuedAt(Date.from(now)) | ||
.signWith(secretKey); | ||
template.apply(builder); | ||
String accessToken = builder.compact(); | ||
return new TokenResponse(accessToken, LocalDateTime.ofInstant(expiredAt, clock.getZone())); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
권한에 따른 Token을 생성할 때, 중복되는 토큰 생성을 피하기 위해 템플릿 메서드 패턴을 사용했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다 글렌 이해하기가 수월했네요 👍
@Component | ||
@RequiredArgsConstructor | ||
public class CompositeAuthenticationTokenExtractor implements AuthenticationTokenExtractor { | ||
|
||
private final JwtTokenParser jwtTokenParser; | ||
private final List<AuthenticationClaimsExtractor> authenticationClaimsExtractors; | ||
|
||
@Override | ||
public Authentication extract(String token) { | ||
Claims claims = jwtTokenParser.getClaims(token); | ||
for (AuthenticationClaimsExtractor claimsExtractor : authenticationClaimsExtractors) { | ||
Authentication authentication = claimsExtractor.extract(claims); | ||
if (authentication.getRole() != Role.ANONYMOUS) { | ||
return authentication; | ||
} | ||
} | ||
return AnonymousAuthentication.getInstance(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claims claims = jwtTokenParser.getClaims(token);
이 부분의 중복을 제거하기 위해서 claim 추출과 AuthenticationClaims 추출을 분리했다는 뜻 맞을까요?
private static final String ADMIN_ID_KEY = "adminId"; | ||
private static final long EXPIRATION_MINUTES = 60L * 24L; | ||
|
||
private final TokenProviderTemplate tokenProviderTemplate; | ||
|
||
public TokenResponse provide(AdminAuthentication adminAuthentication) { | ||
return tokenProviderTemplate.provide(EXPIRATION_MINUTES, | ||
jwtBuilder -> jwtBuilder | ||
.claim(ADMIN_ID_KEY, adminAuthentication.getId()) | ||
.audience().add(Role.ADMIN.name()).and() | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매번 memberId, adminId 와 같이 저희가 설정한 값들에서 추출하는 것 보다는 일괄적으로 subject에 있는 것이 훨씬 직관적일 것 같네요
@Override | ||
public boolean supportsParameter(MethodParameter parameter) { | ||
return parameter.getParameterType().equals(AdminAuthentication.class) && parameter.hasParameterAnnotation( | ||
Admin.class); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameter가 ~Authentication 으로 바뀌었으니 @Admin 과 같은 어노테이션은 없애도 되지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트
- 하위 호환성을 지키기 위해 기존 방식은 유지
📌 관련 이슈
✨ PR 세부 내용
이슈에서 적은 내용대로 JWT의 페이로드의 권한에 따른 클레임을 담기 위해 기존 인증 기능을 새롭게 리팩터링 했습니다.
우선 기존 인증 기능의 프로세스를 정리하자면 다음과 같습니다.
AuthInterceptor
접근AuthInterceptor
에 따라 정의된HttpRequestTokenExtractor
에 따라 토큰 추출2.1 Request Header, Cookie에서 토큰을 추출하는데, 둘 중 하나만 허용, 만약 변경하려면 코드의 수정 필요 -> 문제1
AuthPayload
객체 반환 -> 인증AuthPayload
의 Role이 해당 인터셉터가 허용하는 권한과 맞는지 검증 -> 인가AuthenticateContext
에 인가된 사용자의 식별자와 권한 부여@Member
와 같은 에너테이션이 붙어있는Long
타입의 파리미터에RoleArgumentResolver
를 통해AuthenticateContext
에서 부여된 식별자를 할당6.1 이슈에서 지적한 내용이지만, 식별자로만 받기 때문에 해당 권한의 사용자가 어떠한 권한을 가졌는지 확인하려면 DB를 거쳐야 함 -> 문제2
문제1은 간단히
CompositeHttpRequestTokenExtractor
를 사용하는 것으로 해결되었습니다만, 문제2가 이 PR의 핵심입니다.문제2를 해결하려면
AuthenticateContext
에 식별자와 Role 필드뿐 아닌, 해당 권한을 가진 사용자가 가진 값들이 필요합니다.하지만 해당 값들은 권한마다 다를 수 있기에 모든 값을
AuthenticateContext
에 필드로 둘 수 없습니다.따라서
Authentication
이라는 인터페이스를 정의하여 해당 인터페이스를 의존하게 한 뒤,Authentication
를 받는ArgumentResolver
를 정의하여 사용해야 합니다.그러면 권한마다 별도의
Authentication
구현체가 생기는데, 토큰의 권한을 구분하여Authentication
구현체를 생성하는 기능이 필요합니다.따라서 해당 권한을 구현하기 위해
AuthenticationTokenExtractor
을 설계하였고,AuthenticationTokenExtractor
에서 토큰의 권한을 구분하여Authentication
구현체를 반환합니다.이때
AuthenticationTokenExtractor
구현체를 보면 토큰을 파싱하여 바로Authentication
객체를 반환하지 않고, 그저AuthenticationTokenExtractor
구현체를 의존한 뒤, 이를 위임하여 사용하는 것을 볼 수 있습니다.이렇게 구현한 이유는 문제1을 해결한 것과 비슷한데, 토큰을 추출하는 과정에서 권한에 따라 다시 여러 번 토큰을 파싱하지 않게 하기 위함입니다.
구현한 인증 기능을 사용하려면 컨트롤러의 핸들러 메서드에
@Authorization
어노테이션을 사용한 뒤, 해당 어노테이션의 필드에 Role을 추가하면 되는데, 이게 상당히 번잡합니다. 😂(기존
@MemberAuth
어노테이션이 붙은 컨트롤러도 모두 수정해야 하는 문제점도 발생)하지만
HandlerMethod.getMethodAnnotation()
메서드에는 숨겨진 하나의 비밀이 있는데, 해당 메서드에 다음과 같은 주석이 적혀있습니다.이는 타겟으로 한 어노테이션이 아니더라도, 해당 핸들러 메서드의 어노테이션에 타겟으로 하는 어노테이션이 있으면, 해당 어노테이션을 사용할 수 있습니다.
그래서
MemberAuthV1Controller
의 핸들러 메서드에는Authorization
어노테이션이 붙어있지 않은데, 인증과 관련된 테스트가 모두 통과하는 것을 볼 수 있습니다.여러 디자인 패턴이 사용되었고, 인터페이스와 구현체가 많기에 자세한 설명은 코드에 리뷰로 남기겠습니다!