Skip to content

feat: 7/7 align PostgreSQL catalog compatibility#25527

Open
tabVersion wants to merge 1 commit intoralph/rbac-split-06-set-role-session-semanticsfrom
ralph/rbac-split-07-pg-catalog-compatibility
Open

feat: 7/7 align PostgreSQL catalog compatibility#25527
tabVersion wants to merge 1 commit intoralph/rbac-split-06-set-role-session-semanticsfrom
ralph/rbac-split-07-pg-catalog-compatibility

Conversation

@tabVersion
Copy link
Copy Markdown
Contributor

Stack: 7/7 for splitting ralph/rbac-postgres-final-alignment onto origin/main.

Base: ralph/rbac-split-06-set-role-session-semantics

Scope

  • Add PostgreSQL-compatible RBAC catalog/system-function behavior for pg_auth_members, role/user catalog projections, visibility, and privilege functions.
  • Update planner snapshots and SLT coverage.

Final parity

The stack tip was verified against a synthetic rebase of the source branch diff onto origin/main:

  • git diff --exit-code HEAD..tmp/rbac-rebased-calibration
  • git diff --check origin/main...HEAD

Tests

  • cargo fmt --check
  • cargo check -p risingwave_frontend -p risingwave_meta

Not run

  • Runtime SLTs and planner runner were not run in this extraction pass.

Copy link
Copy Markdown

@capy-ai capy-ai Bot left a comment

Choose a reason for hiding this comment

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

Added 1 comment

Ok(user_catalog.check_privilege_with_grant_option(&object.object, actions))
let memberships = role_memberships_snapshot(role_membership_info_reader);
Ok(actions.iter().all(|(action, require_grant_option)| {
if *require_grant_option {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[🟠 High] [🔵 Bug]

The old code had an early-return if user_catalog.id == object.owner { return Ok(true); } that covered all privilege checks (including WITH GRANT OPTION). The new code splits the logic into two branches: when require_grant_option is true, it calls check_privilege_with_grant_option which only checks is_super (line 210 of user_catalog.rs) and iterates explicit grant_privileges — it does NOT check ownership. For a non-superuser owner of an object, has_table_privilege(owner_name, owned_table, 'SELECT WITH GRANT OPTION') now incorrectly returns false instead of true. In PostgreSQL, object owners implicitly have all privileges including grant option.

// has_privilege.rs, line 234-236
if *require_grant_option {
    user_catalog.check_privilege_with_grant_option(&object.object, &vec![(*action, true)])

The non-grant-option path correctly delegates to principal_has_privilege which checks ownership via has_inherited_privilege_for_catalog_user(user.id == owner), but the grant-option path bypasses this entirely. Fix by adding an ownership check before the grant-option branch, e.g.:

if user_catalog.id == object.owner {
    true
} else if *require_grant_option {
    user_catalog.check_privilege_with_grant_option(...)
} else {
    principal_has_privilege(...)
}

@tabVersion tabVersion changed the title rbac: align PostgreSQL catalog compatibility feat: 7/7 align PostgreSQL catalog compatibility Apr 28, 2026
@github-actions github-actions Bot added type/feature Type: New feature. and removed Invalid PR Title labels Apr 28, 2026
tabVersion added a commit that referenced this pull request May 8, 2026
Actual SELECT/UPDATE binding now uses the same role-membership inheritance helper as session privilege checks instead of checking only direct ACLs on the current role. This keeps runtime table access aligned with the effective privilege model introduced in the frontend role-dispatch layer.

Constraint: The fix belongs in PR #25525 because that stack layer owns frontend privilege checks, effective privilege helpers, and membership cache plumbing.

Rejected: Put this in #25527 catalog compatibility | that would leave actual query execution broken until the final catalog-only layer.

Confidence: high

Scope-risk: narrow

Directive: Keep pg_catalog/system-function compatibility in #25527; this commit only wires actual binder checks to effective privileges.

Tested: cargo fmt --check; git diff --check; cargo check -p risingwave_frontend; cargo test -p risingwave_frontend --test role_dispatch; ./risedev slt-clean './e2e_test/ddl/role_inherited_privilege.slt'; ./risedev slt './e2e_test/ddl/role_inherited_privilege.slt'

Not-tested: Full workspace CI

Co-authored-by: OmX <omx@oh-my-codex.dev>
@tabVersion tabVersion force-pushed the ralph/rbac-split-06-set-role-session-semantics branch from 8559414 to 0940e03 Compare May 8, 2026 10:17
This final stack layer exposes the PostgreSQL-compatible catalog and system-function behavior that depends on role membership, SET ROLE, and effective privilege semantics. It includes pg_auth_members, role/user catalog projections, visibility/privilege functions, planner snapshots, and SLT coverage updates.\n\nConstraint: Catalog visibility depends on the session role context and role membership reader added earlier in the stack.\nRejected: Land catalog snapshots before SET ROLE semantics | would make current_user and visibility behavior stale or misleading.\nConfidence: medium\nScope-risk: moderate\nTested: cargo fmt --check; cargo check -p risingwave_frontend -p risingwave_meta\nNot-tested: SLT runtime and planner snapshot runner in this extraction pass
@tabVersion tabVersion force-pushed the ralph/rbac-split-07-pg-catalog-compatibility branch from d908fb9 to 98cec57 Compare May 8, 2026 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant