Skip to content

Apply client-attributes condition when a client is created - #55038

Draft
cipherprofessor wants to merge 1 commit into
keycloak:mainfrom
cipherprofessor:fix-54154-client-attributes-condition-register
Draft

cipherprofessor wants to merge 1 commit into
keycloak:mainfrom
cipherprofessor:fix-54154-client-attributes-condition-register

Conversation

@cipherprofessor

Copy link
Copy Markdown
Contributor

Closes #54154

Rationale. During registration the context is a ClientCRUDContext but not a ClientModelContext, so ClientAttributesCondition fell through to ABSTAIN and 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 on REGISTER, and on update it also votes yes when the client is being updated to the matching attributes. This follows what ClientAccessTypeCondition does since #51380.

Behaviour changes. Because the old vote at creation was ABSTAIN rather than NO, 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 REGISTER is guarded with instanceof so a custom context that is not a ClientCRUDContext keeps 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. ClientAccessTypeCondition has the same shape; I kept parity instead of merging the two maps.

Tests. ClientPoliciesClientCRUDTest#testClientAttributesConditionClientCRUD covers create with matching attributes, create with a different value, update to the matching value, and the successful update without the rejected setting. It fails on main at the first create. I ran ClientPoliciesClientCRUDTest with the embedded server (4 tests pass) and ./mvnw spotless:check. I did not run the legacy ClientPoliciesConditionTest. 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.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Client policy condition client-attributes does not apply when a client is created, only on later updates

1 participant