diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java index c41f06b4bb..e595b34b41 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java @@ -55,6 +55,7 @@ import javax.ws.rs.core.MediaType; import javax.ws.rs.core.MultivaluedMap; import javax.ws.rs.core.Response; +import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -111,7 +112,7 @@ public class AuthorizationEndpoint extends AuthorizationEndpointBase { } private Response process(MultivaluedMap params) { - String clientId = params.getFirst(OIDCLoginProtocol.CLIENT_ID_PARAM); + String clientId = AuthorizationEndpointRequestParserProcessor.getClientId(event, session, params); checkSsl(); checkRealm(); @@ -125,6 +126,11 @@ public class AuthorizationEndpoint extends AuthorizationEndpointBase { return errorResponse; } + if (request.getInvalidRequestMessage() != null) { + event.error(Errors.INVALID_REQUEST); + return redirectErrorToClient(parsedResponseMode, Errors.INVALID_REQUEST, request.getInvalidRequestMessage()); + } + if (!TokenUtil.isOIDCRequest(request.getScope())) { ServicesLogger.LOGGER.oidcScopeMissing(); } diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthorizationEndpointRequest.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthorizationEndpointRequest.java index 71b1478621..b3426d9165 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthorizationEndpointRequest.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthorizationEndpointRequest.java @@ -25,6 +25,8 @@ import java.util.Map; */ public class AuthorizationEndpointRequest { + String invalidRequestMessage; + String clientId; String redirectUriParam; String responseType; @@ -120,4 +122,8 @@ public class AuthorizationEndpointRequest { public String getDisplay() { return display; } + + public String getInvalidRequestMessage() { + return invalidRequestMessage; + } } diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthorizationEndpointRequestParserProcessor.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthorizationEndpointRequestParserProcessor.java index 74ec64257c..66441b066e 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthorizationEndpointRequestParserProcessor.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthorizationEndpointRequestParserProcessor.java @@ -33,6 +33,7 @@ import org.keycloak.services.messages.Messages; import javax.ws.rs.core.MultivaluedMap; import javax.ws.rs.core.Response; import java.io.InputStream; +import java.util.List; /** * @author Marek Posolda @@ -43,7 +44,13 @@ public class AuthorizationEndpointRequestParserProcessor { try { AuthorizationEndpointRequest request = new AuthorizationEndpointRequest(); - new AuthzEndpointQueryStringParser(requestParams).parseRequest(request); + AuthzEndpointQueryStringParser parser = new AuthzEndpointQueryStringParser(requestParams); + parser.parseRequest(request); + + if (parser.getInvalidRequestMessage() != null) { + request.invalidRequestMessage = parser.getInvalidRequestMessage(); + return request; + } String requestParam = requestParams.getFirst(OIDCLoginProtocol.REQUEST_PARAM); String requestUriParam = requestParams.getFirst(OIDCLoginProtocol.REQUEST_URI_PARAM); @@ -83,4 +90,15 @@ public class AuthorizationEndpointRequestParserProcessor { throw new ErrorPageException(session, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); } } + + public static String getClientId(EventBuilder event, KeycloakSession session, MultivaluedMap requestParams) { + List clientParam = requestParams.get(OIDCLoginProtocol.CLIENT_ID_PARAM); + if (clientParam != null && clientParam.size() == 1) { + return clientParam.get(0); + } else { + event.error(Errors.INVALID_REQUEST); + throw new ErrorPageException(session, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); + } + } + } diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointQueryStringParser.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointQueryStringParser.java index 148576e9a9..cde8233a56 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointQueryStringParser.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointQueryStringParser.java @@ -18,6 +18,7 @@ package org.keycloak.protocol.oidc.endpoints.request; import javax.ws.rs.core.MultivaluedMap; + import java.util.Set; /** @@ -29,23 +30,40 @@ class AuthzEndpointQueryStringParser extends AuthzEndpointRequestParser { private final MultivaluedMap requestParams; + private String invalidRequestMessage = null; + public AuthzEndpointQueryStringParser(MultivaluedMap requestParams) { this.requestParams = requestParams; } @Override protected String getParameter(String paramName) { + checkDuplicated(requestParams, paramName); return requestParams.getFirst(paramName); } @Override protected Integer getIntParameter(String paramName) { + checkDuplicated(requestParams, paramName); String paramVal = requestParams.getFirst(paramName); return paramVal==null ? null : Integer.parseInt(paramVal); } + public String getInvalidRequestMessage() { + return invalidRequestMessage; + } + @Override protected Set keySet() { return requestParams.keySet(); } + + private void checkDuplicated(MultivaluedMap requestParams, String paramName) { + if (invalidRequestMessage == null) { + if (requestParams.get(paramName) != null && requestParams.get(paramName).size() != 1) { + invalidRequestMessage = "duplicated parameter"; + } + } + } + } diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestParser.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestParser.java index efb609c865..50cbc40353 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestParser.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestParser.java @@ -70,10 +70,8 @@ abstract class AuthzEndpointRequestParser { // https://tools.ietf.org/html/rfc7636#section-6.1 KNOWN_REQ_PARAMS.add(OIDCLoginProtocol.CODE_CHALLENGE_PARAM); KNOWN_REQ_PARAMS.add(OIDCLoginProtocol.CODE_CHALLENGE_METHOD_PARAM); - } - public void parseRequest(AuthorizationEndpointRequest request) { String clientId = getParameter(OIDCLoginProtocol.CLIENT_ID_PARAM); @@ -104,7 +102,6 @@ abstract class AuthzEndpointRequestParser { extractAdditionalReqParams(request.additionalReqParams); } - protected void extractAdditionalReqParams(Map additionalReqParams) { for (String paramName : keySet()) { if (!KNOWN_REQ_PARAMS.contains(paramName)) { @@ -130,7 +127,6 @@ abstract class AuthzEndpointRequestParser { return newVal==null ? previousVal : newVal; } - protected abstract String getParameter(String paramName); protected abstract Integer getIntParameter(String paramName); diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java index 345e832c41..88b4e9eedf 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java @@ -157,6 +157,8 @@ public class OAuthClient { private String codeChallengeMethod; private String origin; + private Map customParameters; + private boolean openid = true; private Supplier httpClient = OAuthClient::newCloseableHttpClient; @@ -222,6 +224,7 @@ public class OAuthClient { codeChallenge = null; codeChallengeMethod = null; origin = null; + customParameters = null; openid = true; } @@ -884,7 +887,11 @@ public class OAuthClient { } if (codeChallengeMethod != null) { b.queryParam(OAuth2Constants.CODE_CHALLENGE_METHOD, codeChallengeMethod); - } + } + if (customParameters != null) { + customParameters.keySet().stream().forEach(i -> b.queryParam(i, customParameters.get(i))); + } + return b.build(realm).toString(); } @@ -1053,6 +1060,14 @@ public class OAuthClient { return this; } + public OAuthClient addCustomerParameter(String key, String value) { + if (customParameters == null) { + customParameters = new HashMap<>(); + } + customParameters.put(key, value); + return this; + } + public static class AuthorizationEndpointResponse { private boolean isRedirected; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java index 83e0988ce6..a3b3f098eb 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java @@ -16,13 +16,13 @@ */ package org.keycloak.testsuite.oauth; +import org.jboss.arquillian.graphene.page.Page; import org.junit.Assert; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.keycloak.OAuth2Constants; import org.keycloak.OAuthErrorException; -import org.keycloak.connections.infinispan.InfinispanConnectionProvider; import org.keycloak.events.Details; import org.keycloak.events.Errors; import org.keycloak.models.Constants; @@ -30,6 +30,7 @@ import org.keycloak.protocol.oidc.utils.OIDCResponseMode; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.testsuite.AbstractKeycloakTest; import org.keycloak.testsuite.AssertEvents; +import org.keycloak.testsuite.pages.ErrorPage; import org.keycloak.testsuite.pages.PageUtils; import org.keycloak.testsuite.util.ClientManager; import org.keycloak.testsuite.util.OAuthClient; @@ -38,9 +39,12 @@ import org.openqa.selenium.By; import javax.ws.rs.core.UriBuilder; import java.io.IOException; import java.net.URI; +import java.util.HashMap; import java.util.List; +import java.util.Map; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.keycloak.testsuite.admin.AbstractAdminTest.loadJson; /** @@ -51,6 +55,9 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest { @Rule public AssertEvents events = new AssertEvents(this); + @Page + private ErrorPage errorPage; + @Override public void addTestRealms(List testRealms) { RealmRepresentation realmRepresentation = loadJson(getClass().getResourceAsStream("/testrealm.json"), RealmRepresentation.class); @@ -70,7 +77,7 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest { OAuthClient.AuthorizationEndpointResponse response = oauth.doLogin("test-user@localhost", "password"); - Assert.assertTrue(response.isRedirected()); + assertTrue(response.isRedirected()); Assert.assertNotNull(response.getCode()); assertEquals("OpenIdConnect.AuthenticationProperties=2302984sdlk", response.getState()); Assert.assertNull(response.getError()); @@ -101,7 +108,7 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest { OAuthClient.AuthorizationEndpointResponse response = oauth.doLogin("test-user@localhost", "password"); - Assert.assertTrue(response.isRedirected()); + assertTrue(response.isRedirected()); Assert.assertNotNull(response.getCode()); String codeId = events.expectLogin().assertEvent().getDetails().get(Details.CODE_ID); @@ -113,7 +120,7 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest { OAuthClient.AuthorizationEndpointResponse response = oauth.doLogin("test-user@localhost", "password"); - Assert.assertTrue(response.isRedirected()); + assertTrue(response.isRedirected()); Assert.assertNotNull(response.getCode()); Assert.assertNull(response.getState()); Assert.assertNull(response.getError()); @@ -128,7 +135,7 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest { driver.navigate().to(b.build().toURL()); OAuthClient.AuthorizationEndpointResponse errorResponse = new OAuthClient.AuthorizationEndpointResponse(oauth); - Assert.assertTrue(errorResponse.isRedirected()); + assertTrue(errorResponse.isRedirected()); Assert.assertEquals(errorResponse.getError(), OAuthErrorException.UNSUPPORTED_RESPONSE_TYPE); events.expectLogin().error(Errors.INVALID_REQUEST).user((String) null).session((String) null).clearDetails().detail(Details.RESPONSE_TYPE, "tokenn").assertEvent(); @@ -197,4 +204,39 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest { Assert.assertNull(currentUri.getRawFragment()); } + @Test + public void authorizationRequestParamsMoreThanOnce() throws IOException { + oauth.stateParamHardcoded("OpenIdConnect.AuthenticationProperties=2302984sdlk"); + Map extraParams = new HashMap<>(); + + oauth.addCustomerParameter(OAuth2Constants.SCOPE, "read_write") + .addCustomerParameter(OAuth2Constants.STATE, "abcdefg") + .addCustomerParameter(OAuth2Constants.SCOPE, "pop push"); + + oauth.openLoginForm(); + + assertEquals("invalid_request", oauth.getCurrentQuery().get("error")); + assertEquals("duplicated parameter", oauth.getCurrentQuery().get("error_description")); + + events.expectLogin().error(Errors.INVALID_REQUEST).user((String) null).session((String) null).clearDetails().assertEvent(); + } + + @Test + public void authorizationRequestClientParamsMoreThanOnce() throws IOException { + oauth.stateParamHardcoded("OpenIdConnect.AuthenticationProperties=2302984sdlk"); + + oauth.addCustomerParameter(OAuth2Constants.SCOPE, "read_write") + .addCustomerParameter(OAuth2Constants.CLIENT_ID, "client2client") + .addCustomerParameter(OAuth2Constants.REDIRECT_URI, "https://www.example.com") + .addCustomerParameter(OAuth2Constants.STATE, "abcdefg") + .addCustomerParameter(OAuth2Constants.SCOPE, "pop push"); + + oauth.openLoginForm(); + + assertTrue(errorPage.isCurrent()); + assertEquals("Invalid Request", errorPage.getError()); + + events.expectLogin().error(Errors.INVALID_REQUEST).user((String) null).session((String) null).client((String) null).clearDetails().assertEvent(); + } + }