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

[BE] refactor: JWT에 권한에 따른 클레임을 담기 위해 인증 기능 개편 (#982) #985

Merged
merged 4 commits into from
May 22, 2024

Conversation

seokjin8678
Copy link
Collaborator

📌 관련 이슈

✨ PR 세부 내용

이슈에서 적은 내용대로 JWT의 페이로드의 권한에 따른 클레임을 담기 위해 기존 인증 기능을 새롭게 리팩터링 했습니다.

우선 기존 인증 기능의 프로세스를 정리하자면 다음과 같습니다.

  1. 인증이 필요한 URL 경로 또는 핸들러 메서드에 인증과 관련된 어노테이션이 붙어 있는 경우 AuthInterceptor 접근
  2. 접근된 AuthInterceptor에 따라 정의된 HttpRequestTokenExtractor에 따라 토큰 추출
    2.1 Request Header, Cookie에서 토큰을 추출하는데, 둘 중 하나만 허용, 만약 변경하려면 코드의 수정 필요 -> 문제1
  3. 토큰을 파싱하여, AuthPayload 객체 반환 -> 인증
  4. AuthPayload의 Role이 해당 인터셉터가 허용하는 권한과 맞는지 검증 -> 인가
  5. AuthenticateContext에 인가된 사용자의 식별자와 권한 부여
  6. 컨트롤러의 핸들러 메서드의 파리미터 중 @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() 메서드에는 숨겨진 하나의 비밀이 있는데, 해당 메서드에 다음과 같은 주석이 적혀있습니다.

Also supports merged composed annotations with attribute overrides as of Spring Framework 4.2.2.

이는 타겟으로 한 어노테이션이 아니더라도, 해당 핸들러 메서드의 어노테이션에 타겟으로 하는 어노테이션이 있으면, 해당 어노테이션을 사용할 수 있습니다.

그래서 MemberAuthV1Controller의 핸들러 메서드에는 Authorization 어노테이션이 붙어있지 않은데, 인증과 관련된 테스트가 모두 통과하는 것을 볼 수 있습니다.

여러 디자인 패턴이 사용되었고, 인터페이스와 구현체가 많기에 자세한 설명은 코드에 리뷰로 남기겠습니다!

@seokjin8678 seokjin8678 added BE 백엔드에 관련된 작업 ⚙️ 리팩터링 리팩터링에 관련된 작업 🏗️ 기능 기능 추가에 관한 작업 labels May 19, 2024
@seokjin8678 seokjin8678 self-assigned this May 19, 2024
@github-actions github-actions bot requested review from BGuga, carsago and xxeol2 May 19, 2024 10:58
Copy link

github-actions bot commented May 19, 2024

Test Results

244 files  244 suites   29s ⏱️
797 tests 797 ✅ 0 💤 0 ❌
815 runs  815 ✅ 0 💤 0 ❌

Results for commit ebd3ce7.

♻️ This comment has been updated with latest results.

Comment on lines 23 to 27
@Override
public boolean supportsParameter(MethodParameter parameter) {
return parameter.getParameterType().equals(AdminAuthentication.class) && parameter.hasParameterAnnotation(
Admin.class);
}
Copy link
Collaborator Author

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을 받는 경우일텐데..

Copy link
Member

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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

기존 경로로 권한을 검증하는 기능을 위해 만들어진 인터셉터 입니다.
(어드민, 레거시)

Comment on lines +6 to +26
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;
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

어드민에도 역할이 있는데, ROOT와 일반 어드민의 구분입니다.
새로운 어드민 계정을 생성하는 것은 ROOT 어드민만 가능한데, 해당 AuthenticationisRoot 라는 boolean 필드를 사용해서 ROOT 어드민 유무를 DB를 거치지 않고 확인할 수 있을 것 같네요.

Comment on lines 13 to 24
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()
);
}
Copy link
Collaborator Author

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 응답을 던져주는 임시 패치가 필요함)

Copy link
Member

Choose a reason for hiding this comment

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

매번 memberId, adminId 와 같이 저희가 설정한 값들에서 추출하는 것 보다는 일괄적으로 subject에 있는 것이 훨씬 직관적일 것 같네요

Copy link
Collaborator Author

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 객체를 반환해주면 될 것 같네요!

Comment on lines +12 to +30
@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();
}
}
Copy link
Collaborator Author

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을 사용한 이유이기도 한데, 전자로 구현하면 매 시도마다 불필요하게 토큰을 파싱하기 때문입니다.

Copy link
Member

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 추출을 분리했다는 뜻 맞을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 그렇습니다!

Comment on lines +18 to +43
@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()));
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

권한에 따른 Token을 생성할 때, 중복되는 토큰 생성을 피하기 위해 템플릿 메서드 패턴을 사용했습니다.

Copy link
Member

@BGuga BGuga left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 글렌 이해하기가 수월했네요 👍

Comment on lines +12 to +30
@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();
}
}
Copy link
Member

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 추출을 분리했다는 뜻 맞을까요?

Comment on lines 13 to 24
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()
);
}
Copy link
Member

Choose a reason for hiding this comment

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

매번 memberId, adminId 와 같이 저희가 설정한 값들에서 추출하는 것 보다는 일괄적으로 subject에 있는 것이 훨씬 직관적일 것 같네요

Comment on lines 23 to 27
@Override
public boolean supportsParameter(MethodParameter parameter) {
return parameter.getParameterType().equals(AdminAuthentication.class) && parameter.hasParameterAnnotation(
Admin.class);
}
Copy link
Member

Choose a reason for hiding this comment

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

parameter가 ~Authentication 으로 바뀌었으니 @Admin 과 같은 어노테이션은 없애도 되지 않을까요?

Copy link
Member

@BGuga BGuga left a comment

Choose a reason for hiding this comment

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

테스트

@seokjin8678 seokjin8678 merged commit 9728b00 into dev May 22, 2024
3 checks passed
@seokjin8678 seokjin8678 deleted the feat/#982 branch May 22, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE 백엔드에 관련된 작업 ⚙️ 리팩터링 리팩터링에 관련된 작업 🏗️ 기능 기능 추가에 관한 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BE] 관리자 계정을 추가하기 위해, 인증 기능을 개편한다.
2 participants