[고정완] - 인가(Authorization) 리뷰 요청 드립니다. #9

ghojeong · 2025-02-06T05:24:06Z

시간 여유가 있어서 미션을 빠르게 진행해보았습니다.
1단계와 2단계를 한번에 진행했습니다.

디폴트 AuthorizationManager 를 DenyAllAuthorizationManager 로 설정하여,
SecurityConfig 에서 설정되지 않은 HttpMethod 와 pattern 의 경우 모두 ForbidenException 이 발생하도록 했습니다.

/members 에는 ADMIN 인가를 받은 유저만 접근할 수 있도록 HasAuthorityAuthorizationManager 를.
/members/me 에는 인증된 유저가 접근할 수 있도록 AuthenticatedAuthorizationManager 를.
/search 에는 PermitAllAuthorizationManager 를 설정했습니다.

기존에 작성된 테스트 시나리오를 이해하는 과정에서,
공통으로 쓰이는 TEST_ADMIN_MEMBER 와 TEST_USER_MEMBER 를 Fixture 로 정의하고,
Fixture 를 사용할 수 있도록 테스트를 리팩토링했습니다.

잘 부탁드립니다. 🙇

… HasAuthority

…an 을 등록함

…er 으로 이동

jinyoungchoi95

안녕하세요 정완님 😄

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

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

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

jinyoungchoi95 · 2025-02-10T15:53:51Z

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

+                createMvcMatcher(GET, "/members", new HasAuthorityAuthorizationManager<>("ADMIN")),
+                createMvcMatcher(GET, "/members/me", new AuthenticatedAuthorizationManager<>()),
+                createMvcMatcher(GET, "/search", new PermitAllAuthorizationManager<>())


config에 잘 추가해주셨네요 👍

자바 컨벤션 상 클래스 네이밍은 명사를 활용하기 때문에 Has보다는 조금 더 적절한 네이밍이 들어가면 좋을 것 같아요.
https://www.oracle.com/java/technologies/javase/codeconventions-namingconventions.html

과연 그렇겠네요. HasAuthorityAuthorizationManager 대신 그냥 AuthorityAuthorizationManager 로 네이밍을 바꾸겠습니다.
아래 커밋에서 적용했습니다.

40fea8d

jinyoungchoi95 · 2025-02-10T16:15:27Z

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

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


최근 spring security 버전에서는 check는 사실 deprecated되었는데요. 일반적으로는 잘 아시다시피 대부분의 오픈소스는 결국 추상화를 바라보고 진행하려하며 AuthorizationDecision이 spring security 내에서는 클래스 구현체로 되어있고, 버전이 올라가는 과정에서 이부분 또한 추상화되었어요.

최근 버전에서는 그래서 구현체를 반환하는 AuthorizationDecision을 사용하기보다는 이를 사용하는 default 메소드인 authorize 사용을 권장하고 있습니다.

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 를 보자마자 확장성이 없는 Value Objet 같다는 느낌을 받았는데,
AuthorizationResult 인터페이스가 선언하게 된 배경에 대해서 알려주셔서 감사합니다.

기왕 deprecated 된거 AuthorizationManager.check 와 AuthorizationDecision 을 제외하고,
AuthorizationResult 인터페이스와 AuthorizationManager.authorize 만을 활용하도록 리팩토링하겠습니다.

ad3fc21 커밋에서, AuthorizationDecision check() 대신 AuthorizationResult authorize() 를 사용하도록 모두 변경했습니다.

jinyoungchoi95 · 2025-02-10T16:29:01Z

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

+import nextstep.security.authentication.Authentication;
+
+@FunctionalInterface
+public interface AuthorizationManager<T> {


선택 요구사항에는 verify도 있는데요. 정완님은 verfiy는 어떤 상황에서 사용하면 좋을만하다고 생각하시나요?

verify() 주석을 보면 "throws AccessDeniedException if access is not granted" 라고 적혀있습니다.

AuthorizationResult 를 반환하는 authorize() 메서드와 다르게 verify() 는 예외를 던짐으로써,
verify() 실패시 모든 로직의 수행을 강제로 중단하게 하는 효과를 가지고 있다고 생각합니다.

authorize() 의 경우, 인가를 받지 못할수도 있는 상황을 가정하고, true/false 에 대한 조건문 분기를 통해,
추가적인 로직 수행이 가능하다고 생각합니다.
가령 Secured 어노테이션이 없는 메서드의 경우 authorize 의 인가값이 false 라 할지라도,
정상적인 200 OK Response 를 반환할 수 있습니다.

하지만 verify() 를 통해 인가를 실패한 경우는 Spring Security 의 예외 핸들러에 따라 항상 access denied 403 response 가 반환됩니다. (사실 Spring Security 도입하면서 잘못 설정했을 때 많이 당해봤습니다. ㅋㅋㅋㅋㅋ)

이런 강제적인 로직 수행 중단이 엄격하다고 느껴질 수도 있지만,
보안이 중요한 리소스에 대해서 access 를 확실하게 차단해준다는 점에서 무척 안전하다고 느껴지기도 하네요.

저는 verify() 를 활용한다면 로직의 수행보다도, API 를 통해 조회 혹은 조작할 수 있는 "리소스"가 보안상 얼마나 중요한지를 생각해볼것 같습니다.
만약 해당 리소스가 현금으로 바꿀 수 있는 가치있는 리소스라면, 저는 authorize() 보다도 verify() 를 사용하려 할 것 같습니다.
불편하더라도 동작 및 정보조회가 실패하면 고객 클레임 선에서 끝나지만,
덜 불편하겠다고 현금성 있는 리소스가 털리게 되면 고객은 법적으로 고소를 할테니까요. ㅋㅋ

저도 안정적인 코드를 만들어준다는 점에서 외부에 예외에 대한 처리를 넘겨주는 것보다는 발생할 여지가 있는 곳에서 처리하는 것이 맞다고는 생각하는데요.

제 개인적인 생각을 말씀드리면 verify가 잘못 설계되었다고 생각해요.

우선 interface를 사용하는 입장에서 핵심인 throws를 명시하지 않았기 때문에 정확하게 의도 전달을 할 수 없는 메소드가 되어있어 아직도 불완전하다는 것이 첫번째 이유이구요.

두번째는 예외를 처리하고 말기에는 다양한 로직이 있을 수 있다입니다. 저희는 간단한 부분만을 구현하기 때문에 AuthorizationManager를 호출하고 나서의 구조가 간단하지만, 실제 spring security 내에서는 반환된 AuthorizationResult를 가지고 event 발행을 하여 뒷단에서 실패에 대한 처리를 진행하기도 해요. (복잡한 뒷단 처리도 있지만 간단하게는 인가 실패에 대한 로그를 남기기도 합니다.)
물론 try-catch로 예외를 잡아서 처리할 수도 있는 부분이기는 하지만 예외는 발생 후의 비즈니스 로직을 기대하는 것이 아니라 실패를 기대하기때문에 이런 로직은 예외를 터트리기보다는 인가 실패에 대한 정보를 남기는 것이 더 적절하겠죠.

예시를 로그로 들기는 했지만 예외는 예상치 못한 상태를 처리할때 사용하는 것인데 인가 실패는 예외적인 상황이 아닌 정상적인 흐름이 될 수 있다가 포인트가 될 것 같아요.

jinyoungchoi95 · 2025-02-10T16:34:56Z

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

@@ -0,0 +1,19 @@
+package nextstep.security.authorization.manager;
+
+public enum AuthorizationDecision {


AuthorizationManager.check를 이어서 이야기하면, enum을 사용하기에 적절한지 고려해보면 좋을 것 같아요.

물론 현재는 granted 조건이 true와 false밖에 없는 상황이긴 하지만 확장성을 고려해본다면 interface로 구성하는 것이 맞지 않을까요? 권한이 true인지, false인지 담아둘 수도 있지만 어떤 권한인지도 보유할 상황이 존재하지 않을까요?

Spring Security 의 AuthorizationDecision 코드를 보았을 때, boolean granted 멤버를 가진 Value Objet 라고 생각을 해버렸네요.
어차피 true 와 false 밖에 없고 값객체로 만들거라면 enum 으로 만들어야지! 하고 생각했습니다.

하지만 말씀주신 대로 AuthorizationResult 라는 별도 인터페이스가 존재하는 군요.

AuthorizationManager.check 는 AuthorizationDecision 이 리턴타입이지만,
AuthorizationManager.authorize 는 AuthorizationResult 인터페이스인 것으로 보아서,
말씀하신대로의 확장성을 AuthorizationManager 가 고려하고 있는 상황이지 않을까하는 추측을 하게 되었습니다.

AuthorizationManager.check 가 deprecated 된 이유도, 리턴타입이 인터페이스가 아닌 AuthorizationDecision 라서 확장성이 떨어지기에 check 대신 authorize 를 사용하도록 권고하고 있나 라는 생각이 들었고요.

단, AuthorizationResult 인터페이스도 결국 isGranted() boolean 값을 반환하는 메서드 한개만을 가지고 있기에,
true, false 외에 권한을 확장가능한지에 대해서는 상상하기가 쉽지 않네요.

AuthorizationDecision 에 Authority 나, Role 과 같은 역할을 추가해서 확장하기를 바라신건가요?
제가 "어떤 권한인지 보유할 상황"에 피드백을 제대로 이해했는지 헷갈려서 여쭈어봅니다.

public interface AuthorizationResult extends Serializable { boolean isGranted(); }

추가적으로 확장될 여지가 있기 때문에 enum이 아닌 interface(AuthorizationResult) 혹은 클래스(AuthorizationDecision)이 더 적절할 수 있다는 이야기였어요 :)

말씀해주신대로 권한을 가진 객체도 있습니다.
https://github.com/spring-projects/spring-security/blob/main/core/src/main/java/org/springframework/security/authorization/AuthorityAuthorizationDecision.java

오오! AuthorityAuthorizationDecision 를 읽으니 무엇을 말씀하신건지 단번에 이해가 되네요!
공유 감사합니다!! 😄

jinyoungchoi95 · 2025-02-10T16:36:07Z

src/main/java/nextstep/security/authorization/matcher/AnyRequestMatcher.java

+
+import jakarta.servlet.http.HttpServletRequest;
+
+public class AnyRequestMatcher implements RequestMatcher {


싱글톤으로 잘 구성해주셨네요 👍

jinyoungchoi95 · 2025-02-10T16:38:22Z

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

+        return regex == null || regex.isBlank()
+                ? null
+                : Pattern.compile(regex);


Suggested change

return regex == null || regex.isBlank()

? null

: Pattern.compile(regex);

if (regex == null || regex.isBlank()) {

return null;

}

return Pattern.compile(regex);

개인적으로는 삼항연산자보다는 if문으로 early return 해주는 것이 한줄 한줄을 읽을 때 컨텍스트를 외워두지 않아도 되어서 읽기 편한 코드로 만들어준다고 생각합니다 :)

e23654e 커밋에서 삼항연산자 대신 if 문을 사용하도록 모두 변경하였습니다!

jinyoungchoi95 · 2025-02-10T16:52:02Z

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

+        return AuthorizationDecision.of(hasAuthority(authentication, target.getMethod()));
+    }
+
+    private boolean hasAuthority(Authentication authentication, Method method) {
+        return authentication != null
+                && authentication.isAuthenticated()
+                && method.isAnnotationPresent(Secured.class)
+                && authentication.getAuthorities()
+                .contains(method.getAnnotation(Secured.class).value());
+    }


@Secured 어노테이션이 없는 메소드를 타는 경우에는 인가 값이 true인 AuthorizationDecision을 반환하는 것이 맞을지 고민해보면 좋을 것 같아요.

AuthorizationDecision이 true인 객체를 반환한다는 것은 "인가"되었다는 것인데 @Secured어노테이션이 없는 것은 인가의 프로세스가 필요없지 않나요?

실제 상황을 상상해보니 AuthorizationDecision 이 true 인 경우, 인가되었을 경우 실행되는 horizontal 한 로직이 실행될 위험이 있을 것 같습니다.

저는 AuthorizationDecision is false == Access Denied 이렇게 생각을 해서,
Secured 어노테이션이 없다면 해당 메소드의 access 가 항상 가능해야하기 때문에 true 값을 가진 AuthorizationDecision 을 반환해야겠다고 생각했던 것 같습니다.

하지만 "Authorization" 과 "Access" 의 개념은 분명 다르고,
이건 AuthorizationDecision 이기 때문에 진영님의 말씀이 더 적절한 것 같습니다.

특히 Security Filter 의 경우 인가되었을 경우 추가적인 필터 로직이 실행될지도 모르는데,
AuthorizationDecision 가 false 를 반환하고, 인가되지 않았더라도 Access 가 가능하도록 구조를 짜는게 훨씬 안전할 것 같습니다.

안전성을 고려할때 진영님의 의견에 강하게 동의합니다.

e4211fa 커밋에서 Secured 어노테이션이 없을 경우, 인가 값이 false 가 되도록 수정하였습니다.

jinyoungchoi95 · 2025-02-10T16:53:13Z

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

+        return entries.stream().noneMatch(
+                entry -> entry.requestMatcher().matches(request)
+        );


spring security에서는 성능의 문제로 stream문을 지양합니다.

spring-projects/spring-security#7154

앗 그렇군요! edd41c3 커밋에서 stream 문을 사용하지 않도록 변경했습니다.

jinyoungchoi95 · 2025-02-10T16:56:41Z

src/test/java/nextstep/Fixture.java

+import java.util.Base64;
+import java.util.Set;
+
+public final class Fixture {


Fixture 👍

(반영하지 않으셔도 됩니다) 저는 특정 도메인 객체의 Fixture 를 활용해야하는 경우가 있으면 enum으로 생성하여 활용하는 편이긴합니다. enum에서 제공하는 메소드를 활용할 필요가 있을 때가 간혹 있더라구요.

오! Fixture 를 enum 으로 만들어볼 생각은 못했는데 도전해보고 싶네요.

8e1648f 커밋에서 도전해보았습니다.

jinyoungchoi95 · 2025-02-10T16:58:31Z

src/test/java/nextstep/security/authorization/manager/HasAuthorityAuthorizationManagerTest.java

+        assertAll(
+                () -> assertThat(manager.check(
+                        createAuthentication(true, "ADMIN"), null)
+                ).isEqualTo(GRANTED),
+                () -> assertThat(manager.check(
+                        createAuthentication(true, "USER"), null)
+                ).isEqualTo(GRANTED)


인증된 유저에 대한 테스트를 진행할 때 "ADMIN"과 "USER"는 결국 다른 테스트 케이스에 해당해요. "ADMIN"에 대한 결과나 "USER"에 대한 결과가 다를 수 있는데, 이에 대한 피드백은 하나의 테스트에서 몰아서 진행하기보다는 각 케이스에 맞게 테스트에 대한 피드백을 받아야 어떤 케이스를 보아야할지 명확하게 파악이 될 수 있습니다.

한 테스트에 한 케이스만 넣으라가 명확한 정답은 아니지만 어떤 피드백을 줄수 있냐의 관점에서는 각각의 다른 케이스도 서로 다른 테스트에 해당한다고 볼 수 있을 것 같아요 :)

https://martinfowler.com/bliki/SpecificationByExample.html

추후의 테스트 유지보수를 위해서라도 ADMIN 과 USER 테스트 케이스를 분리하는데 동의합니다.
ADMIN 과 USER 는 추후에 별도의 서비스 혹은 엔티티로 분리될 가능성이 크다고 생각해요.
테스트 케이스를 나누어달라는 피드백은, 도메인 관점에서도 매우 유의미하다는 생각이 드네요.

좋은 글 공유해주셔서 감사합니다!
"엘레강트 오브젝트" 를 쓴 객체지향 히틀러나, "이펙티브 자바"를 쓰신 조슈아 블로크 등도 계시지만,
개인적인 제 스타일은 마틴 파울러가 가장 동의하게 되고 공감하게 되는 것 같아요.

68599c8 커밋에서 테스트 의 분리 진행하였습니다!

…zationManager

…sult 인터페이스를 반환하는 AuthorizationManager.authorize 로 대체

jinyoungchoi95

안녕하세요 정완님 😄

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

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

jinyoungchoi95 · 2025-02-12T14:21:36Z

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

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


한가지 더 고민해보시면 좋을만한 부분은 SecuredAuthorizationManager와 AuthorityAuthorizationManager의 관계인데요.

따지고보면 @Secured에 대한 처리를 제외하고는 AuthorityAuthorizationManager와 로직이 동일해보이지 않으신가요? 실제고 AuthorizationManager들은 서로의 구현체에 대해서 신경쓰지 않고 본인만의 것만 구현하는 것이 아닌 필요한 구현내용들은 서로를 의존하며 구성되어있어요.

즉 @Secured에서 인가될 권한을 꺼내오는 것까지가 SecuredAuthorizationManager의 순수역할, 그리고 그걸 인가처리하는건 AuthorityAuthorizationManager가 되는거죠.

실제 구현체는 사실 다양한 권한체계를 고려하고 있기 때문에 AuthorityAuthorizationManager가 아닌 AuthoritiesAuthorizationManager를 사용하고 있기는 하지만 AuthorityAuthorizationManager를 SecuredAuthorizationManager안에 넣어보는 것도 좋은 인사이트를 얻으실 수 있을 것 같아요.

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

ghojeong added 8 commits February 5, 2025 14:53

refactor: Security 에서의 공통 Exception 처리를 FilterChainProxy 에서 담당하도록 변경

b276f8a

test: 자신의 정보를 조회하는데 인증이 필요한 부분에 대한 테스트 작성

602c8ee

feat: CheckAuthenticationFilter 로직을 대체하는 RequestAuthorizationManager 구현

48331b6

feat: RequestMatcher 와 RequestMatcherEntry 구현

4de11e1

feat: 4가지 AuthorizationManager 구현; PermitAll, DenyAll, Authenticated,…

eb3cb25

… HasAuthority

feat: AuthorizationManager 에서 RequestMatcherEntry 를 활용하도록 Config 에 Be…

b7d14db

…an 을 등록함

refactor: SecuredMethodInterceptor 의 인가 로직을 SecuredAuthorizationManag…

fbc5c7d

…er 으로 이동

fix: Secured 어노테이션이 없으면, MethodInterceptor 에서 인증 및 인가 과정을 거치지 않도록 수정함

6b8f819

jinyoungchoi95 requested changes Feb 10, 2025

View reviewed changes

ghojeong added 7 commits February 11, 2025 23:54

refactor: Rename HasAuthorityAuthorizationManager to AuthorityAuthori…

40fea8d

…zationManager

fix: security 패키지에서 stream 문을 사용하지 않도록 변경

edd41c3

fix: 삼항연산자 대신 if 문으로 early return 을 해주도록 모두 변경

e23654e

fix: Secured 어노테이션이 없는 메서드일 경우 인가 값이 false 가 되도록 수정

e4211fa

refactor: deprecated 된 AuthorizationManager.check 대신, AuthorizationRe…

ad3fc21

…sult 인터페이스를 반환하는 AuthorizationManager.authorize 로 대체

test: Fixture 를 enum 으로 생성

8e1648f

test: 어드민 유저와 일반 유저의 인가 테스트 케이스를 분리

68599c8

jinyoungchoi95 approved these changes Feb 12, 2025

View reviewed changes

jinyoungchoi95 merged commit 289d47a into next-step:ghojeong Feb 12, 2025

ghojeong mentioned this pull request Feb 21, 2025

PR 링크 정리 ghojeong/spring-security-authorization#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[고정완] - 인가(Authorization) 리뷰 요청 드립니다. #9

[고정완] - 인가(Authorization) 리뷰 요청 드립니다. #9

ghojeong commented Feb 6, 2025

jinyoungchoi95 left a comment

jinyoungchoi95 Feb 10, 2025

ghojeong Feb 11, 2025

jinyoungchoi95 Feb 10, 2025

ghojeong Feb 11, 2025

ghojeong Feb 11, 2025

jinyoungchoi95 Feb 10, 2025

ghojeong Feb 11, 2025

jinyoungchoi95 Feb 12, 2025

jinyoungchoi95 Feb 10, 2025

ghojeong Feb 11, 2025

jinyoungchoi95 Feb 12, 2025

ghojeong Feb 12, 2025

jinyoungchoi95 Feb 10, 2025

jinyoungchoi95 Feb 10, 2025

ghojeong Feb 11, 2025

jinyoungchoi95 Feb 10, 2025

ghojeong Feb 11, 2025 •

edited

Loading

ghojeong Feb 11, 2025

jinyoungchoi95 Feb 10, 2025

ghojeong Feb 11, 2025

jinyoungchoi95 Feb 10, 2025

ghojeong Feb 11, 2025

jinyoungchoi95 Feb 10, 2025

ghojeong Feb 11, 2025

jinyoungchoi95 left a comment

jinyoungchoi95 Feb 12, 2025

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

		public enum AuthorizationDecision {


		import jakarta.servlet.http.HttpServletRequest;

		public class AnyRequestMatcher implements RequestMatcher {

-        return regex == null || regex.isBlank()
-                ? null
-                : Pattern.compile(regex);
+        if (regex == null || regex.isBlank()) {
+            return null;
+        }
+        return Pattern.compile(regex);


		import java.lang.reflect.Method;

		public class SecuredAuthorizationManager implements AuthorizationManager<MethodInvocation> {

[고정완] - 인가(Authorization) 리뷰 요청 드립니다. #9

[고정완] - 인가(Authorization) 리뷰 요청 드립니다. #9

Conversation

ghojeong commented Feb 6, 2025

jinyoungchoi95 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

ghojeong Feb 11, 2025 • 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

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

jinyoungchoi95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghojeong Feb 11, 2025 •

edited

Loading