Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Advertised true defaults conflict with actual omission/null behavior for enabled and several SAML settings, potentially misleading callers into disabling clients or document signing.
2 open issues
What changed in this PR
This PR adds default-value metadata to Admin API v2 client schemas to clarify nullable-field behavior for #49038. It changes schema documentation, not runtime default handling.
Changes:
- Adds defaults for shared
enabledand SAML boolean, enum, and string settings. - Updates the JavaScript admin client’s OpenAPI specification to match.
| File | Description |
|---|---|
| rest/admin-v2/api/src/main/java/org/keycloak/representations/admin/v2/SAMLClientRepresentation.java | Adds schema defaults for SAML configuration fields. |
| rest/admin-v2/api/src/main/java/org/keycloak/representations/admin/v2/BaseClientRepresentation.java | Adds a schema default for client enabled state. |
| js/libs/keycloak-admin-client/openapi.json | Reflects the new defaults in the client-generation specification. |
Review effort: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
PATCH defaulting can overwrite an omitted NameID format and enable signature verification without a usable key, unexpectedly changing SAML behavior and causing requests to fail.
1 open issue
3 resolved since last review
Review effort: Balanced
closes: keycloak#49038 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
SAML PUTs with required client signatures and an omitted signing certificate regenerate the certificate on every retry, violating idempotency and unexpectedly rotating trust material.
3 open issues
1 resolved since last review
Review effort: Balanced
| if (create) { | ||
| if (context != PatchClient.class) { | ||
| // create and replace resolve the defaults identically, while patch acts on the current state | ||
| setupClientDefaults(client, model, proposedRepresentation); |
Comment on lines
+432
to
+433
| The exception is `PATCH`, where an omitted field keeps its current value and only a field that is explicitly set to `null` is reset to its default. | ||
| Clients that only send fields that are set therefore cannot reset a field to its default using `PATCH`, and need to send the default value explicitly. |
Comment on lines
+210
to
+211
| if (context != PatchClient.class) { | ||
| // create and replace resolve the defaults identically, while patch acts on the current state |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes: #49038
Defaults were not yet added for limit / offset since there are other issues dealing with that.
@keycloak/cloud-native there are two approaches here:
I think we also briefly talked about one of the phases of request processing being a "defaulting service" (similar to kubernetes), but we never created anything formal for that. A more common service could be considered should we want to better capture / align the v1 and v2 logic.