From d86550f64be1a53125463a46f85f3a3e8d0adcce Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Thu, 28 Feb 2019 10:51:04 -0700 Subject: [PATCH] Polish Tests and Error Messages MockMvc matchers are best matched with the MockMvc execution API - it's a little odd to try and use them inside of an AssertJ assertion since they do their own asserting. It's more readable to place "this." in front of member variables. It's best to test just one class at a time in a unit test. Issue: gh-4187 --- .../logout/HeaderWriterLogoutHandler.java | 2 +- .../writers/ClearSiteDataHeaderWriter.java | 2 +- .../HeaderWriterLogoutHandlerTests.java | 58 ++++--------------- .../ClearSiteDataHeaderWriterTests.java | 35 +++++------ 4 files changed, 26 insertions(+), 71 deletions(-) diff --git a/web/src/main/java/org/springframework/security/web/authentication/logout/HeaderWriterLogoutHandler.java b/web/src/main/java/org/springframework/security/web/authentication/logout/HeaderWriterLogoutHandler.java index 1583aa460e..ad2fdba837 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/logout/HeaderWriterLogoutHandler.java +++ b/web/src/main/java/org/springframework/security/web/authentication/logout/HeaderWriterLogoutHandler.java @@ -38,7 +38,7 @@ public final class HeaderWriterLogoutHandler implements LogoutHandler { * @throws {@link IllegalArgumentException} if headerWriter is null. */ public HeaderWriterLogoutHandler(HeaderWriter headerWriter) { - Assert.notNull(headerWriter, "headerWriter cannot be null."); + Assert.notNull(headerWriter, "headerWriter cannot be null"); this.headerWriter = headerWriter; } diff --git a/web/src/main/java/org/springframework/security/web/header/writers/ClearSiteDataHeaderWriter.java b/web/src/main/java/org/springframework/security/web/header/writers/ClearSiteDataHeaderWriter.java index b86ffbed95..25184550cb 100644 --- a/web/src/main/java/org/springframework/security/web/header/writers/ClearSiteDataHeaderWriter.java +++ b/web/src/main/java/org/springframework/security/web/header/writers/ClearSiteDataHeaderWriter.java @@ -67,7 +67,7 @@ public final class ClearSiteDataHeaderWriter implements HeaderWriter { * @throws {@link IllegalArgumentException} if sources is null or empty. */ public ClearSiteDataHeaderWriter(String ...sources) { - Assert.notEmpty(sources, "Sources cannot be empty or null."); + Assert.notEmpty(sources, "sources cannot be empty or null"); this.requestMatcher = new SecureRequestMatcher(); this.headerValue = Stream.of(sources).map(this::quote).collect(Collectors.joining(", ")); } diff --git a/web/src/test/java/org/springframework/security/web/authentication/logout/HeaderWriterLogoutHandlerTests.java b/web/src/test/java/org/springframework/security/web/authentication/logout/HeaderWriterLogoutHandlerTests.java index e01d72d327..31888de0c9 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/logout/HeaderWriterLogoutHandlerTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/logout/HeaderWriterLogoutHandlerTests.java @@ -20,24 +20,23 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; + import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.core.Authentication; -import org.springframework.security.web.header.writers.ClearSiteDataHeaderWriter; +import org.springframework.security.web.header.HeaderWriter; -import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; +import static org.mockito.Mockito.verify; /** * * @author Rafiullah Hamedy + * @author Josh Cummings * * @see {@link HeaderWriterLogoutHandler} */ public class HeaderWriterLogoutHandlerTests { - private static final String HEADER_NAME = "Clear-Site-Data"; - private MockHttpServletResponse response; private MockHttpServletRequest request; @@ -51,54 +50,19 @@ public class HeaderWriterLogoutHandlerTests { } @Test - public void createInstanceWhenHeaderWriterIsNullThenThrowsException() { + public void constructorWhenHeaderWriterIsNullThenThrowsException() { this.thrown.expect(IllegalArgumentException.class); - this.thrown.expectMessage("headerWriter cannot be null."); + this.thrown.expectMessage("headerWriter cannot be null"); new HeaderWriterLogoutHandler(null); } @Test - public void createInstanceWhenSourceIsNullThenThrowsException() { - this.thrown.expect(IllegalArgumentException.class); - this.thrown.expectMessage("Sources cannot be empty or null."); + public void logoutWhenHasHeaderWriterThenInvoked() { + HeaderWriter headerWriter = mock(HeaderWriter.class); + HeaderWriterLogoutHandler handler = new HeaderWriterLogoutHandler(headerWriter); + handler.logout(this.request, this.response, mock(Authentication.class)); - new HeaderWriterLogoutHandler(new ClearSiteDataHeaderWriter()); - } - - @Test - public void logoutWhenRequestIsNotSecureThenHeaderIsNotPresent() { - HeaderWriterLogoutHandler handler = new HeaderWriterLogoutHandler( - new ClearSiteDataHeaderWriter("cache")); - - handler.logout(request, response, mock(Authentication.class)); - - assertThat(header().doesNotExist(HEADER_NAME)); - } - - @Test - public void logoutWhenRequestIsSecureThenHeaderIsPresentMatchesWildCardSource() { - HeaderWriterLogoutHandler handler = new HeaderWriterLogoutHandler( - new ClearSiteDataHeaderWriter("*")); - - this.request.setSecure(true); - - handler.logout(request, response, mock(Authentication.class)); - - assertThat(header().stringValues(HEADER_NAME, "\"*\"")); - } - - @Test - public void logoutWhenRequestIsSecureThenHeaderValueMatchesSource() { - HeaderWriterLogoutHandler handler = new HeaderWriterLogoutHandler( - new ClearSiteDataHeaderWriter("cache", "cookies", "storage", - "executionContexts")); - - this.request.setSecure(true); - - handler.logout(request, response, mock(Authentication.class)); - - assertThat(header().stringValues(HEADER_NAME, "\"cache\", \"cookies\", \"storage\", " - + "\"executionContexts\"")); + verify(headerWriter).writeHeaders(this.request, this.response); } } diff --git a/web/src/test/java/org/springframework/security/web/header/writers/ClearSiteDataHeaderWriterTests.java b/web/src/test/java/org/springframework/security/web/header/writers/ClearSiteDataHeaderWriterTests.java index fb7ece8804..93f8bf5af2 100644 --- a/web/src/test/java/org/springframework/security/web/header/writers/ClearSiteDataHeaderWriterTests.java +++ b/web/src/test/java/org/springframework/security/web/header/writers/ClearSiteDataHeaderWriterTests.java @@ -20,15 +20,16 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; + import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import static org.assertj.core.api.Assertions.assertThat; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; /** * * @author Rafiullah Hamedy + * @author Josh Cummings * * @see {@link ClearSiteDataHeaderWriter} */ @@ -43,53 +44,43 @@ public class ClearSiteDataHeaderWriterTests { @Before public void setup() { - request = new MockHttpServletRequest(); - request.setSecure(true); - response = new MockHttpServletResponse(); + this.request = new MockHttpServletRequest(); + this.request.setSecure(true); + this.response = new MockHttpServletResponse(); } @Test public void createInstanceWhenMissingSourceThenThrowsException() { this.thrown.expect(Exception.class); - this.thrown.expectMessage("Sources cannot be empty or null."); + this.thrown.expectMessage("sources cannot be empty or null"); new ClearSiteDataHeaderWriter(); } - @Test - public void createInstanceWhenEmptySourceThenThrowsException() { - this.thrown.expect(Exception.class); - this.thrown.expectMessage("Sources cannot be empty or null."); - - new ClearSiteDataHeaderWriter(new String[] {}); - } - @Test public void writeHeaderWhenRequestNotSecureThenHeaderIsNotPresent() { this.request.setSecure(false); - ClearSiteDataHeaderWriter headerWriter = new ClearSiteDataHeaderWriter("cache"); - headerWriter.writeHeaders(request, response); + headerWriter.writeHeaders(this.request, this.response); - assertThat(header().doesNotExist(HEADER_NAME)); + assertThat(this.response.getHeader(HEADER_NAME)).isNull(); } @Test public void writeHeaderWhenRequestIsSecureThenHeaderValueMatchesPassedSource() { ClearSiteDataHeaderWriter headerWriter = new ClearSiteDataHeaderWriter("storage"); - headerWriter.writeHeaders(request, response); + headerWriter.writeHeaders(this.request, this.response); - assertThat(header().stringValues(HEADER_NAME, "\"storage\"")); + assertThat(this.response.getHeader(HEADER_NAME)).isEqualTo("\"storage\""); } @Test public void writeHeaderWhenRequestIsSecureThenHeaderValueMatchesPassedSources() { ClearSiteDataHeaderWriter headerWriter = new ClearSiteDataHeaderWriter("cache", "cookies", "storage", "executionContexts"); + headerWriter.writeHeaders(this.request, this.response); - headerWriter.writeHeaders(request, response); - - assertThat(header().stringValues(HEADER_NAME, "\"cache\", \"cookies\", \"storage\"," - + " \"executionContexts\"")); + assertThat(this.response.getHeader(HEADER_NAME)) + .isEqualTo("\"cache\", \"cookies\", \"storage\", \"executionContexts\""); } }