Repository navigation
Apply client-attributes condition when a client is created - #55038
Draft
cipherprofessor wants to merge 1 commit into
Draft
cipherprofessor wants to merge 1 commit into
cipherprofessor wants to merge 1 commit into
Conversation
The condition only looked at the attributes of an existing client. During registration the client does not exist yet, so the condition abstained and a policy using it was not applied to the client being created. The condition now matches against the attributes of the proposed client on registration, and also when a client is updated to the matching attributes, as the client-access-type condition does. Closes keycloak#54154 Signed-off-by: Mohsin Manzoor Bhat <mohsinmanzoor1913@gmail.com>
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 #54154
Rationale. During registration the context is a
ClientCRUDContextbut not aClientModelContext, soClientAttributesConditionfell through toABSTAINand a policy using only this condition was never applied to the client being created. The condition now matches against the attributes of the proposed client onREGISTER, and on update it also votes yes when the client is being updated to the matching attributes. This follows whatClientAccessTypeConditiondoes since #51380.Behaviour changes. Because the old vote at creation was
ABSTAINrather thanNO, this changes more than the case in the issue: (a) a policy with this condition now applies at creation when the proposed attributes match, which is the fix; (b) with negative logic it now applies at creation to clients that do not carry the attributes, including dynamic client registration and CIMD; (c) a policy that combines this condition with another condition that already voted yes used to apply to every creation and now applies only when the attributes match, which narrows it; (d) policies in strict mode that include this condition can now apply at creation.Decisions. The value comparison is now null-safe, since the condition also reads request bodies and a null attribute value is a valid way to remove an attribute. The cast on
REGISTERis guarded withinstanceofso a custom context that is not aClientCRUDContextkeeps abstaining.Known limitation. For a condition with several attributes and an admin update that sends only some of them, neither the existing client nor the proposed representation matches on its own, so the condition votes no even though the merged client would match.
ClientAccessTypeConditionhas the same shape; I kept parity instead of merging the two maps.Tests.
ClientPoliciesClientCRUDTest#testClientAttributesConditionClientCRUDcovers create with matching attributes, create with a different value, update to the matching value, and the successful update without the rejected setting. It fails onmainat the first create. I ranClientPoliciesClientCRUDTestwith the embedded server (4 tests pass) and./mvnw spotless:check. I did not run the legacyClientPoliciesConditionTest. The null value case is not covered by a test, because the admin client used in the tests does not send null attribute values.AI disclosure. I used Claude Code to help investigate, write and review this change, including the test. I have gone through it and will handle the review myself.