Allow previously authorized users to access the error page

Prior to this commit, the `ErrorPageSecurityFilter` verified if
access to the error page was allowed by invoking the
`WebInvocationPrivilegeEvaluator` with the Authentication from the
`SecurityContextHolder`.
This meant that access to the error page was denied for a `null` Authentication
 or `AnonymousAuthenticationToken` in cases where the error page required
authenticated access. This prevented authorized users from accessing the
error page in case the Authentication wasn't retrievable for the error dispatch,
which is the case for `@Transient` authentication or stateless session policy.

This commit updates the `ErrorPageSecurityFilter` to check access to the error page
only if the error is an authn or authz error in cases where an authentication object
is not found in the SecurityContextHolder. This makes the error response consistent
when bad credentials or no credentials are used while also allowing access to previously
authorized users.

Fixes gh-28953
This commit is contained in:
Madhura Bhave
2021-12-15 12:28:19 -08:00
parent c077ebecf7
commit d9d161cd6b
9 changed files with 379 additions and 97 deletions

View File

@@ -31,6 +31,7 @@ import javax.servlet.http.HttpServletResponse;
import org.springframework.beans.factory.NoSuchBeanDefinitionException;
import org.springframework.context.ApplicationContext;
import org.springframework.security.authentication.AnonymousAuthenticationToken;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.security.web.access.WebInvocationPrivilegeEvaluator;
@@ -67,17 +68,28 @@ public class ErrorPageSecurityFilter implements Filter {
private void doFilter(HttpServletRequest request, HttpServletResponse response, FilterChain chain)
throws IOException, ServletException {
if (DispatcherType.ERROR.equals(request.getDispatcherType()) && !isAllowed(request)) {
sendError(request, response);
Integer errorCode = (Integer) request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE);
if (DispatcherType.ERROR.equals(request.getDispatcherType()) && !isAllowed(request, errorCode)) {
response.sendError((errorCode != null) ? errorCode : 401);
return;
}
chain.doFilter(request, response);
}
private boolean isAllowed(HttpServletRequest request) {
String uri = request.getRequestURI();
private boolean isAllowed(HttpServletRequest request, Integer errorCode) {
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
return getPrivilegeEvaluator().isAllowed(uri, authentication);
if (isUnauthenticated(authentication) && isNotAuthenticationError(errorCode)) {
return true;
}
return getPrivilegeEvaluator().isAllowed(request.getRequestURI(), authentication);
}
private boolean isUnauthenticated(Authentication authentication) {
return (authentication == null || authentication instanceof AnonymousAuthenticationToken);
}
private boolean isNotAuthenticationError(Integer errorCode) {
return (errorCode == null || (errorCode != 401 && errorCode != 403));
}
private WebInvocationPrivilegeEvaluator getPrivilegeEvaluator() {
@@ -98,11 +110,6 @@ public class ErrorPageSecurityFilter implements Filter {
}
}
private void sendError(HttpServletRequest request, HttpServletResponse response) throws IOException {
Integer errorCode = (Integer) request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE);
response.sendError((errorCode != null) ? errorCode : 401);
}
/**
* {@link WebInvocationPrivilegeEvaluator} that always allows access.
*/

View File

@@ -20,6 +20,7 @@ import javax.servlet.DispatcherType;
import javax.servlet.FilterChain;
import javax.servlet.RequestDispatcher;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
@@ -27,6 +28,9 @@ import org.springframework.beans.factory.NoSuchBeanDefinitionException;
import org.springframework.context.ApplicationContext;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.mock.web.MockHttpServletResponse;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContext;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.security.web.access.WebInvocationPrivilegeEvaluator;
import static org.assertj.core.api.Assertions.assertThat;
@@ -64,6 +68,11 @@ class ErrorPageSecurityFilterTests {
this.securityFilter = new ErrorPageSecurityFilter(this.context);
}
@AfterEach
void tearDown() {
SecurityContextHolder.clearContext();
}
@Test
void whenAccessIsAllowedShouldContinueDownFilterChain() throws Exception {
given(this.privilegeEvaluator.isAllowed(anyString(), any())).willReturn(true);
@@ -83,6 +92,9 @@ class ErrorPageSecurityFilterTests {
@Test
void whenAccessIsDeniedAndNoErrorCodeAttributeOnRequest() throws Exception {
given(this.privilegeEvaluator.isAllowed(anyString(), any())).willReturn(false);
SecurityContext securityContext = mock(SecurityContext.class);
SecurityContextHolder.setContext(securityContext);
given(securityContext.getAuthentication()).willReturn(mock(Authentication.class));
this.securityFilter.doFilter(this.request, this.response, this.filterChain);
verifyNoInteractions(this.filterChain);
assertThat(this.response.getStatus()).isEqualTo(401);