2단계 - 인가 리뷰 요청드립니다. #21

jukekxm · 2025-02-13T15:42:55Z

고민한 부분
- securedAuthorizationManager 로직이 수행되는 시점이 SecuredMethodInterceptor 안에서 수행되는 것이 맞는지 궁금합니다.

리뷰 부탁드립니다!

* AuthorizationManager 추가

parkeeseul

안녕하세요. 인가 미션을 함께 하게 된 박이슬 입니다 😄
미션 잘 진행해 주셨네요! 👍
몇 가지 코멘트 남겨두었으니 확인 부탁드립니다!
화이팅 입니다 🔥

parkeeseul · 2025-02-13T22:23:55Z

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

+    @Override
+    public AuthorizationDecision check(Authentication authentication, MethodInvocation invocation) {
+        Method method = invocation.getMethod();
+        Secured secured = method.getAnnotation(Secured.class);


Spring AOP 환경에서는 대상 객체가 프록시 객체로 감싸질 수 있기 때문에,
method.getAnnotation(Secured.class)를 사용하면 실제 구현체의 메서드에서 선언된 @Secured 어노테이션을 찾지 못할 가능성이 있습니다.method.getAnnotation 와 AopUtils.getMostSpecificMethod() 어떤 차이가 있을까요? 😄

AopUtils.getMostSpecificMethod() 로 실제 메서드의 정보를 가져올 수 있겠네요!
그래도 어노테이션 정보를 가져오려면 getAnnotation 메서드는 계속 사용해야 하는 거죠?
수정해봤는데 맞는 방식인지 확인 부탁드립니다!

네 맞습니다
어노테이션 정보를 가져오는 것은 동일하게 getAnnotation(Secured.class)을 사용해야 합니다. 😄

parkeeseul · 2025-02-14T14:19:07Z

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

+public class AuthenticatedAuthorizationManager implements AuthorizationManager<HttpServletRequest> {
+    @Override
+    public AuthorizationDecision check(Authentication authentication, HttpServletRequest request) {
+        request.setAttribute("username", authentication.getPrincipal());


AuthenticatedAuthorizationManager는 인증된 사용자의 접근을 허용할지 여부를 결정하는 인가(Authorization) 역할을 하는데요,
때문에 request.setAttribute("username", authentication.getPrincipal()); 는 인가의 책임을 벗어난 동작으로 보여집니다. 개선해 보면 좋을 것 같아요. 😄

parkeeseul · 2025-02-14T14:28:55Z

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

@@ -24,6 +28,15 @@ public ResponseEntity<List<Member>> list() {
        return ResponseEntity.ok(members);
    }

+    @GetMapping("/members/me")
+    public ResponseEntity<Member> get(HttpServletRequest request) {
+        String username = (String) request.getAttribute("username");


request.setAttribute를 통해 값을 저장하고 다시 가져오는 과정을 거치지 않고,
SecurityContext에서 직접 가져올 수 있을 것 같네요 😄

parkeeseul · 2025-02-14T14:46:59Z

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

+
+    @Override
+    public boolean matches(HttpServletRequest request) {
+        return request.getMethod().matches(method.name()) && request.getRequestURI().matches(uri);


matches()는 정규식 매칭을 수행하는 메서드 이므로, 단순한 문자열 비교에는 적합하지 않을 것 같아요.
HTTP 메서드(GET, POST 등)는 정규식이 필요하지 않으므로, equalsIgnoreCase()를 사용하면 성능 면에서도 더 효율적일 것 같네요 😊

제가 왜 matches를 사용했는지 모르겠네요.. 꼼꼼한 리뷰 감사합니다 👍

parkeeseul · 2025-02-14T14:55:21Z

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

+                .findFirst()
+                .map(RequestMatcherEntry::getEntry)
+                .map(entry -> entry.check(authentication, request))
+                .orElse(new AuthorizationDecision(false));


orElseGet() 와 orElse()는 어떤 차이가 있을까요? 😄

orElse로 구현하게 되면, null이 아니어도 AuthorizationDecision 객체 생성 로직은 수행될 것 같네요.
orElseGet으로 수정했습니다!

parkeeseul · 2025-02-14T15:09:17Z

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

+        if (!method.isAnnotationPresent(Secured.class)) {
+            return invocation.proceed();


@Secured 어노테이션이 존재하는지 다시 검사하고 있는데요,
이미 AnnotationMatchingPointcut을 통해 @Secured이 붙은 메서드만 인터셉트하도록 설정되어 있기 때문에,
if (!method.isAnnotationPresent(Secured.class)) 부분은 제거해도 될 것 같아요. 😊

parkeeseul · 2025-02-14T15:12:20Z

src/test/java/nextstep/app/BasicAuthTest.java

+                .andExpect(jsonPath("$.password").value("password"));
+    }
+
+    @DisplayName("인증된 사용자는 자신의 정보를 조회할 수 있다.")


테스트 설명이 잘못되어 있네요 😄

parkeeseul · 2025-02-14T15:18:53Z

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

+            throw new AuthenticationException();
+        }
+
+        AuthorizationDecision authorizationDecision = securedAuthorizationManager.check(authentication, invocation);


고민한 부분
Q. securedAuthorizationManager 로직이 수행되는 시점이 SecuredMethodInterceptor 안에서 수행되는 것이 맞는지 궁금합니다.

A. AOP를 통해 메서드가 실행되기 전에 인가(Authorization) 로직을 수행하도록 잘 구현해 주셨네요! 👍
혹시 고민했던 부분이 있으셨다면 남겨주시면 저도 고민해보고 답변 드리도록 하겠습니다 😄

다른 AuthorizationManager 들은 Filter에서 인가 로직이 수행되는데 이와 다르게 AOP를 통해 수행되는 로직이어서 맞는 방식인지 궁금했었습니다..! 답변 감사합니다 👍

Filter 기반 인가는 HTTP 요청 단위에서 적용되지만,
@Secured는 개별 메서드 호출 단위에서 보안 검사를 해야 해서 적절한 구현 방식입니다 😄

parkeeseul

피드백 잘 반영해 주셨네요! 고생 많으셨습니다 👍
한 가지 추가 코멘트와 답변 남겨두었으니 확인 부탁드리고
소소한 코멘트라서 이번 미션 이만 머지하도록 하겠습니다!
(시간이 되신다면 선택 미션 진행하셔도 좋을 것 같네요 😄 )

남은 강의들과 미션들도 화이팅 입니다 🔥

parkeeseul · 2025-02-15T06:37:04Z

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

@@ -10,7 +11,9 @@ public class SecuredAuthorizationManager implements AuthorizationManager<MethodI
    @Override
    public AuthorizationDecision check(Authentication authentication, MethodInvocation invocation) {
        Method method = invocation.getMethod();
-        Secured secured = method.getAnnotation(Secured.class);
+        Class<?> targetClass = AopUtils.getTargetClass(method.getClass());


method.getClass()는 Method 클래스 자체의 타입을 반환하므로, 올바른 대상 클래스(Target Class)를 가져올 수 없습니다.
invocation.getThis()를 사용하여 실제 대상 객체(Target Object)의 클래스를 가져오는 것이 좋겠네요 😄
Class<?> targetClass = AopUtils.getTargetClass(invocation.getThis());

parkeeseul · 2025-02-15T06:40:49Z

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

+    @Override
+    public AuthorizationDecision check(Authentication authentication, MethodInvocation invocation) {
+        Method method = invocation.getMethod();
+        Secured secured = method.getAnnotation(Secured.class);


네 맞습니다
어노테이션 정보를 가져오는 것은 동일하게 getAnnotation(Secured.class)을 사용해야 합니다. 😄

parkeeseul · 2025-02-15T06:47:37Z

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

+            throw new AuthenticationException();
+        }
+
+        AuthorizationDecision authorizationDecision = securedAuthorizationManager.check(authentication, invocation);


Filter 기반 인가는 HTTP 요청 단위에서 적용되지만,
@Secured는 개별 메서드 호출 단위에서 보안 검사를 해야 해서 적절한 구현 방식입니다 😄

jukekxm added 4 commits February 11, 2025 02:22

feat: /members/me 엔드포인트 구현 및 테스트 작성

84a1ab5

feat: 권한 검증용 필터 (AuthorizationFilter) 구현

f22629c

feat: AuthorizationFilter에서 인가 로직 수행하도록 수정

29b3db2

* AuthorizationManager 추가

feat: 요청별 권한 검증 정보를 별도의 객체로 분리하여 관리

ccfaf51

parkeeseul requested changes Feb 14, 2025

View reviewed changes

jukekxm added 5 commits February 15, 2025 12:39

fix: SecurityContext에서 username 가져오도록 수정

2bd7aa9

fix: HTTP method, uri를 비교할 때 equalsIgnoreCase로 변경

174cb93

fix: 불필요한 객체 생성을 줄이기 위해 orElseGet으로 변경

92c0221

fix: 잘못된 테스트 설명 수정

1a0595c

fix: 프록시 객체가 아닌 실제 메서드의 정보를 읽을 수 있도록 수정

fabdb86

parkeeseul approved these changes Feb 15, 2025

View reviewed changes

parkeeseul merged commit 257143a into next-step:jukekxm Feb 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2단계 - 인가 리뷰 요청드립니다. #21

2단계 - 인가 리뷰 요청드립니다. #21

jukekxm commented Feb 13, 2025

parkeeseul left a comment

parkeeseul Feb 13, 2025

jukekxm Feb 15, 2025

parkeeseul Feb 15, 2025

parkeeseul Feb 14, 2025

parkeeseul Feb 14, 2025

parkeeseul Feb 14, 2025

jukekxm Feb 15, 2025

parkeeseul Feb 14, 2025

jukekxm Feb 15, 2025

parkeeseul Feb 14, 2025

parkeeseul Feb 14, 2025

parkeeseul Feb 14, 2025

jukekxm Feb 15, 2025

parkeeseul Feb 15, 2025

parkeeseul left a comment

parkeeseul Feb 15, 2025

parkeeseul Feb 15, 2025

parkeeseul Feb 15, 2025

		if (!method.isAnnotationPresent(Secured.class)) {
		return invocation.proceed();

2단계 - 인가 리뷰 요청드립니다. #21

2단계 - 인가 리뷰 요청드립니다. #21

Conversation

jukekxm commented Feb 13, 2025

parkeeseul 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

parkeeseul 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