Repository navigation
Create more validation tests for PUT/PATCH logic - #47282
Conversation
There was a problem hiding this comment.
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), andPatchClientValidationTest(extends PUT) - Enhanced
DefaultClientService.patchClient()with improved error handling for invalid field types and clientId validation - Centralized
setAuthHeader()method inAbstractClientApiV2Testto 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.
|
@vmuzikar Could you please check it? Thanks! |
|
@Pepo48 @shawkins @michalvavrik Can you please review as well? |
| @@ -263,6 +269,12 @@ public void deleteClient(RealmModel realm, String clientId) throws ServiceExcept | |||
| fireAdminEvent(OperationType.DELETE, client); | |||
| } | |||
|
|
|||
| protected void assertSameClientIds(String pathId, String payloadId) { | |||
| if (!Objects.equals(pathId, payloadId)) { | |||
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
Maybe also related to the required clientId? #47282 (comment)
As that might be done in a follow-up.
There was a problem hiding this comment.
There was a problem hiding this comment.
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?
|
Sorry, I didn't read existing comments first, it was already noted here #47282 so forget about it. |
@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>
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
|
@vmuzikar Can you review it again? Thanks! |
There was a problem hiding this comment.
LGTM, thanks for the updates.
* Create more validation tests for PUT/PATCH logic Closes keycloak#47058 Signed-off-by: Martin Bartoš <mabartos@redhat.com> * Test for trying to change the protocol for existing client Signed-off-by: Martin Bartoš <mabartos@redhat.com> * PATCH tests do not extend PUT tests Signed-off-by: Martin Bartoš <mabartos@redhat.com> * Introduce the ProtocolUnmofidiedValidator Signed-off-by: Martin Bartoš <mabartos@redhat.com> * Accomodate changes for the protocol unmodified validator Signed-off-by: Martin Bartoš <mabartos@redhat.com> --------- Signed-off-by: Martin Bartoš <mabartos@redhat.com>
clientIdis specified in the PATCH payload, it needs to be aligned with the path client IDBased on my analysis, we should cover all validation rules for now.
@vmuzikar @edewit Could you please check it? Thanks!