From 54a0e8407070f04467fb7a296db34d7b3e38ccff Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Wed, 14 Jul 2021 11:49:51 -0300 Subject: [PATCH] [KEYCLOAK-18741] - Review error messages when validating PAR requests --- .../org/keycloak/OAuthErrorException.java | 2 + .../AuthzEndpointRequestObjectParser.java | 5 + .../oidc/par/endpoints/ParEndpoint.java | 2 +- .../context/AuthorizationRequestContext.java | 5 + .../executor/SecureRequestObjectExecutor.java | 54 ++++--- .../testsuite/client/ClientPoliciesTest.java | 140 +++++++++++++++--- .../keycloak/testsuite/client/FAPI1Test.java | 2 +- .../org/keycloak/testsuite/par/ParTest.java | 65 +++++++- 8 files changed, 231 insertions(+), 44 deletions(-) diff --git a/core/src/main/java/org/keycloak/OAuthErrorException.java b/core/src/main/java/org/keycloak/OAuthErrorException.java index 4800033e72..a246b383fe 100755 --- a/core/src/main/java/org/keycloak/OAuthErrorException.java +++ b/core/src/main/java/org/keycloak/OAuthErrorException.java @@ -30,6 +30,8 @@ public class OAuthErrorException extends Exception { public static final String UNSUPPORTED_RESPONSE_TYPE = "unsupported_response_type"; public static final String SERVER_ERROR = "server_error"; public static final String TEMPORARILY_UNAVAILABLE = "temporarily_unavailable"; + public static final String INVALID_REQUEST_URI = "invalid_request_uri"; + public static final String INVALID_REQUEST_OBJECT = "invalid_request_object"; // OpenID Connect 1 public static final String INTERACTION_REQUIRED = "interaction_required"; diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestObjectParser.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestObjectParser.java index 7a07132948..28fcf9a95d 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestObjectParser.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestObjectParser.java @@ -31,6 +31,7 @@ import org.keycloak.jose.jws.JWSInput; import org.keycloak.models.ClientModel; import org.keycloak.models.KeycloakSession; import org.keycloak.protocol.oidc.OIDCAdvancedConfigWrapper; +import org.keycloak.protocol.oidc.OIDCLoginProtocol; /** * Parse the parameters from OIDC "request" object @@ -58,6 +59,10 @@ public class AuthzEndpointRequestObjectParser extends AuthzEndpointRequestParser throw new RuntimeException("The client_id in the request object is not the same as the authorizing client"); } + if (requestParams.has(OIDCLoginProtocol.REQUEST_URI_PARAM)) { + throw new RuntimeException("The request_uri claim should not be set in the request object"); + } + session.setAttribute(AuthzEndpointRequestParser.AUTHZ_REQUEST_OBJECT, requestParams); } diff --git a/services/src/main/java/org/keycloak/protocol/oidc/par/endpoints/ParEndpoint.java b/services/src/main/java/org/keycloak/protocol/oidc/par/endpoints/ParEndpoint.java index 7d59ca6422..adf58b6ed8 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/par/endpoints/ParEndpoint.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/par/endpoints/ParEndpoint.java @@ -96,7 +96,7 @@ public class ParEndpoint extends AbstractParEndpoint { try { authorizationRequest = AuthorizationEndpointRequestParserProcessor.parseRequest(event, session, client, httpRequest.getDecodedFormParameters()); } catch (Exception e) { - throw throwErrorResponseException(OAuthErrorException.INVALID_REQUEST, e.getMessage(), Response.Status.BAD_REQUEST); + throw throwErrorResponseException(OAuthErrorException.INVALID_REQUEST_OBJECT, e.getMessage(), Response.Status.BAD_REQUEST); } AuthorizationEndpointChecker checker = new AuthorizationEndpointChecker() diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/context/AuthorizationRequestContext.java b/services/src/main/java/org/keycloak/services/clientpolicy/context/AuthorizationRequestContext.java index a0d1d95e9c..566e9e8f69 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/context/AuthorizationRequestContext.java +++ b/services/src/main/java/org/keycloak/services/clientpolicy/context/AuthorizationRequestContext.java @@ -19,6 +19,7 @@ package org.keycloak.services.clientpolicy.context; import javax.ws.rs.core.MultivaluedMap; +import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.protocol.oidc.endpoints.request.AuthorizationEndpointRequest; import org.keycloak.protocol.oidc.utils.OIDCResponseType; import org.keycloak.services.clientpolicy.ClientPolicyContext; @@ -64,4 +65,8 @@ public class AuthorizationRequestContext implements ClientPolicyContext { public MultivaluedMap getRequestParameters() { return requestParameters; } + + public boolean isParRequest() { + return requestParameters.containsKey(OIDCLoginProtocol.REQUEST_URI_PARAM); + } } diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureRequestObjectExecutor.java b/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureRequestObjectExecutor.java index 45fac88944..dadcb05ce0 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureRequestObjectExecutor.java +++ b/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureRequestObjectExecutor.java @@ -17,6 +17,8 @@ package org.keycloak.services.clientpolicy.executor; +import static org.keycloak.OAuthErrorException.INVALID_REQUEST_OBJECT; + import java.util.ArrayList; import java.util.List; import java.util.Optional; @@ -28,9 +30,7 @@ import org.keycloak.OAuthErrorException; import org.keycloak.common.util.Time; import org.keycloak.models.KeycloakSession; import org.keycloak.protocol.oidc.OIDCLoginProtocol; -import org.keycloak.protocol.oidc.endpoints.request.AuthorizationEndpointRequest; import org.keycloak.protocol.oidc.endpoints.request.AuthzEndpointRequestParser; -import org.keycloak.protocol.oidc.utils.OIDCResponseType; import org.keycloak.representations.idm.ClientPolicyExecutorConfigurationRepresentation; import org.keycloak.services.Urls; import org.keycloak.services.clientpolicy.ClientPolicyContext; @@ -47,7 +47,6 @@ public class SecureRequestObjectExecutor implements ClientPolicyExecutorProvider private static final Logger logger = Logger.getLogger(SecureRequestObjectExecutor.class); - public static final String INVALID_REQUEST_OBJECT = "invalid_request_object"; public static final Integer DEFAULT_AVAILABLE_PERIOD = Integer.valueOf(3600); // (sec) from FAPI 1.0 Advanced requirement private final KeycloakSession session; @@ -126,26 +125,21 @@ public class SecureRequestObjectExecutor implements ClientPolicyExecutorProvider switch (context.getEvent()) { case AUTHORIZATION_REQUEST: AuthorizationRequestContext authorizationRequestContext = (AuthorizationRequestContext)context; - executeOnAuthorizationRequest(authorizationRequestContext.getparsedResponseType(), - authorizationRequestContext.getAuthorizationEndpointRequest(), - authorizationRequestContext.getRedirectUri(), - authorizationRequestContext.getRequestParameters()); + executeOnAuthorizationRequest(authorizationRequestContext); break; default: return; } } - private void executeOnAuthorizationRequest( - OIDCResponseType parsedResponseType, - AuthorizationEndpointRequest request, - String redirectUri, - MultivaluedMap params) throws ClientPolicyException { + private void executeOnAuthorizationRequest(AuthorizationRequestContext context) throws ClientPolicyException { logger.trace("Authz Endpoint - authz request"); + MultivaluedMap params = context.getRequestParameters(); + if (params == null) { logger.trace("request parameter not exist."); - throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST, "Missing parameters"); + throwClientPolicyException(OAuthErrorException.INVALID_REQUEST, "Missing parameters", context); } String requestParam = params.getFirst(OIDCLoginProtocol.REQUEST_PARAM); @@ -154,7 +148,8 @@ public class SecureRequestObjectExecutor implements ClientPolicyExecutorProvider // check whether whether request object exists if (requestParam == null && requestUriParam == null) { logger.trace("request object not exist."); - throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST, "Missing parameter: 'request' or 'request_uri'"); + throwClientPolicyException(OAuthErrorException.INVALID_REQUEST, "Missing parameter: 'request' or 'request_uri'", + context); } JsonNode requestObject = (JsonNode)session.getAttribute(AuthzEndpointRequestParser.AUTHZ_REQUEST_OBJECT); @@ -162,19 +157,21 @@ public class SecureRequestObjectExecutor implements ClientPolicyExecutorProvider // check whether request object exists if (requestObject == null || requestObject.isEmpty()) { logger.trace("request object not exist."); - throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST, "Invalid parameter: : 'request' or 'request_uri'"); + throwClientPolicyException(OAuthErrorException.INVALID_REQUEST, "Invalid parameter: : 'request' or 'request_uri'", + context); } // check whether scope exists in both query parameter and request object if (params.getFirst(OIDCLoginProtocol.SCOPE_PARAM) == null && requestObject.get(OIDCLoginProtocol.SCOPE_PARAM) == null) { logger.trace("scope object not exist."); - throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST, "Parameter 'scope' missing in the request parameters or in 'request' object"); + throwClientPolicyException(OAuthErrorException.INVALID_REQUEST, "Parameter 'scope' missing in the request parameters or in 'request' object", + context); } // check whether "exp" claim exists if (requestObject.get("exp") == null) { logger.trace("exp claim not incuded."); - throw new ClientPolicyException(INVALID_REQUEST_OBJECT, "Missing parameter in the 'request' object: exp"); + throwClientPolicyException(INVALID_REQUEST_OBJECT, "Missing parameter in the 'request' object: exp", context); } // check whether request object not expired @@ -190,21 +187,21 @@ public class SecureRequestObjectExecutor implements ClientPolicyExecutorProvider // check whether "nbf" claim exists if (requestObject.get("nbf") == null) { logger.trace("nbf claim not incuded."); - throw new ClientPolicyException(INVALID_REQUEST_OBJECT, "Missing parameter in the 'request' object: nbf"); + throwClientPolicyException(INVALID_REQUEST_OBJECT, "Missing parameter in the 'request' object: nbf", context); } // check whether request object not yet being processed long nbf = requestObject.get("nbf").asLong(); if (Time.currentTime() < nbf) { // TODO: Time.currentTime() is int while nbf is long... logger.trace("request object not yet being processed."); - throw new ClientPolicyException(INVALID_REQUEST_OBJECT, "Request not yet being processed"); + throwClientPolicyException(INVALID_REQUEST_OBJECT, "Request not yet being processed", context); } // check whether request object's available period is short int availablePeriod = Optional.ofNullable(configuration.getAvailablePeriod()).orElse(DEFAULT_AVAILABLE_PERIOD).intValue(); if (exp - nbf > availablePeriod) { logger.trace("request object's available period is long."); - throw new ClientPolicyException(INVALID_REQUEST_OBJECT, "Request's available period is long"); + throwClientPolicyException(INVALID_REQUEST_OBJECT, "Request's available period is long", context); } } @@ -213,7 +210,7 @@ public class SecureRequestObjectExecutor implements ClientPolicyExecutorProvider JsonNode audience = requestObject.get("aud"); if (audience == null) { logger.trace("aud claim not incuded."); - throw new ClientPolicyException(INVALID_REQUEST_OBJECT, "Missing parameter in the 'request' object: aud"); + throwClientPolicyException(INVALID_REQUEST_OBJECT, "Missing parameter in the 'request' object: aud", context); } if (audience.isArray()) { for (JsonNode node : audience) aud.add(node.asText()); @@ -222,14 +219,14 @@ public class SecureRequestObjectExecutor implements ClientPolicyExecutorProvider } if (aud.isEmpty()) { logger.trace("aud claim not incuded."); - throw new ClientPolicyException(INVALID_REQUEST_OBJECT, "Missing parameter value in the 'request' object: aud"); + throwClientPolicyException(INVALID_REQUEST_OBJECT, "Missing parameter value in the 'request' object: aud", context); } // check whether "aud" claim points to this keycloak as authz server String iss = Urls.realmIssuer(session.getContext().getUri().getBaseUri(), session.getContext().getRealm().getName()); if (!aud.contains(iss)) { logger.trace("aud not points to the intended realm."); - throw new ClientPolicyException(INVALID_REQUEST_OBJECT, "Invalid parameter in the 'request' object: aud"); + throwClientPolicyException(INVALID_REQUEST_OBJECT, "Invalid parameter in the 'request' object: aud", context); } // confirm whether all parameters in query string are included in the request object, and have the same values @@ -240,7 +237,8 @@ public class SecureRequestObjectExecutor implements ClientPolicyExecutorProvider .findFirst(); if (incorrectParam.isPresent()) { logger.warnf("Parameter '%s' does not have same value in 'request' object and in request parameters", incorrectParam.get()); - throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST, "Invalid parameter. Parameters in 'request' object not matching with request parameters"); + throwClientPolicyException(OAuthErrorException.INVALID_REQUEST, "Invalid parameter. Parameters in 'request' object not matching with request parameters", + context); } Boolean encryptionRequired = Optional.ofNullable(configuration.isEncryptionRequired()).orElse(Boolean.FALSE); @@ -258,4 +256,12 @@ public class SecureRequestObjectExecutor implements ClientPolicyExecutorProvider return false; } + private void throwClientPolicyException(String error, String message, + AuthorizationRequestContext context) throws ClientPolicyException { + if (context.isParRequest() && INVALID_REQUEST_OBJECT.equals(error)) { + throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST_URI, message); + } + + throw new ClientPolicyException(error, message); + } } \ No newline at end of file diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java index 0bbc19ffe8..9b55260550 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java @@ -35,6 +35,8 @@ import org.keycloak.authentication.authenticators.client.JWTClientAuthenticator; import org.keycloak.authentication.authenticators.client.JWTClientSecretAuthenticator; import org.keycloak.authentication.authenticators.client.X509ClientAuthenticator; import org.keycloak.client.registration.ClientRegistrationException; +import org.keycloak.common.Profile; +import org.keycloak.common.util.Base64Url; import org.keycloak.events.Details; import org.keycloak.events.Errors; import org.keycloak.events.EventType; @@ -85,6 +87,7 @@ import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude; import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer; import org.keycloak.testsuite.client.resources.TestApplicationResourceUrls; +import org.keycloak.testsuite.client.resources.TestOIDCEndpointsApplicationResource; import org.keycloak.testsuite.rest.resource.TestingOIDCEndpointsApplicationResource.AuthorizationEndpointRequestObject; import org.keycloak.testsuite.services.clientpolicy.condition.TestRaiseExeptionConditionFactory; import org.keycloak.testsuite.services.clientpolicy.executor.TestRaiseExeptionExecutorFactory; @@ -108,16 +111,11 @@ import java.util.List; import java.util.Map; import java.util.Optional; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.fail; -import static org.keycloak.protocol.oidc.grants.ciba.channel.AuthenticationChannelResponse.Status.SUCCEED; import static org.keycloak.testsuite.admin.AbstractAdminTest.loadJson; import static org.keycloak.testsuite.admin.ApiUtil.findUserByUsername; -import static org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer.QUARKUS; -import static org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer.REMOTE; import static org.keycloak.testsuite.util.ClientPoliciesUtil.ClientPoliciesBuilder; import static org.keycloak.testsuite.util.ClientPoliciesUtil.ClientPolicyBuilder; import static org.keycloak.testsuite.util.ClientPoliciesUtil.ClientProfileBuilder; @@ -1237,7 +1235,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { requestObject.exp(null); registerRequestObject(requestObject, clientId, Algorithm.ES256, false); oauth.openLoginForm(); - assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + assertEquals(OAuthErrorException.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); assertEquals("Missing parameter in the 'request' object: exp", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); // check whether request object not expired @@ -1245,7 +1243,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { requestObject.exp(Long.valueOf(0)); registerRequestObject(requestObject, clientId, Algorithm.ES256, true); oauth.openLoginForm(); - assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + assertEquals(OAuthErrorException.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); assertEquals("Request Expired", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); // check whether "nbf" claim exists @@ -1253,7 +1251,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { requestObject.nbf(null); registerRequestObject(requestObject, clientId, Algorithm.ES256, false); oauth.openLoginForm(); - assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + assertEquals(OAuthErrorException.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); assertEquals("Missing parameter in the 'request' object: nbf", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); // check whether request object not yet being processed @@ -1261,7 +1259,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { requestObject.nbf(requestObject.getNbf() + 600); registerRequestObject(requestObject, clientId, Algorithm.ES256, false); oauth.openLoginForm(); - assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + assertEquals(OAuthErrorException.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); assertEquals("Request not yet being processed", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); // check whether request object's available period is short @@ -1269,7 +1267,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { requestObject.exp(requestObject.getNbf() + availablePeriod.intValue() + 1); registerRequestObject(requestObject, clientId, Algorithm.ES256, false); oauth.openLoginForm(); - assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + assertEquals(OAuthErrorException.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); assertEquals("Request's available period is long", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); // check whether "aud" claim exists @@ -1277,7 +1275,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { requestObject.audience((String)null); registerRequestObject(requestObject, clientId, Algorithm.ES256, false); oauth.openLoginForm(); - assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + assertEquals(OAuthErrorException.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); assertEquals("Missing parameter in the 'request' object: aud", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); // check whether "aud" claim points to this keycloak as authz server @@ -1285,7 +1283,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { requestObject.audience(suiteContext.getAuthServerInfo().getContextRoot().toString()); registerRequestObject(requestObject, clientId, Algorithm.ES256, true); oauth.openLoginForm(); - assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + assertEquals(OAuthErrorException.INVALID_REQUEST_URI, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); assertEquals("Invalid parameter in the 'request' object: aud", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); // confirm whether all parameters in query string are included in the request object, and have the same values @@ -1316,7 +1314,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { requestObject.nbf(null); registerRequestObject(requestObject, clientId, Algorithm.ES256, false); oauth.openLoginForm(); - assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + assertEquals(OAuthErrorException.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); assertEquals("Missing parameter in the 'request' object: nbf", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); // check whether request object not yet being processed @@ -1324,7 +1322,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { requestObject.nbf(requestObject.getNbf() + 600); registerRequestObject(requestObject, clientId, Algorithm.ES256, false); oauth.openLoginForm(); - assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + assertEquals(OAuthErrorException.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); assertEquals("Request not yet being processed", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); // check whether request object's available period is short @@ -1332,7 +1330,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { requestObject.exp(requestObject.getNbf() + SecureRequestObjectExecutor.DEFAULT_AVAILABLE_PERIOD + 1); registerRequestObject(requestObject, clientId, Algorithm.ES256, false); oauth.openLoginForm(); - assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + assertEquals(OAuthErrorException.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); assertEquals("Request's available period is long", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); // update profile : not check "nbf" @@ -1373,10 +1371,118 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { requestObject = createValidRequestObjectForSecureRequestObjectExecutor(clientId); registerRequestObject(requestObject, clientId, Algorithm.ES256, false); oauth.openLoginForm(); - assertEquals(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + assertEquals(OAuthErrorException.INVALID_REQUEST_OBJECT, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); assertEquals("Request object not encrypted", oauth.getCurrentQuery().get(OAuth2Constants.ERROR_DESCRIPTION)); } + @Test + public void testParSecureRequestObjectExecutor() throws Exception { + Integer availablePeriod = Integer.valueOf(SecureRequestObjectExecutor.DEFAULT_AVAILABLE_PERIOD + 400); + // register profiles + String json = (new ClientProfilesBuilder()).addProfile( + (new ClientProfileBuilder()).createProfile(PROFILE_NAME, "Prvy Profil") + .addExecutor(SecureRequestObjectExecutorFactory.PROVIDER_ID, + createSecureRequestObjectExecutorConfig(availablePeriod, true)) + .toRepresentation() + ).toString(); + updateProfiles(json); + + // register policies + json = (new ClientPoliciesBuilder()).addPolicy( + (new ClientPolicyBuilder()).createPolicy(POLICY_NAME, "Prva Politika", Boolean.TRUE) + .addCondition(ClientRolesConditionFactory.PROVIDER_ID, + createClientRolesConditionConfig(Arrays.asList(SAMPLE_CLIENT_ROLE))) + .addProfile(PROFILE_NAME) + .toRepresentation() + ).toString(); + updatePolicies(json); + + String clientId = generateSuffixedName(CLIENT_NAME); + String clientSecret = "secret"; + String cid = createClientByAdmin(clientId, (ClientRepresentation clientRep) -> { + clientRep.setSecret(clientSecret); + OIDCAdvancedConfigWrapper.fromClientRepresentation(clientRep).setRequestUris(Arrays.asList(TestApplicationResourceUrls.clientRequestUri())); + }); + + oauth.realm(REALM_NAME); + oauth.clientId(clientId); + + adminClient.realm(REALM_NAME).clients().get(cid).roles().create(RoleBuilder.create().name(SAMPLE_CLIENT_ROLE).build()); + + AuthorizationEndpointRequestObject requestObject = createValidRequestObjectForSecureRequestObjectExecutor(clientId); + + oauth.request(signRequestObject(requestObject)); + OAuthClient.ParResponse pResp = oauth.doPushedAuthorizationRequest(clientId, clientSecret); + assertEquals(201, pResp.getStatusCode()); + String requestUri = pResp.getRequestUri(); + + oauth.scope(null); + oauth.responseType(null); + oauth.request(null); + oauth.requestUri(requestUri); + OAuthClient.AuthorizationEndpointResponse loginResponse = oauth.doLogin(TEST_USER_NAME, TEST_USER_PASSWORD); + assertNotNull(loginResponse.getCode()); + oauth.openLogout(); + + requestObject.exp(null); + oauth.requestUri(null); + oauth.request(signRequestObject(requestObject)); + pResp = oauth.doPushedAuthorizationRequest(clientId, clientSecret); + requestUri = pResp.getRequestUri(); + oauth.request(null); + oauth.requestUri(requestUri); + oauth.openLoginForm(); + assertEquals(OAuthErrorException.INVALID_REQUEST_URI, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + + requestObject = createValidRequestObjectForSecureRequestObjectExecutor(clientId); + requestObject.nbf(null); + oauth.requestUri(null); + oauth.request(signRequestObject(requestObject)); + pResp = oauth.doPushedAuthorizationRequest(clientId, clientSecret); + requestUri = pResp.getRequestUri(); + oauth.request(null); + oauth.requestUri(requestUri); + oauth.openLoginForm(); + assertEquals(OAuthErrorException.INVALID_REQUEST_URI, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + + requestObject = createValidRequestObjectForSecureRequestObjectExecutor(clientId); + requestObject.audience("https://www.other1.example.com/"); + oauth.request(signRequestObject(requestObject)); + oauth.requestUri(null); + pResp = oauth.doPushedAuthorizationRequest(clientId, clientSecret); + requestUri = pResp.getRequestUri(); + oauth.request(null); + oauth.requestUri(requestUri); + oauth.openLoginForm(); + assertEquals(OAuthErrorException.INVALID_REQUEST_URI, oauth.getCurrentQuery().get(OAuth2Constants.ERROR)); + + requestObject = createValidRequestObjectForSecureRequestObjectExecutor(clientId); + requestObject.setOtherClaims(OIDCLoginProtocol.REQUEST_URI_PARAM, "foo"); + oauth.request(signRequestObject(requestObject)); + oauth.requestUri(null); + pResp = oauth.doPushedAuthorizationRequest(clientId, clientSecret); + assertEquals(OAuthErrorException.INVALID_REQUEST_OBJECT, pResp.getError()); + } + + private String signRequestObject(AuthorizationEndpointRequestObject requestObject) throws IOException { + byte[] contentBytes = JsonSerialization.writeValueAsBytes(requestObject); + String encodedRequestObject = Base64Url.encode(contentBytes); + TestOIDCEndpointsApplicationResource client = testingClient.testApp().oidcClientEndpoints(); + + // use and set jwks_url + ClientResource clientResource = ApiUtil.findClientByClientId(adminClient.realm(oauth.getRealm()), oauth.getClientId()); + ClientRepresentation clientRep = clientResource.toRepresentation(); + OIDCAdvancedConfigWrapper.fromClientRepresentation(clientRep).setUseJwksUrl(true); + OIDCAdvancedConfigWrapper.fromClientRepresentation(clientRep).setJwksUrl(TestApplicationResourceUrls.clientJwksUri()); + clientResource.update(clientRep); + client.generateKeys(org.keycloak.crypto.Algorithm.PS256); + client.registerOIDCRequest(encodedRequestObject, org.keycloak.crypto.Algorithm.PS256); + + // do not send any other parameter but the request request parameter + String oidcRequest = client.getOIDCRequest(); + return oidcRequest; + } + @Test public void testSecureSessionEnforceExecutor() throws Exception { // register profiles diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPI1Test.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPI1Test.java index 85f8fd949e..2d2bdcb14a 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPI1Test.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPI1Test.java @@ -514,7 +514,7 @@ public class FAPI1Test extends AbstractClientPoliciesTest { requestObject.nbf(null); registerRequestObject(requestObject, "foo", org.keycloak.jose.jws.Algorithm.PS256, true); oauth.openLoginForm(); - assertRedirectedToClientWithError(SecureRequestObjectExecutor.INVALID_REQUEST_OBJECT,false, "Missing parameter in the 'request' object: nbf"); + assertRedirectedToClientWithError(OAuthErrorException.INVALID_REQUEST_URI,false, "Missing parameter in the 'request' object: nbf"); // Create valid request object - more extensive testing of 'request' object is in ClientPoliciesTest.testSecureRequestObjectExecutor() requestObject = createValidRequestObjectForSecureRequestObjectExecutor("foo"); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/par/ParTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/par/ParTest.java index 51b2f55d32..63f9372e67 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/par/ParTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/par/ParTest.java @@ -44,6 +44,7 @@ import org.keycloak.OAuthErrorException; import org.keycloak.admin.client.resource.ClientResource; import org.keycloak.common.util.Base64Url; import org.keycloak.common.util.Time; +import org.keycloak.crypto.Algorithm; import org.keycloak.models.AdminRoles; import org.keycloak.models.Constants; import org.keycloak.models.ParConfig; @@ -209,6 +210,68 @@ public class ParTest extends AbstractClientPoliciesTest { } } + @Test + public void testWrongSigningAlgorithmForRequestObject() throws Exception { + try { + // setup PAR realm settings + int requestUriLifespan = 45; + setParRealmSettings(requestUriLifespan); + + // create client dynamically + String clientId = createClientDynamically(generateSuffixedName(CLIENT_NAME), + (OIDCClientRepresentation clientRep) -> { + clientRep.setRequirePushedAuthorizationRequests(Boolean.TRUE); + clientRep.setRedirectUris(new ArrayList<>(Arrays.asList(CLIENT_REDIRECT_URI))); + clientRep.setRequestObjectSigningAlg(Algorithm.PS256); + }); + + oauth.clientId(clientId); + + OIDCClientRepresentation oidcCRep = getClientDynamically(clientId); + String clientSecret = oidcCRep.getClientSecret(); + assertEquals(Boolean.TRUE, oidcCRep.getRequirePushedAuthorizationRequests()); + assertTrue(oidcCRep.getRedirectUris().contains(CLIENT_REDIRECT_URI)); + assertEquals(OIDCLoginProtocol.CLIENT_SECRET_BASIC, oidcCRep.getTokenEndpointAuthMethod()); + + TestingOIDCEndpointsApplicationResource.AuthorizationEndpointRequestObject requestObject = new TestingOIDCEndpointsApplicationResource.AuthorizationEndpointRequestObject(); + requestObject.id(KeycloakModelUtils.generateId()); + requestObject.iat(Long.valueOf(Time.currentTime())); + requestObject.exp(requestObject.getIat() + Long.valueOf(300)); + requestObject.nbf(requestObject.getIat()); + requestObject.setClientId(oauth.getClientId()); + requestObject.setResponseType("code"); + requestObject.setRedirectUriParam(CLIENT_REDIRECT_URI); + requestObject.setScope("openid"); + requestObject.setNonce(KeycloakModelUtils.generateId()); + + byte[] contentBytes = JsonSerialization.writeValueAsBytes(requestObject); + String encodedRequestObject = Base64Url.encode(contentBytes); + TestOIDCEndpointsApplicationResource client = testingClient.testApp().oidcClientEndpoints(); + + // use and set jwks_url + ClientResource clientResource = ApiUtil + .findClientByClientId(adminClient.realm(oauth.getRealm()), oauth.getClientId()); + ClientRepresentation clientRep = clientResource.toRepresentation(); + OIDCAdvancedConfigWrapper.fromClientRepresentation(clientRep).setUseJwksUrl(true); + OIDCAdvancedConfigWrapper.fromClientRepresentation(clientRep) + .setJwksUrl(TestApplicationResourceUrls.clientJwksUri()); + clientResource.update(clientRep); + client.generateKeys(org.keycloak.crypto.Algorithm.RS256); + client.registerOIDCRequest(encodedRequestObject, org.keycloak.crypto.Algorithm.RS256); + + // do not send any other parameter but the request request parameter + oauth.request(client.getOIDCRequest()); + oauth.responseType(null); + oauth.redirectUri(null); + oauth.scope(null); + ParResponse pResp = oauth.doPushedAuthorizationRequest(clientId, clientSecret); + assertEquals(400, pResp.getStatusCode()); + assertEquals(OAuthErrorException.INVALID_REQUEST_OBJECT, pResp.getError()); + } finally { + restoreParRealmSettings(); + } + } + @Test public void testSuccessfulUsingRequestParameter() throws Exception { try { @@ -903,7 +966,7 @@ public class ParTest extends AbstractClientPoliciesTest { oauth.redirectUri(CLIENT_REDIRECT_URI); ParResponse pResp = oauth.doPushedAuthorizationRequest(clientId, clientSecret); assertEquals(400, pResp.getStatusCode()); - assertEquals(OAuthErrorException.INVALID_REQUEST, pResp.getError()); + assertEquals(OAuthErrorException.INVALID_REQUEST_OBJECT, pResp.getError()); } // PAR including invalid redirect_uri