Skip to content

scim prototype - #50324

Merged
edewit merged 5 commits into
keycloak:mainfrom
edewit:SCIM-prototype
Jul 27, 2026
Merged

scim prototype#50324
edewit merged 5 commits into
keycloak:mainfrom
edewit:SCIM-prototype

Conversation

@edewit

@edewit edewit commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

reference: #50271
Signed-off-by: Erik Jan de Wit erikjan.dewit@gmail.com

@vmuzikar vmuzikar left a comment

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.

This might be a good start but we'll need to go further with re-using the SCIM implementation. But I realize that this might need some changes and decoupling on the SCIM layer too, and that will be covered in #47451.

@shawkins Can you give this a look too?

import org.keycloak.scim.filter.ScimFilterParser;
import org.keycloak.scim.filter.ScimFilterParserBaseVisitor;

public final class QueryFieldExtractor extends ScimFilterParserBaseVisitor<Void> {

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.

Couldn't we re-use the existing ClientQueryEvaluator?

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.

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)) {

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.

Eventually we need to prohibit queries that are not JPA-backed.

@shawkins shawkins left a comment

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.

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.

@edewit
edewit marked this pull request as ready for review July 22, 2026 11:53
Copilot AI review requested due to automatic review settings July 22, 2026 11:53
@edewit
edewit requested review from a team as code owners July 22, 2026 11:53

Copilot AI left a comment

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.

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.

Copilot AI review requested due to automatic review settings July 22, 2026 12:28

Copilot AI left a comment

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.

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 throwing ScimFilterException. 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, while skip/limit is 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 apply setFirstResult/setMaxResults to the JPA query.
        return closing(em.createQuery(q).getResultStream());

Copilot AI review requested due to automatic review settings July 22, 2026 12:43

Copilot AI left a comment

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.

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/ne into CriteriaBuilder.equal/notEqual, so SQL null logic does not match nullable fields. This breaks cases explicitly covered by ClientQueryEvaluatorTest:243-257 (and documented in docs/guides/admin-api/querying.adoc:119); handle null with isNull/isNotNull, including under not, 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 BY does not preserve the API's nulls-last contract. For example, PostgreSQL places null display names first for descending order, while ClientField uses Comparator.nullsLast and ClientSortAndSliceOptionsTest:74-81 asserts 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));

Copilot AI review requested due to automatic review settings July 22, 2026 12:52

Copilot AI left a comment

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.

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 omits clientId (for example, sort=displayName) reaches this add and throws UnsupportedOperationException. Build orders as a mutable list before appending the tie-breaker.
            orders.add(cb.asc(root.get("clientId")));

Copilot AI review requested due to automatic review settings July 22, 2026 13:02
@keycloak-github-bot

Copy link
Copy Markdown

Unreported flaky test detected

If 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#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRegisterClick

Keycloak CI - Forms IT (firefox)

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

@michalvavrik michalvavrik left a comment

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.

LGTM, thanks

}

@Override
public List<ModelSchema<Model, ClientQueryRepresentation>> getSchemas() {

@michalvavrik michalvavrik Jul 27, 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.

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.

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, to make this better we should refactor this, but would be good to do that in a separate issue / PR

@vmuzikar

Copy link
Copy Markdown
Contributor

@shawkins Can you please give this a final look?

Copilot AI review requested due to automatic review settings July 27, 2026 10:35

Copilot AI left a comment

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.

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 raw asc/desc expressions inherit database collation and null ordering. For example, PostgreSQL places null displayName values 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));

@shawkins shawkins left a comment

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.

LGTM, thanks @edewit

@edewit
edewit enabled auto-merge (squash) July 27, 2026 13:27
@keycloak-github-bot

Copy link
Copy Markdown

Unreported flaky test detected

If 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#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRequiredAction

Keycloak CI - Forms IT (chrome)

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

@keycloak-github-bot keycloak-github-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@keycloak-github-bot keycloak-github-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@keycloak-github-bot

Copy link
Copy Markdown

Unreported flaky test detected

If 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#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRequiredAction

Keycloak CI - Forms IT (chrome)

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

@keycloak-github-bot keycloak-github-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@keycloak-github-bot

Copy link
Copy Markdown

Unreported flaky test detected

If 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#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRequiredAction

Keycloak CI - Forms IT (chrome)

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRegisterClick

Keycloak CI - Forms IT (chrome)

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

@vmuzikar vmuzikar left a comment

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.

LGTM. The test failures seem unrelated.

import static org.junit.jupiter.api.Assertions.assertTrue;

@KeycloakIntegrationTest(config = ScimBackedClientQueryTest.Config.class)
public class ScimBackedClientQueryTest extends AbstractClientApiV2Test {

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.

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.

edewit added 5 commits July 27, 2026 17:59
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>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Copilot AI review requested due to automatic review settings July 27, 2026 15:59
@keycloak-github-bot

Copy link
Copy Markdown

Unreported flaky test detected

If 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#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRequiredAction

Keycloak CI - Forms IT (chrome)

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRegisterClick

Keycloak CI - Forms IT (chrome)

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

@keycloak-github-bot keycloak-github-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

Copilot AI left a comment

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.

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-77 defines case-insensitive string ordering with nulls last, while raw asc/desc delegates 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));

@edewit
edewit merged commit cd0b33e into keycloak:main Jul 27, 2026
95 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants