From b37b28774abfb655c33b5b6f9b9470500c7b5cf4 Mon Sep 17 00:00:00 2001 From: sfleshe Date: Tue, 8 May 2018 15:30:00 -0700 Subject: [PATCH 1/4] adjusts logic to correctly honor whitelist. Updates respective test coverage. --- prerender-java.iml | 2 +- .../greengerong/PrerenderSeoService.java | 6 ++--- .../greengerong/PreRenderSEOFilterTest.java | 26 +++++++++++++++++++ 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/prerender-java.iml b/prerender-java.iml index f74aa01..04d4ff1 100644 --- a/prerender-java.iml +++ b/prerender-java.iml @@ -1,6 +1,6 @@ - + diff --git a/src/main/java/com/github/greengerong/PrerenderSeoService.java b/src/main/java/com/github/greengerong/PrerenderSeoService.java index 250b075..b75726e 100644 --- a/src/main/java/com/github/greengerong/PrerenderSeoService.java +++ b/src/main/java/com/github/greengerong/PrerenderSeoService.java @@ -108,9 +108,9 @@ private boolean shouldShowPrerenderedPage(HttpServletRequest request) throws URI } final List whiteList = prerenderConfig.getWhitelist(); - if (whiteList != null && !isInWhiteList(url, whiteList)) { - log.trace("Whitelist is enabled, but this request is not listed; intercept: no"); - return false; + if (whiteList != null && isInWhiteList(url, whiteList)) { + log.trace("Whitelist is enabled, and this request is listed; intercept: yes"); + return true; } final List blacklist = prerenderConfig.getBlacklist(); diff --git a/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java b/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java index 6776586..51fb2ce 100644 --- a/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java +++ b/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java @@ -169,6 +169,32 @@ public void should_not_handle_when_white_list_is_not_empty_and_url_is_not_in_whi verify(filterChain).doFilter(servletRequest, servletResponse); } + @Test + public void should_handle_when_white_list_is_not_empty_and_url_is_in_white_list() throws Exception { + //given + when(filterConfig.getInitParameter("crawlerUserAgents")).thenReturn("search-bot1,search-bot2"); + when(filterConfig.getInitParameter("whitelist")).thenReturn("\\/test.*,\\/test1.*"); + preRenderSEOFilter.init(filterConfig); + final CloseableHttpResponse httpResponse = mock(CloseableHttpResponse.class); + final StatusLine statusLine = mock(StatusLine.class); + // Ignore copyResponseHeaders mocks + + when(servletRequest.getRequestURL()).thenReturn(new StringBuffer("/test")); + when(servletRequest.getMethod()).thenReturn(METHOD_NAME); + when(servletRequest.getHeaderNames()).thenReturn(mock(Enumeration.class)); + when(httpClient.execute(httpGet)).thenReturn(httpResponse); + when(httpResponse.getStatusLine()).thenReturn(statusLine); + when(servletRequest.getParameterMap()).thenReturn(Maps.newHashMap()); + when(servletRequest.getHeader("User-Agent")).thenReturn("human-browser"); // Regardless of User-Agent + + //when + preRenderSEOFilter.doFilter(servletRequest, servletResponse, filterChain); + + //then + verify(httpClient).execute(httpGet); + verify(filterChain).doFilter(servletRequest, servletResponse); + } + @Test public void should_not_handle_when_black_list_is_not_empty_and_url_is_in_black_list() throws Exception { //given From 9706fde3abd8b745d550d37f8af1fa8c43054991 Mon Sep 17 00:00:00 2001 From: sfleshe Date: Wed, 9 May 2018 10:26:11 -0700 Subject: [PATCH 2/4] - reverts previous commit. Removes change to whitelist logic and corresponding unit test. --- prerender-java.iml | 2 +- .../greengerong/PrerenderSeoService.java | 6 ++--- .../greengerong/PreRenderSEOFilterTest.java | 26 ------------------- 3 files changed, 4 insertions(+), 30 deletions(-) diff --git a/prerender-java.iml b/prerender-java.iml index 04d4ff1..f74aa01 100644 --- a/prerender-java.iml +++ b/prerender-java.iml @@ -1,6 +1,6 @@ - + diff --git a/src/main/java/com/github/greengerong/PrerenderSeoService.java b/src/main/java/com/github/greengerong/PrerenderSeoService.java index b75726e..250b075 100644 --- a/src/main/java/com/github/greengerong/PrerenderSeoService.java +++ b/src/main/java/com/github/greengerong/PrerenderSeoService.java @@ -108,9 +108,9 @@ private boolean shouldShowPrerenderedPage(HttpServletRequest request) throws URI } final List whiteList = prerenderConfig.getWhitelist(); - if (whiteList != null && isInWhiteList(url, whiteList)) { - log.trace("Whitelist is enabled, and this request is listed; intercept: yes"); - return true; + if (whiteList != null && !isInWhiteList(url, whiteList)) { + log.trace("Whitelist is enabled, but this request is not listed; intercept: no"); + return false; } final List blacklist = prerenderConfig.getBlacklist(); diff --git a/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java b/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java index 51fb2ce..6776586 100644 --- a/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java +++ b/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java @@ -169,32 +169,6 @@ public void should_not_handle_when_white_list_is_not_empty_and_url_is_not_in_whi verify(filterChain).doFilter(servletRequest, servletResponse); } - @Test - public void should_handle_when_white_list_is_not_empty_and_url_is_in_white_list() throws Exception { - //given - when(filterConfig.getInitParameter("crawlerUserAgents")).thenReturn("search-bot1,search-bot2"); - when(filterConfig.getInitParameter("whitelist")).thenReturn("\\/test.*,\\/test1.*"); - preRenderSEOFilter.init(filterConfig); - final CloseableHttpResponse httpResponse = mock(CloseableHttpResponse.class); - final StatusLine statusLine = mock(StatusLine.class); - // Ignore copyResponseHeaders mocks - - when(servletRequest.getRequestURL()).thenReturn(new StringBuffer("/test")); - when(servletRequest.getMethod()).thenReturn(METHOD_NAME); - when(servletRequest.getHeaderNames()).thenReturn(mock(Enumeration.class)); - when(httpClient.execute(httpGet)).thenReturn(httpResponse); - when(httpResponse.getStatusLine()).thenReturn(statusLine); - when(servletRequest.getParameterMap()).thenReturn(Maps.newHashMap()); - when(servletRequest.getHeader("User-Agent")).thenReturn("human-browser"); // Regardless of User-Agent - - //when - preRenderSEOFilter.doFilter(servletRequest, servletResponse, filterChain); - - //then - verify(httpClient).execute(httpGet); - verify(filterChain).doFilter(servletRequest, servletResponse); - } - @Test public void should_not_handle_when_black_list_is_not_empty_and_url_is_in_black_list() throws Exception { //given From 3145d3ae42c4c6db28891f34d8e4327d98bfc247 Mon Sep 17 00:00:00 2001 From: sfleshe Date: Wed, 9 May 2018 11:48:48 -0700 Subject: [PATCH 3/4] - Adds support for ignoreUserAgent configuration property. - removes unused import. - Adds two test cases covering use of whitelist and ignoreUserAgent --- .../greengerong/PreRenderSEOFilter.java | 3 +- .../github/greengerong/PrerenderConfig.java | 5 ++ .../greengerong/PrerenderSeoService.java | 7 ++- .../greengerong/PreRenderSEOFilterTest.java | 53 +++++++++++++++++++ 4 files changed, 65 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/github/greengerong/PreRenderSEOFilter.java b/src/main/java/com/github/greengerong/PreRenderSEOFilter.java index 6eb721f..b70c894 100644 --- a/src/main/java/com/github/greengerong/PreRenderSEOFilter.java +++ b/src/main/java/com/github/greengerong/PreRenderSEOFilter.java @@ -7,14 +7,13 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import java.io.IOException; -import java.util.HashMap; import java.util.List; import java.util.Map; public class PreRenderSEOFilter implements Filter { public static final List PARAMETER_NAMES = Lists.newArrayList("preRenderEventHandler", "proxy", "proxyPort", "prerenderToken", "forwardedURLHeader", "crawlerUserAgents", "extensionsToIgnore", "whitelist", - "blacklist", "prerenderServiceUrl"); + "blacklist", "prerenderServiceUrl", "ignoreUserAgent"); private PrerenderSeoService prerenderSeoService; @Override diff --git a/src/main/java/com/github/greengerong/PrerenderConfig.java b/src/main/java/com/github/greengerong/PrerenderConfig.java index 90c2240..6cbc541 100644 --- a/src/main/java/com/github/greengerong/PrerenderConfig.java +++ b/src/main/java/com/github/greengerong/PrerenderConfig.java @@ -127,6 +127,11 @@ public String getPrerenderServiceUrl() { return isNotBlank(prerenderServiceUrl) ? prerenderServiceUrl : getDefaultPrerenderIoServiceUrl(); } + public Boolean getIgnoreUserAgent() { + String s = config.get("ignoreUserAgent"); + return s == null ? null : Boolean.valueOf(s); + } + private String getDefaultPrerenderIoServiceUrl() { final String prerenderServiceUrlInEnv = System.getProperty("PRERENDER_SERVICE_URL"); return isNotBlank(prerenderServiceUrlInEnv) ? prerenderServiceUrlInEnv : PRERENDER_IO_SERVICE_URL; diff --git a/src/main/java/com/github/greengerong/PrerenderSeoService.java b/src/main/java/com/github/greengerong/PrerenderSeoService.java index 250b075..6f9d2f6 100644 --- a/src/main/java/com/github/greengerong/PrerenderSeoService.java +++ b/src/main/java/com/github/greengerong/PrerenderSeoService.java @@ -129,7 +129,7 @@ private boolean shouldShowPrerenderedPage(HttpServletRequest request) throws URI return false; } - if (!isInSearchUserAgent(userAgent)) { + if (!ignoreUserAgent() && !isInSearchUserAgent(userAgent)) { log.trace("Request User-Agent is not a search bot; intercept: no"); return false; } @@ -281,6 +281,11 @@ public boolean apply(String regex) { }); } + private boolean ignoreUserAgent() { + Boolean b = prerenderConfig.getIgnoreUserAgent(); + return b == null ? false : b; + } + private boolean isInSearchUserAgent(final String userAgent) { return from(prerenderConfig.getCrawlerUserAgents()).anyMatch(new Predicate() { @Override diff --git a/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java b/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java index 6776586..8c2f148 100644 --- a/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java +++ b/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java @@ -169,6 +169,59 @@ public void should_not_handle_when_white_list_is_not_empty_and_url_is_not_in_whi verify(filterChain).doFilter(servletRequest, servletResponse); } + @Test + public void should_handle_when_white_list_is_not_empty_and_user_agent_is_ignored() throws Exception { + //given + when(filterConfig.getInitParameter("whitelist")).thenReturn("\\/test.*,\\/test1.*"); + when(filterConfig.getInitParameter("ignoreUserAgent")).thenReturn("true"); + preRenderSEOFilter.init(filterConfig); + final CloseableHttpResponse httpResponse = mock(CloseableHttpResponse.class); + final StatusLine statusLine = mock(StatusLine.class); + // Ignore copyResponseHeaders mocks + + when(servletRequest.getRequestURL()).thenReturn(new StringBuffer("/test")); + when(servletRequest.getMethod()).thenReturn(METHOD_NAME); + when(servletRequest.getHeaderNames()).thenReturn(mock(Enumeration.class)); + when(httpClient.execute(httpGet)).thenReturn(httpResponse); + when(httpResponse.getStatusLine()).thenReturn(statusLine); + when(servletRequest.getParameterMap()).thenReturn(Maps.newHashMap()); + when(servletRequest.getHeader("User-Agent")).thenReturn("human-browser"); // Regardless of User-Agent + + //when + preRenderSEOFilter.doFilter(servletRequest, servletResponse, filterChain); + + //then + verify(httpClient).execute(httpGet); + verify(filterChain).doFilter(servletRequest, servletResponse); + } + + @Test + public void should_not_handle_when_url_in_whitelist_but_user_agent_is_not_ignored() throws Exception { + //given + when(filterConfig.getInitParameter("crawlerUserAgents")).thenReturn("crawler1,crawler2"); + when(filterConfig.getInitParameter("whitelist")).thenReturn("\\/test.*,\\/test1.*"); + when(filterConfig.getInitParameter("ignoreUserAgent")).thenReturn("false"); + preRenderSEOFilter.init(filterConfig); + final CloseableHttpResponse httpResponse = mock(CloseableHttpResponse.class); + final StatusLine statusLine = mock(StatusLine.class); + // Ignore copyResponseHeaders mocks + + when(servletRequest.getRequestURL()).thenReturn(new StringBuffer("/test")); + when(servletRequest.getMethod()).thenReturn(METHOD_NAME); + when(servletRequest.getHeaderNames()).thenReturn(mock(Enumeration.class)); + when(httpClient.execute(httpGet)).thenReturn(httpResponse); + when(httpResponse.getStatusLine()).thenReturn(statusLine); + when(servletRequest.getParameterMap()).thenReturn(Maps.newHashMap()); + when(servletRequest.getHeader("User-Agent")).thenReturn("human-browser"); + + //when + preRenderSEOFilter.doFilter(servletRequest, servletResponse, filterChain); + + //then + verify(httpClient, never()).execute(httpGet); + verify(filterChain).doFilter(servletRequest, servletResponse); + } + @Test public void should_not_handle_when_black_list_is_not_empty_and_url_is_in_black_list() throws Exception { //given From 75314ce90b1ba996a6d249efe0c8964f279d35e5 Mon Sep 17 00:00:00 2001 From: sfleshe Date: Wed, 9 May 2018 11:52:54 -0700 Subject: [PATCH 4/4] - Renames test case to be more accurately descriptive. --- .../java/com/github/greengerong/PreRenderSEOFilterTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java b/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java index 8c2f148..f2cb2ae 100644 --- a/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java +++ b/src/test/java/com/github/greengerong/PreRenderSEOFilterTest.java @@ -170,7 +170,7 @@ public void should_not_handle_when_white_list_is_not_empty_and_url_is_not_in_whi } @Test - public void should_handle_when_white_list_is_not_empty_and_user_agent_is_ignored() throws Exception { + public void should_handle_when_url_in_whitelist_and_user_agent_is_ignored() throws Exception { //given when(filterConfig.getInitParameter("whitelist")).thenReturn("\\/test.*,\\/test1.*"); when(filterConfig.getInitParameter("ignoreUserAgent")).thenReturn("true");