[#11775] improvement(authz): Short-circuit list authorization via parent-scope grant#11778
Open
yuqi1129 wants to merge 1 commit into
Open
[#11775] improvement(authz): Short-circuit list authorization via parent-scope grant#11778yuqi1129 wants to merge 1 commit into
yuqi1129 wants to merge 1 commit into
Conversation
…ent-scope grant When authorization is enabled, list operations (listTables/listSchemas/listCatalogs on both Gravitino REST and Iceberg REST) ran one per-object permission check per result, causing an N+1 explosion (e.g. ~10k tables took ~134s vs ~4s with auth off). Evaluate the ancestor-only portion of the filter expression once per list request: when a privilege is granted (or the object is owned) at a parent scope (metalake/catalog/schema) every child is visible unless an object-level deny overrides it. A new GravitinoAuthorizer#hasDenyPolicyOnType gate (in-memory scan of the user's deny policies in JcasbinAuthorizer; conservative default of true) makes the short-circuit safe, falling back to per-object filtering only when a deny may exist.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes authorization for large list operations (tables/schemas/catalogs) by safely short-circuiting per-object checks when an ancestor-scope grant (or ownership) implies all children are visible and no object-level deny can apply, addressing the N+1 performance issue described in #11775.
Changes:
- Added parent-scope authorization expressions for TABLE/SCHEMA/CATALOG list operations.
- Enhanced
MetadataAuthzHelper.filterByExpression(..., NameIdentifier[])to attempt a one-time parent-scope evaluation and skip per-object filtering when safe. - Extended the
GravitinoAuthorizerSPI withhasDenyPolicyOnType(...)and implemented it inJcasbinAuthorizer, plus added unit tests covering both the short-circuit and deny detection behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java | Adds list short-circuit registry and parent-scope evaluation to avoid per-object auth checks when safe. |
| server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java | Implements deny-policy presence detection by scanning deny-enforcer policies for the current user’s roles. |
| server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java | Introduces parent-scope authorization expressions for table/schema/catalog list short-circuiting. |
| core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java | Adds new SPI default method hasDenyPolicyOnType(...) (conservative default). |
| server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java | Adds unit tests validating short-circuit behavior and fallback when denies may exist. |
| server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java | Adds unit test for hasDenyPolicyOnType(...) correctness across type/privilege matches. |
Code Coverage Report
Files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
When authorization is enabled, list operations evaluated one per-object permission check per result, producing an N+1 explosion. This PR evaluates the ancestor-only portion of the filter expression once per list request: if a privilege is granted (or the object is owned) at a parent scope (metalake/catalog/schema), every child is visible unless an object-level deny overrides it.
AuthorizationExpressionConstantsforTABLE/SCHEMA/CATALOG.MetadataAuthzHelper.filterByExpression(…, NameIdentifier[])attempts the short-circuit before the per-object loop. One insertion point covers both Gravitino REST (listTables/listSchemas/listCatalogs) and Iceberg REST (listTable/listNamespace), since they all route through this helper.GravitinoAuthorizer#hasDenyPolicyOnType(conservative defaulttrue);JcasbinAuthorizerimplements it as an in-memory scan of the current user's deny-enforcer policies. The short-circuit is taken only when no object-level deny may exist for the relevant type/privileges, otherwise it falls back to the existing per-object filtering.Why are the changes needed?
Per #11775, with authorization on, listing a schema of ~10,496 tables took ~134s (vs ~4s with auth off) — a ~70x regression — because permission was checked once per table. Owner/grant at a parent scope makes every child visible (OR semantics); only an object-level deny can subtract, so the ancestor part is a loop-invariant that can be lifted out of the per-object loop. Ancestor-level denies are already handled inside the parent-scope expression's
!ANY(DENY_..., METALAKE, CATALOG, SCHEMA)terms.Fix: #11775
Does this PR introduce any user-facing change?
No. The new method on the
GravitinoAuthorizerSPI is a backward-compatible default method (default returnstrue, i.e. no short-circuit), and authorization decisions are unchanged.How was this patch tested?
New unit tests:
TestMetadataAuthzHelper: parent grant + no deny returns the whole list; parent grant + possible deny falls back and excludes the denied table; schema list short-circuits via a catalog-level grant; catalog list short-circuits via a metalake-level grant.TestJcasbinAuthorizer#testHasDenyPolicyOnType: detects an object-level deny for the matching type/privilege and ignores non-matching type/privilege.Full
:server-commontest suite passes;:server,:iceberg:iceberg-rest-server, and:corecompile clean.