refactor: remove clientdefaultservice / move logic to the type provider by shawkins · Pull Request #52304 · keycloak/keycloak

shawkins · 2026-09-02T02:11:41Z

This is unfortunately larger than I would have liked.

The main goal was to remove DefaultClientService and use the type provider as much as possible.

A good bit of the sort logic was removed from the api layer as we generally won't expect client logic to include the services module, so we just want it to be simple there.

There are now some additional differences between what we are doing in v2 and scim:

SearchOptions was added to account for the difference between v2 and scim sorting. This was just a quick copy-paste, but the general idea is that we'll want what the framework utilizes to be free from the respective rest layers.
- I know I pushed for supporting multiple sort fields, but there is already a compromise that was put in the code that automatically adds a sort on clientId (also marked with a TODO in the code). Perhaps that is good enough to ensure sorting is unique for additional pagination support later. If we want to minimize the initial difference with scim, then we initially should just enforce the sort list can only be a single field - and expand it later only if needed.
the base hasPermission logic was changed so that we can continue to use our workaround for all signatures until Reconcile authorization differences with SCIM logic #52099 is addressed. Doing more consolidation / encapsulation around AdminPermissionEvaluator would be good, and will likely be handled as part of Reconcile authorization differences with SCIM logic #52099
v2 does not use the resource provider populate(model, representation) method due to the complexity we have in create/update logic.

Other changes:

timestamp handling no longer enforces that the values must match. We're differentiating that the timestamps are read-only and simply ignored in put / patch / post (this is similar to scim handling) - otherwise the updated timestamp acts as a defacto resource version. A flush was also added to the server side logic to provide the most up-to-date values, but we may eventually want to revisit this.
minor changes were made to the patch error handling now that we have more consolidated logic.
the PUT behavior with the secret was not idempotent. I'd vote to start off with the changes here that make the secret required on a PUT, rather than allowing for missing with special handling depending on whether the resource exists or not. We can always change it later without it being breaking.
Error handling is called out in Further refactorings to allign v2 and scim logic #51906
The rest layer still contains a good bit of client central logic. There are other issues for making this look more like a generic framework that will cover making this logic more generic while also accounting for type specific custom endpoints.

closes: #51326

Actually this could be associated instead with closing #50273

Copilot

🟡 Changes recommended

Update validation and authorization regressions, plus mutation of the shared JSON mapper, must be corrected.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Moves Admin Client API v2 logic from DefaultClientService into the SCIM resource type provider.

Changes:

Consolidates client CRUD, filtering, sorting, permissions, and validation.
Introduces shared SCIM search and sort options.
Updates API tests, validation expectations, and OpenAPI metadata.

File summaries

File	Description
`server-spi-private/.../AdminPermissionsSchema.java`	Makes `QUERY` constant immutable.
`scim/services/.../ScimResourceTypeResource.java`	Converts SCIM requests into `SearchOptions`.
`scim/model/.../UserResourceTypeProvider.java`	Adopts `SearchOptions`.
`scim/model/.../SchemaResourceTypeProvider.java`	Adopts `SearchOptions`.
`scim/model/.../ResourceTypeProvider.java`	Adopts `SearchOptions`.
`scim/model/.../GroupResourceTypeProvider.java`	Adopts search and update SPI changes.
`scim/model/.../ServiceProviderConfigResourceTypeProvider.java`	Adopts `SearchOptions`.
`scim/core/.../SortField.java`	Adds SCIM sorting representation.
`scim/core/.../SingletonResourceTypeProvider.java`	Adopts `SearchOptions`.
`scim/core/.../SearchOptions.java`	Adds shared search configuration.
`scim/core/.../ScimResourceTypeProvider.java`	Updates provider search contracts.
`scim/core/.../BaseResourceTypeProvider.java`	Refactors permissions and update population.
`scim/core/.../AbstractScimResourceTypeProvider.java`	Preserves SCIM update population behavior.
`rest/admin-v2/tests/.../PatchClientValidationTest.java`	Updates patch error expectations.
`rest/admin-v2/tests/.../AbstractClientValidationTest.java`	Updates validation messages.
`rest/admin-v2/tests/.../ClientApiV2Test.java`	Updates CRUD, sorting, timestamp, and secret tests.
`rest/admin-v2/tests/.../ClientApiV2ClientSecretRotationTest.java`	Updates secret rotation state handling.
`rest/admin-v2/tests/.../ClientApiV2AuthorizationTest.java`	Revises missing-client authorization expectations.
`rest/admin-v2/services/.../ClientSortAndSliceOptionsTest.java`	Removes obsolete service sorting tests.
`rest/admin-v2/services/.../DefaultClientsApiTest.java`	Tests replacement sort parsing.
`rest/admin-v2/services/.../OpenApiSortParameterTest.java`	Uses renamed client sort fields.
`rest/admin-v2/services/.../ServiceException.java`	Defaults unspecified errors to HTTP 500.
`rest/admin-v2/services/.../Service.java`	Removes obsolete service marker.
`rest/admin-v2/services/.../ClientResourceTypeProvider.java`	Absorbs client service and query logic.
`rest/admin-v2/services/.../ClientJpaQueryExecutor.java`	Removes standalone query executor.
`rest/admin-v2/services/.../DefaultClientService.java`	Removes superseded client service.
`rest/admin-v2/services/.../ClientSortField.java`	Defines sortable client fields.
`rest/admin-v2/services/.../ClientService.java`	Removes superseded service API.
`rest/admin-v2/services/.../ClientField.java`	Removes old sortable-field implementation.
`rest/admin-v2/services/.../DefaultAdminApi.java`	Simplifies client API construction.
`rest/admin-v2/services/.../DefaultClientsApi.java`	Delegates collection operations to the provider.
`rest/admin-v2/services/.../DefaultClientApi.java`	Delegates item operations and handles merge patches.
`rest/admin-v2/services/.../OASModelFilter.java`	Uses renamed sort metadata.
`rest/admin-v2/api/.../ListOptionsTest.java`	Tests string-based sort construction.
`rest/admin-v2/api/.../BaseClientRepresentation.java`	Changes timestamp update validation.
`rest/admin-v2/api/.../SortOrder.java`	Removes obsolete API sort type.
`rest/admin-v2/api/.../SortOption.java`	Removes typed sort options.
`rest/admin-v2/api/.../SortFieldResolver.java`	Removes obsolete resolver.
`rest/admin-v2/api/.../SortField.java`	Removes obsolete API sort interface.
`rest/admin-v2/api/.../ListOptions.java`	Simplifies sorting to a raw expression.
`js/libs/keycloak-admin-client/openapi.json`	Updates timestamp descriptions.

Review details

Files reviewed: 41/41 changed files
Comments generated: 4
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🔵 Needs a closer look

PUT and PATCH now incorrectly require VIEW in addition to MANAGE for existing clients.

Review details

Suppressed comments (2)

rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientApi.java:111

This lookup requires VIEW before the explicit MANAGE check below, which rejects valid manage-only FGAP grants. Load the model without a view check, derive the representation for merging, and retain the existing manage check before parsing the patch.

            var current = typeProvider.get(clientId);

rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientApi.java:85

get enforces the independent VIEW scope before update enforces MANAGE, so an FGAP principal granted only MANAGE can no longer PUT an existing client. Use the model lookup only for the existence branch and let typeProvider.update perform the manage check.

            var current = typeProvider.get(clientId);

Files reviewed: 40/40 changed files
Comments generated: 0 new
Review effort level: Balanced

shawkins · 2026-09-02T14:50:54Z

PUT and PATCH now incorrectly require VIEW in addition to MANAGE for existing clients.

Relates to #47804 #46658 and #52151

Someone who has MANAGE logically should have VIEW permission as they are able to create the state in the first place.

Even without #52151 scim requires view for delete / patch / update.

Let's see if we can come to an agreement on how this should actually work.

cc @pedroigor

Copilot

🟡 Changes recommended

Authentication ordering exposes realm existence, and the changed PATCH timestamp behavior lacks replacement coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 40/40 changed files
Comments generated: 2
Review effort level: Balanced

Copilot

🟡 Changes recommended

The unknown-realm test targets a duplicated path, and the new PATCH timestamp behavior lacks regression coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

rest/admin-v2/api/src/main/java/org/keycloak/representations/admin/v2/BaseClientRepresentation.java:35

This removes timestamp validation for both PUT and PATCH, but the replacement coverage only verifies PUT; the prior PATCH assertions were deleted. Add a PATCH case with modified createdTimestamp/updatedTimestamp that verifies both values are ignored and the persisted server values remain unchanged.

        affectedFieldNames = {"uuid"},

Files reviewed: 40/40 changed files
Comments generated: 1
Review effort level: Balanced

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated 5 comments.

Suppressed comments (1)

scim/core/src/main/java/org/keycloak/scim/resource/spi/SearchOptions.java:1

SearchOptions is mutable (setters + non-final fields) and the builder returns the same mutable instance, which makes it easy for callers to accidentally modify options after passing them into providers. Consider making SearchOptions immutable (final fields, defensive copies, unmodifiable lists) and have the builder build a new instance, to keep provider behavior predictable and easier to reason about.

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2026-09-10T00:07:40Z

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.adapter.servlet.SAMLServletAdapterTest#testAdapterRoleMappings

Keycloak CI - Adapter IT Strict Cookies

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

Copilot

🟡 Changes recommended

PATCH incorrectly requires VIEW before MANAGE, blocking administrators with resource-specific MANAGE-only permission.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 41/41 changed files
Comments generated: 1
Review effort level: Balanced

shawkins · 2026-09-10T12:24:27Z

+            var current = typeProvider.get(clientId);
+
+            if (!typeProvider.hasPermission(current, AdminPermissionsSchema.MANAGE)) {


Already mentioned in the PR.

Copilot

🟡 Changes recommended

ClientUpdatedEvent is emitted before service-account mutations complete, exposing stale client state to listeners.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 41/41 changed files
Comments generated: 1
Review effort level: Balanced

keycloak-github-bot · 2026-09-10T13:15:47Z

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.adapter.servlet.SAMLServletAdapterTest#salesPostEncTest

Keycloak CI - Adapter IT Strict Cookies

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

Unreported flaky test detected, please review

michalvavrik · 2026-09-10T13:54:06Z

        this.session = session;
-        var authInfo = AdminRoot.authenticateRealmAdminRequest(session);
+        // TODO: This will be consolidated with context permissions logic later
+        AdminRoot.authenticateRealmAdminRequest(session);


I think @Pepo48 raised it previously in some previous PR. This leads to 2 authentications, one is here and other is in this.typeProvider = new ClientResourceTypeProvider(session); in DefaultClientsApi ctor. Maybe there should be some TODO for that or that is what you mean by this comment TODO: This will be consolidated with context permissions logic later?

@michalvavrik my intention is that this should be addressed with #52099 - @pedroigor indicated he's open to additional session context state that would allow for just a single authenticateRealmAdminRequest call which will then be used by the v2 logic and the context Permissions logic.

alright, thanks

michalvavrik

Keeping in mind @shawkins comments around the PR, I didn't find anything else that wasn't said. Thanks

closes: keycloak#51326 Signed-off-by: Steve Hawkins <shawkins@redhat.com>

Copilot

🔵 Needs a closer look

The broad cross-module migration and intentionally deferred authorization differences require final human validation.

Review details

Files reviewed: 41/41 changed files
Comments generated: 0 new
Review effort level: Balanced

Signed-off-by: Steven Hawkins <shawkins@redhat.com>

Copilot

🔵 Needs a closer look

It changes authorization, persistence queries, client lifecycle events, secrets, and public API contracts across multiple modules.

Review details

Files reviewed: 41/41 changed files
Comments generated: 0 new
Review effort level: Balanced

Pepo48

Still LGTM, thanks Steve!

keycloak-github-bot Bot added team/cloud-native team/core-iam labels Sep 2, 2026

shawkins requested a balanced review from Copilot September 2, 2026 02:11

Copilot started reviewing on behalf of shawkins September 2, 2026 02:12

View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

shawkins force-pushed the iss51326-f branch 4 times, most recently from 027e85f to f3cd8c9 Compare September 2, 2026 12:35

shawkins marked this pull request as ready for review September 2, 2026 13:45

shawkins requested review from a team as code owners September 2, 2026 13:45

shawkins requested review from Copilot, edewit, pedroigor and vmuzikar September 2, 2026 13:45

Copilot started reviewing on behalf of shawkins September 2, 2026 13:46

View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

shawkins mentioned this pull request Sep 2, 2026

fix: do not leak SCIM resource existence to unauthorized callers #52151

Merged

shawkins requested review from Copilot and michalvavrik September 2, 2026 18:24

shawkins force-pushed the iss51326-f branch from f3cd8c9 to 8ba9528 Compare September 4, 2026 11:43

Copilot started reviewing on behalf of shawkins September 4, 2026 11:43

View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/DefaultAdminApi.java Resolved

Comment thread ...min-v2/api/src/main/java/org/keycloak/representations/admin/v2/BaseClientRepresentation.java Resolved

shawkins force-pushed the iss51326-f branch from 8ba9528 to ab9ad6f Compare September 4, 2026 14:19

Copilot AI review requested due to automatic review settings September 4, 2026 14:19

Copilot started reviewing on behalf of shawkins September 4, 2026 14:26

View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2Test.java Resolved

Copilot AI reviewed Sep 9, 2026

View reviewed changes

keycloak-github-bot Bot added the flaky-test label Sep 10, 2026

keycloak-github-bot Bot reviewed Sep 10, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 10, 2026 12:03

shawkins force-pushed the iss51326-f branch from 016a727 to 51c58ea Compare September 10, 2026 12:03

Copilot started reviewing on behalf of shawkins September 10, 2026 12:04

View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 10, 2026 12:18

shawkins force-pushed the iss51326-f branch from 51c58ea to fdf17d7 Compare September 10, 2026 12:18

Copilot started reviewing on behalf of shawkins September 10, 2026 12:19

View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Comment thread ...-v2/services/src/main/java/org/keycloak/services/client/scim/ClientResourceTypeProvider.java Resolved

keycloak-github-bot Bot reviewed Sep 10, 2026

View reviewed changes

michalvavrik reviewed Sep 10, 2026

View reviewed changes

michalvavrik approved these changes Sep 10, 2026

View reviewed changes

refactor: remove clientdefaultservice / move logic to the type provider

68f3108

closes: keycloak#51326 Signed-off-by: Steve Hawkins <shawkins@redhat.com>

Copilot AI review requested due to automatic review settings September 14, 2026 17:59

shawkins force-pushed the iss51326-f branch from fdf17d7 to 68f3108 Compare September 14, 2026 17:59

Copilot started reviewing on behalf of shawkins September 14, 2026 17:59

View session

Copilot AI reviewed Sep 14, 2026

View reviewed changes

Merge branch 'main' into iss51326-f

6c27603

Signed-off-by: Steven Hawkins <shawkins@redhat.com>

Copilot AI review requested due to automatic review settings September 15, 2026 13:29

Copilot started reviewing on behalf of shawkins September 15, 2026 13:37

View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Pepo48 approved these changes Sep 18, 2026

View reviewed changes

shawkins requested a review from vmuzikar September 18, 2026 14:13

shawkins enabled auto-merge (squash) September 18, 2026 14:13

pedroigor approved these changes Sep 18, 2026

View reviewed changes

shawkins merged commit 8ccf388 into keycloak:main Sep 18, 2026
102 of 104 checks passed

		var current = typeProvider.get(clientId);

		if (!typeProvider.hasPermission(current, AdminPermissionsSchema.MANAGE)) {

Conversation

shawkins commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

shawkins commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

keycloak-github-bot Bot commented Sep 10, 2026

Unreported flaky test detected

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#testAdapterRoleMappings

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

shawkins Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

keycloak-github-bot Bot commented Sep 10, 2026

Unreported flaky test detected

org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostEncTest

Uh oh!

keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

michalvavrik Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

shawkins Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

michalvavrik Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

michalvavrik left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

shawkins commented Sep 2, 2026 •

edited

Loading

shawkins commented Sep 2, 2026 •

edited

Loading

Pepo48 left a comment •

edited

Loading