Skip to content

feat: 2/7 add role membership read contract#25522

Open
tabVersion wants to merge 1 commit intoralph/rbac-split-01-parser-surfacefrom
ralph/rbac-split-02-role-membership-read-contract
Open

feat: 2/7 add role membership read contract#25522
tabVersion wants to merge 1 commit intoralph/rbac-split-01-parser-surfacefrom
ralph/rbac-split-02-role-membership-read-contract

Conversation

@tabVersion
Copy link
Copy Markdown
Contributor

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

Base: ralph/rbac-split-01-parser-surface
Next: ralph/rbac-split-03-meta-grant-authority

Scope

  • Add the mostly read/data role membership contract: RoleMembership, ListRoleMemberships, migrations/entity, backup/restore parity, and client/service list passthrough.
  • Add compile/default-semantic slices forced by UserInfo.can_inherit / UpdateField::INHERIT, including default can_inherit = true for created users/fixtures.

Out of scope

  • GrantRole and RevokeRole RPCs and behavior.
  • Frontend role dispatch and pg_catalog compatibility.

Tests

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

@tabVersion tabVersion changed the title rbac: add role membership read contract feat: 2/7 add role membership read contract Apr 28, 2026
@github-actions github-actions Bot added type/feature Type: New feature. and removed Invalid PR Title labels Apr 28, 2026
Copy link
Copy Markdown
Contributor Author

Heads up for the stacked branch: #25521 now includes parser fix 6a8bbd3ca8 for privilege-keyword role names (connect, create, select, usage) in role-membership GRANT / REVOKE.

Because this PR is based on the previous #25521 head and also touches src/sqlparser/src/parser.rs, please rebase/carry that parser fix before merging this step; otherwise the stack can reintroduce the old first-token dispatch behavior. While rebasing, also keep the parse_role_option_for_clause lookahead that lets REVOKE admin FROM user_a / REVOKE inherit FROM user_a / REVOKE set FROM user_a parse as role names when they are not followed by OPTION FOR.

This PR adds the storage and read API contract needed before role grant and revoke behavior can become authoritative. It keeps grant and revoke RPCs out of the contract layer while adding the compile-coupled inherit flag and list membership surfaces.\n\nConstraint: Generated service traits require every added RPC to compile through meta service, RPC client, frontend client, and mocks.\nRejected: Add GrantRole and RevokeRole RPCs in this contract PR | would require behavior stubs before the authoritative controller implementation lands.\nConfidence: high\nScope-risk: moderate\nDirective: Keep write behavior out of this PR; grant and revoke RPCs belong with their meta implementations.\nTested: cargo fmt; cargo fmt --check; cargo check -p risingwave_meta -p risingwave_rpc_client -p risingwave_frontend\nNot-tested: Runtime migration/restore path against a live cluster
@tabVersion tabVersion force-pushed the ralph/rbac-split-02-role-membership-read-contract branch from 32fd4ac to cb6b850 Compare May 4, 2026 13:12
Copy link
Copy Markdown
Contributor Author

@tabVersion tabVersion left a comment

Choose a reason for hiding this comment

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

Stack-aware RBAC review after rebasing this PR onto #25521 head 69f15214e2: the new role-membership read contract mostly matches the intended 2/7 boundary, but I found one default-inheritance edge that should be fixed before this slice becomes the base for the grant-authority PR.

can_create_user: Set(user.can_create_user),
can_login: Set(user.can_login),
is_admin: Set(user.is_admin),
can_inherit: Set(user.can_inherit),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because can_inherit is a proto3 bool, any caller that builds PbUserInfo { name, ..Default::default() } now persists NOINHERIT through this conversion. The frontend create path sets can_inherit: true, but the meta/controller test fixture still uses that default shape, and downstream grant semantics use the member's can_inherit as the default for GRANT role TO member when WITH INHERIT is omitted. That means direct meta-created test users silently get inherit_option = false, which diverges from the PostgreSQL/default contract this PR is introducing. Please update the server-side role/user fixtures or constructors to set the default can_inherit: true and add/keep a regression around an omitted-WITH INHERIT role grant.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces the read-side plumbing for role memberships (storage model, snapshot/restore, meta RPC/service/client passthrough) and extends user metadata with a new can_inherit flag that can be updated via UpdateUserRequest.

Changes:

  • Add user_role_membership SeaORM entity + migration, and include it in meta snapshot v2 backup/restore flows.
  • Add ListRoleMemberships RPC end-to-end (meta service + controller + rpc_client + frontend meta client trait passthrough).
  • Add can_inherit to UserInfo plus update semantics (UpdateField::INHERIT) and default initialization in frontend/test fixtures.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/storage/backup/src/meta_snapshot_v2.rs Adds user_role_memberships to meta snapshot v2 model list.
src/rpc_client/src/meta_client.rs Adds RPC client method + macro registration for ListRoleMemberships.
src/meta/src/controller/user.rs Adds controller query for listing role memberships; adds INHERIT update handling.
src/meta/src/backup_restore/restore_impl/v2.rs Restores role membership rows and updates auto-increment tracking.
src/meta/service/src/user_service.rs Implements ListRoleMemberships RPC handler.
src/meta/model/src/user.rs Adds can_inherit to user model/protobuf conversions.
src/meta/model/src/user_role_membership.rs Introduces SeaORM entity for user_role_membership + Pb conversion.
src/meta/model/src/prelude.rs Re-exports UserRoleMembership entity.
src/meta/model/src/lib.rs Exposes module and RoleMembershipId type alias.
src/meta/model/migration/src/m20260422_000001_role_membership.rs Creates user_role_membership table + uniqueness index + FKs.
src/meta/model/migration/src/m20260422_000002_user_inherit_flag.rs Adds user.can_inherit column with default true.
src/meta/model/migration/src/lib.rs Registers the two new migrations.
src/frontend/src/user/user_catalog.rs Propagates can_inherit into frontend user catalog representation.
src/frontend/src/test_utils.rs Updates mock user update handling and default fixture users with can_inherit = true.
src/frontend/src/meta_client.rs Adds list_role_memberships to frontend meta client trait + impl.
src/frontend/src/handler/create_user.rs Defaults created users to can_inherit = true.
proto/user.proto Adds can_inherit, UpdateField::INHERIT, and role membership messages + RPC.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread proto/user.proto
repeated GrantPrivilege grant_privileges = 8;

bool is_admin = 9;
bool can_inherit = 10;
Comment thread proto/user.proto
Comment on lines +141 to +153
message RoleMembership {
uint32 role_id = 1;
uint32 member_id = 2;
uint32 granted_by = 3;
bool admin_option = 4;
bool inherit_option = 5;
bool set_option = 6;
uint32 id = 7;
}

message ListRoleMembershipsRequest {
repeated uint32 member_ids = 1;
}
Comment on lines 53 to 66
impl From<PbUserInfo> for ActiveModel {
fn from(user: PbUserInfo) -> Self {
let user_id = if user.id == 0 { NotSet } else { Set(user.id) };
Self {
user_id,
name: Set(user.name),
is_super: Set(user.is_super),
can_create_db: Set(user.can_create_db),
can_create_user: Set(user.can_create_user),
can_login: Set(user.can_login),
is_admin: Set(user.is_admin),
can_inherit: Set(user.can_inherit),
auth_info: Set(user.auth_info.as_ref().map(AuthInfo::from)),
}
Comment on lines +152 to +160
let inner = self.inner.read().await;
let memberships = if member_ids.is_empty() {
UserRoleMembership::find().all(&inner.db).await?
} else {
UserRoleMembership::find()
.filter(user_role_membership::Column::MemberId.is_in(member_ids.iter().copied()))
.all(&inner.db)
.await?
};
Comment on lines +148 to +162
pub async fn list_role_memberships(
&self,
member_ids: &[UserId],
) -> MetaResult<Vec<PbRoleMembership>> {
let inner = self.inner.read().await;
let memberships = if member_ids.is_empty() {
UserRoleMembership::find().all(&inner.db).await?
} else {
UserRoleMembership::find()
.filter(user_role_membership::Column::MemberId.is_in(member_ids.iter().copied()))
.all(&inner.db)
.await?
};
Ok(memberships.into_iter().map(Into::into).collect())
}
Comment on lines +80 to +90
manager
.create_index(
Index::create()
.name("idx_user_role_membership_item")
.table(UserRoleMembership::Table)
.unique()
.col(UserRoleMembership::RoleId)
.col(UserRoleMembership::MemberId)
.col(UserRoleMembership::GrantedBy)
.to_owned(),
)
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.

2 participants