feat: support grant/revoke privilege on view #20670
Conversation
There was a problem hiding this comment.
PR Overview
This PR adds support for granting/revoking privileges on views and refactors various privilege checks across the binder, handler, and catalog modules. Key changes include:
- Adding privilege checks for views during binding.
- Standardizing privilege verification by replacing direct check calls with has_privilege where appropriate.
- Updating catalog and proto definitions to align with the new privilege management requirements.
Reviewed Changes
| File | Description |
|---|---|
| src/frontend/src/binder/relation/table_or_source.rs | Updated privilege checks for sources, tables, and views; added new check_privilege for view relations. |
| src/frontend/src/handler/comment.rs | Added additional ownership check after binding a table in comment-related requests. |
| src/frontend/src/binder/insert.rs | Added privilege checks for insert operations using the new check_privilege. |
| src/frontend/src/binder/delete.rs | Updated delete binding to check for privileges before proceeding. |
| src/frontend/src/catalog/system_catalog/mod.rs | Adjusted sys view creation to use non-Arc types and updated schema fields. |
| src/frontend/src/binder/update.rs | Updated update binding to perform privilege checks (note potential AclMode mismatch). |
| src/frontend/src/handler/handle_privilege.rs | Removed redundant relation privilege resolution code. |
| src/frontend/src/expr/function_impl/pg_table_is_visible.rs | Replaced check_privilege with has_privilege for schema access checks. |
| Other files (create_index, create_mv, create_sink, alter_rename, alter_owner, etc.) | Consistent updates to privilege check calls and minor refactoring for clarity. |
| proto/user.proto | Updated field numbers in GrantPrivilege message for compatibility. |
Copilot reviewed 29 out of 29 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/frontend/src/binder/update.rs:130
- The privilege check in the update binder is using AclMode::Insert instead of the expected AclMode::Update. Consider changing it to AclMode::Update to properly reflect the update operation.
self.check_privilege(PbObject::TableId(table_catalog.id.table_id), table_catalog.database_id, AclMode::Insert, table_catalog.owner, )?;
|
Hi, there. 📝 Telemetry Reminder:
|
| let bound_table = self.bind_table(schema_name.as_deref(), &table_name)?; | ||
| let table_catalog = &bound_table.table_catalog; | ||
| Self::check_for_dml(table_catalog, true)?; | ||
| self.check_privilege( | ||
| PbObject::TableId(table_catalog.id.table_id), | ||
| table_catalog.database_id, | ||
| AclMode::Insert, | ||
| table_catalog.owner, | ||
| )?; |
There was a problem hiding this comment.
So we do not have privilege check for DML in the past?
There was a problem hiding this comment.
Yes, the check was performed after the binding process was completed before this PR, which is prone to errors.
| BindFor::Ddl | BindFor::System => { | ||
| // do nothing. | ||
| } |
There was a problem hiding this comment.
In which case this branch will be processed?
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Resolved #6164 .
Fixed #8864 .
Checklist
Documentation
Release note
support view privilege management.