Prevent SCIM user writes from bypassing user-profile edit permissions - #51465
Prevent SCIM user writes from bypassing user-profile edit permissions#51465sguilhen wants to merge 1 commit into
Conversation
Closes keycloak#50991 Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
There was a problem hiding this comment.
Pull request overview
Adds user-profile edit-permission enforcement for SCIM-mapped custom attributes.
Changes:
- Prevents unauthorized custom-attribute mutations.
- Adds PATCH and PUT regression coverage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
AbstractUserModelSchema.java |
Adds editability evaluation. |
UserExtensionModelSchema.java |
Guards direct model writes. |
UserTest.java |
Tests protected attribute updates. |
| return true; | ||
| } | ||
|
|
||
| return !permissions.getEdit().isEmpty(); |
There was a problem hiding this comment.
Is there any reason to instead call Attributes.isReadOnly(name). It is the method that we usually use to check whether or not an attribute is read-only/writable.
There was a problem hiding this comment.
I looked into using Attributes.isReadOnly(name) but it doesn't work for the SCIM context. The problem is that UserProfileContext.SCIM is defined as (false, false, false) , meaning isAdminContext() returns false. Custom attributes with edit=Set.of(ROLE_ADMIN) have their write condition set to ONLY_ADMIN_CONDITION which checks context.getContext().isAdminContext(). So isReadOnly would return true for all admin-editable custom attributes in the SCIM context, effectively making them all read-only via SCIM.
SCIM is used as an admin-level API, so it should be able to write to attributes that have admin edit permissions. The existing tests in AbstractScimTest.addOrReplaceUPAttribute() configure attributes with edit=Set.of(ROLE_ADMIN) and expect SCIM to write to them.
That's why the check directly inspects UPAttributePermissions.getEdit() instead. It checks whether any edit role is configured at all (attribute is managed), without evaluating against the SCIM context's non-admin role. An attribute with edit=[] (empty set, no roles configured) is treated as read-only.
| } | ||
|
|
||
| @Test | ||
| public void testPatchAndUpdateRespectEditPermissions() { |
There was a problem hiding this comment.
Can you please check:
The isEditableAttribute guard is only added to UserExtensionModelSchema.createCustomAttribute(). Core SCIM attributes in UserCoreModelSchema (e.g., displayName, title,
nickName) also map to UP attributes and use UserModel::setSingleAttribute directly without any edit permission check. If UP edit permissions are configured on those
attributes, SCIM would also bypass them.
This may be intentional (core SCIM attributes are arguably always writable by the protocol), but it should be a conscious design decision, not an oversight.
There was a problem hiding this comment.
Yeah, this is intentional. Core SCIM attributes like displayName, title, nickName, name.givenName, etc. are defined as writable by the SCIM protocol (RFC 7643/7644). SCIM clients expect to be able to set these attributes, so blocking them based on UP edit permissions would break protocol compliance.
The isEditableAttribute guard is scoped to extension/custom attributes because those are the attributes where the UP configuration is the authority on editability. For core attributes, the SCIM schema definition is the authority and they're always writable via SCIM regardless of UP config, just as they're always writable via the Admin API (which uses USER_API context where isAdminContext()=true).
Closes #50991