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

ParkSeryu · 2025-02-11T12:55:16Z

안녕하세요 진영님!
인가 미션 리뷰 요청드립니다.

이번주 미션은 잦은 피드백 주기를 목표로 진행하다 보니,
제대로 파악하고 진행한건지 걱정이 살짝 앞서네요 😂

엇나간 방향이 있으면 피드백 받고 고쳐나가도록 하겠습니다!

… 리팩토링

- testFixture 추가 - 기존 테스트 코드 구조 변경 - 사용하지 않는 테스트 클래스 제거

…ager로 리팩토링

…현체를 작성

jinyoungchoi95

안녕하세요 준형님 😄

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

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

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

jinyoungchoi95 · 2025-02-12T12:56:25Z

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

+        List<RequestMatcherEntry<AuthorizationManager>> mappings = new ArrayList<>();
+        mappings.add(new RequestMatcherEntry<>(new MvcRequestMatcher(HttpMethod.GET, "/members"),
+                new AuthorityAuthorizationManager("ADMIN")));
+        mappings.add(new RequestMatcherEntry<>(new MvcRequestMatcher(HttpMethod.GET, "/members/me"),
+                new AuthenticatedAuthorizationManager()));
+        mappings.add(new RequestMatcherEntry<>(new MvcRequestMatcher(HttpMethod.GET, "/search"),
+                new PermitAllAuthorizationManager()));
+        mappings.add(new RequestMatcherEntry<>(AnyRequestMatcher.INSTANCE, new DenyAllAuthorizationManager()));
+        return new RequestMatcherDelegatingAuthorizationManager(mappings);


잘 추가해주셨네요 👍

jinyoungchoi95 · 2025-02-12T12:57:18Z

src/main/java/nextstep/app/ui/MemberController.java

+        if (authentication == null) {
+            throw new AuthenticationException();
+        }


이미 Filter에서 처리하고 있는데 불필요한 로직이 아닐까요?

앗 해당 app패키지에 컨트롤러는
강의실에서 실습이후 체리픽 해온것이라 열어볼 생각을 못했네요.
수정해두겠습니다!

jinyoungchoi95 · 2025-02-12T13:01:08Z

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

+
+    @Override
+    public boolean matches(HttpServletRequest request) {
+        return this.method.equals(HttpMethod.valueOf(request.getMethod())) && request.getRequestURI().equals(pattern);


Suggested change

return this.method.equals(HttpMethod.valueOf(request.getMethod())) && request.getRequestURI().equals(pattern);

return this.method.equals(HttpMethod.valueOf(request.getMethod())) && pattern.equals(request.getRequestURI());

null-safe 관점에서는 파라미터로 들어오는 request.getRequestURI()는 null일 수 있기 때문에 별도로 null 체크를 하지 않는 이상 위와 같이 표현하는 것이 npe가 발생하지 않는 안전한 코드를 만들 수 있습니다.

간과하고 있었네요. 수정해두겠습니다! 👍

jinyoungchoi95 · 2025-02-12T13:13:12Z

src/main/java/nextstep/security/authorization/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

오.. 불과 4달전에 업데이트된 기능이군요!
피드백 주신 대로 반영해보면서 어떤 식으로
오픈소스가 개선되어 나가는지 체험 해 볼 수 있었네요 감사합니다.

jinyoungchoi95 · 2025-02-12T13:14:29Z

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

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


준형님이 생각하시기에 선택사항으로 주어진 verfiy는 check와 비교하여 어떤 상황에서 사용하면 좋을 것 같으신가요?

음.. 결국엔 인가실패시 예외를 발생시킨다. 가 핵심이라고 생각해요.
check로는 null이 올 수도 있고, 권한이 없다 할 지어도 에러코드 등에서 개발자 마음대로 핸들링 할 수 있는 반면에,
verify는 인가에 실패한 경우 동일한 에러가 발생된다. 라는 점에서 추적에 용이할 것 같고, 인적오류를 최소화 할 수 있을 것 같다고 생각이 드네요!
따라서 관리자 전용 API등 권한이 없으면 추가적인 로직 없이 바로 예외를 던질때 사용하면 좋을 것 같습니다~

네 사실 verify를 단일로 쓰면 저희가 흔히 알고 있듯 예외를 발생시킬 수 있는 곳에서 예외를 발생시키는 것이 맞기는 합니다.

다만 AuthorizationManager의 원래 책임은 인가가 된 유저인지를 확인하는 것이고 이건 인가가 실패되었다는 것이 에러 상황이 아닌 정상적인 비즈니스의 흐름이라고 볼 수 있어요. 인가가 실패한 것에 대한 결과가 예외가 나는 것은 AuthorizationFilter가 인가 실패의 결과가 예외이다라는 것을 실행하는 것일뿐인 것이구요.

실제 AuthorizationManager들의 구현체를 보시면 이런 이유들도 겹쳐서 check안에서 거의 예외가 발생하지 않고 있는 것을 확인할 수 있어요.

jinyoungchoi95 · 2025-02-12T13:15:49Z

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

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


true 혹은 false만 가지는 AuthorizationDecision이 자주 사용되는데 사용될때마다 인스턴스화하기보다는 불변임을 활용하여 미리 만들어준 상수를 사용하도록 유도할 수 있을 것 같네요.

현 상황에서 메모리 효율성을 더 높일 수 있겠네요!

jinyoungchoi95 · 2025-02-12T13:18:32Z

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

+        boolean hasAuthority = authentication.getAuthorities().stream()
+                .anyMatch(authority::equals);


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

spring-projects/spring-security#7154

가독성 때문에 stream을 선호하는 편인데, spring security같은 라이브러리는 성능이 중요하다보니 그렇게 된거군요..
구현체 코드 보면서 가독성이 생각만큼은 아닌것 같았는데, 그 위화감이 여기서 기인한 것 같습니다.

jinyoungchoi95 · 2025-02-12T13:19:48Z

...n/java/nextstep/security/authorization/web/RequestMatcherDelegatingAuthorizationManager.java

+    @Override
+    public AuthorizationDecision check(Authentication authentication, HttpServletRequest request) {
+        for (RequestMatcherEntry<AuthorizationManager> mapping : mappings) {
+            if (mapping.getRequestMatcher().matches(request)) {


getter로 호출하여 일치 여부를 확인하는 것보다는 mapping에서 처리하도록 수정하면 책임이 명확하게 넘어갈 수 있겠네요 :)

음.. 제 생각으로는 mapping인 RequestMatcherEntry는 RequestMatcher랑 AuthorizationManager를
단순히 묶어주는 역할을 수행한다고 생각해서,
해당 객체에 matches()의 로직까지 있으면 오히려 책임이 과해지고,
원래 RequestMatcher의 역할이 흐려질것 같아 getter로 호출해서 일치 여부를 확인했습니다.

제가 제대로 이해한거라면 진영님 생각이 궁금합니다!

Suggested change

if (mapping.getRequestMatcher().matches(request)) {

if (mapping.matches(request)) {

제가 의도 드렸던 내용은 getter를 꺼내와서 다시 호출하는 것이 아닌 해당 객체 내에서 처리할 수 있게 메시지를 던지는 형태를 말씀드린 것이었어요. getter를 호출해서 메소드 체이닝이 되는 것은 객체의 역할이 있다기보단 단순히 래핑클래스가 되어버려 책임과 역할이 불분명해질 수 있습니다.

public boolean matches(HttpServletRequest request) { return requestMatcher.matches(request); }

jinyoungchoi95 · 2025-02-12T13:30:31Z

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

+import nextstep.security.authorization.Secured;
+import org.aopalliance.intercept.MethodInvocation;
+
+public class SecuredAuthorizationManager implements AuthorizationManager<MethodInvocation> {


AuthorityAuthorizationManager의 로직과 함께보면

boolean hasAuthority = authentication.getAuthorities().stream() .anyMatch(authorities::contains); return new AuthorizationDecision(hasAuthority);

부분이 동일한 것을 확인해볼 수 있어요. 각 AuthorizatinManager는 단일에서 각각 본인의 것을 모두 구성하는 것이 아닌 서로 유기적으로 결합되어 사용하기도 하는데요.

즉, SecuredAuthorizationManager는 @Secured에 있는 정보를 가지고 처리하며, authority에 대한 권한체크는 AuthorizatinManager가 온전히 담당하는거죠.

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

실제로는 권한체계가 다양하기 때문에 AuthoritiesAuthorizationManager를 사용하고 있기는 하지만 관련하여 참고하시면 이해에 도움이 되실 것 같아요.

83a4efd
으로 반영해보았습니다!
각자의 Manager가 단일 책임이 아닌 결합해서 사용하기도 하는군요!
저도 작업하면서 같은 로직이 들어간다고 생각이 들었는데,
이런식으로도 역할과 책임 분배를 가져갈 수 있겠네요!

현재 말씀해주신 AuthoritiesAuthorizationManager 처럼 다양한 권한 체계는 없기 떄문에
기존 SecuredAuthorizationManager에서 AuthorityAuthorizationManager를 사용해보는 방향으로 변경해보았습니다.

jinyoungchoi95 · 2025-02-12T13:31:19Z

src/test/java/nextstep/security/fixture/MemberTestFixture.java

+import java.util.Set;
+import nextstep.app.domain.Member;
+
+public class MemberTestFixture {


(반영하지 않으셔도 됩니다.) 개인적으로는 enum에서 지원되는 메소드들을 활용하는 경우도 많아 특정 도메인의 fixture는 enum 으로 생성하는 편입니다 😄

3단계 수행하면서 변경해 보겠습니다! 😄

jinyoungchoi95

안녕하세요 준형님 😄

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

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

jinyoungchoi95 · 2025-02-12T23:42:47Z

src/main/java/nextstep/security/authorization/web/DenyAllAuthorizationManager.java

+
+public class DenyAllAuthorizationManager implements AuthorizationManager<HttpServletRequest> {
+    @Override
+    public AuthorizationDecision check(Authentication authentication, HttpServletRequest object) {


Suggested change

public AuthorizationDecision check(Authentication authentication, HttpServletRequest object) {

public �AuthorizationResult check(Authentication authentication, HttpServletRequest object) {

반환 타입들도 잘 추상화해주신 클래스로 만들어주면 좋겠네요 :)

jinyoungchoi95 · 2025-02-12T23:43:34Z

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

+        return granted;
+    }
+
+    public static AuthorizationDecision of(boolean granted) {


Suggested change

public static AuthorizationDecision of(boolean granted) {

public static AuthorizationDecision from(boolean granted) {

관습적으로 정적 팩토리 메소드는 파라미터가 하나일때 from, 여러개일때 of를 활용합니다.

boorownie and others added 8 commits February 9, 2025 23:09

practice - /members/me

313c011

practice - authorization filter

7e036b1

feat: AuthorizationManager, AuthorizationDecision 구현

c953150

refactor: 기존의 AuthorizationFilter 인가 로직을 RequestAuthorizationManager로…

439e1a5

… 리팩토링

test: 테스트 코드 구조화

a63a21e

- testFixture 추가 - 기존 테스트 코드 구조 변경 - 사용하지 않는 테스트 클래스 제거

refactor: 기존의 SecuredMethodInterceptor 인가 로직을 SecuredAuthorizationMan…

8cdf3dc

…ager로 리팩토링

feat: RequestMatcherRegistry와 RequestMatcher를 작성하고, RequestMatcher의 구…

21e639b

…현체를 작성

feat: 각 요청별 인가 로직을 담당하는 AuthorizationManager들 작성

706c626

jinyoungchoi95 requested changes Feb 12, 2025

View reviewed changes

ParkSeryu added 5 commits February 12, 2025 22:58

remove: 피드백 반영 - app 패키지의 불필요한 코드 제거

0f8d213

refactor: 피드백 반영 - match 메소드의 null-safe한 코드로 변경

0119cca

refactor: 피드백 반영 - 인스턴스화 -> 상수 사용으로 변경

f87ce1e

refactor: 피드백 반영 - 역할과 책임 분리

83a4efd

refactor: 피드백 반영 check -> authorize로 변경

eca7352

jinyoungchoi95 approved these changes Feb 12, 2025

View reviewed changes

jinyoungchoi95 merged commit afda744 into next-step:parkseryu Feb 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ParkSeryu commented Feb 11, 2025

jinyoungchoi95 left a comment

jinyoungchoi95 Feb 12, 2025

jinyoungchoi95 Feb 12, 2025

ParkSeryu Feb 12, 2025

jinyoungchoi95 Feb 12, 2025

ParkSeryu Feb 12, 2025

jinyoungchoi95 Feb 12, 2025

ParkSeryu Feb 12, 2025 •

edited

Loading

jinyoungchoi95 Feb 12, 2025

ParkSeryu Feb 12, 2025

jinyoungchoi95 Feb 12, 2025

jinyoungchoi95 Feb 12, 2025

ParkSeryu Feb 12, 2025

jinyoungchoi95 Feb 12, 2025

ParkSeryu Feb 12, 2025

jinyoungchoi95 Feb 12, 2025

ParkSeryu Feb 12, 2025 •

edited

Loading

jinyoungchoi95 Feb 12, 2025

jinyoungchoi95 Feb 12, 2025

ParkSeryu Feb 12, 2025

jinyoungchoi95 Feb 12, 2025

ParkSeryu Feb 12, 2025

jinyoungchoi95 left a comment

jinyoungchoi95 Feb 12, 2025

jinyoungchoi95 Feb 12, 2025

	return this.method.equals(HttpMethod.valueOf(request.getMethod())) && request.getRequestURI().equals(pattern);
	return this.method.equals(HttpMethod.valueOf(request.getMethod())) && pattern.equals(request.getRequestURI());

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

		public class AuthorizationDecision {

		boolean hasAuthority = authentication.getAuthorities().stream()
		.anyMatch(authority::equals);

	if (mapping.getRequestMatcher().matches(request)) {
	if (mapping.matches(request)) {

	public AuthorizationDecision check(Authentication authentication, HttpServletRequest object) {
	public �AuthorizationResult check(Authentication authentication, HttpServletRequest object) {

	public static AuthorizationDecision of(boolean granted) {
	public static AuthorizationDecision from(boolean granted) {

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

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

Conversation

ParkSeryu commented Feb 11, 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

ParkSeryu Feb 12, 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

Choose a reason for hiding this comment

ParkSeryu Feb 12, 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

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

ParkSeryu Feb 12, 2025 •

edited

Loading

ParkSeryu Feb 12, 2025 •

edited

Loading