🙌 2단계 - 인가(Authorization) 리뷰 요청 드립니다. #10

parkjun5 · 2025-02-09T14:33:16Z

안녕하세요.
인가(Authorization) 리뷰 요청합니다.

이번 미션에서 구현할 내용은 적었지만 이해 하는 게 쉽지 않았던 것 같아요.

특히 RequestMatcherDelegatingAuthorizationManager가 한번에 이해하기 어려웠습니다.
권한 인가 처리를 할 때에 AOP를 쓴다면 인터셉터나 Aspect를 사용될 것 인데, 필터와 사용되는 곳도 처리도 다르다고 생각이 했습니다.

그래서 AuthorizationManager 중에 필터에서 사용되는 종류와 인터셉터와 Aspect에서 사용되는 종류를 구분하였습니다.
예를 들어 AuthorityAuthorizationManager 에서 필터 기반으로 권한 인가 처리를 하고, SecuredAuthorizationManager 인터셉터( 어노테이션) 기반으로 다시 한번 권한 인가 처리가 하고 있습니다.

하지만 권한 인가 처리가 중복이 되고 있는 것 아닌가 하는 궁금증도 생기네요!

리뷰 잘 부탁 드립니다.

- more test for new API

- 하나의 필터에서 여러 인가 로직을 처리하도록 변경

- 인증된 사용자 체크를 위한 AuthorityAuthorizationManager - 권한 체크를 위한 SecuredAuthorizationManager 추가

- MvcRequestMatcher for matching by HttpMethod And URI - AnyRequestMatcher for matching any request

- 인가를 처리하는 매니져들 생성 - AuthenticatedAuthorizationManager 단순 인증 확인 - AuthorityAuthorizationManager 메서드에서 어노테이션을 가져와 권한 확인 - PermitAllAuthorizationManager 모든 요청 허용 - DenyAllAuthorizationManager 모든 허용 거부

- 유저 권한 제어를 Interceptor에게 위임하기 위한 Secured 어노테이션 추가

…izationManager - 인터셉터와 필터는 더이상 직접 인가를 해주지 않고, AuthorizationManager 를 사용한다.

…nged - RequestMatcherDelegatingAuthorizationManager 에서 요청에 맞는 인가를 시도한다.

- 필터는 한곳에서 여러 인가를 처리하지만, MethodInterceptor or Aspect 에서는 하나의 인가를 처리 그래서 굳이 합쳐서 처리할 필요가 없는듯함. 만약 필요하다면 RequestMatcher 형식으로 Annotation 을 파라미터로 받도록 처리? - RequestMatcherDelegatingAuthorizationManager 는 HttpServletRequest 를 사용하는 매니저만 등록(Filter 사용하는 애들)

- AuthorityAuthorizationManager 필터에서 정해진 값과 Authentication.getAuthorities() 비교 - SecuredMethodInterceptor 인터셉터에서 Annotation 값과 Authentication.getAuthorities() 비교

jinyoungchoi95

안녕하세요 세준님 😄

Authorization 미션을 함께하게 된 최진영입니다.

미션 잘 진행해주셨네요 👍 몇가지 코멘트 남겨두었으니 확인부탁드려요 😄

궁금하거나 고민이 되는 부분이 있으시다면 언제든 pr 코멘트 또는 dm으로 요청 부탁드립니다.
감사합니다 🙇‍♂️

jinyoungchoi95 · 2025-02-11T12:27:16Z

src/main/java/nextstep/security/authorization/manager/SecuredAuthorizationManager.java

+import java.lang.reflect.Method;
+import java.util.Arrays;
+
+public class SecuredAuthorizationManager implements AuthorizationManager<MethodInvocation> {


조금 헷갈리실 수 있을 법한 부분이라고 생각해요. 두가지를 분리해놓고 생각해보면,

SecuredAuthorizationManager는 @Secured 어노테이션을 보고 인가 과정을 체크하는 것이고, AuthorityAuthorizationManager 는 명확하게 인가 처리를 직접 처리하는 객체에요.

그래서 사실 어떻게보면 SecuredAuthorizationManager와 AuthorityAuthorizationManager는 분리되어있다기보다는 실제로는 SecuredAuthorizationManager가 AuthorityAuthorizationManager를 가지고 있고, @Secured에서 꺼낸 것을 가지고 AuthorityAuthorizationManager로 다시 검증을 진행하고 있다고 볼 수 있어요. 명확하게 용도 자체가 분리되어있고, 공통화된 인가에 대한 처리를 SecuredAuthorizationManager�가 사용하는거죠. (실제로는 단일 권한만 검증하기 보다는 Authorities로 다중 권한을 가지고 체크합니다.)

https://github.com/spring-projects/spring-security/blob/main/core/src/main/java/org/springframework/security/authorization/method/SecuredAuthorizationManager.java

SecuredAuthorizationManager 에서 정확한 인가 처리를 AuthorityAuthorizationManager 를 위임하도록 할 수 있겠네요.
AuthorityAuthorizationManager에서도 제네릭을 통해 전달받은 파라미터에 따라서 다른 처리를 할 수 도 있을 것 같아요.

제가 이해한 내용을 정리하면 SecuredAuthorizationManager 는 인가에 필요한 값을 어노테이션 기반으로 동적으로 사용합니다.
하지만 SecuredAuthorizationManager 는 정확히 어떤 인가 작업을 할지는 관심이 없습니다. 그래서 AuthorityAuthorizationManager 에게 인가 작업을 위임합니다.
제네릭을 사용하는 AuthorityAuthorizationManager 에서는 구현체나 제네릭 값을 기반으로 전달 받은 값과 Authentication 을 통해서 인가를 작업해주는 것 같습니다.

네 이해해주신 내용이 맞습니다 :)

jinyoungchoi95 · 2025-02-11T12:27:41Z

README.md

필요 요구사항 잘 정리해주셨네요 👍

jinyoungchoi95 · 2025-02-11T12:29:09Z

src/main/java/nextstep/app/SecurityConfig.java

    }
 //    @Bean
 //    public SecuredAspect securedAspect() {
 //        return new SecuredAspect();
 //    }

+    @Bean
+    public RequestMatcherDelegatingAuthorizationManager requestMatcherDelegatingAuthorizationManager() {


적절하게 잘 추가해주셨네요 :)

jinyoungchoi95 · 2025-02-11T12:30:48Z

src/main/java/nextstep/security/authorization/AuthorizationDecision.java

+    public static AuthorizationDecision granted() {
+        return new AuthorizationDecision(true);
+    }
+
+    public static AuthorizationDecision denied() {
+        return new AuthorizationDecision(false);
+    }


메소드로 정의할 수도 있지만 결국 동일한 객체를 여러번 생성하는 것이기 때문에 불변한 객체를 만들 수 있는 현재 상황이라면 단순히 상수로 만들고 재사용하는 것도 방법이 되겠네요 :)

AuthorizationDecision 에서 성공 실패만 가지고 있기 때문에 좋은 방법이네요.

jinyoungchoi95 · 2025-02-11T12:36:31Z

src/main/java/nextstep/security/authorization/AuthorizationFilter.java

-            return;
+        AuthorizationDecision authorizationDecision = authorizationManager.checkInFilter(request, authentication);
+        if (authorizationDecision.isDenied()) {
+            throw new AuthenticationException();


인가권한이 거절되었는데 AuthenticationException인 인증 에러가 발생하는 것이 맞을까요?

Forbidden 으로 처리하도록 변경해야겠네요.

이 부분에서도 이쪽에서도 왜 check보다 verify를 권장하는 지 보여주는 것 같네요. AuthorizationDecision 인가 결과에 대한 처리에 일관성을 보장해주는 것이 필요해 보이네요!

jinyoungchoi95 · 2025-02-11T12:37:19Z

src/main/java/nextstep/security/matcher/MvcRequestMatcher.java

+        boolean matchURI = Arrays.stream(requestURIs)
+                .anyMatch(uri -> uri.equalsIgnoreCase(request.getRequestURI()));


실제 spring security에서는 성능의 문제로 인해 stream 사용을 제한하고 있습니다.

spring-projects/spring-security#7154

성능 문제와 가독성은 언제나 고민이 되는 부분입니다.
요즘은 개발하면 Stream 을 가독성이 조금 더 좋다고 생각해서 선택하는 것 같네요.
말씀해주신 대로 ForEach 로 수정해보겠습니다!

jinyoungchoi95 · 2025-02-11T12:40:46Z

src/main/java/nextstep/security/authorization/manager/SecuredAuthorizationManager.java

+        if (method.isAnnotationPresent(Secured.class)) {
+            Secured secured = method.getAnnotation(Secured.class);
+            boolean hasNoRole = authentication.getAuthorities()
+                    .stream()
+                    .noneMatch(auth -> Arrays.asList(secured.value()).contains(auth));
+            if (hasNoRole) {
+                throw new ForbiddenException();
+            }
+        }


질문주신 내용에 답변 달아두었던 것처럼 AuthoritiesAuthorizationManager로 구현하는 것까지는 아니어도 인가부분에 대해서 AuthorityAuthorizationManager를 사용하는 것으로 변경해보면 좋을 것 같아요.

인가 부분을 위임하도록 변경해보았습니다!

jinyoungchoi95 · 2025-02-11T12:41:21Z

src/main/java/nextstep/security/authorization/manager/AuthorityAuthorizationManager.java

+
+import java.util.Set;
+
+public class AuthorityAuthorizationManager implements AuthorizationManager<HttpServletRequest> {


Suggested change

public class AuthorityAuthorizationManager implements AuthorizationManager<HttpServletRequest> {

public class AuthorityAuthorizationManager implements AuthorizationManager<T> {

위에서 나열한 이유에서 사실 HttpServletRequest보다는 제네릭 타입을 활용하여 다른 곳에서 활용할 수 있게끔 하는 것이 좋겠죠.

필터와 인터셉터 혹은 인터셉터에서도 여러 종류 값을 사용하기 위해서 좋은 선택 같습니다,

그렇다면 AuthorityAuthorizationManager 는 여러 종류의 구현체를 만들어서 관리할 수 도 있을 것 같습니다.

예를 들어 RequestMatcherDelegatingAuthorizationManager 처럼 여러 구현체에 support 메서드나 Matcher를 만듭니다.
이를 통해 처리할 수 있는 인가 작업인지 확인하고 그에 맞는 AuthorityAuthorizationManager 구현체가 작동하는 방식으로 쉽게 관리 될 것 같아요!

jinyoungchoi95 · 2025-02-11T12:43:02Z

src/main/java/nextstep/security/authorization/manager/AuthorizationManager.java

+import nextstep.security.authentication.Authentication;
+import nextstep.security.authorization.AuthorizationDecision;
+
+@FunctionalInterface


선택사항에 나와있는 verify는 구현하지 않으셔도 되긴 하지만, check와 verify는 각각 어떤 상황에서 사용할 해야 좋은지 세준님의 의견은 어떠신가요?

제가 이 미션을 진행하면서 느낌 점은 세 가지입니다.

AuthorizationDecision 의 널의 위험성

AuthorizationDecision 가 실패됬을 때의 예외 처리 일관성

사용도 안하는 결과를 리턴
이런 상황때문에 verify를 권장하는 것 같습니다.
하지만 AuthorizationDecision 의 결과를 커스텀 예외나 조건부 로직이 필요한 경우만 check을 사용해 결과를 핸들링 할 것 같습니다.

사실 spring security에서는 verify를 권장하는 것은 아니에요. (오해의 소지가 있을 수 있을 것 같아 미리 말씀드리면 정답을 요구하기 위해 남겨드린 질문은 아닙니다.)

말씀해주신 이유 중 먼저 1에 대해서 이야기하면 null 위험성을 발생시키는 것은 맞지만 AuthorizationDecision의 반환은 인가됨(true), 인가안됨(false)만 있는 것이 아니라 인가 상황이 없음을 나타내기도 합니다. 가령 @Secured가 걸린 곳이 없어 검증할 authorities가 없는 경우가 있겠네요.

public AuthorizationDecision check(Supplier<Authentication> authentication, MethodInvocation mi) { Set<String> authorities = getAuthorities(mi); return authorities.isEmpty() ? null : this.authoritiesAuthorizationManager.check(authentication, authorities); } private Set<String> getAuthorities(MethodInvocation methodInvocation) { Method method = methodInvocation.getMethod(); Object target = methodInvocation.getThis(); Class<?> targetClass = (target != null) ? target.getClass() : null; MethodClassKey cacheKey = new MethodClassKey(method, targetClass); return this.cachedAuthorities.computeIfAbsent(cacheKey, (k) -> resolveAuthorities(method, targetClass)); } private Set<String> resolveAuthorities(Method method, Class<?> targetClass) { Secured secured = findSecuredAnnotation(method, targetClass); return (secured != null) ? Set.of(secured.value()) : Collections.emptySet(); }

다음으로 중요한 것은 인가를 체크했을때 인가되지 않은 유저인 것이 AuthorizationManager입장에서는 예외상황은 아닐 수 있다에요. 예외를 발생시키는 것은 이 상황이 실패했으니 실패를 전파하는 것이 목적일텐데요. spring security 입장에서는 AuthorizationManager는 인가가 되었는지 확인해주는 객체이고, 실제 에러는 AuthorizationFilter 등과 같은 필터에서 인가가 안되었는 상황을 판단하여 예외를 발생시켜주고 있어요.

그래서 내부적으론 조금 더 복잡하지만 AuthorizationFilter에서는 인가 결과에 대한 이벤트를 발행한 다음 예외를 발생시키기도 해요.

AuthorizationResult result = this.authorizationManager.authorize(this::getAuthentication, request); this.eventPublisher.publishAuthorizationEvent(this::getAuthentication, request, result); if (result != null && !result.isGranted()) { throw new AuthorizationDeniedException("Access Denied", result); } chain.doFilter(request, response);

spring security의 AuthorizationManager를 보면 예외를 발생시키는 코드가 없는 것을 보시게 될텐데 그런 것들도 위와 같은 이유에서일 가능성이 높습니다 :)

자세한 설명 감사합니다. 단순 인가 성공, 실패 뿐만 아니라 여러 케이스에 대해서는 깊이 생각 못했었는데, 고민해볼 포인트네요!

AuthorizationManager에서는 인가 확인, AuthorizationFilter 전체적인 맥락을 파악하여 필터링 해주는 것이군요!

jinyoungchoi95 · 2025-02-11T12:44:53Z

src/main/java/nextstep/security/authorization/manager/AuthorizationManager.java

+
+@FunctionalInterface
+public interface AuthorizationManager<T> {
+    AuthorizationDecision check(Authentication authentication, T object);


최근 spring security 버전에서는 check는 deprecated되었습니다. AuthorizationDecision이 지금 만들어주신 것과 같이 구현체로 되어있고, 일반적으로 오픈소스들이 커질수록 단순하게 구현해두었던 구현체들을 추상화하는 형태로 개선해나가는데요.

그래서 최근 버전에서는 구현체를 반환하는 check를 그대로 사용하기보다는 deprecated해둔 뒤 이를 추상화한 AuthorizationResult를 반환하는 메소드를 추가하여 해당 메소드 사용을 권장하고 있습니다.

https://github.com/franticticktick/spring-security/blob/main/core/src/main/java/org/springframework/security/authorization/AuthorizationManager.java

spring-projects/spring-security#14712
spring-projects/spring-security#14846

미션을 하다보니 말씀해주신 대로 AuthorizationDecision 가 deprecated된 이유가 간접적으로 느껴지는 것 같습니다.
RequestMatcherDelegatingAuthorizationManager 에서도 checkInFilter 보다는 verifyInFilter를 사용한다면
AuthorizationFilter 에서의 인가 요청, 예외처리 두가지 역할에서 인가 요청만 남게되어서 훨씬 깔끔합니다!

- String과 Collection String을 지원하도록 변경

- AuthorizationDecision 의 널의 위험성과 사용도 안하는 결과를 리턴하면서 결과에 대한 예외 처리도 일괄성을 가지지 못하기에 verify 를 통해 검증까지 위임

…인가 작업을 위임하도록 변경 - SecuredAuthorizationManager 에서는 Annotation 값을 통한 동적 인가 - RequestMatcherDelegatingAuthorizationManager 에서는 인가 작업을 하지 않음

jinyoungchoi95

안녕하세요 세준님 😄

피드백 잘 반영해주셨네요 👍 필요한 내용들을 모두 잘 학습해주신듯하여 머지하겠습니다.
선택사항의 경우 미션 진행을 원하시면 말씀해주시면 이어서 진행하고, 원하지 않으시면 미션 종료처리하겠습니다.

궁금하거나 고민이 되는 부분이 있으시다면 언제든 pr 코멘트 또는 dm으로 요청 부탁드립니다.
감사합니다 🙇‍♂️

jinyoungchoi95 · 2025-02-12T23:32:09Z

src/main/java/nextstep/security/matcher/MvcRequestMatcher.java

+public class MvcRequestMatcher implements RequestMatcher {
+
+    private final HttpMethod httpMethod;
+    private final String[] requestURIs;


실제로는 httpMethod, requestUri각각 하나만을 가지므로 String[]으로 표현되진 않습니다. 딱 하나로 지정된 method, uri만을 검증하면 되는거죠.

여러개의 uri가 필요한 경우라면 필요한 객체가 RequestMatcher를 여러개로 들고 있는 형태가 더 자연스럽습니다 :)

하나의 Matcher에 하나의 URI 검증하는 것이 더 이해하기 좋겠네요!

jinyoungchoi95 · 2025-02-12T23:33:20Z

src/main/java/nextstep/security/authorization/AuthorizationDecision.java

@@ -0,0 +1,25 @@
+package nextstep.security.authorization;
+
+public class AuthorizationDecision {


미션에서는 spring security의 기능이 축소되어 true, false가 잘 안와닿으실 수 있으나 실제로는 그보다 더 다양한 구현체들이 생길 수 있습니다.

https://github.com/spring-projects/spring-security/blob/main/core/src/main/java/org/springframework/security/authorization/AuthorityAuthorizationDecision.java

parkjun5 added 15 commits February 9, 2025 18:26

feat: add /members/me API

d4add2f

- more test for new API

ref: refactoring CheckAuthenticationFilter to AuthorizationFilter

bc949f3

- 하나의 필터에서 여러 인가 로직을 처리하도록 변경

feat: add AuthorizationManager for authorize

6d5c693

- 인증된 사용자 체크를 위한 AuthorityAuthorizationManager - 권한 체크를 위한 SecuredAuthorizationManager 추가

feat: add request matcher for find AuthorizationManager

646c495

- MvcRequestMatcher for matching by HttpMethod And URI - AnyRequestMatcher for matching any request

ref: change test and Add SecureAnnotation to Get /members API

e301ed4

- 유저 권한 제어를 Interceptor에게 위임하기 위한 Secured 어노테이션 추가

feat: Filter And Interceptor delegate authorization control to Author…

8b70fdd

…izationManager - 인터셉터와 필터는 더이상 직접 인가를 해주지 않고, AuthorizationManager 를 사용한다.

feat: add RequestMatcherDelegatingAuthorizationManager and config cha…

1f37f27

…nged - RequestMatcherDelegatingAuthorizationManager 에서 요청에 맞는 인가를 시도한다.

docs: 요구 사항 정리

a0c8b51

polishing

b83d2a2

polishing

0179bf5

ref:

fbbf311

- AuthorityAuthorizationManager 필터에서 정해진 값과 Authentication.getAuthorities() 비교 - SecuredMethodInterceptor 인터셉터에서 Annotation 값과 Authentication.getAuthorities() 비교

polishing

bb7a79e

polishing

9bf5f29

jinyoungchoi95 requested changes Feb 11, 2025

View reviewed changes

MZC01-PARKJUN5 added 6 commits February 12, 2025 15:13

ref: Change AuthorityAuthorizationManager to Generic

8126c74

- String과 Collection String을 지원하도록 변경

ref: use verify not check method

7e23368

- AuthorizationDecision 의 널의 위험성과 사용도 안하는 결과를 리턴하면서 결과에 대한 예외 처리도 일괄성을 가지지 못하기에 verify 를 통해 검증까지 위임

ref: AuthorizationDecision 을 static 객체를 사용하도록 변경

7a213b0

ref: mvc match logic not Stream change to ForEach

0e5d010

test: 인증 실패에서는 AccessDeniedException 인가 실패에는 ForbiddenException 나도록 변경

fc947f5

jinyoungchoi95 approved these changes Feb 12, 2025

View reviewed changes

jinyoungchoi95 merged commit 7a4adfd into next-step:parkjun5 Feb 12, 2025

		boolean matchURI = Arrays.stream(requestURIs)
		.anyMatch(uri -> uri.equalsIgnoreCase(request.getRequestURI()));


		import java.util.Set;

		public class AuthorityAuthorizationManager implements AuthorizationManager<HttpServletRequest> {

		@@ -0,0 +1,25 @@
		package nextstep.security.authorization;

		public class AuthorizationDecision {

🙌 2단계 - 인가(Authorization) 리뷰 요청 드립니다. #10

🙌 2단계 - 인가(Authorization) 리뷰 요청 드립니다. #10

Uh oh!

Conversation

parkjun5 commented Feb 9, 2025

Uh oh!

jinyoungchoi95 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinyoungchoi95 Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinyoungchoi95 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jinyoungchoi95 Feb 11, 2025 •

edited

Loading