fix(frontend): align role rule merge semantics#24929
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? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
I found two blocking regressions in the frontend changes.
-
pkg/frontend/rewrite_rule.go:428-446narrows mergeable rewrite rules to bare column /*projections only. That makes identical row-preserving projections likeselect a + 1 as b from db1.t1 where ...stop merging, so the later role silently overwrites the earlier one instead of unioning both roles' visibility. The new expectation inpkg/frontend/rewrite_rule_test.go:777-779already demonstrates this regression. -
pkg/frontend/authenticate.go:3503-3531stops rebinding the current role duringSET SECONDARY ROLE ALL, but the privilege/rule loaders still seed fromtenant.GetDefaultRoleID()unconditionally (pkg/frontend/rewrite_rule.go:223-239,pkg/frontend/authenticate.go:7273-7287,8688-8704). If the session's current primary role was revoked after login,SET SECONDARY ROLE ALLnow keeps that stale role active and reloads caches with it, which is a privilege-revocation regression. Please either validate/fallback the primary role here or stop treatingDefaultRoleIDas implicitly valid downstream.
What type of PR is this?
Which issue(s) this PR fixes:
issue #23271
What this PR does / why we need it:
This PR ports the latest role rewrite-rule and SET ROLE behavior from #24792 to the current branch.
Changes:
WHERE (...) OR (...)instead of usingUNION DISTINCT.SET SECONDARY ROLE ALL; it only enables secondary roles.SET ROLE, switch the current role to the target role and invalidate the privilege/rule cache so subsequent privilege checks use the new role.Validation:
gofmt -w pkg/frontend/authenticate.go pkg/frontend/authenticate_test.go pkg/frontend/rewrite_rule.go pkg/frontend/rewrite_rule_test.gogit diff --checkGOFLAGS=-mod=readonly go test ./pkg/frontend -run 'TestMergeRewriteRules|TestDoSetSecondaryRoleAll|TestDoSwitchRole|TestRewriteRule'cannot complete in this local environment because CGO dependencies are missing headers:usearch.handxxhash.h.