Skip to content

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

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

Merged
merged 13 commits into from
Feb 12, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 35 additions & 8 deletions src/main/java/nextstep/app/SecurityConfig.java
Original file line number Diff line number Diff line change
@@ -1,26 +1,39 @@
package nextstep.app;

import jakarta.servlet.http.HttpServletRequest;
import java.util.ArrayList;
import nextstep.app.domain.Member;
import nextstep.app.domain.MemberRepository;
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.AuthorizationFilter;
import nextstep.security.authorization.AuthorizationManager;
import nextstep.security.authorization.SecuredMethodInterceptor;
import nextstep.security.authorization.method.SecuredAuthorizationManager;
import nextstep.security.authorization.web.AuthenticatedAuthorizationManager;
import nextstep.security.authorization.web.AuthorityAuthorizationManager;
import nextstep.security.authorization.web.DenyAllAuthorizationManager;
import nextstep.security.authorization.web.RequestMatcherDelegatingAuthorizationManager;
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 nextstep.security.authorization.web.PermitAllAuthorizationManager;
import org.aopalliance.intercept.MethodInvocation;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.EnableAspectJAutoProxy;

import java.util.List;
import java.util.Set;
import org.springframework.http.HttpMethod;

@EnableAspectJAutoProxy
@Configuration
Expand All @@ -44,12 +57,26 @@ public FilterChainProxy filterChainProxy(List<SecurityFilterChain> securityFilte

@Bean
public SecuredMethodInterceptor securedMethodInterceptor() {
return new SecuredMethodInterceptor();
return new SecuredMethodInterceptor(securedAuthorizationManager());
}

@Bean
public AuthorizationManager<MethodInvocation> securedAuthorizationManager() {
return new SecuredAuthorizationManager();
}

@Bean
public AuthorizationManager<HttpServletRequest> requestAuthorizationManager() {
List<RequestMatcherEntry<AuthorizationManager>> mappings = new ArrayList<>();
mappings.add(new RequestMatcherEntry<>(new MvcRequestMatcher(HttpMethod.GET, "/members"),
new AuthorityAuthorizationManager<HttpServletRequest>(Set.of("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);
}
// @Bean
// public SecuredAspect securedAspect() {
// return new SecuredAspect();
// }

@Bean
public SecurityFilterChain securityFilterChain() {
Expand All @@ -58,7 +85,7 @@ public SecurityFilterChain securityFilterChain() {
new SecurityContextHolderFilter(),
new UsernamePasswordAuthenticationFilter(userDetailsService()),
new BasicAuthenticationFilter(userDetailsService()),
new CheckAuthenticationFilter()
new AuthorizationFilter(requestAuthorizationManager())
)
);
}
Expand Down
14 changes: 14 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,15 @@ 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();

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,22 @@
package nextstep.security.authorization;

import nextstep.security.authorization.web.AuthorizationResult;

public class AuthorizationDecision implements AuthorizationResult {
public static final AuthorizationDecision ALLOW = new AuthorizationDecision(true);
public static final AuthorizationDecision DENY = new AuthorizationDecision(false);

private final boolean granted;

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

public boolean isGranted() {
return granted;
}

public static AuthorizationDecision of(boolean granted) {

Choose a reason for hiding this comment

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

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

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

return granted ? ALLOW : DENY;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package nextstep.security.authorization;

import nextstep.security.authentication.Authentication;
import nextstep.security.authorization.web.AuthorizationResult;
import nextstep.security.context.SecurityContextHolder;
import org.springframework.web.filter.OncePerRequestFilter;

import jakarta.servlet.FilterChain;
import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import java.io.IOException;

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 {
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
AuthorizationResult authorizeResult = authorizationManager.authorize(authentication, request);

if (!authorizeResult.isGranted()) {
throw new ForbiddenException();
}

filterChain.doFilter(request, response);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package nextstep.security.authorization;

import nextstep.security.authentication.Authentication;
import nextstep.security.authorization.web.AuthorizationResult;

@FunctionalInterface
public interface AuthorizationManager<T> {

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

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

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

@Deprecated
AuthorizationDecision check(Authentication authentication, T object);

Choose a reason for hiding this comment

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

현재 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

Copy link
Author

@ParkSeryu ParkSeryu Feb 12, 2025

Choose a reason for hiding this comment

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

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


default AuthorizationResult authorize(Authentication authentication, T object) {
return check(authentication, object);
}
}

This file was deleted.

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

import nextstep.security.authentication.Authentication;
import nextstep.security.authentication.AuthenticationException;
import nextstep.security.authorization.web.AuthorizationResult;
import nextstep.security.context.SecurityContextHolder;
import org.aopalliance.aop.Advice;
import org.aopalliance.intercept.MethodInterceptor;
Expand All @@ -11,29 +11,25 @@
import org.springframework.aop.framework.AopInfrastructureBean;
import org.springframework.aop.support.annotation.AnnotationMatchingPointcut;

import java.lang.reflect.Method;

public class SecuredMethodInterceptor implements MethodInterceptor, PointcutAdvisor, AopInfrastructureBean {

private final AuthorizationManager<MethodInvocation> authorizationManager;
private final Pointcut pointcut;

public SecuredMethodInterceptor() {
public SecuredMethodInterceptor(AuthorizationManager<MethodInvocation> authorizationManager) {
this.authorizationManager = authorizationManager;
this.pointcut = new AnnotationMatchingPointcut(null, Secured.class);
}

@Override
public Object invoke(MethodInvocation invocation) throws Throwable {
Method method = invocation.getMethod();
if (method.isAnnotationPresent(Secured.class)) {
Secured secured = method.getAnnotation(Secured.class);
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
if (authentication == null) {
throw new AuthenticationException();
}
if (!authentication.getAuthorities().contains(secured.value())) {
throw new ForbiddenException();
}
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
AuthorizationResult authorizationResult = authorizationManager.authorize(authentication, invocation);

if (!authorizationResult.isGranted()) {
throw new ForbiddenException();
}

return invocation.proceed();
}

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

import java.lang.reflect.Method;
import java.util.Collection;
import java.util.Collections;
import java.util.Set;
import nextstep.security.authentication.Authentication;
import nextstep.security.authorization.AuthorizationDecision;
import nextstep.security.authorization.AuthorizationManager;
import nextstep.security.authorization.Secured;
import nextstep.security.authorization.web.AuthorityAuthorizationManager;
import org.aopalliance.intercept.MethodInvocation;

public class SecuredAuthorizationManager implements AuthorizationManager<MethodInvocation> {

Choose a reason for hiding this comment

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

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를 사용하고 있기는 하지만 관련하여 참고하시면 이해에 도움이 되실 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

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

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


private AuthorityAuthorizationManager<Collection<String>> authorityAuthorizationManager;

public void setAuthorityAuthorizationManager(Collection<String> authorities) {
authorityAuthorizationManager = new AuthorityAuthorizationManager<>(authorities);
}

@Override
public AuthorizationDecision check(Authentication authentication, MethodInvocation invocation) {
Collection<String> authorities = getAuthorities(invocation);

if (authorities.isEmpty()) {
return null;
}
setAuthorityAuthorizationManager(authorities);
return authorities.isEmpty() ? null : authorityAuthorizationManager.check(authentication, authorities);
}

private Collection<String> getAuthorities(MethodInvocation invocation) {
Method method = invocation.getMethod();

if (!method.isAnnotationPresent(Secured.class)) {
return Collections.emptySet();
}

Secured secured = method.getAnnotation(Secured.class);
return Set.of(secured.value());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package nextstep.security.authorization.web;

import jakarta.servlet.http.HttpServletRequest;
import nextstep.security.authentication.Authentication;
import nextstep.security.authorization.AuthorizationDecision;
import nextstep.security.authorization.AuthorizationManager;

public class AuthenticatedAuthorizationManager implements AuthorizationManager<HttpServletRequest> {
@Override
public AuthorizationDecision check(Authentication authentication, HttpServletRequest object) {
if (authentication != null && authentication.isAuthenticated()) {
return AuthorizationDecision.ALLOW;
}
return AuthorizationDecision.DENY;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package nextstep.security.authorization.web;

import java.util.Collection;
import nextstep.security.authentication.Authentication;
import nextstep.security.authentication.AuthenticationException;
import nextstep.security.authorization.AuthorizationDecision;
import nextstep.security.authorization.AuthorizationManager;

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

private final Collection<String> authorities;

public AuthorityAuthorizationManager(Collection<String> authorities) {
this.authorities = authorities;
}


@Override
public AuthorizationDecision check(Authentication authentication, T object) {
if (authentication == null) {
throw new AuthenticationException();
}

boolean hasAuthority = isAuthorized(authentication, authorities);

return AuthorizationDecision.of(hasAuthority);
}


private boolean isAuthorized(Authentication authentication, Collection<String> authorities) {
for (String authority : authentication.getAuthorities()) {
if (authorities.contains(authority)) {
return true;
}
}
return false;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package nextstep.security.authorization.web;

public interface AuthorizationResult {
boolean isGranted();
}
Loading