scim prototype - #50324
Conversation
b8276b4 to
726eee1
Compare
| import org.keycloak.scim.filter.ScimFilterParser; | ||
| import org.keycloak.scim.filter.ScimFilterParserBaseVisitor; | ||
|
|
||
| public final class QueryFieldExtractor extends ScimFilterParserBaseVisitor<Void> { |
There was a problem hiding this comment.
Couldn't we re-use the existing ClientQueryEvaluator?
There was a problem hiding this comment.
No, QueryFieldExtractor returns Set<String> of attribute names and ClientQueryEvaluator boolean. Also ClientQueryEvaluator needs a client. If we would to merge them it would only save on the methods, the implementation is totally different
| ClientProjectionOptions projectionOptions, | ||
| ClientSearchOptions searchOptions, | ||
| ClientSortAndSliceOptions sortAndSliceOptions) { | ||
| if (!canUseJpaQuery(realm, searchOptions)) { |
There was a problem hiding this comment.
Eventually we need to prohibit queries that are not JPA-backed.
shawkins
left a comment
There was a problem hiding this comment.
This is a step in the right direction. We ideally should eventually be sharing as much logic as possible with scim. Creating our own AbstractScimResourceTypeProvider implementations would be one possible vision - but without additional refactoring that would require our representations to become ResourceTypeRepresentations, or to introduce an intermediate ResourceTypeRepresentation that we map to our representation classes.
But we can at least borrow from their design as needed to encapsulate the basic type / version logic behind something like type provider so that all of the boilerplate service logic to get, list, patch, update will be the same as additional types are added.
There was a problem hiding this comment.
Pull request overview
Adds an opt-in SCIM-backed JPA query prototype for Admin Client API v2, with fallback to the existing service.
Changes:
- Adds SCIM schema, filter extraction, and JPA query execution.
- Selects the SCIM-backed service through a disabled-by-default property.
- Adds integration coverage and SCIM model dependency.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
ScimBackedClientQueryTest.java |
Tests SCIM-backed filtering, pagination, fallback, and projection. |
ServiceExceptionMapper.java |
Adds local exception debug logging. |
ScimBackedClientService.java |
Implements SCIM-backed listing with default-service delegation. |
ClientQueryRepresentation.java |
Defines the query metadata resource type. |
ClientJpaQuerySchema.java |
Maps API fields to JPA attributes. |
ClientJpaQueryProvider.java |
Adapts query metadata to the SCIM evaluator. |
ClientJpaQueryExecutor.java |
Builds and executes filtered JPA queries. |
QueryFieldExtractor.java |
Extracts referenced fields from SCIM filters. |
ClientServiceFactory.java |
Selects the service using an opt-in property. |
ClientService.java |
Exposes parsed sort options. |
DefaultClientsApi.java |
Uses the client-service factory for collections. |
DefaultClientApi.java |
Uses the client-service factory for individual clients. |
pom.xml |
Adds the SCIM model dependency. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
rest/admin-v2/services/src/main/java/org/keycloak/services/client/ScimBackedClientService.java:90
- The JPA evaluator rejects field/operator combinations that the Admin v2 validator and current evaluator accept, such as
enabled co "ru", by throwingScimFilterException. Because that exception is not handled, enabling this backend turns a valid query into a 500 response; fall back to the delegate when the JPA evaluator cannot represent a validated query.
} catch (ClientQueryException e) {
throw new ServiceException(e.getMessage(), jakarta.ws.rs.core.Response.Status.BAD_REQUEST);
} catch (ModelException e) {
throw new ServiceException(e.getMessage(), jakarta.ws.rs.core.Response.Status.BAD_REQUEST);
}
rest/admin-v2/services/src/main/java/org/keycloak/services/client/scim/ClientJpaQueryExecutor.java:59
- This query is not paginated at the database layer:
getResultStream()is unbounded, whileskip/limitis applied later in Java after lazy-loading each skipped client's protocol. Large offsets therefore scan and load every preceding match; pass offset/limit into this executor and applysetFirstResult/setMaxResultsto the JPA query.
return closing(em.createQuery(q).getResultStream());
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
rest/admin-v2/services/src/main/java/org/keycloak/services/client/scim/ClientJpaQueryExecutor.java:52
- Null literals lose the existing Admin API semantics here: the SCIM predicate provider turns
eq/neintoCriteriaBuilder.equal/notEqual, so SQL null logic does not match nullable fields. This breaks cases explicitly covered byClientQueryEvaluatorTest:243-257(and documented indocs/guides/admin-api/querying.adoc:119); handle null withisNull/isNotNull, including undernot, or fall back to the delegate for such filters.
predicates.add(evaluator.visit(filterContext).predicate());
rest/admin-v2/services/src/main/java/org/keycloak/services/client/scim/ClientJpaQueryExecutor.java:59
- This raw
ORDER BYdoes not preserve the API's nulls-last contract. For example, PostgreSQL places null display names first for descending order, whileClientFieldusesComparator.nullsLastandClientSortAndSliceOptionsTest:74-81asserts that behavior; add an explicit null-rank order before each value order so SCIM-backed pagination remains compatible.
q.orderBy(sortOptions.stream().map(sortOption -> {
var field = ClientJpaQuerySchema.INSTANCE.getAttributeByPath(sortOption.field().toQueryValue())
.getModelAttributeName();
return sortOption.isAscending() ? cb.asc(root.get(field)) : cb.desc(root.get(field));
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
rest/admin-v2/services/src/main/java/org/keycloak/services/client/scim/ClientJpaQueryExecutor.java:64
Stream.toList()returns an unmodifiable list, so an explicit sort that omitsclientId(for example,sort=displayName) reaches thisaddand throwsUnsupportedOperationException. Buildordersas a mutable list before appending the tie-breaker.
orders.add(cb.asc(root.get("clientId")));
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRegisterClickKeycloak CI - Forms IT (firefox) |
| } | ||
|
|
||
| @Override | ||
| public List<ModelSchema<Model, ClientQueryRepresentation>> getSchemas() { |
There was a problem hiding this comment.
This is not Erik's fault, but the way generics are handled in this API is terrible, I am not sure how this could be designed in a way that you are supposed to create <M extends Model> without knowing what M class is is bad. There are unchecked overriding all over the place, I checked other implementations and the only one that actually returns something has the same problem.
There was a problem hiding this comment.
right, to make this better we should refactor this, but would be good to do that in a separate issue / PR
|
@shawkins Can you please give this a final look? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
rest/admin-v2/services/src/main/java/org/keycloak/services/client/scim/ClientJpaQueryExecutor.java:62
- The existing API comparator defines string ordering as case-insensitive and always places nulls last (
ClientField.java:65-77), but these rawasc/descexpressions inherit database collation and null ordering. For example, PostgreSQL places nulldisplayNamevalues first for descending order, so applying pagination here changes which clients are returned when the SCIM service is enabled; construct portable order expressions that preserve the API comparator's case and null semantics.
return sortOption.isAscending() ? cb.asc(root.get(field)) : cb.desc(root.get(field));
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRequiredActionKeycloak CI - Forms IT (chrome) |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRequiredActionKeycloak CI - Forms IT (chrome) |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRequiredActionKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRegisterClickKeycloak CI - Forms IT (chrome) |
vmuzikar
left a comment
There was a problem hiding this comment.
LGTM. The test failures seem unrelated.
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| @KeycloakIntegrationTest(config = ScimBackedClientQueryTest.Config.class) | ||
| public class ScimBackedClientQueryTest extends AbstractClientApiV2Test { |
There was a problem hiding this comment.
One thing to note. We should later run the full Client v2 testsuite again with both SCIM service layer, and the original one. Of course the SCIM service layer is not feature complete, we'd need to exclude some tests.
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRequiredActionKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRegisterClickKeycloak CI - Forms IT (chrome) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
rest/admin-v2/services/src/main/java/org/keycloak/services/client/scim/ClientJpaQueryExecutor.java:62
- The JPA ordering does not preserve the API's sort contract:
ClientField.java:65-77defines case-insensitive string ordering with nulls last, while rawasc/descdelegates both behaviors to the database (for example, PostgreSQL puts nulls first for descending order). Because pagination is applied immediately afterward, enabling this service can return different clients on each page; add explicit null ranking and case-normalized ordering for string fields before paginating.
var orders = new ArrayList<>(sortOptions.stream().map(sortOption -> {
var field = ClientJpaQuerySchema.INSTANCE.getAttributeByPath(sortOption.field().toQueryValue())
.getModelAttributeName();
return sortOption.isAscending() ? cb.asc(root.get(field)) : cb.desc(root.get(field));
reference: #50271
Signed-off-by: Erik Jan de Wit erikjan.dewit@gmail.com