Repository navigation
Replies: 2 comments
|
Thanks for putting this together. This addresses my use case: several services update different attributes on the same user, and today each has to GET and PUT the full representation. I’d want a patch to change only the named attribute keys and leave other attributes and fields such as enabled untouched. Removing a named attribute with null would also be useful. One point I’d be careful with is the concurrency wording: server-side merging avoids stale client-side GET → PUT updates, but I wouldn’t assume it guarantees that simultaneous PATCH requests both succeed without testing that. |
|
I've updated the proposal above with:
@vmuzikar, since partial updates were previously deferred to a new Admin API and the Admin API v2 working group is defining HTTP verb behaviour in #43169: would a v1 @pedroigor, you mentioned in #28220 that PATCH wasn't planned at the time, and this would build on A short "yes / no / wait" is enough for me to either start on a PR or redirect the effort (for example to docs on PUT semantics and pointing users at SCIM). |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Problem
PUT /admin/realms/{realm}/users/{id}is the only way to update a user, and its semantics are hard to use safely:attributesis omitted, existing attributes are kept (UserResource.updateUserbackfills them withputIfAbsent).attributesis present, it's treated as the complete set.profile.update(rep.getAttributes() != null)removes every attribute that isn't sent. That includesfirstName/lastNameif they aren't re-sent (DefaultUserProfile.updateInternal).enabled,emailVerified,requiredActions) are only applied when non-null, so they can be updated on their own.This behavior is intended and covered by tests (
UserAttributesTest#testKeepRootAttributeWhenOtherAttributesAreSet: "user profile requires sending all attributes otherwise they are removed"), so changing PUT isn't an option.The result is that updating one attribute means GET → modify → PUT of the whole user. When several services write attributes on the same user, that's a lost-update race. A write based on a stale read can revert another service's change, delete an attribute added in the meantime, or, because the full representation is re-sent, flip
enabledback. People work around it with external per-user locks (see #19691). Related issues: #44867, #19691, #28908 / #28220.Proposal
Add an opt-in endpoint and leave PUT unchanged:
using RFC 7396 JSON Merge Patch, the same format Admin API v2 already uses for clients (
PatchTypeNames.JSON_MERGE).{ "attributes": { "department": ["eng"], "legacyId": null } }firstName,lastName,email,enabled, …) are replaced when present.attributesmerges per key: a value replaces that attribute,nullremoves it, and attributes that aren't mentioned are untouched.UserProfile(USER_APIcontext) validation and update, attribute-level permissions and read-only rules,updateUserFromRep, and the adminUPDATEevent. Validation and permission behavior therefore match PUT exactly, and there's no client-side stale copy. (This removes the stale-read window on the client; it doesn't serialize concurrent writes on the server, so the tests will cover that case explicitly rather than assume it.)204 No Content, matching PUT and the REST API guideline. Unsupported content type →415. A body that isn't a JSON object →400.Scope would include the endpoint, OpenAPI annotations, Java and JS admin client methods, docs (including a short section on which fields PUT replaces versus leaves alone), and integration tests in
tests/base(including concurrent patches to different attributes).Relation to the SCIM API
Since 26.8.0 the SCIM API is supported and supports
PATCH /realms/{realm}/scim/v2/Users/{id}, which covers part of this use case. Custom attributes can be patched once they're mapped to a SCIM schema extension in the User Profile configuration. Where an Admin APIPATCHwould still be useful:attributesmap.UserResourceTypeProvider#getModel), and it refuses to manage users who hold administrative roles.If maintainers think SCIM is the intended answer for partial updates, I'd be glad to instead contribute docs that point Admin API users to it, alongside documenting PUT's semantics.
Out of scope (possible follow-up)
ETag/If-Match/If-Unmodified-Since). Users now have a persistedlastModifiedTimestamp(added for SCIM), but today only SCIM updates it; Admin API PUT doesn't. That could be a basis for preconditions later, but it needs its own design.Open questions for maintainers
v1 or v2? Would you accept this on the current Admin API, or would you rather partial user updates wait for a users API in Admin API v2? If v1 is acceptable, is reusing v2's merge-patch media type the right way to keep them consistent?
Removing attributes.
DefaultUserProfileonly removes attributes in the "full set" mode (removeAttributes=true). Withfalse, empty values for custom attributes are skipped (isIgnoreAttributeUpdate). For "remove exactly these keys" I see two options:UserProfile(e.g.update(removeAttributes, Set<String> attributesToRemove, …)), orupdate(true), making sure attributes the admin can't see are kept.The SCIM user
patchalready takes the second approach: it copies the user's current attributes, applies the operations, then callsprofile.update(true)(UserResourceTypeProvider#patch). I'd lean toward doing the same for consistency and to avoid changing theUserProfileSPI. Which would you prefer?Fields with side effects. For
credentials,requiredActions,federatedIdentitiesandgroupsin a patch body, would you rather reject them with400or ignore them? I'd lean towards rejecting everything except profile fields andenabled/emailVerified/requiredActions.I'm happy to implement this and would like confirmation on the direction (especially question 1) before opening a PR.
All reactions