Repository navigation
Conversation
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
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
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
closes: keycloak#49038 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
There was a problem hiding this comment.
🟡 Changes recommended
Updates can require SAML request signatures without provisioning or retaining a verification certificate, causing subsequent requests to fail signature verification.
1 open issue
3 resolved since last review
Review effort: Balanced
| @Override | ||
| protected void addProtocolAttributes(Map<String, Attribute<ClientModel, SAMLClientRepresentation>> map) { | ||
| map.put("nameIdFormat", protocolStringAttr("nameIdFormat", SAML_NAME_ID_FORMAT, (rep, v) -> rep.setNameIdFormat(SAMLClientRepresentation.NameIdFormat.fromJson(v)), (model, value) -> setAttribute(model, SAML_NAME_ID_FORMAT, value != null ? ((SAMLClientRepresentation.NameIdFormat) value).toJson() : null))); | ||
| map.put("nameIdFormat", protocolStringAttr("nameIdFormat", SAML_NAME_ID_FORMAT, (rep, v) -> rep.setNameIdFormat(SAMLClientRepresentation.NameIdFormat.fromJson(v)), (model, value) -> setAttribute(model, SAML_NAME_ID_FORMAT, value != null ? ((SAMLClientRepresentation.NameIdFormat) value).toJson() : SAMLClientRepresentation.NameIdFormat.USERNAME.toJson()))); |
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.