Fixes #25703: prevent non-admin users from managing other users via API#27749
Fixes #25703: prevent non-admin users from managing other users via API#27749sonika-shah wants to merge 1 commit intomainfrom
Conversation
…r users
The default DataConsumerPolicy granted EditDescription (and similar edit
operations) on all resources, including the user resource. This allowed
any authenticated user to PATCH another user's profile via
PATCH /api/v1/users/{id} without admin privileges, since the authorization
check in patchInternal() would match the EditDescription operation against
the DataConsumerPolicy rule.
Additionally, the DELETE endpoints on /api/v1/users lacked an explicit
admin-only guard, relying solely on policy evaluation which could allow
edge cases through the OrganizationPolicy owner-rule.
Fix:
- patch(): add a self-check at the start — non-admin users can only patch
their own profile (id matches their own user id). Attempting to patch a
different user now requires admin, matching the behavior of PUT /v1/users.
- delete(), deleteByIdAsync(), delete(by name): add authorizeAdmin() before
delegating to the parent delete implementations, making user deletion
an explicit admin-only operation.
- Refactor the personaPreferences self-check to reuse the loggedInUser
fetched for the new self-check (avoids a redundant DB lookup).
Integration test: UserManagementRbacIT verifies that non-admin users
receive HTTP 403 when attempting to PATCH or DELETE other users, and that
they can still PATCH their own profile.
github-actions
Bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| // Check if updating personaPreferences - users can only update their own | ||
| if (path.startsWith("/personaPreferences")) { | ||
| String authenticatedUserName = securityContext.getUserPrincipal().getName(); | ||
| User authenticatedUser = | ||
| repository.getByName(uriInfo, authenticatedUserName, new Fields(Set.of("id"))); | ||
| if (!authenticatedUser.getId().equals(id)) { | ||
| if (!loggedInUser.getId().equals(id)) { | ||
| throw new AuthorizationException("Users can only update their own persona preferences"); | ||
| } |
There was a problem hiding this comment.
⚠️ Bug: personaPreferences check blocks admins from updating other users
After the new admin gate at line 1063-1065, only admins (or self) can reach the personaPreferences path check at line 1075-1078. For self-patches, the check passes. But for an admin patching another user's personaPreferences, the unconditional !loggedInUser.getId().equals(id) check throws AuthorizationException, blocking the operation.
While this was also the behavior before this PR, the refactoring makes it more visible: admins who pass the top-level gate are then blocked by a field-level check that ignores admin status. If admin management of persona preferences is a valid use case (consistent with admins being able to patch all other fields), this should be fixed.
Suggested fix:
if (path.startsWith("/personaPreferences")) {
if (!loggedInUser.getId().equals(id)) {
authorizer.authorizeAdmin(securityContext);
}
}
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
There was a problem hiding this comment.
Pull request overview
This PR closes a broken access control gap in the Users REST API by enforcing explicit admin-only authorization for modifying or deleting other users, independent of broad default RBAC policies.
Changes:
- Enforce “self-only” PATCH for non-admins on
PATCH /v1/users/{id}; require admin when patching someone else. - Make user deletion endpoints explicitly admin-only (
DELETE /v1/users/{id},/async/{id}, and/name/{name}). - Add integration tests to validate non-admin cannot PATCH/DELETE other users, while preserving admin and self-patch behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/UserResource.java | Adds explicit admin gating for cross-user PATCH and all DELETE user operations. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/UserManagementRbacIT.java | Adds RBAC integration tests covering PATCH/DELETE permissions for admin vs non-admin users. |
| User victim = createTestUser(ns, "victim"); | ||
| String attackerEmail = ns.prefix("attacker") + "@test.openmetadata.org"; | ||
| String attackerName = ns.prefix("attacker"); | ||
|
|
||
| String attackerToken = | ||
| JwtAuthProvider.tokenFor(attackerName, attackerEmail, new String[] {}, TOKEN_TTL_SECONDS); | ||
|
|
There was a problem hiding this comment.
The test generates an attacker JWT for a username that is never created in OpenMetadata. Since UserResource.patch() now looks up the logged-in user via repository.getByName(), this request can fail with 404 (attacker user not found) instead of exercising the intended 403 authorization path. Create the attacker user (via admin API) before calling PATCH, or loosen the assertion to accept the not-found case if that’s an expected auth behavior in this suite.
| User victim = createTestUser(ns, "del_victim"); | ||
| String attackerEmail = ns.prefix("del_attacker") + "@test.openmetadata.org"; | ||
| String attackerName = ns.prefix("del_attacker"); | ||
|
|
||
| String attackerToken = | ||
| JwtAuthProvider.tokenFor(attackerName, attackerEmail, new String[] {}, TOKEN_TTL_SECONDS); | ||
|
|
There was a problem hiding this comment.
Same issue as the PATCH test: this test generates a token for del_attacker without creating that user first. With the new UserResource self/admin gate relying on DB lookup of the current user, the DELETE call may return 404 for an unknown principal rather than the expected 403. Create the attacker user before issuing the DELETE, so the test reliably validates the RBAC behavior.
|
🟡 Playwright Results — all passed (12 flaky)✅ 3961 passed · ❌ 0 failed · 🟡 12 flaky · ⏭️ 86 skipped
🟡 12 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
Summary
PATCH /api/v1/users/{id}. The root cause: the defaultDataConsumerPolicygrantsEditDescription,EditTags,EditGlossaryTerms,EditTier, andEditCertificationon all resources. Since thepatch()method inUserResourceonly checked admin for specific fields (/isAdmin,/isBot,/roles) and self for/personaPreferences, but delegated everything else topatchInternal(), the policy evaluator's broad allow-rules let non-admin users modify any field on another user.DELETE /api/v1/users/…endpoints also lacked an explicit admin-only gate; they relied entirely on policy evaluation, which could admit non-admin requests through theOrganizationPolicy-Owner-Ruleor future policy changes.Fix
UserResource.javapatch(): Added a self-check at the start of the method. If the authenticated user is trying to patch a different user (their id ≠ the targetid),authorizer.authorizeAdmin()is called immediately — mirroring the identical pattern increateOrUpdateUser()(PUT). Non-admin users can still patch their own profile.delete(),deleteByIdAsync(),delete(name): Addedauthorizer.authorizeAdmin()before delegating to the parent delete implementations. User deletion is now an explicit admin-only operation.personaPreferencesself-check to reuse theloggedInUseralready fetched for the new check (eliminates one redundant DB round-trip).Testing
New integration test:
UserManagementRbacITtest_nonAdminCannotPatchOtherUsertest_nonAdminCannotDeleteOtherUsertest_adminCanPatchOtherUsertest_nonAdminCanPatchOwnProfileTo reproduce the vulnerability against an unpatched deployment:
Closes #25703
Generated by Claude Code