Skip to content

Commit

Permalink
Fix AuthorizationFilter incorrectly extending OncePerRequestFilter
Browse files Browse the repository at this point in the history 
Closes gh-12102
  • Loading branch information
@marcusdacoregio @sjohnr
marcusdacoregio authored and sjohnr committed Oct 28, 2022
1 parent e7fe778 commit 1f481aa
Show file tree
Hide file tree
Showing 2 changed files with 215 additions and 7 deletions.
97 changes: 91 additions & 6 deletions web/src/main/java/org/springframework/security/web/access/intercept/AuthorizationFilter.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2021 the original author or authors.
* Copyright 2002-2022 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -18,8 +18,11 @@

import java.io.IOException;

import javax.servlet.DispatcherType;
import javax.servlet.FilterChain;
import javax.servlet.ServletException;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

Expand All @@ -28,7 +31,7 @@
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.util.Assert;
import org.springframework.web.filter.OncePerRequestFilter;
import org.springframework.web.filter.GenericFilterBean;

/**
* An authorization filter that restricts access to the URL using
Expand All @@ -37,10 +40,16 @@
* @author Evgeniy Cheban
* @since 5.5
*/
public class AuthorizationFilter extends OncePerRequestFilter {
public class AuthorizationFilter extends GenericFilterBean {

private final AuthorizationManager<HttpServletRequest> authorizationManager;

private boolean observeOncePerRequest = true;

private boolean filterErrorDispatch = false;

private boolean filterAsyncDispatch = false;

/**
* Creates an instance.
* @param authorizationManager the {@link AuthorizationManager} to use
Expand All @@ -51,11 +60,53 @@ public AuthorizationFilter(AuthorizationManager<HttpServletRequest> authorizatio
}

@Override
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain chain)
throws ServletException, IOException {

this.authorizationManager.verify(this::getAuthentication, request);
filterChain.doFilter(request, response);
HttpServletRequest request = (HttpServletRequest) servletRequest;
HttpServletResponse response = (HttpServletResponse) servletResponse;

if (this.observeOncePerRequest && isApplied(request)) {
chain.doFilter(request, response);
return;
}

if (skipDispatch(request)) {
chain.doFilter(request, response);
return;
}

String alreadyFilteredAttributeName = getAlreadyFilteredAttributeName();
request.setAttribute(alreadyFilteredAttributeName, Boolean.TRUE);
try {
this.authorizationManager.verify(this::getAuthentication, request);
chain.doFilter(request, response);
}
finally {
request.removeAttribute(alreadyFilteredAttributeName);
}
}

private boolean skipDispatch(HttpServletRequest request) {
if (DispatcherType.ERROR.equals(request.getDispatcherType()) && !this.filterErrorDispatch) {
return true;
}
if (DispatcherType.ASYNC.equals(request.getDispatcherType()) && !this.filterAsyncDispatch) {
return true;
}
return false;
}

private boolean isApplied(HttpServletRequest request) {
return request.getAttribute(getAlreadyFilteredAttributeName()) != null;
}

private String getAlreadyFilteredAttributeName() {
String name = getFilterName();
if (name == null) {
name = getClass().getName();
}
return name + ".APPLIED";
}

private Authentication getAuthentication() {
Expand All @@ -75,4 +126,38 @@ public AuthorizationManager<HttpServletRequest> getAuthorizationManager() {
return this.authorizationManager;
}

public boolean isObserveOncePerRequest() {
return this.observeOncePerRequest;
}

/**
* Sets whether this filter apply only once per request. By default, this is
* <code>true</code>, meaning the filter will only execute once per request. Sometimes
* users may wish it to execute more than once per request, such as when JSP forwards
* are being used and filter security is desired on each included fragment of the HTTP
* request.
* @param observeOncePerRequest whether the filter should only be applied once per
* request
*/
public void setObserveOncePerRequest(boolean observeOncePerRequest) {
this.observeOncePerRequest = observeOncePerRequest;
}

/**
* If set to true, the filter will be applied to error dispatcher. Defaults to false.
* @param filterErrorDispatch whether the filter should be applied to error dispatcher
*/
public void setFilterErrorDispatch(boolean filterErrorDispatch) {
this.filterErrorDispatch = filterErrorDispatch;
}

/**
* If set to true, the filter will be applied to the async dispatcher. Defaults to
* false.
* @param filterAsyncDispatch whether the filter should be applied to async dispatch
*/
public void setFilterAsyncDispatch(boolean filterAsyncDispatch) {
this.filterAsyncDispatch = filterAsyncDispatch;
}

}
125 changes: 124 additions & 1 deletion ...test/java/org/springframework/security/web/access/intercept/AuthorizationFilterTests.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2021 the original author or authors.
* Copyright 2002-2022 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -16,15 +16,20 @@

package org.springframework.security.web.access.intercept;

import java.io.IOException;
import java.util.function.Supplier;

import javax.servlet.DispatcherType;
import javax.servlet.FilterChain;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;

import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.mockito.ArgumentCaptor;

import org.springframework.mock.web.MockFilterChain;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.mock.web.MockHttpServletResponse;
import org.springframework.security.access.AccessDeniedException;
Expand All @@ -36,13 +41,15 @@
import org.springframework.security.core.context.SecurityContext;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.security.core.context.SecurityContextImpl;
import org.springframework.test.util.ReflectionTestUtils;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.BDDMockito.willThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;

Expand All @@ -53,6 +60,24 @@
*/
public class AuthorizationFilterTests {

private static final String ALREADY_FILTERED_ATTRIBUTE_NAME = "org.springframework.security.web.access.intercept.AuthorizationFilter.APPLIED";

private AuthorizationFilter filter;

private AuthorizationManager<HttpServletRequest> authorizationManager;

private MockHttpServletRequest request = new MockHttpServletRequest();

private final MockHttpServletResponse response = new MockHttpServletResponse();

private final FilterChain chain = new MockFilterChain();

@Before
public void setup() {
this.authorizationManager = mock(AuthorizationManager.class);
this.filter = new AuthorizationFilter(this.authorizationManager);
}

@After
public void tearDown() {
SecurityContextHolder.clearContext();
Expand Down Expand Up @@ -132,4 +157,102 @@ public void getAuthorizationManager() {
assertThat(authorizationFilter.getAuthorizationManager()).isSameAs(authorizationManager);
}

@Test
public void doFilterWhenObserveOncePerRequestTrueAndIsAppliedThenNotInvoked() throws ServletException, IOException {
setIsAppliedTrue();
this.filter.setObserveOncePerRequest(true);
this.filter.doFilter(this.request, this.response, this.chain);
verifyNoInteractions(this.authorizationManager);
}

@Test
public void doFilterWhenObserveOncePerRequestTrueAndNotAppliedThenInvoked() throws ServletException, IOException {
this.filter.setObserveOncePerRequest(true);
this.filter.doFilter(this.request, this.response, this.chain);
verify(this.authorizationManager).verify(any(), any());
}

@Test
public void doFilterWhenObserveOncePerRequestFalseAndIsAppliedThenInvoked() throws ServletException, IOException {
setIsAppliedTrue();
this.filter.setObserveOncePerRequest(false);
this.filter.doFilter(this.request, this.response, this.chain);
verify(this.authorizationManager).verify(any(), any());
}

@Test
public void doFilterWhenObserveOncePerRequestFalseAndNotAppliedThenInvoked() throws ServletException, IOException {
this.filter.setObserveOncePerRequest(false);
this.filter.doFilter(this.request, this.response, this.chain);
verify(this.authorizationManager).verify(any(), any());
}

@Test
public void doFilterWhenFilterErrorDispatchFalseAndIsErrorThenNotInvoked() throws ServletException, IOException {
this.request.setDispatcherType(DispatcherType.ERROR);
this.filter.setFilterErrorDispatch(false);
this.filter.doFilter(this.request, this.response, this.chain);
verifyNoInteractions(this.authorizationManager);
}

@Test
public void doFilterWhenFilterErrorDispatchTrueAndIsErrorThenInvoked() throws ServletException, IOException {
this.request.setDispatcherType(DispatcherType.ERROR);
this.filter.setFilterErrorDispatch(true);
this.filter.doFilter(this.request, this.response, this.chain);
verify(this.authorizationManager).verify(any(), any());
}

@Test
public void doFilterWhenFilterThenSetAlreadyFilteredAttribute() throws ServletException, IOException {
this.request = mock(MockHttpServletRequest.class);
this.filter.doFilter(this.request, this.response, this.chain);
verify(this.request).setAttribute(ALREADY_FILTERED_ATTRIBUTE_NAME, Boolean.TRUE);
}

@Test
public void doFilterWhenFilterThenRemoveAlreadyFilteredAttribute() throws ServletException, IOException {
this.request = spy(MockHttpServletRequest.class);
this.filter.doFilter(this.request, this.response, this.chain);
verify(this.request).setAttribute(ALREADY_FILTERED_ATTRIBUTE_NAME, Boolean.TRUE);
assertThat(this.request.getAttribute(ALREADY_FILTERED_ATTRIBUTE_NAME)).isNull();
}

@Test
public void doFilterWhenFilterAsyncDispatchTrueAndIsAsyncThenInvoked() throws ServletException, IOException {
this.request.setDispatcherType(DispatcherType.ASYNC);
this.filter.setFilterAsyncDispatch(true);
this.filter.doFilter(this.request, this.response, this.chain);
verify(this.authorizationManager).verify(any(), any());
}

@Test
public void doFilterWhenFilterAsyncDispatchFalseAndIsAsyncThenNotInvoked() throws ServletException, IOException {
this.request.setDispatcherType(DispatcherType.ASYNC);
this.filter.setFilterAsyncDispatch(false);
this.filter.doFilter(this.request, this.response, this.chain);
verifyNoInteractions(this.authorizationManager);
}

@Test
public void filterWhenFilterErrorDispatchDefaultThenFalse() {
Boolean filterErrorDispatch = (Boolean) ReflectionTestUtils.getField(this.filter, "filterErrorDispatch");
assertThat(filterErrorDispatch).isFalse();
}

@Test
public void filterWhenFilterAsyncDispatchDefaultThenFalse() {
Boolean filterAsyncDispatch = (Boolean) ReflectionTestUtils.getField(this.filter, "filterAsyncDispatch");
assertThat(filterAsyncDispatch).isFalse();
}

@Test
public void filterWhenObserveOncePerRequestDefaultThenTrue() {
assertThat(this.filter.isObserveOncePerRequest()).isTrue();
}

private void setIsAppliedTrue() {
this.request.setAttribute(ALREADY_FILTERED_ATTRIBUTE_NAME, Boolean.TRUE);
}

}

0 comments on commit 1f481aa

Please sign in to comment.