From 5924ed885b73f6a40e0fce8573c0474f5766971c Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Fri, 31 Jul 2020 22:33:29 -0700 Subject: [PATCH] Polish spring-security-openid main code Manually polish `spring-security-openid` following the formatting and checkstyle fixes. Issue gh-8945 --- .../security/openid/OpenID4JavaConsumer.java | 45 +++++---------- .../openid/OpenIDAuthenticationFilter.java | 25 +-------- .../openid/OpenIDAuthenticationProvider.java | 55 ++++++++----------- .../openid/OpenIDAuthenticationToken.java | 1 - .../openid/RegexBasedAxFetchListFactory.java | 1 - 5 files changed, 38 insertions(+), 89 deletions(-) diff --git a/openid/src/main/java/org/springframework/security/openid/OpenID4JavaConsumer.java b/openid/src/main/java/org/springframework/security/openid/OpenID4JavaConsumer.java index aa8dac2356..33e65240e8 100644 --- a/openid/src/main/java/org/springframework/security/openid/OpenID4JavaConsumer.java +++ b/openid/src/main/java/org/springframework/security/openid/OpenID4JavaConsumer.java @@ -80,27 +80,28 @@ public class OpenID4JavaConsumer implements OpenIDConsumer { @Override public String beginConsumption(HttpServletRequest req, String identityUrl, String returnToUrl, String realm) throws OpenIDConsumerException { - List discoveries; + List discoveries = getDiscoveries(identityUrl); + DiscoveryInformation information = this.consumerManager.associate(discoveries); + req.getSession().setAttribute(DISCOVERY_INFO_KEY, information); + AuthRequest authReq = getAuthRequest(req, identityUrl, returnToUrl, realm, information); + return authReq.getDestinationUrl(true); + } + private List getDiscoveries(String identityUrl) throws OpenIDConsumerException { try { - discoveries = this.consumerManager.discover(identityUrl); + return this.consumerManager.discover(identityUrl); } catch (DiscoveryException ex) { throw new OpenIDConsumerException("Error during discovery", ex); } + } - DiscoveryInformation information = this.consumerManager.associate(discoveries); - req.getSession().setAttribute(DISCOVERY_INFO_KEY, information); - - AuthRequest authReq; - + private AuthRequest getAuthRequest(HttpServletRequest req, String identityUrl, String returnToUrl, String realm, + DiscoveryInformation information) throws OpenIDConsumerException { try { - authReq = this.consumerManager.authenticate(information, returnToUrl, realm); - + AuthRequest authReq = this.consumerManager.authenticate(information, returnToUrl, realm); this.logger.debug("Looking up attribute fetch list for identifier: " + identityUrl); - List attributesToFetch = this.attributesToFetchFactory.createAttributeList(identityUrl); - if (!attributesToFetch.isEmpty()) { req.getSession().setAttribute(ATTRIBUTE_LIST_KEY, attributesToFetch); FetchRequest fetchRequest = FetchRequest.createFetchRequest(); @@ -112,12 +113,11 @@ public class OpenID4JavaConsumer implements OpenIDConsumer { } authReq.addExtension(fetchRequest); } + return authReq; } catch (MessageException | ConsumerException ex) { throw new OpenIDConsumerException("Error processing ConsumerManager authentication", ex); } - - return authReq.getDestinationUrl(true); } @Override @@ -125,42 +125,32 @@ public class OpenID4JavaConsumer implements OpenIDConsumer { // extract the parameters from the authentication response // (which comes in as a HTTP request from the OpenID provider) ParameterList openidResp = new ParameterList(request.getParameterMap()); - // retrieve the previously stored discovery information DiscoveryInformation discovered = (DiscoveryInformation) request.getSession().getAttribute(DISCOVERY_INFO_KEY); - if (discovered == null) { throw new OpenIDConsumerException( "DiscoveryInformation is not available. Possible causes are lost session or replay attack"); } - List attributesToFetch = (List) request.getSession() .getAttribute(ATTRIBUTE_LIST_KEY); - request.getSession().removeAttribute(DISCOVERY_INFO_KEY); request.getSession().removeAttribute(ATTRIBUTE_LIST_KEY); - // extract the receiving URL from the HTTP request StringBuffer receivingURL = request.getRequestURL(); String queryString = request.getQueryString(); - if (StringUtils.hasLength(queryString)) { receivingURL.append("?").append(request.getQueryString()); } - // verify the response VerificationResult verification; - try { verification = this.consumerManager.verify(receivingURL.toString(), openidResp, discovered); } catch (MessageException | AssociationException | DiscoveryException ex) { throw new OpenIDConsumerException("Error verifying openid response", ex); } - // examine the verification result and extract the verified identifier Identifier verified = verification.getVerifiedId(); - if (verified == null) { Identifier id = discovered.getClaimedIdentifier(); return new OpenIDAuthenticationToken(OpenIDAuthenticationStatus.FAILURE, @@ -168,30 +158,23 @@ public class OpenID4JavaConsumer implements OpenIDConsumer { "Verification status message: [" + verification.getStatusMsg() + "]", Collections.emptyList()); } - List attributes = fetchAxAttributes(verification.getAuthResponse(), attributesToFetch); - return new OpenIDAuthenticationToken(OpenIDAuthenticationStatus.SUCCESS, verified.getIdentifier(), "some message", attributes); } List fetchAxAttributes(Message authSuccess, List attributesToFetch) throws OpenIDConsumerException { - if (attributesToFetch == null || !authSuccess.hasExtension(AxMessage.OPENID_NS_AX)) { return Collections.emptyList(); } - this.logger.debug("Extracting attributes retrieved by attribute exchange"); - List attributes = Collections.emptyList(); - try { MessageExtension ext = authSuccess.getExtension(AxMessage.OPENID_NS_AX); if (ext instanceof FetchResponse) { FetchResponse fetchResp = (FetchResponse) ext; attributes = new ArrayList<>(attributesToFetch.size()); - for (OpenIDAttribute attr : attributesToFetch) { List values = fetchResp.getAttributeValues(attr.getName()); if (!values.isEmpty()) { @@ -205,11 +188,9 @@ public class OpenID4JavaConsumer implements OpenIDConsumer { catch (MessageException ex) { throw new OpenIDConsumerException("Attribute retrieval failed", ex); } - if (this.logger.isDebugEnabled()) { this.logger.debug("Retrieved attributes" + attributes); } - return attributes; } diff --git a/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationFilter.java b/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationFilter.java index a8d65af41c..c0c3cffbb8 100644 --- a/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationFilter.java +++ b/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationFilter.java @@ -95,7 +95,6 @@ public class OpenIDAuthenticationFilter extends AbstractAuthenticationProcessing @Override public void afterPropertiesSet() { super.afterPropertiesSet(); - if (this.consumer == null) { try { this.consumer = new OpenID4JavaConsumer(); @@ -104,7 +103,6 @@ public class OpenIDAuthenticationFilter extends AbstractAuthenticationProcessing throw new IllegalArgumentException("Failed to initialize OpenID", ex); } } - if (this.returnToUrlParameters.isEmpty() && getRememberMeServices() instanceof AbstractRememberMeServices) { this.returnToUrlParameters = new HashSet<>(); this.returnToUrlParameters.add(((AbstractRememberMeServices) getRememberMeServices()).getParameter()); @@ -124,12 +122,9 @@ public class OpenIDAuthenticationFilter extends AbstractAuthenticationProcessing public Authentication attemptAuthentication(HttpServletRequest request, HttpServletResponse response) throws AuthenticationException, IOException { OpenIDAuthenticationToken token; - String identity = request.getParameter("openid.identity"); - if (!StringUtils.hasText(identity)) { String claimedIdentity = obtainUsername(request); - try { String returnToUrl = buildReturnToUrl(request); String realm = lookupRealm(returnToUrl); @@ -139,7 +134,6 @@ public class OpenIDAuthenticationFilter extends AbstractAuthenticationProcessing this.logger.debug("Redirecting to " + openIdUrl); } response.sendRedirect(openIdUrl); - // Indicate to parent class that authentication is continuing. return null; } @@ -149,34 +143,27 @@ public class OpenIDAuthenticationFilter extends AbstractAuthenticationProcessing "Unable to process claimed identity '" + claimedIdentity + "'"); } } - if (this.logger.isDebugEnabled()) { this.logger.debug("Supplied OpenID identity is " + identity); } - try { token = this.consumer.endConsumption(request); } - catch (OpenIDConsumerException oice) { - throw new AuthenticationServiceException("Consumer error", oice); + catch (OpenIDConsumerException ex) { + throw new AuthenticationServiceException("Consumer error", ex); } - token.setDetails(this.authenticationDetailsSource.buildDetails(request)); - // delegate to the authentication provider Authentication authentication = this.getAuthenticationManager().authenticate(token); - return authentication; } protected String lookupRealm(String returnToUrl) { String mapping = this.realmMapping.get(returnToUrl); - if (mapping == null) { try { URL url = new URL(returnToUrl); int port = url.getPort(); - StringBuilder realmBuffer = new StringBuilder(returnToUrl.length()).append(url.getProtocol()) .append("://").append(url.getHost()); if (port > 0) { @@ -189,7 +176,6 @@ public class OpenIDAuthenticationFilter extends AbstractAuthenticationProcessing this.logger.warn("returnToUrl was not a valid URL: [" + returnToUrl + "]", ex); } } - return mapping; } @@ -201,25 +187,20 @@ public class OpenIDAuthenticationFilter extends AbstractAuthenticationProcessing */ protected String buildReturnToUrl(HttpServletRequest request) { StringBuffer sb = request.getRequestURL(); - Iterator iterator = this.returnToUrlParameters.iterator(); boolean isFirst = true; - while (iterator.hasNext()) { String name = iterator.next(); // Assume for simplicity that there is only one value String value = request.getParameter(name); - if (value == null) { continue; } - if (isFirst) { sb.append("?"); isFirst = false; } sb.append(utf8UrlEncode(name)).append("=").append(utf8UrlEncode(value)); - if (iterator.hasNext()) { sb.append("&"); } @@ -232,12 +213,10 @@ public class OpenIDAuthenticationFilter extends AbstractAuthenticationProcessing */ protected String obtainUsername(HttpServletRequest req) { String claimedIdentity = req.getParameter(this.claimedIdentityFieldName); - if (!StringUtils.hasText(claimedIdentity)) { this.logger.error("No claimed identity supplied in authentication request"); return ""; } - return claimedIdentity.trim(); } diff --git a/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationProvider.java b/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationProvider.java index d823896056..b1f71d54ff 100644 --- a/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationProvider.java +++ b/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationProvider.java @@ -66,42 +66,33 @@ public class OpenIDAuthenticationProvider implements AuthenticationProvider, Ini @Override public Authentication authenticate(final Authentication authentication) throws AuthenticationException { - if (!supports(authentication.getClass())) { return null; } - - if (authentication instanceof OpenIDAuthenticationToken) { - OpenIDAuthenticationToken response = (OpenIDAuthenticationToken) authentication; - OpenIDAuthenticationStatus status = response.getStatus(); - - // handle the various possibilities - if (status == OpenIDAuthenticationStatus.SUCCESS) { - // Lookup user details - UserDetails userDetails = this.userDetailsService.loadUserDetails(response); - - return createSuccessfulAuthentication(userDetails, response); - - } - else if (status == OpenIDAuthenticationStatus.CANCELLED) { - throw new AuthenticationCancelledException("Log in cancelled"); - } - else if (status == OpenIDAuthenticationStatus.ERROR) { - throw new AuthenticationServiceException("Error message from server: " + response.getMessage()); - } - else if (status == OpenIDAuthenticationStatus.FAILURE) { - throw new BadCredentialsException("Log in failed - identity could not be verified"); - } - else if (status == OpenIDAuthenticationStatus.SETUP_NEEDED) { - throw new AuthenticationServiceException( - "The server responded setup was needed, which shouldn't happen"); - } - else { - throw new AuthenticationServiceException("Unrecognized return value " + status.toString()); - } + if (!(authentication instanceof OpenIDAuthenticationToken)) { + return null; } - - return null; + OpenIDAuthenticationToken response = (OpenIDAuthenticationToken) authentication; + OpenIDAuthenticationStatus status = response.getStatus(); + // handle the various possibilities + if (status == OpenIDAuthenticationStatus.SUCCESS) { + // Lookup user details + UserDetails userDetails = this.userDetailsService.loadUserDetails(response); + return createSuccessfulAuthentication(userDetails, response); + } + if (status == OpenIDAuthenticationStatus.CANCELLED) { + throw new AuthenticationCancelledException("Log in cancelled"); + } + if (status == OpenIDAuthenticationStatus.ERROR) { + throw new AuthenticationServiceException("Error message from server: " + response.getMessage()); + } + if (status == OpenIDAuthenticationStatus.FAILURE) { + throw new BadCredentialsException("Log in failed - identity could not be verified"); + } + if (status == OpenIDAuthenticationStatus.SETUP_NEEDED) { + throw new AuthenticationServiceException("The server responded setup was needed, which shouldn't happen"); + } + throw new AuthenticationServiceException("Unrecognized return value " + status.toString()); } /** diff --git a/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationToken.java b/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationToken.java index 00192b67aa..3a3d0de93e 100644 --- a/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationToken.java +++ b/openid/src/main/java/org/springframework/security/openid/OpenIDAuthenticationToken.java @@ -63,7 +63,6 @@ public class OpenIDAuthenticationToken extends AbstractAuthenticationToken { * Created by the OpenIDAuthenticationProvider on successful authentication. * @param principal usually the UserDetails returned by the configured * UserDetailsService used by the OpenIDAuthenticationProvider. - * */ public OpenIDAuthenticationToken(Object principal, Collection authorities, String identityUrl, List attributes) { diff --git a/openid/src/main/java/org/springframework/security/openid/RegexBasedAxFetchListFactory.java b/openid/src/main/java/org/springframework/security/openid/RegexBasedAxFetchListFactory.java index 12a98ef511..9a41eb1090 100644 --- a/openid/src/main/java/org/springframework/security/openid/RegexBasedAxFetchListFactory.java +++ b/openid/src/main/java/org/springframework/security/openid/RegexBasedAxFetchListFactory.java @@ -57,7 +57,6 @@ public class RegexBasedAxFetchListFactory implements AxFetchListFactory { return entry.getValue(); } } - return Collections.emptyList(); }