Skip to content

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

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

Open
wants to merge 10 commits into
base: yeongunheo
Choose a base branch
from
Open
40 changes: 27 additions & 13 deletions src/main/java/nextstep/app/SecurityConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,28 @@
import nextstep.security.authentication.AuthenticationException;
import nextstep.security.authentication.BasicAuthenticationFilter;
import nextstep.security.authentication.UsernamePasswordAuthenticationFilter;
import nextstep.security.authorization.CheckAuthenticationFilter;
import nextstep.security.authorization.SecuredAspect;
import nextstep.security.authorization.SecuredMethodInterceptor;
import nextstep.security.authorization.AuthenticatedAuthorizationManager;
import nextstep.security.authorization.AuthorityAuthorizationManager;
import nextstep.security.authorization.AuthorizationFilter;
import nextstep.security.authorization.AuthorizationManagerBeforeMethodInterceptor;
import nextstep.security.authorization.AuthenticatedAuthorizationStrategy;
import nextstep.security.authorization.PermitAllAuthorizationManager;
import nextstep.security.authorization.RequestMatcherDelegatingAuthorizationManager;
import nextstep.security.authorization.SecuredAuthorizationManager;
import nextstep.security.config.DefaultSecurityFilterChain;
import nextstep.security.config.DelegatingFilterProxy;
import nextstep.security.config.FilterChainProxy;
import nextstep.security.config.SecurityFilterChain;
import nextstep.security.context.SecurityContextHolderFilter;
import nextstep.security.userdetails.UserDetails;
import nextstep.security.userdetails.UserDetailsService;
import nextstep.security.util.AnyRequestMatcher;
import nextstep.security.util.MvcRequestMatcher;
import nextstep.security.util.RequestMatcherEntry;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.EnableAspectJAutoProxy;
import org.springframework.http.HttpMethod;

import java.util.List;
import java.util.Set;
Expand All @@ -42,27 +51,32 @@ public FilterChainProxy filterChainProxy(List<SecurityFilterChain> securityFilte
return new FilterChainProxy(securityFilterChains);
}

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

@Bean
public SecurityFilterChain securityFilterChain() {
return new DefaultSecurityFilterChain(
List.of(
new SecurityContextHolderFilter(),
new UsernamePasswordAuthenticationFilter(userDetailsService()),
new BasicAuthenticationFilter(userDetailsService()),
new CheckAuthenticationFilter()
new AuthorizationFilter(new RequestMatcherDelegatingAuthorizationManager(List.of(

Choose a reason for hiding this comment

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

질문 주신 부분에 대한 저의 의견 여기에 남기겠습니다.

Q. RequestMatcherRegistry의 역할이 필요한 RequestMatcher을 모두 등록한 뒤 HTTP 요청이 들어왔을 때 적절한 RequestMatcher를 찾아서 반환하는 것으로 이해했는데 올바르게 이해한걸까요?

A.RequestMatcherRegistry는 보안 규칙을 등록하는 역할을 합니다.
요청이 들어오면 Spring Security의 AuthorizationFilter가 RequestMatcherDelegatingAuthorizationManager를 통해
적절한 RequestMatcher를 찾아 인가(Authorization)를 수행합니다.

즉, RequestMatcherRegistry는 보안 설정을 구성하는 단계에서 사용되며,
실제 요청이 들어왔을 때 요청과 매칭되는 RequestMatcher를 찾아 적용하는 것은 RequestMatcherDelegatingAuthorizationManager이고,
이를 실행하여 최종적으로 인가를 결정하는 것은 AuthorizationFilter의 역할일 것 같네요 😄

new RequestMatcherEntry<>(new MvcRequestMatcher(HttpMethod.GET, "/members/me"), new AuthenticatedAuthorizationManager<>(new AuthenticatedAuthorizationStrategy())),
new RequestMatcherEntry<>(new MvcRequestMatcher(HttpMethod.GET, "/members"), new AuthorityAuthorizationManager<>(Set.of("ADMIN"))),
new RequestMatcherEntry<>(new MvcRequestMatcher(HttpMethod.GET, "/search"), new PermitAllAuthorizationManager<>()),
new RequestMatcherEntry<>(new AnyRequestMatcher(), new PermitAllAuthorizationManager())
)))
)
);
}

@Bean
public AuthorizationManagerBeforeMethodInterceptor authorizationManagerBeforeMethodInterceptor() {
return new AuthorizationManagerBeforeMethodInterceptor(securedAuthorizationManager());
}

private SecuredAuthorizationManager securedAuthorizationManager() {
return new SecuredAuthorizationManager(new AuthorityAuthorizationManager<>(Set.of("ADMIN")));

Choose a reason for hiding this comment

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

현재는 @Secured 를 사용할 경우 ADMIN 권한 밖에 체크가 불가능 하겠네요 🤔
Spring Security 에서는 AuthoritiesAuthorizationManager 를 사용하여 @Secured("ROLE_ADMIN")와 같은 권한을 체크하고 있는데 이부분에 대해 한번 학습해보고 개선해보면 좋을 것 같아요 😄

}

@Bean
public UserDetailsService userDetailsService() {
return username -> {
Expand Down
17 changes: 17 additions & 0 deletions src/main/java/nextstep/app/ui/MemberController.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@

import nextstep.app.domain.Member;
import nextstep.app.domain.MemberRepository;
import nextstep.security.authentication.Authentication;
import nextstep.security.authentication.AuthenticationException;
import nextstep.security.authorization.Secured;
import nextstep.security.context.SecurityContextHolder;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RestController;
Expand Down Expand Up @@ -30,4 +33,18 @@ public ResponseEntity<List<Member>> search() {
List<Member> members = memberRepository.findAll();
return ResponseEntity.ok(members);
}

@GetMapping("/members/me")
public ResponseEntity<Member> me() {
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
if (authentication == null) {
throw new AuthenticationException();
}

String email = authentication.getPrincipal().toString();
Member member = memberRepository.findByEmail(email)
.orElseThrow(RuntimeException::new);

return ResponseEntity.ok(member);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import jakarta.servlet.FilterChain;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import nextstep.security.authorization.ForbiddenException;
import nextstep.security.context.SecurityContext;
import nextstep.security.context.SecurityContextHolder;
import nextstep.security.userdetails.UserDetailsService;
Expand Down Expand Up @@ -40,8 +41,12 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
SecurityContextHolder.setContext(context);

filterChain.doFilter(request, response);
} catch (Exception e) {
} catch (AuthenticationException e) {
response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
} catch (ForbiddenException e) {
response.setStatus(HttpServletResponse.SC_FORBIDDEN);
} catch (Exception e) {
response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package nextstep.security.authorization;

import nextstep.security.authentication.Authentication;

public class AuthenticatedAuthorizationManager<T> implements AuthorizationManager<T> {

private final AuthorizationStrategy authorizationStrategy;

public AuthenticatedAuthorizationManager(AuthorizationStrategy authorizationStrategy) {
this.authorizationStrategy = authorizationStrategy;
}

@Override
public AuthorizationDecision check(Authentication authentication, T object) {
var granted = authorizationStrategy.isGranted(authentication);
return new AuthorizationDecision(granted);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package nextstep.security.authorization;

import nextstep.security.authentication.Authentication;

public class AuthenticatedAuthorizationStrategy implements AuthorizationStrategy {

@Override
public boolean isGranted(Authentication authentication) {
if (authentication == null) {
return false;
}

return authentication.isAuthenticated();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package nextstep.security.authorization;

import nextstep.security.authentication.Authentication;

import java.util.Set;

public class AuthorityAuthorizationManager<T> implements AuthorizationManager<T> {

private final Set<String> authorities;

public AuthorityAuthorizationManager(Set<String> authorities) {

Choose a reason for hiding this comment

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

가변인자(String... authorities)를 사용하면 여러 권한을 더 쉽게 전달할 수 있을 것 같네요 😄
new AuthorityAuthorizationManager("ROLE_ADMIN", "ROLE_USER")

this.authorities = authorities;
}

@Override
public AuthorizationDecision check(Authentication authentication, T object) {

boolean granted = authentication.getAuthorities()
.stream()
.anyMatch(authorities::contains);

return new AuthorizationDecision(granted);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package nextstep.security.authorization;

public class AuthorizationDecision {

private final boolean granted;

public AuthorizationDecision(boolean granted) {
this.granted = granted;
}

public boolean isGranted() {
return granted;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,31 +8,28 @@
import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;

import java.io.IOException;
import java.util.Set;

public class CheckAuthenticationFilter extends OncePerRequestFilter {
private static final String DEFAULT_REQUEST_URI = "/members";
public class AuthorizationFilter extends OncePerRequestFilter {

private final AuthorizationManager<HttpServletRequest> authorizationManager;

public AuthorizationFilter(AuthorizationManager<HttpServletRequest> authorizationManager) {
this.authorizationManager = authorizationManager;
}

@Override
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException {
if (!DEFAULT_REQUEST_URI.equals(request.getRequestURI())) {
filterChain.doFilter(request, response);
return;
}

Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
if (authentication == null || !authentication.isAuthenticated()) {
response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
return;
}
AuthorizationDecision decision = this.authorizationManager.check(authentication, request);

Set<String> authorities = authentication.getAuthorities();
if (!authorities.contains("ADMIN")) {
response.setStatus(HttpServletResponse.SC_FORBIDDEN);
return;
if (decision != null && !decision.isGranted()) {
throw new ForbiddenException();
}

filterChain.doFilter(request, response);

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package nextstep.security.authorization;

import nextstep.security.authentication.Authentication;

@FunctionalInterface
public interface AuthorizationManager<T> {
AuthorizationDecision check(Authentication authentication, T object);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package nextstep.security.authorization;

import nextstep.security.authentication.Authentication;
import nextstep.security.context.SecurityContextHolder;
import org.aopalliance.aop.Advice;
import org.aopalliance.intercept.MethodInterceptor;
import org.aopalliance.intercept.MethodInvocation;
import org.springframework.aop.Pointcut;
import org.springframework.aop.PointcutAdvisor;
import org.springframework.aop.support.annotation.AnnotationMatchingPointcut;

public class AuthorizationManagerBeforeMethodInterceptor implements MethodInterceptor, PointcutAdvisor {

Choose a reason for hiding this comment

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

Q.애노테이션을 활용한 인가 처리가 잘 이해가 되지 않아, SecuredAuthorizationManager를 만든 뒤 AuthorizationManagerBeforeMethodInterceptor에서 이를 가져다 사용하는 방향으로 구현했는데, 이게 괜찮은 방향인지는 잘 모르겠네요. 이부분에 대한 피드백을 한번 받아보고 싶습니다!

A. 잘 구현해 주셨네요! 👍
AuthorizationManagerBeforeMethodInterceptor 에서 @Secured가 적용된 메서드를 감지하고, AuthorizationManager를 실행하여 사용자의 권한을 확인한 후 인가 여부를 결정하는 구조로 동작하는 방식으로 잘 구현해 주셨습니다.

다만, 다른 코멘트에서 남겼듯이 AuthoritiesAuthorizationManager 에 대해서는 한번 확인해 보시고 개선해 보면 좋을 것 같아요.
혹시 추가적으로 어려우신 부분이 있다면 남겨주시면 답변 드리도록 하겠습니다 😄


private final AuthorizationManager<MethodInvocation> authorizationManager;

public AuthorizationManagerBeforeMethodInterceptor(AuthorizationManager<MethodInvocation> authorizationManager) {
this.authorizationManager = authorizationManager;
}

@Override
public Object invoke(MethodInvocation methodInvocation) throws Throwable {

Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
AuthorizationDecision decision = authorizationManager.check(authentication, methodInvocation);

if (decision != null && !decision.isGranted()) {
throw new ForbiddenException();
}

return methodInvocation.proceed();
}

@Override
public Pointcut getPointcut() {
return new AnnotationMatchingPointcut(null, Secured.class);
}

@Override
public Advice getAdvice() {
return this;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package nextstep.security.authorization;

import nextstep.security.authentication.Authentication;

public interface AuthorizationStrategy {

boolean isGranted(Authentication authentication);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package nextstep.security.authorization;

import nextstep.security.authentication.Authentication;

public class PermitAllAuthorizationManager<T> implements AuthorizationManager<T> {

@Override
public AuthorizationDecision check(Authentication authentication, T object) {
return new AuthorizationDecision(true);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package nextstep.security.authorization;

import jakarta.servlet.http.HttpServletRequest;
import nextstep.security.authentication.Authentication;
import nextstep.security.util.RequestMatcher;
import nextstep.security.util.RequestMatcherEntry;

import java.util.List;

public class RequestMatcherDelegatingAuthorizationManager implements AuthorizationManager<HttpServletRequest> {

private final List<RequestMatcherEntry<AuthorizationManager>> mappings;

public RequestMatcherDelegatingAuthorizationManager(List<RequestMatcherEntry<AuthorizationManager>> mappings) {
this.mappings = mappings;
}

@Override
public AuthorizationDecision check(Authentication authentication, HttpServletRequest request) {

for (RequestMatcherEntry<AuthorizationManager> mapping : this.mappings) {
RequestMatcher matcher = mapping.getRequestMatcher();
RequestMatcher.MatchResult matchResult = matcher.matcher(request);
if (matchResult.isMatch()) {
AuthorizationManager manager = mapping.getEntry();

return manager.check(authentication, request);
}
}

return new AuthorizationDecision(false);
}
}
Loading