Skip to content

fix(graphql): authorize patchEntity by URN type with Edit Entity or the type's management privilege - #19872

Merged
supersingh05 merged 3 commits into
masterfrom
fix/patch-entity-api-privileges
Sep 22, 2026
Merged

supersingh05 merged 3 commits into
masterfrom
fix/patch-entity-api-privileges

Conversation

@supersingh05

@supersingh05 supersingh05 commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

AuthorizationUtils.isAuthorizedForPatch, used by both the patchEntity and patchEntities GraphQL mutations, took the entity type for its authorization check from the client-supplied entityType argument and only fell back to the URN when that argument was absent. The patch itself is always applied to the URN's entity type (PatchResolverUtils resolves the type from the URN). So the resource type used to match policies and the type actually written could differ: a caller holding an Edit Entity grant scoped to, say, datasets could get a write to a URN of another type authorized by claiming entityType: "dataset".

This PR makes two changes to that check.

  1. Type from the URN. The entity type is derived from the URN only. A URN that cannot be parsed is rejected, and a client-supplied entityType that disagrees with the URN is rejected with a warning.
  2. Edit Entity or the type's management privilege. Authorization succeeds when the actor holds Edit Entity on the target, as before, or the entity type's own API UPDATE privilege from the entity privilege map: Manage Policies for dataHubPolicy, Manage Secrets for dataHubSecret, Manage Global Settings for globalSettings, the user-and-group privileges for corpuser / corpGroup. This matches what the dedicated GraphQL mutations for those types already accept. For entity types without a type-specific rule the check is simply Edit Entity. This is a pure widening: nobody who could patch before loses the ability.
  • AuthorizationUtils.isAuthorizedForPatch: URN-derived type, deny on unparseable URN, deny on mismatch; privilege group is the type's API UPDATE privileges plus Edit Entity.
  • PatchEntityResolverTest: a glossary-term URN with entityType: "dataset" is rejected and never reaches ingestProposal even with an always-allow authorizer; an unparseable URN is rejected; a policy patch succeeds with Manage Policies alone and with Edit Entity alone, and is denied with an unrelated privilege.
  • docs/how/updating-datahub.md: Breaking Changes entry.

Behavior change

Clients that omit entityType or pass the URN's actual type see no change to what is required of them. Clients that pass a mismatched entityType now receive an authorization error instead of having the request checked as the claimed type. Actors holding a type-specific management privilege (for example Manage Policies) without Edit Entity can now use patchEntity for that type, as they already could through the dedicated mutations.

Scope note

An earlier revision of this PR required the type-specific management privilege for policies, secrets, and settings and no longer accepted Edit Entity for them. That was reverted after review: unscoped Edit Entity is the highest privilege in this model and the MANAGE_* privileges are UI carve-outs, so requiring one on top of the other inverted the hierarchy and would have broken deployments whose policies are built around unscoped Edit Entity. The current rule accepts either.

Verification

  • spotlessApply on datahub-graphql-core; markdown prettier on docs.
  • PatchEntityResolverTest (7) and PatchEntitiesResolverTest (7): 14 tests, 0 failures.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly PR Title Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions github-actions Bot added docs Issues and Improvements to docs product PR or Issue related to the DataHub UI/UX devops PR or Issue related to DataHub backend & deployment labels Sep 18, 2026
@cursor

cursor Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

PR Summary

Overview
Closes an authorization bypass on patchEntity / patchEntities: policy checks now use the entity type from the target URN only, not the optional client entityType. Mismatched entityType, unparseable URNs, or missing type after parse are rejected (unauthorized) instead of evaluating grants against a claimed type.

Privilege rules are aligned with dedicated type mutations: a patch is allowed with Edit Entity on the target (unchanged) or the entity type’s API UPDATE management privilege (e.g. Manage Policies for dataHubPolicy). Types without a specific mapping still require Edit Entity.

Tests cover URN/type mismatch, bad URNs, and policy patches with Manage Policies vs Edit Entity vs unrelated privileges. Updating DataHub documents this as a breaking change for clients that passed a wrong entityType.

Reviewed by Cursor Bugbot for commit c5fc28d. Bugbot is set up for automated code reviews on this repo. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 3470eb7. Configure here.

@codecov

codecov Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ahub/graphql/authorization/AuthorizationUtils.java 92.85% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@supersingh05

Copy link
Copy Markdown
Contributor Author

CI note: the first push failed Pytest Smoke Tests (Batch 3/7) on tests/authorization/test_privilege_grant_auth.py (test_group_owner_can_add_another_member, test_group_owner_cannot_add_self_to_owned_group). Those tests rely on a group owner holding Edit Entity through ownership and patching the group via patchEntity, with PrivilegeGrantAuthorizationValidator guarding the membership/role aspects. My first version required the corpGroup entity-API privileges alone (Update Users & Groups etc.), which denied that owner.

Fixed in 7e68485: Edit Entity remains sufficient for every entity type except the platform-managed ones (dataHubPolicy, dataHubSecret, globalSettings), where the management privilege is required and Edit Entity no longer substitutes. This matches the dedicated GraphQL mutations for groups (canEditGroupMembers also accepts Edit Entity) and keeps the reported policy bypass closed. Added testPatchGroupStillAllowedWithEditEntity; the release note and description are updated to the narrower scope.

@maggiehays maggiehays added the needs-review Label for PRs that need review from a maintainer. label Sep 19, 2026
@supersingh05
supersingh05 marked this pull request as draft September 21, 2026 18:16
The patch authorization check used the client-supplied entityType as the
resource type for policy matching, while the patch itself is applied to the
URN's entity type. A grant scoped to one entity type could therefore be
matched against a URN of another type by claiming the wrong entityType.
Take the type from the URN, and reject requests whose URN cannot be parsed
or whose entityType disagrees with it.
@supersingh05
supersingh05 force-pushed the fix/patch-entity-api-privileges branch from 7e68485 to e3f8617 Compare September 21, 2026 20:35
@supersingh05 supersingh05 changed the title fix(graphql): authorize patchEntity with entity-type-specific privileges fix(graphql): derive patchEntity authorization entity type from the URN Sep 21, 2026
@supersingh05

Copy link
Copy Markdown
Contributor Author

Scope reduced, force-pushed as a single commit (e3f8617).

Following the review discussion: unscoped Edit Entity is treated as the top of the privilege hierarchy in this codebase, and the MANAGE_* privileges are UI-endpoint carve-outs rather than a tier above it. Requiring a MANAGE_* privilege on top of Edit Entity for patchEntity therefore encodes a model the codebase does not follow, and it would be backwards-incompatible for deployments whose policies are built around unscoped Edit Entity. That change, and the aspect-layer validator, are removed pending a separate decision on how the GraphQL and REST privilege rules should align.

What remains is the part that is correct under either model: the authorization check now takes the entity type from the URN (which is what the patch is applied to) instead of the client-supplied entityType, and rejects an unparseable URN or a mismatched type. The required privilege is unchanged. Description and release note updated to match.

…hEntity

In addition to Edit Entity on the target, patchEntity and patchEntities now
accept the entity type's own API UPDATE privilege (for example Manage
Policies for dataHubPolicy), matching the dedicated GraphQL mutations for
those types. Edit Entity remains sufficient for every entity type.
@supersingh05 supersingh05 changed the title fix(graphql): derive patchEntity authorization entity type from the URN fix(graphql): authorize patchEntity by URN type with Edit Entity or the type's management privilege Sep 22, 2026
@supersingh05

Copy link
Copy Markdown
Contributor Author

Second commit (0ad2a43) adds the rule agreed with the security lead: patchEntity / patchEntities authorize when the actor holds Edit Entity on the target or the entity type's own API UPDATE privilege from the entity privilege map (Manage Policies for dataHubPolicy, Manage Secrets for dataHubSecret, Manage Global Settings for globalSettings, the user/group privileges for corpuser / corpGroup). For types without a specific rule this is just Edit Entity.

This is a pure widening: Edit Entity remains sufficient everywhere, so unscoped Edit Entity keeps its place at the top of the hierarchy, and actors who were granted a type-specific management privilege can now use the generic patch mutation the same way they already can use the dedicated mutations. The URN-derived type check from the first commit is unchanged. Tests cover Manage Policies alone (allowed), Edit Entity alone (allowed), and an unrelated privilege (denied). Description and release note updated.

@supersingh05
supersingh05 merged commit 4c4f06b into master Sep 22, 2026
91 of 92 checks passed
@supersingh05
supersingh05 deleted the fix/patch-entity-api-privileges branch September 22, 2026 19:01
david-leifker pushed a commit that referenced this pull request Sep 23, 2026
…he type's management privilege (#19872)

(cherry picked from commit 4c4f06b)

This branch was successfully deployed

1 active deployment
Preview — c5fc28d1 Deployed Sep 22, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

devops PR or Issue related to DataHub backend & deployment docs Issues and Improvements to docs needs-review Label for PRs that need review from a maintainer. product PR or Issue related to the DataHub UI/UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants