Create more validation tests for PUT/PATCH logic by mabartos · Pull Request #47282 · keycloak/keycloak

mabartos · 2026-03-19T07:57:58Z

Closes Create more validation tests for PUT/PATCH logic #47058
Number of tests increased from 19 to 99 🚀
There are many common tests that run automatically with POST/PUT/PATCH (bigger coverage)
Tests run with OIDC and SAML (bigger coverage)
Improving the error message for invalid type of field with PATCH
When the clientId is specified in the PATCH payload, it needs to be aligned with the path client ID
Added more tests for client-secret-based validations
Covering more corner cases

Based on my analysis, we should cover all validation rules for now.

@vmuzikar @edewit Could you please check it? Thanks!

Copilot

Pull request overview

This PR significantly expands validation test coverage for PUT/PATCH operations in the Keycloak Client Admin API v2. The test count increases from 19 to 99 tests by systematically testing validation rules across POST, PUT, and PATCH HTTP methods, with both OIDC and SAML client protocols. Additionally, the implementation improves error messages for PATCH operations when type mismatches occur and adds validation for clientId consistency in PATCH requests.

Changes:

Refactored monolithic validation test class into a composable hierarchy: AbstractClientValidationTest (base with common tests), PostClientValidationTest, PutClientValidationTest (extends abstract), and PatchClientValidationTest (extends PUT)
Enhanced DefaultClientService.patchClient() with improved error handling for invalid field types and clientId validation
Centralized setAuthHeader() method in AbstractClientApiV2Test to reduce code duplication across multiple test classes

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
AbstractClientValidationTest.java	New abstract base class containing 14+ common validation test methods covering clientId, protocol, URL, collection element, and unknown field validations
PostClientValidationTest.java	New concrete test class implementing POST method tests with UUID validation
PutClientValidationTest.java	New concrete test class implementing PUT method tests including secret authentication validation
PatchClientValidationTest.java	New concrete test class extending PUT tests, adding PATCH-specific tests and disabling inappropriate inherited tests
DefaultClientService.java	Enhanced patchClient() with JsonMappingException handling for better error messages and clientId validation in PATCH payloads
AbstractClientApiV2Test.java	Centralized setAuthHeader() method for test infrastructure reuse
ClientApiV2Test.java, ClientPoliciesV2Test.java, AdminEventV2Test.java	Refactored to use centralized setAuthHeader() from parent class

You can also share your feedback on Copilot code review. Take the survey.

mabartos · 2026-03-23T10:40:47Z

@vmuzikar Could you please check it? Thanks!

vmuzikar

@mabartos Thank you for the PR!

...rc/test/java/org/keycloak/tests/admin/client/v2/validation/AbstractClientValidationTest.java

...s/src/test/java/org/keycloak/tests/admin/client/v2/validation/PatchClientValidationTest.java

...rc/test/java/org/keycloak/tests/admin/client/v2/validation/AbstractClientValidationTest.java

vmuzikar · 2026-03-25T17:16:21Z

This conflicts/overlaps with #47375. CC @edewit

vmuzikar · 2026-03-25T17:36:50Z

@Pepo48 @shawkins @michalvavrik Can you please review as well?

rest/admin-v2/api/src/main/java/org/keycloak/services/client/DefaultClientService.java

michalvavrik · 2026-03-26T12:22:24Z

rest/admin-v2/api/src/main/java/org/keycloak/services/client/DefaultClientService.java

    }

+    protected void assertSameClientIds(String pathId, String payloadId) {
+        if (!Objects.equals(pathId, payloadId)) {


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"?

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

As that might be done in a follow-up.

Follow-up:

Require 'clientId' field in the payload #47524

michalvavrik

I didn't find a test that tries to change protocol from OIDC to SAML. I hope I didn't miss something, can you double-check, please?

michalvavrik · 2026-03-26T12:37:34Z

Sorry, I didn't read existing comments first, it was already noted here #47282 so forget about it.

mabartos · 2026-03-26T15:08:31Z

I didn't find a test that tries to change protocol from OIDC to SAML. I hope I didn't miss something, can you double-check, please?

@michalvavrik It's similar to this, and should be already fixed: #47282 (comment). I'll verify if we have it verified on the PUT as well.

Closes keycloak#47058 Signed-off-by: Martin Bartoš <mabartos@redhat.com>

Signed-off-by: Martin Bartoš <mabartos@redhat.com>

mabartos · 2026-03-27T13:55:37Z

@vmuzikar Can you review it again? Thanks!

vmuzikar

LGTM, thanks for the updates.

mabartos self-assigned this Mar 19, 2026

mabartos requested review from edewit and vmuzikar March 19, 2026 08:19

mabartos added the team/admin-api-wg Admin API v2 Working Group label Mar 19, 2026

mabartos marked this pull request as ready for review March 19, 2026 08:19

mabartos requested review from a team as code owners March 19, 2026 08:19

keycloak-github-bot bot added the team/cloud-native label Mar 19, 2026

mabartos requested a review from Copilot March 19, 2026 08:45

Copilot AI reviewed Mar 19, 2026

View reviewed changes

mabartos mentioned this pull request Mar 19, 2026

New client service - PUT/POST - update or create client #47029

Open

edewit approved these changes Mar 19, 2026

View reviewed changes

vmuzikar reviewed Mar 25, 2026

View reviewed changes

vmuzikar requested review from Pepo48, michalvavrik and shawkins March 25, 2026 17:36

shawkins reviewed Mar 25, 2026

View reviewed changes

rest/admin-v2/api/src/main/java/org/keycloak/services/client/DefaultClientService.java Outdated Show resolved Hide resolved

michalvavrik reviewed Mar 26, 2026

View reviewed changes

mabartos force-pushed the KC-47058 branch from 928cc95 to 799c845 Compare March 26, 2026 13:59

mabartos marked this pull request as draft March 26, 2026 15:18

michalvavrik approved these changes Mar 26, 2026

View reviewed changes

mabartos added 3 commits March 27, 2026 13:46

Create more validation tests for PUT/PATCH logic

6111030

Closes keycloak#47058 Signed-off-by: Martin Bartoš <mabartos@redhat.com>

Test for trying to change the protocol for existing client

c55eda0

Signed-off-by: Martin Bartoš <mabartos@redhat.com>

PATCH tests do not extend PUT tests

607c398

Signed-off-by: Martin Bartoš <mabartos@redhat.com>

mabartos added 2 commits March 27, 2026 13:46

Introduce the ProtocolUnmofidiedValidator

8cdda53

Signed-off-by: Martin Bartoš <mabartos@redhat.com>

Accomodate changes for the protocol unmodified validator

0adfb2d

Signed-off-by: Martin Bartoš <mabartos@redhat.com>

mabartos force-pushed the KC-47058 branch from 981c59a to 0adfb2d Compare March 27, 2026 12:47

mabartos marked this pull request as ready for review March 27, 2026 12:48

vmuzikar approved these changes Mar 27, 2026

View reviewed changes

vmuzikar merged commit a69f08f into keycloak:main Mar 27, 2026
85 checks passed

Conversation

mabartos commented Mar 19, 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

Pull request overview

Reviewed changes

Uh oh!

mabartos commented Mar 23, 2026

Uh oh!

vmuzikar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vmuzikar commented Mar 25, 2026

Uh oh!

vmuzikar commented Mar 25, 2026

Uh oh!

Uh oh!

michalvavrik Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mabartos Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michalvavrik Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

mabartos Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

michalvavrik left a comment

Choose a reason for hiding this comment

Uh oh!

michalvavrik commented Mar 26, 2026

Uh oh!

mabartos commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mabartos commented Mar 27, 2026

Uh oh!

vmuzikar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mabartos commented Mar 19, 2026 •

edited

Loading

michalvavrik Mar 26, 2026 •

edited

Loading

mabartos Mar 26, 2026 •

edited

Loading

mabartos commented Mar 26, 2026 •

edited

Loading