Repository: cxf Updated Branches: refs/heads/master e5fa40503 -> 08068c8a6
Make sure the State is always returned to the client on an error Project: http://git-wip-us.apache.org/repos/asf/cxf/repo Commit: http://git-wip-us.apache.org/repos/asf/cxf/commit/08068c8a Tree: http://git-wip-us.apache.org/repos/asf/cxf/tree/08068c8a Diff: http://git-wip-us.apache.org/repos/asf/cxf/diff/08068c8a Branch: refs/heads/master Commit: 08068c8a6c784e866ba9bb9b2b16e5b35d569e1b Parents: 68af196 Author: Colm O hEigeartaigh <cohei...@apache.org> Authored: Fri Dec 11 15:10:31 2015 +0000 Committer: Colm O hEigeartaigh <cohei...@apache.org> Committed: Fri Dec 11 15:24:06 2015 +0000 ---------------------------------------------------------------------- .../oauth2/client/OAuthClientUtils.java | 11 +-- .../oauth2/services/AbstractOAuthService.java | 9 ++- .../oauth2/services/AbstractTokenService.java | 81 ++++++++++++-------- .../oauth2/services/AccessTokenService.java | 3 +- .../services/DirectAuthorizationService.java | 41 ++++++---- .../services/RedirectionBasedGrantService.java | 79 ++++++++++--------- .../oauth2/services/TokenRevocationService.java | 3 +- 7 files changed, 132 insertions(+), 95 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cxf/blob/08068c8a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/client/OAuthClientUtils.java ---------------------------------------------------------------------- diff --git a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/client/OAuthClientUtils.java b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/client/OAuthClientUtils.java index e00ce0b..0f6807d 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/client/OAuthClientUtils.java +++ b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/client/OAuthClientUtils.java @@ -68,18 +68,11 @@ public final class OAuthClientUtils { String redirectUri, String state, String scope) { - UriBuilder ub = getAuthorizationURIBuilder(authorizationServiceURI, + return getAuthorizationURIBuilder(authorizationServiceURI, clientId, redirectUri, state, - scope); - if (redirectUri != null) { - ub.queryParam(OAuthConstants.REDIRECT_URI, redirectUri); - } - if (state != null) { - ub.queryParam(OAuthConstants.STATE, state); - } - return ub.build(); + scope).build(); } public static UriBuilder getAuthorizationURIBuilder(String authorizationServiceURI, http://git-wip-us.apache.org/repos/asf/cxf/blob/08068c8a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AbstractOAuthService.java ---------------------------------------------------------------------- diff --git a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AbstractOAuthService.java b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AbstractOAuthService.java index 994f0d7..56121d3 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AbstractOAuthService.java +++ b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AbstractOAuthService.java @@ -123,13 +123,14 @@ public abstract class AbstractOAuthService { } } - protected void reportInvalidRequestError(String errorDescription) { - reportInvalidRequestError(errorDescription, MediaType.APPLICATION_JSON_TYPE); + protected void reportInvalidRequestError(String errorDescription, String state) { + reportInvalidRequestError(errorDescription, state, MediaType.APPLICATION_JSON_TYPE); } - protected void reportInvalidRequestError(String errorDescription, MediaType mt) { + protected void reportInvalidRequestError(String errorDescription, String state, MediaType mt) { OAuthError error = new OAuthError(OAuthConstants.INVALID_REQUEST, errorDescription); + error.setState(state); reportInvalidRequestError(error, mt); } @@ -144,7 +145,7 @@ public abstract class AbstractOAuthService { } throw ExceptionUtils.toBadRequestException(null, rb.entity(entity).build()); } - + /** * HTTPS is the default transport for OAuth 2.0 services, this property * can be used to block all the requests issued over HTTP http://git-wip-us.apache.org/repos/asf/cxf/blob/08068c8a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AbstractTokenService.java ---------------------------------------------------------------------- diff --git a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AbstractTokenService.java b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AbstractTokenService.java index a31fb5d..7fd1ed9 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AbstractTokenService.java +++ b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AbstractTokenService.java @@ -63,29 +63,30 @@ public class AbstractTokenService extends AbstractOAuthService { String clientId = retrieveClientId(params); if (clientId != null) { client = getAndValidateClientFromIdAndSecret(clientId, - params.getFirst(OAuthConstants.CLIENT_SECRET)); + params.getFirst(OAuthConstants.CLIENT_SECRET), + params); } } else { String clientId = retrieveClientId(params); if (clientId != null) { - client = getClient(clientId); + client = getClient(clientId, params); } else if (principal.getName() != null) { - client = getClient(principal.getName()); + client = getClient(principal.getName(), params); } } if (client == null) { - client = getClientFromTLSCertificates(sc, getTlsSessionInfo()); + client = getClientFromTLSCertificates(sc, getTlsSessionInfo(), params); if (client == null) { // Basic Authentication is expected by default - client = getClientFromBasicAuthScheme(); + client = getClientFromBasicAuthScheme(params); } } if (client != null && !client.getApplicationCertificates().isEmpty()) { // Validate the client application certificates - compareTlsCertificates(getTlsSessionInfo(), client.getApplicationCertificates()); + compareTlsCertificates(getTlsSessionInfo(), client.getApplicationCertificates(), params); } if (client == null) { - reportInvalidClient(); + reportInvalidClient(params.getFirst(OAuthConstants.STATE)); } return client; } @@ -107,21 +108,22 @@ public class AbstractTokenService extends AbstractOAuthService { } // Get the Client and check the id and secret - protected Client getAndValidateClientFromIdAndSecret(String clientId, String providedClientSecret) { - Client client = getClient(clientId); + protected Client getAndValidateClientFromIdAndSecret(String clientId, String providedClientSecret, + MultivaluedMap<String, String> params) { + Client client = getClient(clientId, params); if (!client.getClientId().equals(clientId)) { - reportInvalidClient(); + reportInvalidClient(params.getFirst(OAuthConstants.STATE)); } if (isValidPublicClient(client, clientId, providedClientSecret)) { return client; } if (!client.isConfidential() - || !isConfidenatialClientSecretValid(client, providedClientSecret)) { - reportInvalidClient(); + || !isConfidentialClientSecretValid(client, providedClientSecret)) { + reportInvalidClient(params.getFirst(OAuthConstants.STATE)); } return client; } - protected boolean isConfidenatialClientSecretValid(Client client, String providedClientSecret) { + protected boolean isConfidentialClientSecretValid(Client client, String providedClientSecret) { if (clientSecretVerifier != null) { return clientSecretVerifier.validateClientSecret(client, providedClientSecret); } else { @@ -136,22 +138,23 @@ public class AbstractTokenService extends AbstractOAuthService { && clientSecret == null; } - protected Client getClientFromBasicAuthScheme() { + protected Client getClientFromBasicAuthScheme(MultivaluedMap<String, String> params) { String[] userInfo = AuthorizationUtils.getBasicAuthUserInfo(getMessageContext()); if (userInfo != null && userInfo.length == 2) { - return getAndValidateClientFromIdAndSecret(userInfo[0], userInfo[1]); + return getAndValidateClientFromIdAndSecret(userInfo[0], userInfo[1], params); } else { return null; } } - protected Client getClientFromTLSCertificates(SecurityContext sc, TLSSessionInfo tlsSessionInfo) { + protected Client getClientFromTLSCertificates(SecurityContext sc, TLSSessionInfo tlsSessionInfo, + MultivaluedMap<String, String> params) { Client client = null; if (tlsSessionInfo != null && StringUtils.isEmpty(sc.getAuthenticationScheme())) { // Pure 2-way TLS authentication String clientId = getClientIdFromTLSCertificates(sc, tlsSessionInfo); if (!StringUtils.isEmpty(clientId)) { - client = getClient(clientId); + client = getClient(clientId, params); } } return client; @@ -167,7 +170,8 @@ public class AbstractTokenService extends AbstractOAuthService { } protected void compareTlsCertificates(TLSSessionInfo tlsInfo, - List<String> base64EncodedCerts) { + List<String> base64EncodedCerts, + MultivaluedMap<String, String> params) { if (tlsInfo != null) { Certificate[] clientCerts = tlsInfo.getPeerCertificates(); if (clientCerts.length == base64EncodedCerts.size()) { @@ -177,7 +181,7 @@ public class AbstractTokenService extends AbstractOAuthService { byte[] encodedKey = x509Cert.getEncoded(); byte[] clientKey = Base64Utility.decode(base64EncodedCerts.get(i)); if (!Arrays.equals(encodedKey, clientKey)) { - reportInvalidClient(); + reportInvalidClient(params.getFirst(OAuthConstants.STATE)); } } return; @@ -186,23 +190,28 @@ public class AbstractTokenService extends AbstractOAuthService { } } } - reportInvalidClient(); + reportInvalidClient(params.getFirst(OAuthConstants.STATE)); } - protected Response handleException(OAuthServiceException ex, String error) { + protected Response handleException(OAuthServiceException ex, String error, String state) { OAuthError customError = ex.getError(); if (writeCustomErrors && customError != null) { + customError.setState(state); return createErrorResponseFromBean(customError); } else { - return createErrorResponseFromBean(new OAuthError(error)); + OAuthError oauthError = new OAuthError(error); + oauthError.setState(state); + return createErrorResponseFromBean(oauthError); } } protected Response createErrorResponse(MultivaluedMap<String, String> params, String error) { - return createErrorResponseFromBean(new OAuthError(error)); + OAuthError oauthError = new OAuthError(error); + oauthError.setState(params.getFirst(OAuthConstants.STATE)); + return createErrorResponseFromBean(oauthError); } protected Response createErrorResponseFromBean(OAuthError errorBean) { @@ -211,32 +220,44 @@ public class AbstractTokenService extends AbstractOAuthService { /** * Get the {@link Client} reference - * @param clientId the provided client id + * @param clientId The Client Id + * @param params request parameters * @return Client the client reference - * @throws {@link javax.ws.rs.WebApplicationException} if no matching Client is found + * @throws {@link javax.ws.rs.WebApplicationException} if no matching Client is found, + * the error is returned directly to the end user without + * following the redirect URI if any */ - protected Client getClient(String clientId) { + protected Client getClient(String clientId, MultivaluedMap<String, String> params) { + String state = null; + if (params != null) { + state = params.getFirst(OAuthConstants.STATE); + } + if (clientId == null) { - reportInvalidRequestError("Client ID is null"); + reportInvalidRequestError("Client ID is null", state); return null; } + Client client = null; try { client = getValidClient(clientId); } catch (OAuthServiceException ex) { if (ex.getError() != null) { + ex.getError().setState(state); reportInvalidClient(ex.getError()); return null; } } if (client == null) { - reportInvalidClient(); + reportInvalidClient(state); } return client; } - protected void reportInvalidClient() { - reportInvalidClient(new OAuthError(OAuthConstants.INVALID_CLIENT)); + protected void reportInvalidClient(String state) { + OAuthError error = new OAuthError(OAuthConstants.INVALID_CLIENT); + error.setState(state); + reportInvalidClient(error); } protected void reportInvalidClient(OAuthError error) { http://git-wip-us.apache.org/repos/asf/cxf/blob/08068c8a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AccessTokenService.java ---------------------------------------------------------------------- diff --git a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AccessTokenService.java b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AccessTokenService.java index 8af601a..27cf21a 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AccessTokenService.java +++ b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/AccessTokenService.java @@ -119,7 +119,8 @@ public class AccessTokenService extends AbstractTokenService { // restriction on a number of return statements OAuthServiceException oauthEx = ex instanceof OAuthServiceException ? (OAuthServiceException)ex : new OAuthServiceException(ex); - return handleException(oauthEx, OAuthConstants.INVALID_GRANT); + return handleException(oauthEx, OAuthConstants.INVALID_GRANT, + params.getFirst(OAuthConstants.STATE)); } if (serverToken == null) { return createErrorResponse(params, OAuthConstants.INVALID_GRANT); http://git-wip-us.apache.org/repos/asf/cxf/blob/08068c8a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/DirectAuthorizationService.java ---------------------------------------------------------------------- diff --git a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/DirectAuthorizationService.java b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/DirectAuthorizationService.java index 26212d8..5e0abe1 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/DirectAuthorizationService.java +++ b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/DirectAuthorizationService.java @@ -52,7 +52,7 @@ public class DirectAuthorizationService extends AbstractOAuthService { SecurityContext sc = getAndValidateSecurityContext(params); // Create a UserSubject representing the end user UserSubject userSubject = createUserSubject(sc); - Client client = getClient(params); + Client client = getClient(params.getFirst(OAuthConstants.CLIENT_ID), params); AccessTokenRegistration reg = new AccessTokenRegistration(); reg.setClient(client); @@ -96,35 +96,48 @@ public class DirectAuthorizationService extends AbstractOAuthService { return OAuthUtils.createSubject(securityContext); } } - - public SubjectCreator getSubjectCreator() { - return subjectCreator; - } - - public void setSubjectCreator(SubjectCreator subjectCreator) { - this.subjectCreator = subjectCreator; - } - protected Client getClient(MultivaluedMap<String, String> params) { - return getClient(params.getFirst(OAuthConstants.CLIENT_ID)); - } - protected Client getClient(String clientId) { + + /** + * Get the {@link Client} reference + * @param clientId The Client Id + * @param params request parameters + * @return Client the client reference + * @throws {@link javax.ws.rs.WebApplicationException} if no matching Client is found, + * the error is returned directly to the end user without + * following the redirect URI if any + */ + protected Client getClient(String clientId, MultivaluedMap<String, String> params) { Client client = null; + String state = null; + + if (params != null) { + state = params.getFirst(OAuthConstants.STATE); + } try { client = getValidClient(clientId); } catch (OAuthServiceException ex) { if (ex.getError() != null) { + ex.getError().setState(state); reportInvalidRequestError(ex.getError(), null); } } if (client == null) { - reportInvalidRequestError("Client ID is invalid", null); + reportInvalidRequestError("Client ID is invalid", state, null); } return client; } + public SubjectCreator getSubjectCreator() { + return subjectCreator; + } + + public void setSubjectCreator(SubjectCreator subjectCreator) { + this.subjectCreator = subjectCreator; + } + public boolean isPartialMatchScopeValidation() { return partialMatchScopeValidation; } http://git-wip-us.apache.org/repos/asf/cxf/blob/08068c8a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/RedirectionBasedGrantService.java ---------------------------------------------------------------------- diff --git a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/RedirectionBasedGrantService.java b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/RedirectionBasedGrantService.java index 53cedaf..442c625 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/RedirectionBasedGrantService.java +++ b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/RedirectionBasedGrantService.java @@ -118,7 +118,7 @@ public abstract class RedirectionBasedGrantService extends AbstractOAuthService SecurityContext sc = getAndValidateSecurityContext(params); // Create a UserSubject representing the end user UserSubject userSubject = createUserSubject(sc); - Client client = getClient(params); + Client client = getClient(params.getFirst(OAuthConstants.CLIENT_ID), params); return startAuthorization(params, userSubject, client); } @@ -128,7 +128,8 @@ public abstract class RedirectionBasedGrantService extends AbstractOAuthService // Validate the provided request URI, if any, against the ones Client provided // during the registration - String redirectUri = validateRedirectUri(client, params.getFirst(OAuthConstants.REDIRECT_URI)); + String redirectUri = validateRedirectUri(client, params.getFirst(OAuthConstants.REDIRECT_URI), + params.getFirst(OAuthConstants.STATE)); // Enforce the client confidentiality requirements if (!OAuthUtils.isGrantSupportedForClient(client, canSupportPublicClient(client), supportedGrantType)) { @@ -286,8 +287,9 @@ public abstract class RedirectionBasedGrantService extends AbstractOAuthService OAuthRedirectionState state = recreateRedirectionStateFromSession(userSubject, params, sessionToken); - Client client = getClient(state.getClientId()); - String redirectUri = validateRedirectUri(client, state.getRedirectUri()); + Client client = getClient(state.getClientId(), params); + String redirectUri = validateRedirectUri(client, state.getRedirectUri(), + params.getFirst(OAuthConstants.STATE)); // Get the end user decision value String decision = params.getFirst(OAuthConstants.AUTHORIZATION_DECISION_KEY); @@ -368,27 +370,60 @@ public abstract class RedirectionBasedGrantService extends AbstractOAuthService return securityContext; } - protected String validateRedirectUri(Client client, String redirectUri) { + protected String validateRedirectUri(Client client, String redirectUri, String state) { List<String> uris = client.getRedirectUris(); if (redirectUri != null) { if (!uris.contains(redirectUri)) { - reportInvalidRequestError("Client Redirect Uri is invalid"); + reportInvalidRequestError("Client Redirect Uri is invalid", state); } } else if (uris.size() == 1 && useRegisteredRedirectUriIfPossible) { redirectUri = uris.get(0); } if (redirectUri == null && uris.size() == 0 && !canRedirectUriBeEmpty(client)) { - reportInvalidRequestError("Client Redirect Uri is invalid"); + reportInvalidRequestError("Client Redirect Uri is invalid", state); } if (redirectUri != null && matchRedirectUriWithApplicationUri && client.getApplicationWebUri() != null && !redirectUri.startsWith(client.getApplicationWebUri())) { - reportInvalidRequestError("Client Redirect Uri is invalid"); + reportInvalidRequestError("Client Redirect Uri is invalid", state); } return redirectUri; } + /** + * Get the {@link Client} reference + * @param clientId The Client Id + * @param params request parameters + * @return Client the client reference + * @throws {@link javax.ws.rs.WebApplicationException} if no matching Client is found, + * the error is returned directly to the end user without + * following the redirect URI if any + */ + protected Client getClient(String clientId, MultivaluedMap<String, String> params) { + Client client = null; + String state = null; + + if (params != null) { + state = params.getFirst(OAuthConstants.STATE); + } + + try { + client = getValidClient(clientId); + } catch (OAuthServiceException ex) { + if (ex.getError() != null) { + ex.getError().setState(state); + reportInvalidRequestError(ex.getError(), null); + } + } + + if (client == null) { + reportInvalidRequestError("Client ID is invalid", state, null); + } + return client; + + } + private void addAuthenticityTokenToSession(OAuthAuthorizationData secData, MultivaluedMap<String, String> params, UserSubject subject) { @@ -422,34 +457,6 @@ public abstract class RedirectionBasedGrantService extends AbstractOAuthService } } - /** - * Get the {@link Client} reference - * @param params request parameters - * @return Client the client reference - * @throws {@link javax.ws.rs.WebApplicationException} if no matching Client is found, - * the error is returned directly to the end user without - * following the redirect URI if any - */ - protected Client getClient(String clientId) { - Client client = null; - - try { - client = getValidClient(clientId); - } catch (OAuthServiceException ex) { - if (ex.getError() != null) { - reportInvalidRequestError(ex.getError(), null); - } - } - - if (client == null) { - reportInvalidRequestError("Client ID is invalid", null); - } - return client; - - } - protected Client getClient(MultivaluedMap<String, String> params) { - return this.getClient(params.getFirst(OAuthConstants.CLIENT_ID)); - } protected String getSupportedGrantType() { return this.supportedGrantType; } http://git-wip-us.apache.org/repos/asf/cxf/blob/08068c8a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/TokenRevocationService.java ---------------------------------------------------------------------- diff --git a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/TokenRevocationService.java b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/TokenRevocationService.java index 092b9ec..16d6ce7 100644 --- a/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/TokenRevocationService.java +++ b/rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/services/TokenRevocationService.java @@ -56,7 +56,8 @@ public class TokenRevocationService extends AbstractTokenService { try { getDataProvider().revokeToken(client, token, tokenTypeHint); } catch (OAuthServiceException ex) { - return handleException(ex, OAuthConstants.UNSUPPORTED_TOKEN_TYPE); + return handleException(ex, OAuthConstants.UNSUPPORTED_TOKEN_TYPE, + params.getFirst(OAuthConstants.STATE)); } return Response.ok().build(); }