From e8dc10b4a17d2ae6d82ef29491f56da173a414c5 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Mon, 1 Jun 2020 16:27:50 -0300 Subject: [PATCH] [KEYCLOAK-11330] - Properly handling POST formdata and UriInfo --- .../DefaultAuthenticationFlow.java | 53 ++++++++++--------- .../client/X509ClientAuthenticator.java | 2 +- .../oidc/endpoints/TokenEndpoint.java | 11 +++- .../resources/account/AccountLoader.java | 4 +- .../resources/AbstractResourceService.java | 3 ++ .../account/resources/ResourcesService.java | 4 +- ...urceOwnerPasswordCredentialsGrantTest.java | 4 ++ 7 files changed, 52 insertions(+), 29 deletions(-) diff --git a/services/src/main/java/org/keycloak/authentication/DefaultAuthenticationFlow.java b/services/src/main/java/org/keycloak/authentication/DefaultAuthenticationFlow.java index 06c14b7af7..4e512a15f7 100755 --- a/services/src/main/java/org/keycloak/authentication/DefaultAuthenticationFlow.java +++ b/services/src/main/java/org/keycloak/authentication/DefaultAuthenticationFlow.java @@ -28,6 +28,7 @@ import org.keycloak.models.UserModel; import org.keycloak.services.ServicesLogger; import org.keycloak.sessions.AuthenticationSessionModel; +import javax.ws.rs.HttpMethod; import javax.ws.rs.core.MultivaluedMap; import javax.ws.rs.core.Response; import java.util.ArrayList; @@ -97,38 +98,42 @@ public class DefaultAuthenticationFlow implements AuthenticationFlow { throw new AuthenticationFlowException("Execution not found", AuthenticationFlowError.INTERNAL_ERROR); } - MultivaluedMap inputData = processor.getRequest().getDecodedFormParameters(); - String authExecId = inputData.getFirst(Constants.AUTHENTICATION_EXECUTION); + if (HttpMethod.POST.equals(processor.getRequest().getHttpMethod())) { + MultivaluedMap inputData = processor.getRequest().getDecodedFormParameters(); + String authExecId = inputData.getFirst(Constants.AUTHENTICATION_EXECUTION); - // User clicked on "try another way" link - if (inputData.containsKey("tryAnotherWay")) { - logger.trace("User clicked on link 'Try Another Way'"); + // User clicked on "try another way" link + if (inputData.containsKey("tryAnotherWay")) { + logger.trace("User clicked on link 'Try Another Way'"); - List selectionOptions = createAuthenticationSelectionList(model); + List selectionOptions = createAuthenticationSelectionList(model); - AuthenticationProcessor.Result result = processor.createAuthenticatorContext(model, null, null); - result.setAuthenticationSelections(selectionOptions); - return result.form().createSelectAuthenticator(); - } + AuthenticationProcessor.Result result = processor.createAuthenticatorContext(model, null, null); + result.setAuthenticationSelections(selectionOptions); + return result.form().createSelectAuthenticator(); + } - // check if the user has switched to a new authentication execution, and if so switch to it. - if (authExecId != null && !authExecId.isEmpty()) { + // check if the user has switched to a new authentication execution, and if so switch to it. + if (authExecId != null && !authExecId.isEmpty()) { - List selectionOptions = createAuthenticationSelectionList(model); + List selectionOptions = createAuthenticationSelectionList(model); - // Check if switch to the requested authentication execution is allowed - selectionOptions.stream() - .filter(authSelectionOption -> authExecId.equals(authSelectionOption.getAuthExecId())) - .findFirst() - .orElseThrow(() -> new AuthenticationFlowException("Requested authentication execution is not allowed", AuthenticationFlowError.INTERNAL_ERROR) - ); + // Check if switch to the requested authentication execution is allowed + selectionOptions.stream() + .filter(authSelectionOption -> authExecId.equals(authSelectionOption.getAuthExecId())) + .findFirst() + .orElseThrow(() -> new AuthenticationFlowException("Requested authentication execution is not allowed", + AuthenticationFlowError.INTERNAL_ERROR) + ); - model = processor.getRealm().getAuthenticationExecutionById(authExecId); + model = processor.getRealm().getAuthenticationExecutionById(authExecId); - Response response = processSingleFlowExecutionModel(model, false); - if (response == null) { - return continueAuthenticationAfterSuccessfulAction(model); - } else return response; + Response response = processSingleFlowExecutionModel(model, false); + if (response == null) { + return continueAuthenticationAfterSuccessfulAction(model); + } else + return response; + } } //handle case where execution is a flow - This can happen during user registration for example diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/client/X509ClientAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/client/X509ClientAuthenticator.java index 200153c93c..399a84680b 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/client/X509ClientAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/client/X509ClientAuthenticator.java @@ -53,7 +53,7 @@ public class X509ClientAuthenticator extends AbstractClientAuthenticator { boolean hasFormData = mediaType != null && mediaType.isCompatible(MediaType.APPLICATION_FORM_URLENCODED_TYPE); MultivaluedMap formData = hasFormData ? context.getHttpRequest().getDecodedFormParameters() : null; - MultivaluedMap queryParams = context.getHttpRequest().getUri().getQueryParameters(); + MultivaluedMap queryParams = context.getSession().getContext().getUri().getQueryParameters(); if (formData != null) { client_id = formData.getFirst(OAuth2Constants.CLIENT_ID); diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java index d5ae189170..259e1f5523 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java @@ -91,12 +91,14 @@ import org.keycloak.sessions.RootAuthenticationSessionModel; import org.keycloak.util.TokenUtil; import org.keycloak.utils.ProfileHelper; +import javax.ws.rs.Consumes; import javax.ws.rs.OPTIONS; import javax.ws.rs.POST; import javax.ws.rs.Path; import javax.ws.rs.core.Context; import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.MultivaluedHashMap; import javax.ws.rs.core.MultivaluedMap; import javax.ws.rs.core.Response; import javax.ws.rs.core.Response.Status; @@ -160,11 +162,18 @@ public class TokenEndpoint { this.event = event; } + @Consumes(MediaType.APPLICATION_FORM_URLENCODED) @POST public Response processGrantRequest() { cors = Cors.add(request).auth().allowedMethods("POST").auth().exposedHeaders(Cors.ACCESS_CONTROL_ALLOW_METHODS); - formParams = request.getDecodedFormParameters(); + MultivaluedMap formParameters = request.getDecodedFormParameters(); + + if (formParameters == null) { + formParameters = new MultivaluedHashMap<>(); + } + + formParams = formParameters; grantType = formParams.getFirst(OIDCLoginProtocol.GRANT_TYPE_PARAM); // https://tools.ietf.org/html/rfc6749#section-5.1 diff --git a/services/src/main/java/org/keycloak/services/resources/account/AccountLoader.java b/services/src/main/java/org/keycloak/services/resources/account/AccountLoader.java index 9aff7760ea..9dd1de7855 100644 --- a/services/src/main/java/org/keycloak/services/resources/account/AccountLoader.java +++ b/services/src/main/java/org/keycloak/services/resources/account/AccountLoader.java @@ -35,6 +35,7 @@ import javax.ws.rs.NotAuthorizedException; import javax.ws.rs.NotFoundException; import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.UriInfo; import java.io.IOException; import java.util.List; @@ -61,10 +62,11 @@ public class AccountLoader { Theme theme = getTheme(session); boolean deprecatedAccount = isDeprecatedFormsAccountConsole(theme); + UriInfo uriInfo = session.getContext().getUri(); if (request.getHttpMethod().equals(HttpMethod.OPTIONS)) { return new CorsPreflightService(request); - } else if ((accepts.contains(MediaType.APPLICATION_JSON_TYPE) || MediaType.APPLICATION_JSON_TYPE.equals(content)) && !request.getUri().getPath().endsWith("keycloak.json")) { + } else if ((accepts.contains(MediaType.APPLICATION_JSON_TYPE) || MediaType.APPLICATION_JSON_TYPE.equals(content)) && !uriInfo.getPath().endsWith("keycloak.json")) { AuthenticationManager.AuthResult authResult = new AppAuthManager().authenticateBearerToken(session); if (authResult == null) { throw new NotAuthorizedException("Bearer token required"); diff --git a/services/src/main/java/org/keycloak/services/resources/account/resources/AbstractResourceService.java b/services/src/main/java/org/keycloak/services/resources/account/resources/AbstractResourceService.java index 8e42974b36..d4fd04fe01 100644 --- a/services/src/main/java/org/keycloak/services/resources/account/resources/AbstractResourceService.java +++ b/services/src/main/java/org/keycloak/services/resources/account/resources/AbstractResourceService.java @@ -35,6 +35,7 @@ import org.keycloak.authorization.store.ResourceStore; import org.keycloak.authorization.store.ScopeStore; import org.keycloak.models.ClientModel; import org.keycloak.models.KeycloakSession; +import org.keycloak.models.KeycloakUriInfo; import org.keycloak.models.UserModel; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.UserRepresentation; @@ -53,6 +54,7 @@ public abstract class AbstractResourceService { protected final PermissionTicketStore ticketStore; protected final ResourceStore resourceStore; protected final ScopeStore scopeStore; + protected final KeycloakUriInfo uriInfo; protected HttpRequest request; protected Auth auth; @@ -64,6 +66,7 @@ public abstract class AbstractResourceService { ticketStore = provider.getStoreFactory().getPermissionTicketStore(); resourceStore = provider.getStoreFactory().getResourceStore(); scopeStore = provider.getStoreFactory().getScopeStore(); + uriInfo = session.getContext().getUri(); } protected Response cors(Response.ResponseBuilder response) { diff --git a/services/src/main/java/org/keycloak/services/resources/account/resources/ResourcesService.java b/services/src/main/java/org/keycloak/services/resources/account/resources/ResourcesService.java index 46585fcabe..bccba19d82 100644 --- a/services/src/main/java/org/keycloak/services/resources/account/resources/ResourcesService.java +++ b/services/src/main/java/org/keycloak/services/resources/account/resources/ResourcesService.java @@ -219,14 +219,14 @@ public class ResourcesService extends AbstractResourceService { if (nextPage) { links.add(Link.fromUri( - KeycloakUriBuilder.fromUri(request.getUri().getRequestUri()).replaceQuery("first={first}&max={max}") + KeycloakUriBuilder.fromUri(uriInfo.getRequestUri()).replaceQuery("first={first}&max={max}") .build(first + max, max)) .rel("next").build()); } if (first > 0) { links.add(Link.fromUri( - KeycloakUriBuilder.fromUri(request.getUri().getRequestUri()).replaceQuery("first={first}&max={max}") + KeycloakUriBuilder.fromUri(uriInfo.getRequestUri()).replaceQuery("first={first}&max={max}") .build(Math.max(first - max, 0), max)) .rel("prev").build()); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ResourceOwnerPasswordCredentialsGrantTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ResourceOwnerPasswordCredentialsGrantTest.java index ceabb41484..a8d7e6e14d 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ResourceOwnerPasswordCredentialsGrantTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ResourceOwnerPasswordCredentialsGrantTest.java @@ -65,6 +65,9 @@ import java.util.List; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import javax.ws.rs.core.HttpHeaders; +import javax.ws.rs.core.MediaType; + /** * @author Stian Thorgersen */ @@ -610,6 +613,7 @@ public class ResourceOwnerPasswordCredentialsGrantTest extends AbstractKeycloakT try (CloseableHttpClient client = HttpClientBuilder.create().build()) { HttpPost post = new HttpPost(oauth.getResourceOwnerPasswordCredentialGrantUrl()); + post.addHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_FORM_URLENCODED); OAuthClient.AccessTokenResponse response = new OAuthClient.AccessTokenResponse(client.execute(post)); assertEquals(400, response.getStatusCode());