Polish spring-security-web main code

Manually polish `spring-security-web` following the formatting and checkstyle fixes. Issue gh-8945
spring-projects · Aug 24, 2020 · 5bdd757 · 5bdd757
1 parent ef951ba
commit 5bdd757
Show file tree

Hide file tree

Showing 178 changed files with 1,662 additions and 2,777 deletions.
diff --git a/web/src/main/java/org/springframework/security/web/AuthenticationEntryPoint.java b/web/src/main/java/org/springframework/security/web/AuthenticationEntryPoint.java
@@ -45,7 +45,6 @@ public interface AuthenticationEntryPoint {
 	 * @param request that resulted in an <code>AuthenticationException</code>
 	 * @param response so that the user agent can begin authentication
 	 * @param authException that caused the invocation
-	 *
 	 */
 	void commence(HttpServletRequest request, HttpServletResponse response, AuthenticationException authException)
 			throws IOException, ServletException;

diff --git a/web/src/main/java/org/springframework/security/web/DefaultRedirectStrategy.java b/web/src/main/java/org/springframework/security/web/DefaultRedirectStrategy.java
@@ -24,7 +24,9 @@
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
+import org.springframework.core.log.LogMessage;
 import org.springframework.security.web.util.UrlUtils;
+import org.springframework.util.Assert;
 
 /**
  * Simple implementation of <tt>RedirectStrategy</tt> which is the default used throughout
@@ -51,11 +53,7 @@ public class DefaultRedirectStrategy implements RedirectStrategy {
 	public void sendRedirect(HttpServletRequest request, HttpServletResponse response, String url) throws IOException {
 		String redirectUrl = calculateRedirectUrl(request.getContextPath(), url);
 		redirectUrl = response.encodeRedirectURL(redirectUrl);
-
-		if (this.logger.isDebugEnabled()) {
-			this.logger.debug("Redirecting to '" + redirectUrl + "'");
-		}
-
+		this.logger.debug(LogMessage.format("Redirecting to '%s'", redirectUrl));
 		response.sendRedirect(redirectUrl);
 	}
 
@@ -64,30 +62,20 @@ protected String calculateRedirectUrl(String contextPath, String url) {
 			if (isContextRelative()) {
 				return url;
 			}
-			else {
-				return contextPath + url;
-			}
+			return contextPath + url;
 		}
-
 		// Full URL, including http(s)://
-
 		if (!isContextRelative()) {
 			return url;
 		}
-
-		if (!url.contains(contextPath)) {
-			throw new IllegalArgumentException("The fully qualified URL does not include context path.");
-		}
-
+		Assert.isTrue(url.contains(contextPath), "The fully qualified URL does not include context path.");
 		// Calculate the relative URL from the fully qualified URL, minus the last
 		// occurrence of the scheme and base context.
-		url = url.substring(url.lastIndexOf("://") + 3); // strip off scheme
+		url = url.substring(url.lastIndexOf("://") + 3);
 		url = url.substring(url.indexOf(contextPath) + contextPath.length());
-
 		if (url.length() > 1 && url.charAt(0) == '/') {
 			url = url.substring(1);
 		}
-
 		return url;
 	}
 

diff --git a/web/src/main/java/org/springframework/security/web/DefaultSecurityFilterChain.java b/web/src/main/java/org/springframework/security/web/DefaultSecurityFilterChain.java
@@ -26,6 +26,7 @@
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
+import org.springframework.core.log.LogMessage;
 import org.springframework.security.web.util.matcher.RequestMatcher;
 
 /**
@@ -47,7 +48,7 @@ public DefaultSecurityFilterChain(RequestMatcher requestMatcher, Filter... filte
 	}
 
 	public DefaultSecurityFilterChain(RequestMatcher requestMatcher, List<Filter> filters) {
-		logger.info("Creating filter chain: " + requestMatcher + ", " + filters);
+		logger.info(LogMessage.format("Creating filter chain: %s, %s", requestMatcher, filters));
 		this.requestMatcher = requestMatcher;
 		this.filters = new ArrayList<>(filters);
 	}

diff --git a/web/src/main/java/org/springframework/security/web/FilterChainProxy.java b/web/src/main/java/org/springframework/security/web/FilterChainProxy.java
@@ -32,6 +32,7 @@
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
+import org.springframework.core.log.LogMessage;
 import org.springframework.security.core.context.SecurityContextHolder;
 import org.springframework.security.web.firewall.DefaultRequestRejectedHandler;
 import org.springframework.security.web.firewall.FirewalledRequest;
@@ -173,47 +174,37 @@ public void afterPropertiesSet() {
 	public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
 			throws IOException, ServletException {
 		boolean clearContext = request.getAttribute(FILTER_APPLIED) == null;
-		if (clearContext) {
-			try {
-				request.setAttribute(FILTER_APPLIED, Boolean.TRUE);
-				doFilterInternal(request, response, chain);
-			}
-			catch (RequestRejectedException ex) {
-				this.requestRejectedHandler.handle((HttpServletRequest) request, (HttpServletResponse) response, ex);
-			}
-			finally {
-				SecurityContextHolder.clearContext();
-				request.removeAttribute(FILTER_APPLIED);
-			}
+		if (!clearContext) {
+			doFilterInternal(request, response, chain);
+			return;
 		}
-		else {
+		try {
+			request.setAttribute(FILTER_APPLIED, Boolean.TRUE);
 			doFilterInternal(request, response, chain);
 		}
+		catch (RequestRejectedException ex) {
+			this.requestRejectedHandler.handle((HttpServletRequest) request, (HttpServletResponse) response, ex);
+		}
+		finally {
+			SecurityContextHolder.clearContext();
+			request.removeAttribute(FILTER_APPLIED);
+		}
 	}
 
 	private void doFilterInternal(ServletRequest request, ServletResponse response, FilterChain chain)
 			throws IOException, ServletException {
-
-		FirewalledRequest fwRequest = this.firewall.getFirewalledRequest((HttpServletRequest) request);
-		HttpServletResponse fwResponse = this.firewall.getFirewalledResponse((HttpServletResponse) response);
-
-		List<Filter> filters = getFilters(fwRequest);
-
+		FirewalledRequest firewallRequest = this.firewall.getFirewalledRequest((HttpServletRequest) request);
+		HttpServletResponse firewallResponse = this.firewall.getFirewalledResponse((HttpServletResponse) response);
+		List<Filter> filters = getFilters(firewallRequest);
 		if (filters == null || filters.size() == 0) {
-			if (logger.isDebugEnabled()) {
-				logger.debug(UrlUtils.buildRequestUrl(fwRequest)
-						+ ((filters != null) ? " has an empty filter list" : " has no matching filters"));
-			}
-
-			fwRequest.reset();
-
-			chain.doFilter(fwRequest, fwResponse);
-
+			logger.debug(LogMessage.of(() -> UrlUtils.buildRequestUrl(firewallRequest)
+					+ ((filters != null) ? " has an empty filter list" : " has no matching filters")));
+			firewallRequest.reset();
+			chain.doFilter(firewallRequest, firewallResponse);
 			return;
 		}
-
-		VirtualFilterChain vfc = new VirtualFilterChain(fwRequest, chain, filters);
-		vfc.doFilter(fwRequest, fwResponse);
+		VirtualFilterChain virtualFilterChain = new VirtualFilterChain(firewallRequest, chain, filters);
+		virtualFilterChain.doFilter(firewallRequest, firewallResponse);
 	}
 
 	/**
@@ -227,7 +218,6 @@ private List<Filter> getFilters(HttpServletRequest request) {
 				return chain.getFilters();
 			}
 		}
-
 		return null;
 	}
 
@@ -286,7 +276,6 @@ public String toString() {
 		sb.append("Filter Chains: ");
 		sb.append(this.filterChains);
 		sb.append("]");
-
 		return sb.toString();
 	}
 
@@ -317,30 +306,19 @@ private VirtualFilterChain(FirewalledRequest firewalledRequest, FilterChain chai
 		@Override
 		public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException {
 			if (this.currentPosition == this.size) {
-				if (logger.isDebugEnabled()) {
-					logger.debug(UrlUtils.buildRequestUrl(this.firewalledRequest)
-							+ " reached end of additional filter chain; proceeding with original chain");
-				}
-
+				logger.debug(LogMessage.of(() -> UrlUtils.buildRequestUrl(this.firewalledRequest)
+						+ " reached end of additional filter chain; proceeding with original chain"));
 				// Deactivate path stripping as we exit the security filter chain
 				this.firewalledRequest.reset();
-
 				this.originalChain.doFilter(request, response);
+				return;
 			}
-			else {
-				this.currentPosition++;
-
-				Filter nextFilter = this.additionalFilters.get(this.currentPosition - 1);
-
-				if (logger.isDebugEnabled()) {
-					logger.debug(
-							UrlUtils.buildRequestUrl(this.firewalledRequest) + " at position " + this.currentPosition
-									+ " of " + this.size + " in additional filter chain; firing Filter: '"
-									+ nextFilter.getClass().getSimpleName() + "'");
-				}
-
-				nextFilter.doFilter(request, response, this);
-			}
+			this.currentPosition++;
+			Filter nextFilter = this.additionalFilters.get(this.currentPosition - 1);
+			logger.debug(LogMessage.of(() -> UrlUtils.buildRequestUrl(this.firewalledRequest) + " at position "
+					+ this.currentPosition + " of " + this.size + " in additional filter chain; firing Filter: '"
+					+ nextFilter.getClass().getSimpleName() + "'"));
+			nextFilter.doFilter(request, response, this);
 		}
 
 	}

diff --git a/web/src/main/java/org/springframework/security/web/FilterInvocation.java b/web/src/main/java/org/springframework/security/web/FilterInvocation.java
@@ -37,6 +37,7 @@
 
 import org.springframework.http.HttpHeaders;
 import org.springframework.security.web.util.UrlUtils;
+import org.springframework.util.Assert;
 
 /**
  * Holds objects associated with a HTTP filter.
@@ -65,10 +66,7 @@ public class FilterInvocation {
 	private HttpServletResponse response;
 
 	public FilterInvocation(ServletRequest request, ServletResponse response, FilterChain chain) {
-		if ((request == null) || (response == null) || (chain == null)) {
-			throw new IllegalArgumentException("Cannot pass null values to constructor");
-		}
-
+		Assert.isTrue(request != null && response != null && chain != null, "Cannot pass null values to constructor");
 		this.request = (HttpServletRequest) request;
 		this.response = (HttpServletResponse) response;
 		this.chain = chain;
@@ -84,9 +82,7 @@ public FilterInvocation(String contextPath, String servletPath, String method) {
 
 	public FilterInvocation(String contextPath, String servletPath, String pathInfo, String query, String method) {
 		DummyRequest request = new DummyRequest();
-		if (contextPath == null) {
-			contextPath = "/cp";
-		}
+		contextPath = (contextPath != null) ? contextPath : "/cp";
 		request.setContextPath(contextPath);
 		request.setServletPath(servletPath);
 		request.setRequestURI(contextPath + servletPath + ((pathInfo != null) ? pathInfo : ""));
@@ -256,9 +252,7 @@ public int getIntHeader(String name) {
 			if (value == null) {
 				return -1;
 			}
-			else {
-				return Integer.parseInt(value);
-			}
+			return Integer.parseInt(value);
 		}
 
 		void addHeader(String name, String value) {
@@ -267,8 +261,8 @@ void addHeader(String name, String value) {
 
 		@Override
 		public String getParameter(String name) {
-			String[] arr = this.parameters.get(name);
-			return (arr != null && arr.length > 0) ? arr[0] : null;
+			String[] array = this.parameters.get(name);
+			return (array != null && array.length > 0) ? array[0] : null;
 		}
 
 		@Override
@@ -317,7 +311,6 @@ private Object invokeDefaultMethod(Object proxy, Method method, Object[] args) t
 		private Object invokeDefaultMethodForJdk8(Object proxy, Method method, Object[] args) throws Throwable {
 			Constructor<Lookup> constructor = Lookup.class.getDeclaredConstructor(Class.class);
 			constructor.setAccessible(true);
-
 			Class<?> clazz = method.getDeclaringClass();
 			return constructor.newInstance(clazz).in(clazz).unreflectSpecial(method, clazz).bindTo(proxy)
 					.invokeWithArguments(args);

diff --git a/web/src/main/java/org/springframework/security/web/PortMapperImpl.java b/web/src/main/java/org/springframework/security/web/PortMapperImpl.java
@@ -56,7 +56,6 @@ public Integer lookupHttpPort(Integer httpsPort) {
 				return httpPort;
 			}
 		}
-
 		return null;
 	}
 
@@ -88,24 +87,19 @@ public Integer lookupHttpsPort(Integer httpPort) {
 	 */
 	public void setPortMappings(Map<String, String> newMappings) {
 		Assert.notNull(newMappings, "A valid list of HTTPS port mappings must be provided");
-
 		this.httpsPortMappings.clear();
-
 		for (Map.Entry<String, String> entry : newMappings.entrySet()) {
 			Integer httpPort = Integer.valueOf(entry.getKey());
 			Integer httpsPort = Integer.valueOf(entry.getValue());
-
-			if ((httpPort < 1) || (httpPort > 65535) || (httpsPort < 1) || (httpsPort > 65535)) {
-				throw new IllegalArgumentException(
-						"one or both ports out of legal range: " + httpPort + ", " + httpsPort);
-			}
-
+			Assert.isTrue(isInPortRange(httpPort) && isInPortRange(httpsPort),
+					() -> "one or both ports out of legal range: " + httpPort + ", " + httpsPort);
 			this.httpsPortMappings.put(httpPort, httpsPort);
 		}
+		Assert.isTrue(!this.httpsPortMappings.isEmpty(), "must map at least one port");
+	}
 
-		if (this.httpsPortMappings.size() < 1) {
-			throw new IllegalArgumentException("must map at least one port");
-		}
+	private boolean isInPortRange(int port) {
+		return port >= 1 && port <= 65535;
 	}
 
 }
diff --git a/web/src/main/java/org/springframework/security/web/PortResolverImpl.java b/web/src/main/java/org/springframework/security/web/PortResolverImpl.java
@@ -45,24 +45,19 @@ public PortMapper getPortMapper() {
 	@Override
 	public int getServerPort(ServletRequest request) {
 		int serverPort = request.getServerPort();
-		Integer portLookup = null;
-
 		String scheme = request.getScheme().toLowerCase();
+		Integer mappedPort = getMappedPort(serverPort, scheme);
+		return (mappedPort != null) ? mappedPort : serverPort;
+	}
 
+	private Integer getMappedPort(int serverPort, String scheme) {
 		if ("http".equals(scheme)) {
-			portLookup = this.portMapper.lookupHttpPort(serverPort);
-
+			return this.portMapper.lookupHttpPort(serverPort);
 		}
-		else if ("https".equals(scheme)) {
-			portLookup = this.portMapper.lookupHttpsPort(serverPort);
+		if ("https".equals(scheme)) {
+			return this.portMapper.lookupHttpsPort(serverPort);
 		}
-
-		if (portLookup != null) {
-			// IE 6 bug
-			serverPort = portLookup;
-		}
-
-		return serverPort;
+		return null;
 	}
 
 	public void setPortMapper(PortMapper portMapper) {