fix(frontend): scope table and view grant option checks - #24755
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
matrix-meow
added
size/L
This reverts commit ffa9d85.
When the target table/view of a scoped grant does not resolve, the WGO authorization check degraded to an obj_type-only lookup, so any unrelated same-type grant option satisfied GRANT ... ON db.missing_obj and the failure surfaced later as "no such table/view" instead of an authorization error. Fall back instead to only the object-id-independent scopes: the global *.* scope always, plus the database-wide db.* scope when the grant targets a table and the database itself still resolves. This keeps holders of global or db-wide grant option authorized while preventing grants on unrelated specific objects from satisfying the check. The success path reuses the same helper, leaving its SQL sequence unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Thorough privilege scoping fix with excellent test coverage (1586 lines). A few concerns:
Correctness concerns
1. VALUES privilege ownership promotion asymmetric between scoped/unscoped paths (authenticate.go). In the new scoped code, PrivilegeTypeValues sits in the same switch arm as Select/Insert/Update..., calling ownership-fallback SQL. In the old unscoped path, getSqlForCheckRoleHasPrivilegeWGODependsOnPrivType(PrivilegeTypeValues) returns only getSqlForCheckRoleHasPrivilegeWGO — no ownership fallback. Net effect: GRANT VALUES ON db.t checks TableOwnership (new behavior), but GRANT VALUES ON *.* does not (falls through to old unscoped path). If this promotion-of-VALUES is intended, it should apply uniformly.
2. determineUserCanGrantPrivilegesToOthers returns true when gp.Privileges is empty (authenticate.go:8166-8199). The loop over gp.Privileges is a no-op for an empty slice, so ret stays true. An empty-privileges GRANT statement would silently pass. While the parser should reject this upstream, worth a defensive len(gp.Privileges) == 0 check for consistency with the existing gp.Level == nil guard.
Hardening
3. build_constraint_util.go duplicates orderedDmlAliases helper. The new manual aliasByIdx array construction does the same thing as the existing orderedDmlAliases (line 255). Use the existing helper.
4. PrivilegeTypeExecute excluded from scoped checks — intentional but subtle. Execute.Scope() returns PrivilegeScopeTable, but getRoleSetThatPrivilegeGrantedToWGOScoped early-outs for non-TABLE/VIEW object types, so Execute always falls through to the unscoped path. This is correct today because Execute maps to OBJECT_TYPE_FUNCTION/PROCEDURE, but a comment documenting this dependency on the early-out would help future maintainers.
Scope concern (pre-existing, not a regression)
5. *.* account-level ownership doesn't authorize *.* table-level grants. The scoped path for *.* only queries obj_type = "table" rows. AccountOwnership (obj_type = "account") is never checked. The unscoped fallback similarly checks TableAll/TableOwnership, not AccountAll/AccountOwnership. This matches pre-existing behavior — worth documenting as a design constraint.
Verified
- Test coverage (1586 lines) is thorough: positive/negative for
*.*coversd.*,d.*coversd.t, cross-database isolation, view legacy paths, nil/empty role-set merging, error propagation.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Upgrading to approve. The findings from my prior comment are non-blocking:
- VALUES privilege ownership asymmetry between scoped/unscoped paths is a design inconsistency, not a correctness bug. If it matters, it'll surface in a dedicated VALUES-grant test.
- Empty-privileges defensive check — parser rejects empty privileges upstream, so unreachable in practice.
- Duplicated
orderedDmlAliasespattern, Execute exclusion subtlety, and*.*account-ownership scope are hardening/design notes.
Test coverage is excellent (1586 lines). LGTM.
The fallback UPDATE builder appended each update expression while ranging over the updateKeys map, so the projected column layout depended on Go's randomized map iteration. Iterate table columns in definition order instead, which keeps the appended update-expression positions stable across plan builds. Also harden TestUpdateFallbackGeneratedColumnChainUsesFreshExpr so it no longer asserts on a fixed slot: it now checks emp's appended region as a whole — both generated columns must be freshly recomputed to empno and none may reference the stale mgr column — making it order-independent and immune to map-iteration flakiness. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
I reviewed this from three angles: correctness, runtime/impact, and test coverage.
There is one blocking correctness issue in the new VIEW compatibility path:
getRoleSetThatPrivilegeGrantedToWGOScoped()now checks bothobjectTypeViewand legacyobjectTypeTablerecords for VIEW targets. That is fine for exact-object legacy rows, but it is not safe for broader scopes likedb.*and*.*: at those scopes, a real TABLE grant-option row and a legacy VIEW grant-option row are indistinguishable inmo_role_privs(obj_type = table, sameobj_id, same privilege level). So a genuine TABLEWITH GRANT OPTIONondb.*or*.*can still authorize re-granting on VIEW targets.
That means the table/view grant-option leak is still present for database/global scopes, even though this PR is trying to scope table and view checks separately. The added test grant privilege [ObjectType: View] database scoped view grant succeeds on legacy table record only demonstrates exactly this behavior.
I think the legacy fallback needs to be narrowed so it does not re-open cross-type authorization at broad scopes (for example, only allowing legacy exact-object view rows, or introducing a migration/marker that can distinguish legacy VIEW rows from actual TABLE rows before consulting them).
Separately, the frontend side would still benefit from an end-to-end BVT that covers object-scoped WITH GRANT OPTION isolation (db1.t1 must not authorize re-grant on db1.t2 / db2.t2), but the main reason for request-changes is the correctness issue above.
|
Follow-up after the longer logic review: there is another blocking correctness issue.
This should be scoped by database object as well, while still allowing the intended broader coverage from |
fengttt
left a comment
fengttt
left a comment
There was a problem hiding this comment.
This kind of change need a bvt test.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed from three angles:
- correctness: grant-option resolution is now scoped by object type/object identity and the final logic keeps exact legacy-view compatibility without letting table wildcard grants authorize view grants
- performance: the extra catalog lookups are limited to GRANT authorization paths and are acceptable for this control-flow
- test coverage: unit tests plus the tenant privilege BVT cover the key regressions and wildcard/legacy edge cases
Looks good to merge.
Merge Queue Status
This pull request spent 53 minutes 15 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonThe merge conditions cannot be satisfied due to failing checks HintYou may have to fix your CI before adding the pull request to the queue again. |
Merge Queue Status
This pull request spent 2 minutes 2 seconds in the queue, including 16 seconds running CI. Required conditions to merge
|
6 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #24713
What this PR does / why we need it:
This PR scopes grant-option checks for table/view privileges by object type and object id. It prevents table/view grants and database-scoped grants from leaking across unrelated objects while still allowing global
*.* WITH GRANT OPTIONto cover narrower grant targets.It also preserves compatibility for view grants that may be stored under the legacy table object type, and adds focused frontend coverage for global wildcard coverage, cross-database isolation, and view-level grant-option checks.
Validation:
go test ./pkg/frontend -run Test_determineGrantPrivilege -count=1git diff --checkmake && make static-check