Skip to content

fix(frontend): align role rule merge semantics#24929

Open
ouyuanning wants to merge 3 commits into
matrixorigin:mainfrom
ouyuanning:improve-mo-from-24792
Open

fix(frontend): align role rule merge semantics#24929
ouyuanning wants to merge 3 commits into
matrixorigin:mainfrom
ouyuanning:improve-mo-from-24792

Conversation

@ouyuanning

@ouyuanning ouyuanning commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

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:

  • Merge same-name role rewrite rules by combining compatible simple SELECT filters with WHERE (...) OR (...) instead of using UNION DISTINCT.
  • Keep the current primary role unchanged for SET SECONDARY ROLE ALL; it only enables secondary roles.
  • After SET ROLE, switch the current role to the target role and invalidate the privilege/rule cache so subsequent privilege checks use the new role.
  • Update frontend unit tests and distributed access-control cases for the new behavior.

Validation:

  • gofmt -w pkg/frontend/authenticate.go pkg/frontend/authenticate_test.go pkg/frontend/rewrite_rule.go pkg/frontend/rewrite_rule_test.go
  • git diff --check
  • GOFLAGS=-mod=readonly go test ./pkg/frontend -run 'TestMergeRewriteRules|TestDoSetSecondaryRoleAll|TestDoSwitchRole|TestRewriteRule' cannot complete in this local environment because CGO dependencies are missing headers: usearch.h and xxhash.h.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@ouyuanning ouyuanning changed the title fix(frontend): improve role rule merge and set role behavior fix(frontend): align role rule merge semantics Jun 11, 2026

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I found two blocking regressions in the frontend changes.

  1. pkg/frontend/rewrite_rule.go:428-446 narrows mergeable rewrite rules to bare column / * projections only. That makes identical row-preserving projections like select 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 in pkg/frontend/rewrite_rule_test.go:777-779 already demonstrates this regression.

  2. pkg/frontend/authenticate.go:3503-3531 stops rebinding the current role during SET SECONDARY ROLE ALL, but the privilege/rule loaders still seed from tenant.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 ALL now 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 treating DefaultRoleID as implicitly valid downstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement size/L Denotes a PR that changes [500,999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants