8000 Create more validation tests for PUT/PATCH logic by mabartos · Pull Request #47282 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Create more validation tests for PUT/PATCH logic #47282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vmuzikar merged 5 commits into from
Mar 27, 2026
Merged

Create more validation tests for PUT/PATCH logic #47282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6111030
Create more validation tests for PUT/PATCH logic
mabartos Mar 18, 2026
c55eda0
Test for trying to change the protocol for existing client
mabartos Mar 26, 2026
607c398
PATCH tests do not extend PUT tests
mabartos Mar 26, 2026
8cdda53
Introduce the ProtocolUnmofidiedValidator
mabartos Mar 26, 2026
0adfb2d
Accomodate changes for the protocol unmodified validator
mabartos Mar 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ...-v2/api/src/main/java/org/keycloak/representations/admin/v2/BaseClientRepresentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.keycloak.representations.admin.v2.validation.ClientUuidProvider;
import org.keycloak.representations.admin.v2.validation.CreateClient;
import org.keycloak.representations.admin.v2.validation.PatchClient;
import org.keycloak.representations.admin.v2.validation.ProtocolUnmodified;
import org.keycloak.representations.admin.v2.validation.PutClient;
import org.keycloak.representations.admin.v2.validation.UuidUnmodified;

Expand All @@ -30,6 +31,7 @@
@JsonSubTypes.Type(value = SAMLClientRepresentation.class, name = SAMLClientRepresentation.PROTOCOL)
})
@UuidUnmodified(uuidProvider = ClientUuidProvider.class, groups = {PutClient.class, PatchClient.class})
@ProtocolUnmodified(groups = {PutClient.class, PatchClient.class})
public abstract class BaseClientRepresentation extends BaseRepresentation implements RepresentationWithUuid {
public static final String DISCRIMINATOR_FIELD = "protocol";

Expand Down
20 changes: 20 additions & 0 deletions ...pi/src/main/java/org/keycloak/representations/admin/v2/validation/ProtocolUnmodified.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package org.keycloak.representations.admin.v2.validation;

import java.lang.annotation.Documented;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import jakarta.validation.Constraint;
import jakarta.validation.Payload;

@Target({ElementType.TYPE})
@Retention(RetentionPolicy.RUNTIME)
@Constraint(validatedBy = {ProtocolUnmodifiedValidator.class})
@Documented
public @interface ProtocolUnmodified {
String message() default "protocol cannot be changed for an existing client";
Class<?>[] groups() default {};
Class<? extends Payload>[] payload() default {};
}
32 changes: 32 additions & 0 deletions ...in/java/org/keycloak/representations/admin/v2/validation/ProtocolUnmodifiedValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package org.keycloak.representations.admin.v2.validation;

import java.util.Optional;

import jakarta.validation.ConstraintValidator;
import jakarta.validation.ConstraintValidatorContext;

import org.keycloak.models.ClientModel;
import org.keycloak.representations.admin.v2.BaseClientRepresentation;
import org.keycloak.validation.jakarta.ValidationContext;

public class ProtocolUnmodifiedValidator implements ConstraintValidator<ProtocolUnmodified, BaseClientRepresentation> {

@Override
public boolean isValid(BaseClientRepresentation representation, ConstraintValidatorContext context) {
ValidationContext validationContext = ValidationContext.unwrap(context);

String persistedProtocol = Optional.ofNullable(validationContext.realm().getClientByClientId(representation.getClientId()))
.map(ClientModel::getProtocol)
.orElse(null);

if (persistedProtocol == null || persistedProtocol.equals(representation.getProtocol())) {
return true;
}

context.disableDefaultConstraintViolation();
context.buildConstraintViolationWithTemplate(context.getDefaultConstraintMessageTemplate())
.addPropertyNode("protocol")
.addConstraintViolation();
return false;
}
}
16 changes: 13 additions & 3 deletions rest/admin-v2/api/src/main/java/org/keycloak/services/client/DefaultClientService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import org.keycloak.validation.jakarta.ValidationContext;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.ObjectReader;
Expand Down Expand Up @@ -110,9 +111,6 @@ public BaseClientRepresentation createClient(RealmModel realm, BaseClientReprese

@Override
public CreateOrUpdateResult createOrUpdateClient(RealmModel realm, String clientId, BaseClientRepresentation client) throws ServiceException {
if (!Objects.equals(clientId, client.getClientId())) {
throw new ServiceException("Field 'clientId' in payload does not match the provided 'clientId'", Response.Status.BAD_REQUEST);
}
return createOrUpdate(realm, clientId, client, CreateOrUpdateStrategy.PUT);
}

Expand All @@ -134,6 +132,9 @@ public Class<?> getValidationGroup() {

private CreateOrUpdateResult createOrUpdate(RealmModel realm, String clientId, BaseClientRepresentation client, CreateOrUpdateStrategy strategy) throws ServiceException {
validateUnknownFields(client);
if (!strategy.equals(CreateOrUpdateStrategy.ONLY_CREATE)) {
assertSameClientIds(clientId, client.getClientId());
}

boolean created = false;
ClientModel model;
Expand Down Expand Up @@ -234,6 +235,9 @@ public BaseClientRepresentation patchClient(RealmModel realm, String clientId, P
}
final ObjectReader objectReader = MAPPER.readerForUpdating(getOriginalClient.get());
updated = objectReader.readValue(patch);
} catch (JsonMappingException e) {
var invalidFields = e.getPath().stream().map(JsonMappingException.Reference::getFieldName).collect(Collectors.joining(", "));
throw new ServiceException("Invalid values for these fields: %s".formatted((invalidFields)));
} catch (JsonProcessingException e) {
throw new ServiceException(e.getMessage(), Response.Status.BAD_REQUEST);
} catch (IOException e) {
Expand Down Expand Up @@ -263,6 +267,12 @@ public void deleteClient(RealmModel realm, String clientId) throws ServiceExcept
fireAdminEvent(OperationType.DELETE, client);
}

protected void assertSameClientIds(String pathId, String payloadId) {
if (!Objects.equals(pathId, payloadId)) {
@michalvavrik michalvavrik Mar 26, 2026 •
edited
Loading
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone left the client id out of the patch, here payloadId would be "null" string. Maybe you could improve it to say something like "clientId cannot be removed via PATCH"?

@mabartos mabartos Mar 26, 2026 •
edited
Loading
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also related to the required clientId? #47282 (comment)

As that might be done in a follow-up.

@michalvavrik michalvavrik Mar 26, 2026
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@mabartos mabartos Mar 27, 2026
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up:

throw new ServiceException("Field 'clientId' in payload does not match the provided 'clientId'", Response.Status.BAD_REQUEST);
}
}

/**
* Declaratively manage client roles - ensures the client has exactly the roles specified in 'rolesFromRep'
* <p>
Expand Down
10000
11 changes: 10 additions & 1 deletion ...in-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/AbstractClientApiV2Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import org.keycloak.admin.client.Keycloak;
import org.keycloak.services.client.ClientServiceHelper;
import org.keycloak.testframework.annotations.InjectAdminClient;
import org.keycloak.testframework.remote.runonserver.InjectRunOnServer;
import org.keycloak.testframework.remote.runonserver.RunOnServerClient;

Expand All @@ -16,6 +17,9 @@
public abstract class AbstractClientApiV2Test {
protected static ObjectMapper mapper;

@InjectAdminClient
protected Keycloak adminClient;

public String getRealmName() {
return "master";
}
Expand Down Expand Up @@ -48,8 +52,13 @@ public void setupNewClientService() {
}
// END

protected void setAuthHeader(HttpMessage request) {
String token = adminClient.tokenManager().getAccessTokenString();
request.setHeader(HttpHeaders.AUTHORIZATION, "Bearer " + token);
}

// TODO Rewrite the tests to not need explicit auth. They should use the admin client directly.
static void setAuthHeader(HttpMessage request, Keycloak adminClient) {
protected static void setAuthHeader(HttpMessage request, Keycloak adminClient) {
String token = adminClient.tokenManager().getAccessTokenString();
request.setHeader(HttpHeaders.AUTHORIZATION, "Bearer " + token);
}
Expand Down
11 changes: 0 additions & 11 deletions rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/AdminEventV2Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import jakarta.ws.rs.core.HttpHeaders;
import jakarta.ws.rs.core.MediaType;

import org.keycloak.admin.client.Keycloak;
import org.keycloak.authentication.authenticators.client.ClientIdAndSecretAuthenticator;
import org.keycloak.common.Profile;
import org.keycloak.events.admin.OperationType;
Expand All @@ -31,7 +30,6 @@
import org.keycloak.representations.idm.AdminEventRepresentation;
import org.keycloak.representations.idm.RealmEventsConfigRepresentation;
import org.keycloak.services.PatchTypeNames;
import org.keycloak.testframework.annotations.InjectAdminClient;
import org.keycloak.testframework.annotations.InjectHttpClient;
import org.keycloak.testframework.annotations.InjectRealm;
import org.keycloak.testframework.annotations.KeycloakIntegrationTest;
Expand All @@ -41,7 +39,6 @@
import org.keycloak.testframework.server.KeycloakServerConfig;
import org.keycloak.testframework.server.KeycloakServerConfigBuilder;

import org.apache.http.HttpMessage;
import org.apache.http.client.methods.HttpDelete;
import org.apache.http.client.methods.HttpPatch;
import org.apache.http.client.methods.HttpPost;
Expand Down Expand Up @@ -77,9 +74,6 @@ public class AdminEventV2Test extends AbstractClientApiV2Test {
@InjectHttpClient
CloseableHttpClient client;

@InjectAdminClient
Keycloak adminClient;

@InjectRealm(attachTo = "master", ref = "master")
ManagedRealm masterRealm;

Expand Down Expand Up @@ -381,11 +375,6 @@ private OIDCClientRepresentation getTestClientRepresentation() {
return rep;
}

private void setAuthHeader(HttpMessage request) {
String token = adminClient.tokenManager().getAccessTokenString();
request.setHeader(HttpHeaders.AUTHORIZATION, "Bearer " + token);
}

public static class AdminV2EventConfig implements KeycloakServerConfig {
@Override
public KeycloakServerConfigBuilder configure(KeycloakServerConfigBuilder config) {
Expand Down
8 changes: 0 additions & 8 deletions rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
import org.keycloak.testframework.server.KeycloakServerConfigBuilder;

import com.fasterxml.jackson.core.JsonProcessingException;
import org.apache.http.HttpMessage;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.methods.HttpOptions;
import org.apache.http.client.methods.HttpPatch;
Expand Down Expand Up @@ -85,9 +84,6 @@ public class ClientApiV2Test extends AbstractClientApiV2Test{
@InjectHttpClient
CloseableHttpClient client;

@InjectAdminClient
Keycloak adminClient;

@InjectRealm(config = NoAccessRealmConfig.class)
ManagedRealm testRealm;

Expand Down Expand Up @@ -988,10 +984,6 @@ private OIDCClientRepresentation getTestingFullClientRep() {
return rep;
}

private void setAuthHeader(HttpMessage request) {
setAuthHeader(request, this.adminClient);
}

public static class AdminV2Config implements KeycloakServerConfig {
@Override
public KeycloakServerConfigBuilder configure(KeycloakServerConfigBuilder config) {
Expand Down
Loading
Loading
0