diff --git a/services/src/main/java/org/keycloak/services/clientregistration/AbstractClientRegistrationProvider.java b/services/src/main/java/org/keycloak/services/clientregistration/AbstractClientRegistrationProvider.java index 88b5a340c1..d634b006b7 100755 --- a/services/src/main/java/org/keycloak/services/clientregistration/AbstractClientRegistrationProvider.java +++ b/services/src/main/java/org/keycloak/services/clientregistration/AbstractClientRegistrationProvider.java @@ -29,8 +29,12 @@ import org.keycloak.models.utils.RepresentationToModel; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.services.ErrorResponseException; import org.keycloak.services.ForbiddenException; +import org.keycloak.services.resources.admin.AdminRoot; +import org.keycloak.services.validation.ClientValidator; +import org.keycloak.services.validation.ValidationMessages; import javax.ws.rs.core.Response; +import java.util.Properties; /** * @author Stian Thorgersen @@ -50,6 +54,16 @@ public abstract class AbstractClientRegistrationProvider implements ClientRegist auth.requireCreate(); + ValidationMessages validationMessages = new ValidationMessages(); + if (!ClientValidator.validate(client, validationMessages)) { + String errorCode = validationMessages.fieldHasError("redirectUris") ? ErrorCodes.INVALID_REDIRECT_URI : ErrorCodes.INVALID_CLIENT_METADATA; + throw new ErrorResponseException( + errorCode, + validationMessages.getStringMessages(), + Response.Status.BAD_REQUEST + ); + } + try { ClientModel clientModel = RepresentationToModel.createClient(session, session.getContext().getRealm(), client, true); @@ -104,6 +118,16 @@ public abstract class AbstractClientRegistrationProvider implements ClientRegist throw new ErrorResponseException(ErrorCodes.INVALID_CLIENT_METADATA, "Client Identifier modified", Response.Status.BAD_REQUEST); } + ValidationMessages validationMessages = new ValidationMessages(); + if (!ClientValidator.validate(rep, validationMessages)) { + String errorCode = validationMessages.fieldHasError("redirectUris") ? ErrorCodes.INVALID_REDIRECT_URI : ErrorCodes.INVALID_CLIENT_METADATA; + throw new ErrorResponseException( + errorCode, + validationMessages.getStringMessages(), + Response.Status.BAD_REQUEST + ); + } + RepresentationToModel.updateClient(rep, client); rep = ModelToRepresentation.toRepresentation(client); diff --git a/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java b/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java index fc5b3c9e93..dd17b07a8e 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java @@ -40,6 +40,7 @@ import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.CredentialRepresentation; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.representations.idm.UserSessionRepresentation; +import org.keycloak.services.ErrorResponseException; import org.keycloak.services.ServicesLogger; import org.keycloak.services.clientregistration.ClientRegistrationTokenUtils; import org.keycloak.services.managers.ClientManager; @@ -48,6 +49,8 @@ import org.keycloak.services.managers.ResourceAdminManager; import org.keycloak.services.resources.KeycloakApplication; import org.keycloak.services.ErrorResponse; import org.keycloak.common.util.Time; +import org.keycloak.services.validation.ClientValidator; +import org.keycloak.services.validation.ValidationMessages; import javax.ws.rs.Consumes; import javax.ws.rs.DELETE; @@ -63,10 +66,7 @@ import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import javax.ws.rs.core.UriInfo; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; +import java.util.*; import static java.lang.Boolean.TRUE; @@ -126,6 +126,16 @@ public class ClientResource { throw new NotFoundException("Could not find client"); } + ValidationMessages validationMessages = new ValidationMessages(); + if (!ClientValidator.validate(rep, validationMessages)) { + Properties messages = AdminRoot.getMessages(session, realm, auth.getAuth().getToken().getLocale()); + throw new ErrorResponseException( + validationMessages.getStringMessages(), + validationMessages.getStringMessages(messages), + Response.Status.BAD_REQUEST + ); + } + try { updateClientFromRep(rep, client, session); adminEvent.operation(OperationType.UPDATE).resourcePath(uriInfo).representation(rep).success(); diff --git a/services/src/main/java/org/keycloak/services/resources/admin/ClientsResource.java b/services/src/main/java/org/keycloak/services/resources/admin/ClientsResource.java index 207f8d1fb5..dacea26b68 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/ClientsResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/ClientsResource.java @@ -17,7 +17,6 @@ package org.keycloak.services.resources.admin; import org.jboss.resteasy.annotations.cache.NoCache; -import org.jboss.resteasy.spi.NotFoundException; import org.jboss.resteasy.spi.ResteasyProviderFactory; import org.keycloak.events.admin.OperationType; import org.keycloak.events.admin.ResourceType; @@ -28,16 +27,13 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.services.ErrorResponse; +import org.keycloak.services.ErrorResponseException; import org.keycloak.services.ServicesLogger; import org.keycloak.services.managers.ClientManager; +import org.keycloak.services.validation.ClientValidator; +import org.keycloak.services.validation.ValidationMessages; -import javax.ws.rs.Consumes; -import javax.ws.rs.GET; -import javax.ws.rs.POST; -import javax.ws.rs.Path; -import javax.ws.rs.PathParam; -import javax.ws.rs.Produces; -import javax.ws.rs.QueryParam; +import javax.ws.rs.*; import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; @@ -45,6 +41,7 @@ import javax.ws.rs.core.UriInfo; import java.util.ArrayList; import java.util.List; +import java.util.Properties; /** * Base resource class for managing a realm's clients. @@ -122,6 +119,16 @@ public class ClientsResource { public Response createClient(final @Context UriInfo uriInfo, final ClientRepresentation rep) { auth.requireManage(); + ValidationMessages validationMessages = new ValidationMessages(); + if (!ClientValidator.validate(rep, validationMessages)) { + Properties messages = AdminRoot.getMessages(session, realm, auth.getAuth().getToken().getLocale()); + throw new ErrorResponseException( + validationMessages.getStringMessages(), + validationMessages.getStringMessages(messages), + Response.Status.BAD_REQUEST + ); + } + try { ClientModel clientModel = ClientManager.createClient(session, realm, rep, true); diff --git a/services/src/main/java/org/keycloak/services/validation/ClientValidator.java b/services/src/main/java/org/keycloak/services/validation/ClientValidator.java new file mode 100644 index 0000000000..22290c5a12 --- /dev/null +++ b/services/src/main/java/org/keycloak/services/validation/ClientValidator.java @@ -0,0 +1,54 @@ +/* + * + * * Copyright 2016 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.services.validation; + +import org.keycloak.representations.idm.ClientRepresentation; + +/** + * @author Vaclav Muzikar + */ +public class ClientValidator { + /** + * Checks if the Client's Redirect URIs doesn't contain any URI fragments (like http://example.org/auth#fragment) + * + * @see KEYCLOAK-3421 + * @param client + * @param messages + * @return true if Redirect URIs doesn't contain any URI with fragments + */ + public static boolean validate(ClientRepresentation client, ValidationMessages messages) { + boolean isValid = true; + + if (client.getRedirectUris() != null) { + long urisWithFragmentCount = client.getRedirectUris().stream().filter(p -> p.contains("#")).count(); + if (urisWithFragmentCount > 0) { + messages.add("redirectUris", "Redirect URIs must not contain an URI fragment", "clientRedirectURIsFragmentError"); + isValid = false; + } + } + + if (client.getRootUrl() != null && client.getRootUrl().contains("#")) { + messages.add("rootUrl", "Root URL must not contain an URL fragment", "clientRootURLFragmentError"); + isValid = false; + } + + return isValid; + } +} diff --git a/services/src/main/java/org/keycloak/services/validation/ValidationMessage.java b/services/src/main/java/org/keycloak/services/validation/ValidationMessage.java new file mode 100644 index 0000000000..7e4dac12a5 --- /dev/null +++ b/services/src/main/java/org/keycloak/services/validation/ValidationMessage.java @@ -0,0 +1,100 @@ +/* + * + * * Copyright 2016 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.services.validation; + +import java.text.MessageFormat; +import java.util.Properties; + +/** + * @author Vaclav Muzikar + */ +public class ValidationMessage { + private String fieldId; + private String message; + private String localizedMessageKey; + private Object[] localizedMessageParameters; + + public ValidationMessage(String message) { + this.message = message; + } + + public ValidationMessage(String message, String localizedMessageKey, Object... localizedMessageParameters) { + this.message = message; + this.localizedMessageKey = localizedMessageKey; + this.localizedMessageParameters = localizedMessageParameters; + } + + public String getFieldId() { + return fieldId; + } + + public void setFieldId(String fieldId) { + this.fieldId = fieldId; + } + + public String getLocalizedMessageKey() { + return localizedMessageKey; + } + + public void setLocalizedMessageKey(String localizedMessageKey) { + this.localizedMessageKey = localizedMessageKey; + } + + public Object[] getLocalizedMessageParameters() { + return localizedMessageParameters; + } + + public void setLocalizedMessageParameters(Object[] localizedMessageParameters) { + this.localizedMessageParameters = localizedMessageParameters; + } + + public String getMessage() { + return message; + } + + public String getMessage(Properties localizedMessages) { + if (getLocalizedMessageKey() != null) { + return MessageFormat.format(localizedMessages.getProperty(getLocalizedMessageKey(), getMessage()), getLocalizedMessageParameters()); + } + else { + return getMessage(); + } + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + + ValidationMessage message1 = (ValidationMessage) o; + + if (getFieldId() != null ? !getFieldId().equals(message1.getFieldId()) : message1.getFieldId() != null) + return false; + return getMessage() != null ? getMessage().equals(message1.getMessage()) : message1.getMessage() == null; + + } + + @Override + public int hashCode() { + int result = getFieldId() != null ? getFieldId().hashCode() : 0; + result = 31 * result + (getMessage() != null ? getMessage().hashCode() : 0); + return result; + } +} diff --git a/services/src/main/java/org/keycloak/services/validation/ValidationMessages.java b/services/src/main/java/org/keycloak/services/validation/ValidationMessages.java new file mode 100644 index 0000000000..e26ebff397 --- /dev/null +++ b/services/src/main/java/org/keycloak/services/validation/ValidationMessages.java @@ -0,0 +1,83 @@ +/* + * + * * Copyright 2016 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.services.validation; + +import java.util.*; +import java.util.function.Function; +import java.util.stream.Collectors; + + +/** + * @author Vaclav Muzikar + */ +public class ValidationMessages { + private Set messages = new LinkedHashSet<>(); + + public ValidationMessages() {} + + public ValidationMessages(String... messages) { + for (String message : messages) { + add(message); + } + } + + public void add(String message) { + messages.add(new ValidationMessage(message)); + } + + public void add(String message, String localizedMessageKey) { + messages.add(new ValidationMessage(message, localizedMessageKey)); + } + + public void add(String fieldId, String message, String localizedMessageKey) { + ValidationMessage validationMessage = new ValidationMessage(message, localizedMessageKey); + validationMessage.setFieldId(fieldId); + add(validationMessage); + } + + public void add(ValidationMessage message) { + messages.add(message); + } + + public boolean fieldHasError(String fieldId) { + for (ValidationMessage message : messages) { + if (message.getFieldId().equals(fieldId)) { + return true; + } + } + return false; + } + + public Set getMessages() { + return Collections.unmodifiableSet(messages); + } + + protected String getStringMessages(Function function) { + return messages.stream().map(function).collect(Collectors.joining("; ")); + } + + public String getStringMessages() { + return getStringMessages(ValidationMessage::getMessage); + } + + public String getStringMessages(Properties localizedMessages) { + return getStringMessages(x -> x.getMessage(localizedMessages)); + } +} diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java index ee06750e93..b6f9bdea8b 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java @@ -33,16 +33,14 @@ import org.keycloak.representations.adapters.action.PushNotBeforeAction; import org.keycloak.representations.adapters.action.TestAvailabilityAction; import org.keycloak.representations.idm.*; +import javax.ws.rs.BadRequestException; import javax.ws.rs.NotFoundException; import javax.ws.rs.core.Response; import java.io.IOException; -import java.util.Collections; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; +import org.keycloak.services.ErrorResponseException; import org.keycloak.testsuite.Assert; import org.keycloak.testsuite.util.AdminEventPaths; import org.keycloak.testsuite.util.ClientBuilder; @@ -212,6 +210,52 @@ public class ClientTest extends AbstractAdminTest { assertEquals("service-account-serviceclient", userRep.getUsername()); } + // KEYCLOAK-3421 + @Test + public void createClientWithFragments() { + ClientRepresentation client = ClientBuilder.create() + .clientId("client-with-fragment") + .rootUrl("http://localhost/base#someFragment") + .redirectUris("http://localhost/auth", "http://localhost/auth#fragment", "http://localhost/auth*", "/relative") + .build(); + + Response response = realm.clients().create(client); + assertUriFragmentError(response); + } + + // KEYCLOAK-3421 + @Test + public void updateClientWithFragments() { + ClientRepresentation client = ClientBuilder.create() + .clientId("client-with-fragment") + .redirectUris("http://localhost/auth", "http://localhost/auth*") + .build(); + Response response = realm.clients().create(client); + ClientResource clientResource = realm.clients().get(ApiUtil.getCreatedId(response)); + + client = clientResource.toRepresentation(); + client.setRootUrl("http://localhost/base#someFragment"); + List redirectUris = client.getRedirectUris(); + redirectUris.add("http://localhost/auth#fragment"); + redirectUris.add("/relative"); + client.setRedirectUris(redirectUris); + + try { + clientResource.update(client); + fail("Should fail"); + } + catch (BadRequestException e) { + assertUriFragmentError(e.getResponse()); + } + } + + private void assertUriFragmentError(Response response) { + assertEquals(response.getStatus(), 400); + String error = response.readEntity(OAuth2ErrorRepresentation.class).getError(); + assertTrue("Error response doesn't mention Redirect URIs fragments", error.contains("Redirect URIs must not contain an URI fragment")); + assertTrue("Error response doesn't mention Root URL fragments", error.contains("Root URL must not contain an URL fragment")); + } + @Test public void pushRevocation() { testingClient.testApp().clearAdminActions(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCClientRegistrationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCClientRegistrationTest.java index 9c78c4b040..608d7a73c5 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCClientRegistrationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCClientRegistrationTest.java @@ -102,6 +102,23 @@ public class OIDCClientRegistrationTest extends AbstractClientRegistrationTest { return response; } + private void assertCreateFail(OIDCClientRepresentation client, int expectedStatusCode) { + assertCreateFail(client, expectedStatusCode, null); + } + + private void assertCreateFail(OIDCClientRepresentation client, int expectedStatusCode, String expectedErrorContains) { + try { + reg.oidc().create(client); + Assert.fail("Not expected to successfuly register client"); + } catch (ClientRegistrationException expected) { + HttpErrorException httpEx = (HttpErrorException) expected.getCause(); + Assert.assertEquals(expectedStatusCode, httpEx.getStatusLine().getStatusCode()); + if (expectedErrorContains != null) { + assertTrue("Error response doesn't contain expected text", httpEx.getErrorResponse().contains(expectedErrorContains)); + } + } + } + @Test public void testCreateWithTrustedHost() throws Exception { reg.auth(null); @@ -109,17 +126,10 @@ public class OIDCClientRegistrationTest extends AbstractClientRegistrationTest { OIDCClientRepresentation client = createRep(); // Failed to create client - try { - reg.oidc().create(client); - Assert.fail("Not expected to successfuly register client"); - } catch (ClientRegistrationException expected) { - HttpErrorException httpEx = (HttpErrorException) expected.getCause(); - Assert.assertEquals(401, httpEx.getStatusLine().getStatusCode()); - } + assertCreateFail(client, 401); // Create trusted host entry - Response response = adminClient.realm(REALM_NAME).clientRegistrationTrustedHost().create(ClientRegistrationTrustedHostRepresentation.create("localhost", 2, 2)); - Assert.assertEquals(201, response.getStatus()); + createTrustedHost("localhost", 2); // Successfully register client reg.oidc().create(client); @@ -132,13 +142,20 @@ public class OIDCClientRegistrationTest extends AbstractClientRegistrationTest { reg.oidc().create(client); // Failed to create 3rd client - try { - reg.oidc().create(client); - Assert.fail("Not expected to successfuly register client"); - } catch (ClientRegistrationException expected) { - HttpErrorException httpEx = (HttpErrorException) expected.getCause(); - Assert.assertEquals(401, httpEx.getStatusLine().getStatusCode()); - } + assertCreateFail(client, 401); + } + + // KEYCLOAK-3421 + @Test + public void createClientWithUriFragment() { + reg.auth(null); + + createTrustedHost("localhost", 1); + + OIDCClientRepresentation client = createRep(); + client.setRedirectUris(Arrays.asList("http://localhost/auth", "http://localhost/auth#fragment", "http://localhost/auth*")); + + assertCreateFail(client, 400, "URI fragment"); } @Test @@ -323,4 +340,9 @@ public class OIDCClientRegistrationTest extends AbstractClientRegistrationTest { } } + private void createTrustedHost(String name, int count) { + Response response = adminClient.realm(REALM_NAME).clientRegistrationTrustedHost().create(ClientRegistrationTrustedHostRepresentation.create(name, count, count)); + Assert.assertEquals(201, response.getStatus()); + } + } diff --git a/themes/src/main/resources/theme/base/admin/messages/messages_en.properties b/themes/src/main/resources/theme/base/admin/messages/messages_en.properties index 95e16db33a..345cb2503c 100644 --- a/themes/src/main/resources/theme/base/admin/messages/messages_en.properties +++ b/themes/src/main/resources/theme/base/admin/messages/messages_en.properties @@ -12,3 +12,6 @@ ldapErrorMissingClientId=Client ID needs to be provided in config when Realm Rol ldapErrorCantPreserveGroupInheritanceWithUIDMembershipType=Not possible to preserve group inheritance and use UID membership type together. ldapErrorCantWriteOnlyForReadOnlyLdap=Can't set write only when LDAP provider mode is not WRITABLE ldapErrorCantWriteOnlyAndReadOnly=Can't set write-only and read-only together + +clientRedirectURIsFragmentError=Redirect URIs must not contain an URI fragment +clientRootURLFragmentError=Root URL must not contain an URL fragment \ No newline at end of file diff --git a/themes/src/main/resources/theme/base/admin/resources/js/controllers/clients.js b/themes/src/main/resources/theme/base/admin/resources/js/controllers/clients.js index 3f13573653..24efd88e68 100755 --- a/themes/src/main/resources/theme/base/admin/resources/js/controllers/clients.js +++ b/themes/src/main/resources/theme/base/admin/resources/js/controllers/clients.js @@ -1111,6 +1111,12 @@ module.controller('ClientDetailCtrl', function($scope, realm, client, templates, }, $scope.client, function() { $route.reload(); Notifications.success("Your changes have been saved to the client."); + }, function(error) { + if (error.status == 400 && error.data.error_description) { + Notifications.error(error.data.error_description); + } else { + Notifications.error('Unexpected error when updating client'); + } }); } }; @@ -1225,6 +1231,12 @@ module.controller('CreateClientCtrl', function($scope, realm, client, templates, var id = l.substring(l.lastIndexOf("/") + 1); $location.url("/realms/" + realm.realm + "/clients/" + id); Notifications.success("The client has been created."); + }, function(error) { + if (error.status == 400 && error.data.error_description) { + Notifications.error(error.data.error_description); + } else { + Notifications.error('Unexpected error when creating client'); + } }); };