8000 Querying MVP in Client v2 by Pepo48 · Pull Request #48342 · keycloak/keycloak · GitHub
[go: up one dir, main page]

Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion js/libs/keycloak-admin-client/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,8 @@ paths:
/admin/api/{realmName}/clients/v2:
get:
summary: Get all clients
description: Returns a list of all clients in the realm
description: "Returns a list of clients in the realm, optionally filtered by\
\ a query expression"
operationId: getClients
tags:
- Clients (v2)
Expand All @@ -240,6 +241,12 @@ paths:
uniqueItems: true
items:
type: string
- description: "Filter expression using SCIM-like syntax, e.g. clientId eq \"\
8000 my-app\" and enabled eq true"
name: q
in: query
schema:
type: string
responses:
"200":
description: OK
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,35 @@ public class ListOptions {
@Parameter(description = "Set of fields to include in the response. Must be top-level fields. If omitted or empty, all fields will be populated.")
@QueryParam("fields")
protected Set<String> fields;


@Parameter(description = "Filter expression using SCIM-like syntax, e.g. clientId eq \"my-app\" and enabled eq true")
@QueryParam("q")
protected String query;

public ListOptions fields(Set<String> fields) {
this.setFields(fields);
return this;
}

public ListOptions query(String query) {
this.setQuery(query);
return this;
}

public Set<String> getFields() {
return fields;
}

public void setFields(Set<String> fields) {
this.fields = fields;
}

public String getQuery() {
return query;
}

public void setQuery(String query) {
this.query = query;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,12 @@ public interface ClientsApi {
@GET
@Produces(MediaType.APPLICATION_JSON)
@Consumes(MediaType.APPLICATION_JSON)
8000 @Operation(summary = "Get all clients", description = "Returns a list of all clients in the realm")
@Operation(summary = "Get all clients", description = "Returns a list of clients in the realm, optionally filtered by a query expression")
@APIResponses(value = {
@APIResponse(responseCode = "200", content = @Content(schema = @Schema(type = SchemaType.ARRAY, implementation = BaseClientRepresentation.class)))
})
Stream<BaseClientRepresentation> getClients(@BeanParam ListOptions params);

default Stream<BaseClientRepresentation> getClients() {
return getClients(new ListOptions());
}
Expand Down
11 changes: 11 additions & 0 deletions rest/admin-v2/services/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,17 @@
<groupId>org.keycloak</groupId>
<artifactId>keycloak-admin-v2-api</artifactId>
</dependency>
<dependency>
<groupId>org.keycloak</groupId>
<artifactId>keycloak-scim-core</artifactId>
</dependency>

<!-- Test dependencies -->
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter</artifactId>
<scope>test</scope>
</dependency>
<!-- used by OAS Filter during OpenAPI spec generation -->
<dependency>
<groupId>io.smallrye</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,15 @@ public void toModel(BaseClientRepresentation rep, ClientModel existingModel) {
fields.values().forEach(m -> m.toModel(rep, existingModel));
}

@SuppressWarnings("unchecked")
public void applyProjection(BaseClientRepresentation rep, Set<String> includeFields) {
if (includeFields == null || includeFields.isEmpty()) return;
fields.entrySet().stream()
.filter(e -> !includeFields.contains(e.getKey()))
.filter(e -> e.getValue().repSetter != null)
.forEach(e -> e.getValue().repSetter.accept(rep, null));
}

protected abstract T createClientRepresentation();

}
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package org.keycloak.models.mapper;
import java.util.Map;
import java.util.Optional;
import java.util.Set;

import org.keycloak.representations.admin.v2.BaseClientRepresentation;
import org.keycloak.representations.admin.v2.OIDCClientRepresentation;
import org.keycloak.representations.admin.v2.SAMLClientRepresentation;

Expand All @@ -19,6 +21,26 @@ public boolean isKnownField(String name) {
return mappers.values().stream().anyMatch(f -> f.fields.containsKey(name));
}

public Object resolveFieldValue(String name, BaseClientRepresentation rep) {
String protocol = rep.getProtocol();
var mapper = protocol != null ? mappers.get(protocol) : null;
if (mapper != null) {
var field = mapper.fields.get(name);
if (field != null) {
return field.getValue(rep);
}
}
return null;
}

public void applyProjection(BaseClientRepresentation rep, Set<String> includeFields) {
String protocol = rep.getProtocol();
var mapper = protocol != null ? mappers.get(protocol) : null;
if (mapper != null) {
mapper.applyProjection(rep, includeFields);
}
}

public Optional<BaseClientModelMapper<?>> getMapper(String protocol) {
return Optional.ofNullable(mappers.get(protocol));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import jakarta.annotation.Nonnull;
import jakarta.validation.Valid;
import jakarta.ws.rs.BadRequestException;
import jakarta.ws.rs.ForbiddenException;
import jakarta.ws.rs.POST;
import jakarta.ws.rs.Path;
Expand All @@ -19,6 +20,7 @@
import org.keycloak.services.client.ClientService;
import org.keycloak.services.client.ClientService.ClientProjectionOptions;
import org.keycloak.services.client.DefaultClientService;
import org.keycloak.services.client.query.ClientQueryException;
import org.keycloak.services.resources.admin.fgap.AdminPermissionEvaluator;

public class DefaultClientsApi implements ClientsApi {
Expand All @@ -39,7 +41,12 @@ public DefaultClientsApi(@Nonnull KeycloakSession session,

@Override
public Stream<BaseClientRepresentation> getClients(ListOptions params) {
return clientService.getClients(realm, new ClientProjectionOptions(params.getFields()), null, null);
try {
var searchOptions = params.getQuery() != null ? new ClientService.ClientSearchOptions(params.getQuery()) : null;
return clientService.getClients(realm, new ClientProjectionOptions(params.getFields()), searchOptions, null);
} catch (ClientQueryException e) {
throw new BadRequestException(e.getMessage());
}
}

@POST
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@

public interface ClientService extends Service {

class ClientSearchOptions {
// TODO
}
record ClientSearchOptions(String query) {}

class ClientProjectionOptions {
private final LinkedHashSet<String> fields = new LinkedHashSet<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@
import org.keycloak.services.PatchType;
import org.keycloak.services.RolesService;
import org.keycloak.services.ServiceException;
import org.keycloak.services.client.query.ClientQueryEvaluator;
import org.keycloak.services.client.query.QueryParseUtils;
import org.keycloak.services.clientpolicy.ClientPolicyException;
import org.keycloak.services.clientpolicy.context.AdminClientRegisterContext;
import org.keycloak.services.clientpolicy.context.AdminClientRegisteredContext;
Expand Down Expand Up @@ -118,12 +120,12 @@ public Stream<BaseClientRepresentation> getClients(@Nonnull RealmModel realm,
ClientSearchOptions searchOptions,
ClientSortAndSliceOptions sortAndSliceOptions) {
permissions.clients().requireList();

// TODO: this check is weak
// a stronger check 10BC8 is whether the remaining fields have repSetters
// however this highlights an issue we may hit with polymorphism a field may
// be projectable in one subtype, but fixed in another

projectionOptions.getFields().forEach(s -> {
if (!MAPPERS.isKnownField(s)) {
throw new ServiceException("%s is an unknown field".formatted(s), Response.Status.BAD_REQUEST);
Expand All @@ -134,16 +136,36 @@ public Stream<BaseClientRepresentation> getClients(@Nonnull RealmModel realm,
// When disabled, we fall back to in-memory filtering by VIEW_CLIENTS role.
boolean canView = AdminPermissionsSchema.SCHEMA.isAdminPermissionsEnabled(realm) || permissions.clients().canView();
try {
return realm.getClientsStream()
Stream<BaseClientRepresentation> stream = realm.getClientsStream()
.filter(client -> canView || permissions.clients().canView(client))
.filter(client -> client.getProtocol() != null)
.map(client -> getMapper(client.getProtocol()).fromModel(client, projectionOptions.getFields()))
.map(client -> getMapper(client.getProtocol()).fromModel(client))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We are focused on the contract, not the implementation, so this is not something that needs addressed - but I wanted to mention that fully mapping, then preforming the projection on top of that will cause all representations to be fully materialized.

With the filtering pushed further down, either to the Model layer or all the way to JPA, the projection will work more as expected.

.filter(java.util.Objects::nonNull);

stream = applySearchFilter(stream, searchOptions);
return applyProjection(stream, projectionOptions);
} catch (ModelException e) {
throw new ServiceException(e.getMessage(), Response.Status.BAD_REQUEST);
}
}

protected Stream<BaseClientRepresentation> applySearchFilter(Stream<BaseClientRepresentation> stream, ClientSearchOptions searchOptions) {
if (searchOptions != null && searchOptions.query() != null && !searchOptions.query().isBlank()) {
var queryCtx = QueryParseUtils.parse(searchOptions.query());
QueryParseUtils.validate(queryCtx);
return stream.filter(client -> ClientQueryEvaluator.matches(queryCtx, client));
}
return stream;
}

protected Stream<BaseClientRepresentation> applyProjection(Stream<BaseClientRepresentation> stream, ClientProjectionOptions projectionOptions) {
if (projectionOptions.getFields().isEmpty()) return stream;
return stream.map(rep -> {
MAPPERS.applyProjection(rep, projectionOptions.getFields());
return rep;
});
}

@Override
public BaseClientRepresentation createClient(RealmModel realm, BaseClientRepresentation client) throws ServiceException {
return createOrUpdate(realm, null, client, CreateOrUpdateStrategy.ONLY_CREATE, false).representation();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
package org.keycloak.services.client.query;

import java.util.Collection;
import java.util.Objects;

import org.keycloak.representations.admin.v2.BaseClientRepresentation;
import org.keycloak.scim.filter.FilterUtils;
import org.keycloak.scim.filter.ScimFilterParser;
import org.keycloak.scim.filter.ScimFilterParserBaseVisitor;

public class ClientQueryEvaluator extends ScimFilterParserBaseVisitor<Boolean> {

private final BaseClientRepresentation client;

private ClientQueryEvaluator(BaseClientRepresentation client) {
this.client = client;
}

public static boolean matches(ScimFilterParser.FilterContext filter, BaseClientRepresentation client) {
return new ClientQueryEvaluator(client).visit(filter);
}

@Override
public Boolean visitFilter(ScimFilterParser.FilterContext ctx) {
return visit(ctx.expression());
}

@Override
public Boolean visitExpression(ScimFilterParser.ExpressionContext ctx) {
if (ctx.OR() != null) {
return visit(ctx.expression()) || visit(ctx.andExpression());
}
return visit(ctx.andExpression());
}

@Override
public Boolean visitAndExpression(ScimFilterParser.AndExpressionContext ctx) {
if (ctx.AND() != null) {
return visit(ctx.andExpression()) && visit(ctx.notExpression());
}
return visit(ctx.notExpression());
}

@Override
public Boolean visitNotExpression(ScimFilterParser.NotExpressionContext ctx) {
if (ctx.NOT() != null) {
return !visit(ctx.notExpression());
}
return visit(ctx.atom());
}

@Override
public Boolean visitAtom(ScimFilterParser.AtomContext ctx) {
if (ctx.attributeExpression() != null) {
return visit(ctx.attributeExpression());
}
return visit(ctx.expression());
}

@Override
public Boolean visitPresentExpression(ScimFilterParser.PresentExpressionContext ctx) {
String fieldPath = ctx.ATTRPATH().getText();
return FieldResolver.resolve(fieldPath, client) != null;
}

@Override
public Boolean visitComparisonExpression(ScimFilterParser.ComparisonExpressionContext ctx) {
String fieldPath = ctx.ATTRPATH().getText();
Object fieldValue = FieldResolver.resolve(fieldPath, client);
String operator = ctx.compareOp().getText().toLowerCase();
String queryValue = FilterUtils.extractCompValue(ctx.compValue());
@michalvavrik michalvavrik May 25, 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.

queryValue can be null, so when users does "contains", "startsWith" or "endsWith" null, it will result in NPE In matchScalar, because java.lang.String#contains, java.lang.String#startsWith(java.lang.String) and java.lang.String#endsWith require non null parameter. I makes no sense to start/end/contain null, but I can imagine that some programmatically generated expression can do description sw null by mistake. Maybe you can guard against NPE and throw something like IllegalArgumentException?

@Pepo48 Pepo48 May 25, 2026 •
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Turns out to be deeper - the NPE exists in ScimJPAPredicateProvider too, so I fixed it at the source. FilterUtils.parseFilter() now rejects null for operators other than eq/ne ("contains null" or "starts with null" is imo meaningless, pr is the right way to test presence).

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.

agreed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you double check with the spec on whether it uses three valued boolean logic like SQL?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For filtered attributes that are not part of a particular resource type, the service provider SHALL treat the attribute as if there is no attribute value.  
For example, a presence or equality filter for an undefined attribute evaluates to false.

Which I read as no "unknown" state compared to SQL, although the "For example" claim only covers pr and eq. Spec does not specify how ne behaves on absent attributes (at least I don't see it).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right, it's not very specific.

For filtered attributes that are not part of a particular resource type

From the current implementation this is interpretted as an attribute that does not exist for the given schemas - ScimJPAPredicateProvider.createPredicate

As far as I can see from our current SCIM implementation, we're handing predicates (potentially with explicit nulls) directly mapped to JPA. So my expectation is that three valued boolean logic will be used underneath on attributes that are defined for the schema, but may be null.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, just for the record, we have two distinct cases:

  1. undefined attribute - resolve() returns null -> cb.disjunction(), always false per RFC 7644.

  2. defined but null-valued - predicates go straight to SQL. Both ne "x" and not (eq "x") hit three-valued logic on a NULL column and exclude the row.

This MVP in-memory evaluator returns true in case 2 (a missing value isn't equal to anything). This is another task for JPA filtering follow-up (#48766), as we may need explicit IS NULL handling if we want to preserve that.


return switch (operator) {
case "eq" -> fieldValue == null ? queryValue == null : matchScalar(fieldValue, queryValue, Objects::equals);
case "ne" -> fieldValue == null ? queryValue != null : !matchScalar(fieldValue, queryValue, Objects::equals);
case "co" -> fieldValue != null && matchScalar(fieldValue, queryValue, (fv, qv) -> fv != null && fv.contains(qv));
case "sw" -> fieldValue != null && matchScalar(fieldValue, queryValue, (fv, qv) -> fv != null && fv.startsWith(qv));
case "ew" -> fieldValue != null && matchScalar(fieldValue, queryValue, (fv, qv) -> fv != null && fv.endsWith(qv));
default -> false;
};
}

private boolean matchScalar(Object fieldValue, String queryValue,
java.util.function.BiPredicate<String, String> predicate) {
if (fieldValue instanceof Collection<?> collection) {
return collection.stream()
.anyMatch(item -> predicate.test(item.toString(), queryValue));
}
return predicate.test(fieldValue.toString(), queryValue);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package org.keycloak.services.client.query;

public class ClientQueryException extends RuntimeException {

public ClientQueryException(String message) {
super(message);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package org.keycloak.services.client.query;

import org.keycloak.models.mapper.ClientModelMappers;
import org.keycloak.representations.admin.v2.BaseClientRepresentation;
import org.keycloak.representations.admin.v2.OIDCClientRepresentation;

public class FieldResolver {

private static final ClientModelMappers MAPPERS = new ClientModelMappers();

public static boolean isKnownField(String fieldPath) {
if ("auth.method".equals(fieldPath)) {
return true;
}
return MAPPERS.isKnownField(fieldPath);
}

public static Object resolve(String fieldPath, BaseClientRepresentation client) {
if ("auth.method".equals(fieldPath)) {
if (client instanceof OIDCClientRepresentation oidc && oidc.getAuth() != null) {
return oidc.getAuth().getMethod();
}
return null;
}
return MAPPERS.resolveFieldValue(fieldPath, client);
}
}
Loading
Loading
0