Skip to content

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

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 15 commits into from
Feb 12, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
b276f8a
refactor: Security 에서의 공통 Exception 처리를 FilterChainProxy 에서 담당하도록 변경
ghojeong Feb 5, 2025
602c8ee
test: 자신의 정보를 조회하는데 인증이 필요한 부분에 대한 테스트 작성
ghojeong Feb 5, 2025
48331b6
feat: CheckAuthenticationFilter 로직을 대체하는 RequestAuthorizationManager 구현
ghojeong Feb 5, 2025
4de11e1
feat: RequestMatcher 와 RequestMatcherEntry 구현
ghojeong Feb 5, 2025
eb3cb25
feat: 4가지 AuthorizationManager 구현; PermitAll, DenyAll, Authenticated,…
ghojeong Feb 6, 2025
b7d14db
feat: AuthorizationManager 에서 RequestMatcherEntry 를 활용하도록 Config 에 Be…
ghojeong Feb 6, 2025
fbc5c7d
refactor: SecuredMethodInterceptor 의 인가 로직을 SecuredAuthorizationManag…
ghojeong Feb 6, 2025
6b8f819
fix: Secured 어노테이션이 없으면, MethodInterceptor 에서 인증 및 인가 과정을 거치지 않도록 수정함
ghojeong Feb 9, 2025
40fea8d
refactor: Rename HasAuthorityAuthorizationManager to AuthorityAuthori…
ghojeong Feb 11, 2025
edd41c3
fix: security 패키지에서 stream 문을 사용하지 않도록 변경
ghojeong Feb 11, 2025
e23654e
fix: 삼항연산자 대신 if 문으로 early return 을 해주도록 모두 변경
ghojeong Feb 11, 2025
e4211fa
fix: Secured 어노테이션이 없는 메서드일 경우 인가 값이 false 가 되도록 수정
ghojeong Feb 11, 2025
ad3fc21
refactor: deprecated 된 AuthorizationManager.check 대신, AuthorizationRe…
ghojeong Feb 11, 2025
8e1648f
test: Fixture 를 enum 으로 생성
ghojeong Feb 11, 2025
68599c8
test: 어드민 유저와 일반 유저의 인가 테스트 케이스를 분리
ghojeong Feb 11, 2025
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
25 changes: 18 additions & 7 deletions src/main/java/nextstep/app/SecurityConfig.java
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
package nextstep.app;

import jakarta.servlet.http.HttpServletRequest;
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.SecuredMethodInterceptor;
import nextstep.security.authorization.manager.AuthenticatedAuthorizationManager;
import nextstep.security.authorization.manager.AuthorityAuthorizationManager;
import nextstep.security.authorization.manager.AuthorizationManager;
import nextstep.security.authorization.manager.DenyAllAuthorizationManager;
import nextstep.security.authorization.manager.PermitAllAuthorizationManager;
import nextstep.security.authorization.manager.RequestAuthorizationManager;
import nextstep.security.config.DefaultSecurityFilterChain;
import nextstep.security.config.DelegatingFilterProxy;
import nextstep.security.config.FilterChainProxy;
Expand All @@ -22,6 +28,10 @@
import java.util.List;
import java.util.Set;

import static nextstep.security.authorization.matcher.RequestMatcherEntry.createDefaultMatcher;
import static nextstep.security.authorization.matcher.RequestMatcherEntry.createMvcMatcher;
import static org.springframework.http.HttpMethod.GET;

@EnableAspectJAutoProxy
@Configuration
public class SecurityConfig {
Expand All @@ -46,19 +56,20 @@ public FilterChainProxy filterChainProxy(List<SecurityFilterChain> securityFilte
public SecuredMethodInterceptor securedMethodInterceptor() {
return new SecuredMethodInterceptor();
}
// @Bean
// public SecuredAspect securedAspect() {
// return new SecuredAspect();
// }

@Bean
public SecurityFilterChain securityFilterChain() {
final AuthorizationManager<HttpServletRequest> authorizationManager = new RequestAuthorizationManager(List.of(
createMvcMatcher(GET, "/members", new AuthorityAuthorizationManager<>("ADMIN")),
createMvcMatcher(GET, "/members/me", new AuthenticatedAuthorizationManager<>()),
createMvcMatcher(GET, "/search", new PermitAllAuthorizationManager<>())
), createDefaultMatcher(new DenyAllAuthorizationManager<>()));
return new DefaultSecurityFilterChain(
List.of(
new SecurityContextHolderFilter(),
new UsernamePasswordAuthenticationFilter(userDetailsService()),
new BasicAuthenticationFilter(userDetailsService()),
new CheckAuthenticationFilter()
new AuthorizationFilter(authorizationManager)
)
);
}
Expand Down
22 changes: 22 additions & 0 deletions src/main/java/nextstep/app/ui/MemberController.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,16 @@

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;

import java.util.List;
import java.util.Optional;

@RestController
public class MemberController {
Expand All @@ -30,4 +34,22 @@ public ResponseEntity<List<Member>> search() {
List<Member> members = memberRepository.findAll();
return ResponseEntity.ok(members);
}

@Secured("USER")
@GetMapping("/members/me")
public ResponseEntity<Member> me() {
final Authentication authentication = SecurityContextHolder
.getContext().getAuthentication();
return ResponseEntity.ok(
Optional.ofNullable(authentication)
.flatMap(this::findMe)
.orElseThrow(AuthenticationException::new)
);
}

private Optional<Member> findMe(Authentication authentication) {
return memberRepository.findByEmail(
authentication.getPrincipal().toString()
);
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package nextstep.security.authentication;

import jakarta.servlet.FilterChain;
import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import nextstep.security.context.SecurityContext;
Expand All @@ -10,6 +11,7 @@
import org.springframework.util.StringUtils;
import org.springframework.web.filter.OncePerRequestFilter;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.Base64;
import java.util.List;
Expand All @@ -26,23 +28,23 @@ public BasicAuthenticationFilter(UserDetailsService userDetailsService) {
}

@Override
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) {
try {
Authentication authentication = convert(request);
if (authentication == null) {
filterChain.doFilter(request, response);
return;
}

Authentication authResult = this.authenticationManager.authenticate(authentication);
SecurityContext context = SecurityContextHolder.createEmptyContext();
context.setAuthentication(authResult);
SecurityContextHolder.setContext(context);

protected void doFilterInternal(
HttpServletRequest request,
HttpServletResponse response,
FilterChain filterChain
) throws ServletException, IOException {
Authentication authentication = convert(request);
if (authentication == null) {
filterChain.doFilter(request, response);
} catch (Exception e) {
response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
return;
}

Authentication authResult = this.authenticationManager.authenticate(authentication);
SecurityContext context = SecurityContextHolder.createEmptyContext();
context.setAuthentication(authResult);
SecurityContextHolder.setContext(context);

filterChain.doFilter(request, response);
}

private Authentication convert(HttpServletRequest request) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package nextstep.security.authorization;

import jakarta.servlet.FilterChain;
import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import nextstep.security.authentication.Authentication;
import nextstep.security.authentication.AuthenticationException;
import nextstep.security.authorization.manager.AuthorizationManager;
import nextstep.security.context.SecurityContextHolder;
import org.springframework.web.filter.OncePerRequestFilter;

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 {
final Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
if (authentication == null || !authentication.isAuthenticated()) {
throw new AuthenticationException();
}
if (!authorizationManager.authorize(authentication, request).isGranted()) {
throw new ForbiddenException();
}
filterChain.doFilter(request, response);
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import nextstep.security.authentication.Authentication;
import nextstep.security.authentication.AuthenticationException;
import nextstep.security.authorization.manager.SecuredAuthorizationManager;
import nextstep.security.context.SecurityContextHolder;
import org.aopalliance.aop.Advice;
import org.aopalliance.intercept.MethodInterceptor;
Expand All @@ -11,8 +12,6 @@
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 Pointcut pointcut;
Expand All @@ -23,16 +22,17 @@ public SecuredMethodInterceptor() {

@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();
}
if (!invocation.getMethod().isAnnotationPresent(Secured.class)) {
return invocation.proceed();
}
final Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
if (authentication == null || !authentication.isAuthenticated()) {
throw new AuthenticationException();
}
if (!SecuredAuthorizationManager.getInstance().authorize(
authentication, invocation
).isGranted()) {
throw new ForbiddenException();
}
return invocation.proceed();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package nextstep.security.authorization.manager;

import nextstep.security.authentication.Authentication;

public class AuthenticatedAuthorizationManager<T> implements AuthorizationManager<T> {
@Override
public AuthorizationResult authorize(Authentication authentication, T target) {
return AuthorizationDecision.of(
authentication != null && authentication.isAuthenticated()
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package nextstep.security.authorization.manager;

import nextstep.security.authentication.Authentication;

import java.util.Set;

public class AuthorityAuthorizationManager<T> implements AuthorizationManager<T> {
private final Set<String> authorities;

public AuthorityAuthorizationManager(String... authorities) {
this.authorities = Set.of(authorities);
}

@Override
public AuthorizationResult authorize(Authentication authentication, T target) {
return AuthorizationDecision.of(isGranted(authentication));
}

private boolean isGranted(Authentication authentication) {
return authentication != null
&& authentication.isAuthenticated()
&& anyMatch(authentication);
}

private boolean anyMatch(Authentication authentication) {
for (var authority : authentication.getAuthorities()) {
if (authorities.contains(authority)) {
return true;
}
}
return false;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package nextstep.security.authorization.manager;

public enum AuthorizationDecision implements AuthorizationResult {
GRANTED(true), NOT_GRANTED(false);

private final boolean granted;

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

public static AuthorizationDecision of(boolean granted) {
if (granted) {
return GRANTED;
}
return NOT_GRANTED;
}

public boolean isGranted() {
return this.granted;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package nextstep.security.authorization.manager;

import nextstep.security.authentication.Authentication;

@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.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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() 를 사용하려 할 것 같습니다.
불편하더라도 동작 및 정보조회가 실패하면 고객 클레임 선에서 끝나지만,
덜 불편하겠다고 현금성 있는 리소스가 털리게 되면 고객은 법적으로 고소를 할테니까요. ㅋㅋ

Choose a reason for hiding this comment

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

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

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

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

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

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

AuthorizationResult authorize(Authentication authentication, T target);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package nextstep.security.authorization.manager;

public interface AuthorizationResult {
boolean isGranted();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package nextstep.security.authorization.manager;

import nextstep.security.authentication.Authentication;

import static nextstep.security.authorization.manager.AuthorizationDecision.NOT_GRANTED;

public class DenyAllAuthorizationManager<T> implements AuthorizationManager<T> {
@Override
public AuthorizationResult authorize(Authentication authentication, T target) {
return NOT_GRANTED;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package nextstep.security.authorization.manager;

import nextstep.security.authentication.Authentication;

import static nextstep.security.authorization.manager.AuthorizationDecision.GRANTED;

public class PermitAllAuthorizationManager<T> implements AuthorizationManager<T> {
@Override
public AuthorizationResult authorize(Authentication authentication, T target) {
return GRANTED;
}
}
Loading