From 36b01cbea093d38c495c9e0f2ef66feb3a3f9e41 Mon Sep 17 00:00:00 2001 From: Thomas Darimont Date: Tue, 29 Oct 2024 00:16:50 +0100 Subject: [PATCH] Revise PAR request object parameter handlig (#34352) We now store the original parameter value as-is, in case only a single parameter value is provided. In case multiple parameter values are provided for the same parameter, we only retain the first parameter. This ensures that the original value is retained. Previously the value list from the `decodedFormParameters` `MultivaluedMap` was converted to a String while replacing '[' and ']' with an empty string, which corrupted the original parameter values stored. Fixes #34352 Signed-off-by: Thomas Darimont --- .../oidc/par/endpoints/ParEndpoint.java | 45 ++++++-- .../testsuite/oauth/par/ParEndpointTest.java | 101 ++++++++++++++++++ 2 files changed, 137 insertions(+), 9 deletions(-) create mode 100644 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/par/ParEndpointTest.java 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 188aad9e3f..9c46fe7061 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 @@ -17,6 +17,7 @@ package org.keycloak.protocol.oidc.par.endpoints; +import jakarta.ws.rs.core.MultivaluedMap; import org.keycloak.http.HttpRequest; import org.keycloak.OAuthErrorException; import org.keycloak.common.Profile; @@ -46,6 +47,7 @@ import jakarta.ws.rs.core.UriBuilder; import static org.keycloak.protocol.oidc.OIDCLoginProtocol.REQUEST_URI_PARAM; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.UUID; @@ -88,12 +90,14 @@ public class ParEndpoint extends AbstractParEndpoint { checkRealm(); authorizeClient(); - if (httpRequest.getDecodedFormParameters().containsKey(REQUEST_URI_PARAM)) { + MultivaluedMap decodedFormParameters = httpRequest.getDecodedFormParameters(); + + if (decodedFormParameters.containsKey(REQUEST_URI_PARAM)) { throw throwErrorResponseException(OAuthErrorException.INVALID_REQUEST, "It is not allowed to include request_uri to PAR.", Response.Status.BAD_REQUEST); } try { - authorizationRequest = ParEndpointRequestParserProcessor.parseRequest(event, session, client, httpRequest.getDecodedFormParameters()); + authorizationRequest = ParEndpointRequestParserProcessor.parseRequest(event, session, client, decodedFormParameters); } catch (Exception e) { throw throwErrorResponseException(OAuthErrorException.INVALID_REQUEST_OBJECT, e.getMessage(), Response.Status.BAD_REQUEST); } @@ -138,7 +142,7 @@ public class ParEndpoint extends AbstractParEndpoint { } try { - session.clientPolicy().triggerOnEvent(new PushedAuthorizationRequestContext(authorizationRequest, httpRequest.getDecodedFormParameters())); + session.clientPolicy().triggerOnEvent(new PushedAuthorizationRequestContext(authorizationRequest, decodedFormParameters)); } catch (ClientPolicyException cpe) { throw throwErrorResponseException(cpe.getError(), cpe.getErrorDetail(), Response.Status.BAD_REQUEST); } @@ -150,11 +154,8 @@ public class ParEndpoint extends AbstractParEndpoint { int expiresIn = realm.getParPolicy().getRequestUriLifespan(); - httpRequest.getDecodedFormParameters().forEach((k, v) -> { - // PAR store only accepts Map so that MultivaluedMap needs to be converted to Map. - String singleValue = String.valueOf(v).replace("[", "").replace("]", ""); - params.put(k, singleValue); - }); + flattenDecodedFormParametersToParamsMap(decodedFormParameters, params); + params.put(PAR_CREATED_TIME, String.valueOf(System.currentTimeMillis())); SingleUseObjectProvider singleUseStore = session.singleUseObjects(); @@ -168,4 +169,30 @@ public class ParEndpoint extends AbstractParEndpoint { .type(MediaType.APPLICATION_JSON_TYPE)); } -} \ No newline at end of file + /** + * Flattens the given decodedFormParameters MultivaluedMap to a plain Map. + * Rationale: The SingleUseObjectProvider used as store for PARs only accepts Map so that MultivaluedMap needs to be converted to Map. + * @param decodedFormParameters form parameters sent in request body + * @param params target parameter Map + */ + public static void flattenDecodedFormParametersToParamsMap( + MultivaluedMap decodedFormParameters, + Map params) { + + for (var parameterEntry : decodedFormParameters.entrySet()) { + String parameterName = parameterEntry.getKey(); + List parameterValues = parameterEntry.getValue(); + + if (parameterValues.isEmpty()) { + // We emit the empty parameter as a marker, but only if it does not exist yet. This prevents "accidental" value overrides. + params.putIfAbsent(parameterName, null); + } else { + // We flatten the MultivaluedMap values list by emitting the first value only. + // We override potential empty parameters that were added to the params map before. + params.put(parameterName, parameterValues.get(0)); + } + + } + } + +} diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/par/ParEndpointTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/par/ParEndpointTest.java new file mode 100644 index 0000000000..b7a378655f --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/par/ParEndpointTest.java @@ -0,0 +1,101 @@ +/* + * Copyright 2024 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.testsuite.oauth.par; + +import jakarta.ws.rs.core.MultivaluedHashMap; +import org.junit.Assert; +import org.junit.Test; +import org.keycloak.protocol.oidc.par.endpoints.ParEndpoint; + + +import java.util.HashMap; +import java.util.List; + +import static org.keycloak.protocol.oidc.OIDCLoginProtocol.AUTHORIZATION_DETAILS_PARAM; + +public class ParEndpointTest { + + @Test + public void testFlattenDecodedFormParametersRetainAuthorizationDetails() { + var decodedFormParameters = new MultivaluedHashMap(); + String authorizationDetails = "[{\"type\": \"urn:openfinanceuae:account-access-consent:v1.0\",\"foo\":\"bar\"},{\"type\": \"urn:openfinanceuae:account-access-consent:v1.0\",\"gugu\":\"gaga\"}]"; + decodedFormParameters.put(AUTHORIZATION_DETAILS_PARAM, List.of(authorizationDetails)); + var params = new HashMap(); + + ParEndpoint.flattenDecodedFormParametersToParamsMap(decodedFormParameters, params); + + Assert.assertEquals(authorizationDetails, params.get(AUTHORIZATION_DETAILS_PARAM)); + } + + @Test + public void testFlattenDecodedFormParametersMultipleValues() { + var decodedFormParameters = new MultivaluedHashMap(); + decodedFormParameters.put("param", List.of("paramValue1", "paramValue2")); + var params = new HashMap(); + + ParEndpoint.flattenDecodedFormParametersToParamsMap(decodedFormParameters, params); + + Assert.assertEquals("paramValue1", params.get("param")); + } + + @Test + public void testFlattenDecodedFormParametersSingleValue() { + var decodedFormParameters = new MultivaluedHashMap(); + decodedFormParameters.put("param", List.of("single")); + var params = new HashMap(); + + ParEndpoint.flattenDecodedFormParametersToParamsMap(decodedFormParameters, params); + + Assert.assertEquals("single", params.get("param")); + } + + @Test + public void testFlattenDecodedFormParametersNullValue() { + var decodedFormParameters = new MultivaluedHashMap(); + decodedFormParameters.add("param", null); + var params = new HashMap(); + + ParEndpoint.flattenDecodedFormParametersToParamsMap(decodedFormParameters, params); + + Assert.assertNull(params.get("param")); + } + + @Test + public void testFlattenDecodedFormParametersValueWithNull() { + var decodedFormParameters = new MultivaluedHashMap(); + decodedFormParameters.add("param", "value"); + decodedFormParameters.add("param", null); + var params = new HashMap(); + + ParEndpoint.flattenDecodedFormParametersToParamsMap(decodedFormParameters, params); + + Assert.assertEquals("value", params.get("param")); + } + + @Test + public void testFlattenDecodedFormParametersMultipleDistinctValues() { + var decodedFormParameters = new MultivaluedHashMap(); + decodedFormParameters.add("param", "valueAAA"); + decodedFormParameters.add("param", "valueBBB"); + var params = new HashMap(); + + ParEndpoint.flattenDecodedFormParametersToParamsMap(decodedFormParameters, params); + + Assert.assertEquals("valueAAA", params.get("param")); + } +}