Cwikius-Spring
/
spring-security
mirror of https://github.com/spring-projects/spring-security.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Revert "Cache Control only written if not set" 

This reverts commit 242b831f20.
Spring MVC fixed the issue we were working around and the changes
in Spring Security were unreliable.

Fixes gh-3975
This commit is contained in:
Rob Winch 2016-10-24 14:16:26 -05:00
parent e62596f36d
commit 57d7ad05f9
4 changed files with 36 additions and 218 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

58
web/src/main/java/org/springframework/security/web/header/HeaderWriterFilter.java
View File

@ -15,17 +15,15 @@
*/ */
package org.springframework.security.web.header; package org.springframework.security.web.header;
import java.io.IOException; import org.springframework.util.Assert;
import java.util.List; import org.springframework.web.filter.OncePerRequestFilter;
import javax.servlet.FilterChain; import javax.servlet.FilterChain;
import javax.servlet.ServletException; import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpServletResponse;
import java.io.IOException;
import org.springframework.security.web.util.OnCommittedResponseWrapper; import java.util.*;
import org.springframework.util.Assert;
import org.springframework.web.filter.OncePerRequestFilter;
/** /**
* Filter implementation to add headers to the current response. Can be useful to add * Filter implementation to add headers to the current response. Can be useful to add
@ -58,52 +56,12 @@ public class HeaderWriterFilter extends OncePerRequestFilter {
@Override @Override
protected void doFilterInternal(HttpServletRequest request, protected void doFilterInternal(HttpServletRequest request,
HttpServletResponse response, FilterChain filterChain) HttpServletResponse response, FilterChain filterChain)
throws ServletException, IOException { throws ServletException, IOException {
HeaderWriterResponse headerWriterResponse = new HeaderWriterResponse(request, for (HeaderWriter headerWriter : headerWriters) {
response, this.headerWriters); headerWriter.writeHeaders(request, response);
try {
filterChain.doFilter(request, headerWriterResponse);
}
finally {
headerWriterResponse.writeHeaders();
} }
filterChain.doFilter(request, response);
} }
static class HeaderWriterResponse extends OnCommittedResponseWrapper {
private final HttpServletRequest request;
private final List<HeaderWriter> headerWriters;
HeaderWriterResponse(HttpServletRequest request, HttpServletResponse response,
List<HeaderWriter> headerWriters) {
super(response);
this.request = request;
this.headerWriters = headerWriters;
}
/*
* (non-Javadoc)
*
* @see org.springframework.security.web.util.OnCommittedResponseWrapper#
* onResponseCommitted()
*/
@Override
protected void onResponseCommitted() {
writeHeaders();
this.disableOnResponseCommitted();
}
protected void writeHeaders() {
if (isDisableOnResponseCommitted()) {
return;
}
for (HeaderWriter headerWriter : this.headerWriters) {
headerWriter.writeHeaders(this.request, getHttpResponse());
}
}
private HttpServletResponse getHttpResponse() {
return (HttpServletResponse) getResponse();
}
}
} }

46
web/src/main/java/org/springframework/security/web/header/writers/CacheControlHeadersWriter.java
View File

@ -15,20 +15,14 @@
*/ */
package org.springframework.security.web.header.writers; package org.springframework.security.web.header.writers;
import java.lang.reflect.Method;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.List; import java.util.List;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.springframework.security.web.header.Header; import org.springframework.security.web.header.Header;
import org.springframework.security.web.header.HeaderWriter;
import org.springframework.util.ReflectionUtils;
/** /**
* Inserts headers to prevent caching if no cache control headers have been specified. * A {@link StaticHeadersWriter} that inserts headers to prevent caching. Specifically it
* Specifically it adds the following headers: * adds the following headers:
* <ul> * <ul>
* <li>Cache-Control: no-cache, no-store, max-age=0, must-revalidate</li> * <li>Cache-Control: no-cache, no-store, max-age=0, must-revalidate</li>
* <li>Pragma: no-cache</li> * <li>Pragma: no-cache</li>
@ -38,47 +32,21 @@ import org.springframework.util.ReflectionUtils;
* @author Rob Winch * @author Rob Winch
* @since 3.2 * @since 3.2
*/ */
public final class CacheControlHeadersWriter implements HeaderWriter { public final class CacheControlHeadersWriter extends StaticHeadersWriter {
private static final String EXPIRES = "Expires";
private static final String PRAGMA = "Pragma";
private static final String CACHE_CONTROL = "Cache-Control";
private final Method getHeaderMethod;
private final HeaderWriter delegate;
/** /**
* Creates a new instance * Creates a new instance
*/ */
public CacheControlHeadersWriter() { public CacheControlHeadersWriter() {
this.delegate = new StaticHeadersWriter(createHeaders()); super(createHeaders());
this.getHeaderMethod = ReflectionUtils.findMethod(HttpServletResponse.class,
"getHeader", String.class);
}
@Override
public void writeHeaders(HttpServletRequest request, HttpServletResponse response) {
if (hasHeader(response, CACHE_CONTROL) || hasHeader(response, EXPIRES)
|| hasHeader(response, PRAGMA)) {
return;
}
this.delegate.writeHeaders(request, response);
}
private boolean hasHeader(HttpServletResponse response, String headerName) {
if (this.getHeaderMethod == null) {
return false;
}
return ReflectionUtils.invokeMethod(this.getHeaderMethod, response,
headerName) != null;
} }
private static List<Header> createHeaders() { private static List<Header> createHeaders() {
List<Header> headers = new ArrayList<Header>(2); List<Header> headers = new ArrayList<Header>(2);
headers.add(new Header(CACHE_CONTROL, headers.add(new Header("Cache-Control",
"no-cache, no-store, max-age=0, must-revalidate")); "no-cache, no-store, max-age=0, must-revalidate"));
headers.add(new Header(PRAGMA, "no-cache")); headers.add(new Header("Pragma", "no-cache"));
headers.add(new Header(EXPIRES, "0")); headers.add(new Header("Expires", "0"));
return headers; return headers;
} }
} }

56
web/src/test/java/org/springframework/security/web/header/HeaderWriterFilterTests.java
View File

@ -15,32 +15,21 @@
*/ */
package org.springframework.security.web.header; package org.springframework.security.web.header;
import java.io.IOException; import static org.assertj.core.api.Assertions.assertThat;
import java.util.ArrayList; import static org.mockito.Mockito.verify;
import java.util.Arrays;
import java.util.List;
import javax.servlet.FilterChain; import java.util.ArrayList;
import javax.servlet.ServletException; import java.util.List;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.junit.Test; import org.junit.Test;
import org.junit.runner.RunWith; import org.junit.runner.RunWith;
import org.mockito.Mock; import org.mockito.Mock;
import org.mockito.runners.MockitoJUnitRunner; import org.mockito.runners.MockitoJUnitRunner;
import org.springframework.mock.web.MockFilterChain; import org.springframework.mock.web.MockFilterChain;
import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.mock.web.MockHttpServletResponse;
import org.springframework.security.web.header.HeaderWriter;
import static org.assertj.core.api.Assertions.assertThat; import org.springframework.security.web.header.HeaderWriterFilter;
import static org.mockito.Matchers.any;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.verifyZeroInteractions;
/** /**
* Tests for the {@code HeadersFilter} * Tests for the {@code HeadersFilter}
@ -71,8 +60,8 @@ public class HeaderWriterFilterTests {
@Test @Test
public void additionalHeadersShouldBeAddedToTheResponse() throws Exception { public void additionalHeadersShouldBeAddedToTheResponse() throws Exception {
List<HeaderWriter> headerWriters = new ArrayList<HeaderWriter>(); List<HeaderWriter> headerWriters = new ArrayList<HeaderWriter>();
headerWriters.add(this.writer1); headerWriters.add(writer1);
headerWriters.add(this.writer2); headerWriters.add(writer2);
HeaderWriterFilter filter = new HeaderWriterFilter(headerWriters); HeaderWriterFilter filter = new HeaderWriterFilter(headerWriters);
@ -82,34 +71,9 @@ public class HeaderWriterFilterTests {
filter.doFilter(request, response, filterChain); filter.doFilter(request, response, filterChain);
verify(this.writer1).writeHeaders(request, response); verify(writer1).writeHeaders(request, response);
verify(this.writer2).writeHeaders(request, response); verify(writer2).writeHeaders(request, response);
assertThat(filterChain.getRequest()).isEqualTo(request); // verify the filterChain assertThat(filterChain.getRequest()).isEqualTo(request); // verify the filterChain
// continued // continued
} }
// gh-2953
@Test
public void headersDelayed() throws Exception {
HeaderWriterFilter filter = new HeaderWriterFilter(
Arrays.<HeaderWriter>asList(this.writer1));
MockHttpServletRequest request = new MockHttpServletRequest();
MockHttpServletResponse response = new MockHttpServletResponse();
filter.doFilter(request, response, new FilterChain() {
@Override
public void doFilter(ServletRequest request, ServletResponse response)
throws IOException, ServletException {
verifyZeroInteractions(HeaderWriterFilterTests.this.writer1);
response.flushBuffer();
verify(HeaderWriterFilterTests.this.writer1).writeHeaders(
any(HttpServletRequest.class), any(HttpServletResponse.class));
}
});
verifyNoMoreInteractions(this.writer1);
}
} }

94
web/src/test/java/org/springframework/security/web/header/writers/CacheControlHeadersWriterTests.java
View File

@ -15,32 +15,19 @@
*/ */
package org.springframework.security.web.header.writers; package org.springframework.security.web.header.writers;
import java.util.Arrays; import static org.assertj.core.api.Assertions.assertThat;
import javax.servlet.http.HttpServletResponse; import java.util.Arrays;
import org.junit.Before; import org.junit.Before;
import org.junit.Test; import org.junit.Test;
import org.junit.runner.RunWith;
import org.powermock.core.classloader.annotations.PrepareOnlyThisForTest;
import org.powermock.modules.junit4.PowerMockRunner;
import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.mock.web.MockHttpServletResponse;
import org.springframework.util.ReflectionUtils;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Matchers.anyString;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.when;
import static org.powermock.api.mockito.PowerMockito.spy;
/** /**
* @author Rob Winch * @author Rob Winch
* *
*/ */
@RunWith(PowerMockRunner.class)
@PrepareOnlyThisForTest(ReflectionUtils.class)
public class CacheControlHeadersWriterTests { public class CacheControlHeadersWriterTests {
private MockHttpServletRequest request; private MockHttpServletRequest request;
@ -51,79 +38,20 @@ public class CacheControlHeadersWriterTests {
@Before @Before
public void setup() { public void setup() {
this.request = new MockHttpServletRequest(); request = new MockHttpServletRequest();
this.response = new MockHttpServletResponse(); response = new MockHttpServletResponse();
this.writer = new CacheControlHeadersWriter(); writer = new CacheControlHeadersWriter();
} }
@Test @Test
public void writeHeaders() { public void writeHeaders() {
this.writer.writeHeaders(this.request, this.response); writer.writeHeaders(request, response);
assertThat(this.response.getHeaderNames().size()).isEqualTo(3); assertThat(response.getHeaderNames().size()).isEqualTo(3);
assertThat(this.response.getHeaderValues("Cache-Control")).isEqualTo( assertThat(response.getHeaderValues("Cache-Control")).isEqualTo(
Arrays.asList("no-cache, no-store, max-age=0, must-revalidate")); Arrays.asList("no-cache, no-store, max-age=0, must-revalidate"));
assertThat(this.response.getHeaderValues("Pragma")) assertThat(response.getHeaderValues("Pragma")).isEqualTo(
.isEqualTo(Arrays.asList("no-cache")); Arrays.asList("no-cache"));
assertThat(this.response.getHeaderValues("Expires")) assertThat(response.getHeaderValues("Expires")).isEqualTo(Arrays.asList("0"));
.isEqualTo(Arrays.asList("0"));
}
@Test
public void writeHeadersServlet25() {
spy(ReflectionUtils.class);
when(ReflectionUtils.findMethod(HttpServletResponse.class, "getHeader",
String.class)).thenReturn(null);
this.response = spy(this.response);
doThrow(NoSuchMethodError.class).when(this.response).getHeader(anyString());
this.writer = new CacheControlHeadersWriter();
this.writer.writeHeaders(this.request, this.response);
assertThat(this.response.getHeaderNames().size()).isEqualTo(3);
assertThat(this.response.getHeaderValues("Cache-Control")).isEqualTo(
Arrays.asList("no-cache, no-store, max-age=0, must-revalidate"));
assertThat(this.response.getHeaderValues("Pragma"))
.isEqualTo(Arrays.asList("no-cache"));
assertThat(this.response.getHeaderValues("Expires"))
.isEqualTo(Arrays.asList("0"));
}
// gh-2953
@Test
public void writeHeadersDisabledIfCacheControl() {
this.response.setHeader("Cache-Control", "max-age: 123");
this.writer.writeHeaders(this.request, this.response);
assertThat(this.response.getHeaderNames()).hasSize(1);
assertThat(this.response.getHeaderValues("Cache-Control"))
.containsOnly("max-age: 123");
assertThat(this.response.getHeaderValue("Pragma")).isNull();
assertThat(this.response.getHeaderValue("Expires")).isNull();
}
@Test
public void writeHeadersDisabledIfPragma() {
this.response.setHeader("Pragma", "mock");
this.writer.writeHeaders(this.request, this.response);
assertThat(this.response.getHeaderNames()).hasSize(1);
assertThat(this.response.getHeaderValues("Pragma")).containsOnly("mock");
assertThat(this.response.getHeaderValue("Expires")).isNull();
assertThat(this.response.getHeaderValue("Cache-Control")).isNull();
}
@Test
public void writeHeadersDisabledIfExpires() {
this.response.setHeader("Expires", "mock");
this.writer.writeHeaders(this.request, this.response);
assertThat(this.response.getHeaderNames()).hasSize(1);
assertThat(this.response.getHeaderValues("Expires")).containsOnly("mock");
assertThat(this.response.getHeaderValue("Cache-Control")).isNull();
assertThat(this.response.getHeaderValue("Pragma")).isNull();
} }
} }