From 745d2a60f0b91fddf20cdaad8d9d5da219d66b19 Mon Sep 17 00:00:00 2001 From: Kexiang Wang Date: Tue, 14 May 2024 01:01:48 +0800 Subject: [PATCH] fix(catalog): grant view when grant table (#16699) --- e2e_test/batch/catalog/has_privilege.slt.part | 42 +++++++++++++++++-- proto/user.proto | 2 +- src/common/src/acl/mod.rs | 2 +- src/frontend/src/handler/handle_privilege.rs | 35 +++++++++++----- src/frontend/src/user/user_catalog.rs | 1 - src/frontend/src/user/user_privilege.rs | 2 +- src/meta/service/src/user_service.rs | 23 +++++++++- src/meta/src/controller/catalog.rs | 15 ++++++- src/meta/src/controller/utils.rs | 3 +- src/meta/src/manager/catalog/database.rs | 8 ++++ src/meta/src/manager/catalog/mod.rs | 4 ++ 11 files changed, 116 insertions(+), 21 deletions(-) diff --git a/e2e_test/batch/catalog/has_privilege.slt.part b/e2e_test/batch/catalog/has_privilege.slt.part index 21c12bac45593..a742db0c51d6f 100644 --- a/e2e_test/batch/catalog/has_privilege.slt.part +++ b/e2e_test/batch/catalog/has_privilege.slt.part @@ -32,6 +32,9 @@ GRANT ALL PRIVILEGES ON foo TO test_user GRANTED BY root; statement ok GRANT INSERT ON bar TO test_user WITH GRANT OPTION GRANTED BY root; +statement ok +GRANT INSERT ON foo_view TO test_user WITH GRANT OPTION GRANTED BY root; + statement ok GRANT SELECT ON ALL TABLES IN SCHEMA public TO test_user WITH GRANT OPTION GRANTED BY root; @@ -44,6 +47,9 @@ GRANT SELECT ON ALL SOURCES IN SCHEMA public TO test_user WITH GRANT OPTION GRAN statement ok GRANT CREATE ON SCHEMA test_schema TO test_user; +query error table not found: bar_err +GRANT INSERT ON bar_err TO test_user WITH GRANT OPTION GRANTED BY root; + query error Invalid parameter user: User test_user_err not found SELECT has_table_privilege('test_user_err', 'foo', 'SELECT'); @@ -101,21 +107,25 @@ SELECT has_table_privilege('test_user', 'foo', 'DELETE WITH GRANT OPTION, INSERT ---- f -# FIXME(Kexiang): Currently, RW's grant privilege on all table doesn't apply to VIEWS. query I SELECT has_table_privilege('test_user', 'foo_view', 'SELECT'); ---- -f +t query I SELECT has_table_privilege('test_user', 'foo_view'::regclass, 'INSERT'); ---- +t + +query I +SELECT has_table_privilege('test_user', 'foo_view'::regclass, 'UPDATE'); +---- f query I SELECT has_any_column_privilege('test_user', 'foo_view'::regclass, 'INSERT'); ---- -f +t query I SELECT has_table_privilege('test_user', 'foo_mv', 'SELECT'); @@ -203,6 +213,32 @@ SELECT has_schema_privilege('test_user', 'test_schema', 'CREATE'); ---- t +statement ok +REVOKE SELECT ON ALL TABLES IN SCHEMA public FROM test_user GRANTED BY root; + +query I +SELECT has_table_privilege('test_user', 'bar'::regclass, 'SELECT'); +---- +f + +query I +SELECT has_table_privilege('test_user', 'foo_view', 'SELECT'); +---- +f + +query I +SELECT has_table_privilege('test_user', 'foo_view', 'INSERT'); +---- +t + +statement ok +REVOKE INSERT ON foo_view FROM test_user GRANTED BY root; + +query I +SELECT has_table_privilege('test_user', 'foo_view', 'INSERT'); +---- +f + statement ok DROP SOURCE foo_source; diff --git a/proto/user.proto b/proto/user.proto index 383e78cb57b28..6218fea4fd98d 100644 --- a/proto/user.proto +++ b/proto/user.proto @@ -66,7 +66,7 @@ message GrantPrivilege { uint32 all_tables_schema_id = 11; uint32 all_sources_schema_id = 12; - uint32 all_dml_tables_schema_id = 13; + uint32 all_dml_relations_schema_id = 13; uint32 subscription_id = 14; } repeated ActionWithGrantOption action_with_opts = 7; diff --git a/src/common/src/acl/mod.rs b/src/common/src/acl/mod.rs index 91f69fea51310..391ad0e5781f4 100644 --- a/src/common/src/acl/mod.rs +++ b/src/common/src/acl/mod.rs @@ -100,11 +100,11 @@ pub static ALL_AVAILABLE_DATABASE_MODES: LazyLock = LazyLock::new(|| make_bitflags!(AclMode::{Create | Connect}).into()); pub static ALL_AVAILABLE_SCHEMA_MODES: LazyLock = LazyLock::new(|| make_bitflags!(AclMode::{Create | Usage}).into()); +// Including TABLES and VIEWS pub static ALL_AVAILABLE_TABLE_MODES: LazyLock = LazyLock::new(|| make_bitflags!(AclMode::{Select | Insert | Update | Delete}).into()); pub static ALL_AVAILABLE_SOURCE_MODES: LazyLock = LazyLock::new(AclModeSet::readonly); pub static ALL_AVAILABLE_MVIEW_MODES: LazyLock = LazyLock::new(AclModeSet::readonly); -pub static ALL_AVAILABLE_VIEW_MODES: LazyLock = LazyLock::new(AclModeSet::readonly); pub static ALL_AVAILABLE_SINK_MODES: LazyLock = LazyLock::new(AclModeSet::empty); pub static ALL_AVAILABLE_SUBSCRIPTION_MODES: LazyLock = LazyLock::new(AclModeSet::empty); diff --git a/src/frontend/src/handler/handle_privilege.rs b/src/frontend/src/handler/handle_privilege.rs index 44e5b8fdac33c..e8a37b0ab407b 100644 --- a/src/frontend/src/handler/handle_privilege.rs +++ b/src/frontend/src/handler/handle_privilege.rs @@ -21,6 +21,7 @@ use super::RwPgResponse; use crate::binder::Binder; use crate::catalog::root_catalog::SchemaPath; use crate::catalog::table_catalog::TableType; +use crate::catalog::CatalogError; use crate::error::{ErrorCode, Result}; use crate::handler::HandlerArgs; use crate::session::SessionImpl; @@ -93,17 +94,31 @@ fn make_prost_privilege( Binder::resolve_schema_qualified_name(db_name, name)?; let schema_path = SchemaPath::new(schema_name.as_deref(), &search_path, user_name); - let (table, _) = reader.get_table_by_name(db_name, schema_path, &table_name)?; - match table.table_type() { - TableType::Table => {} - _ => { - return Err(ErrorCode::InvalidInputSyntax(format!( - "{table_name} is not a table", - )) - .into()); + match reader.get_table_by_name(db_name, schema_path, &table_name) { + Ok((table, _)) => { + match table.table_type() { + TableType::Table => { + grant_objs.push(PbObject::TableId(table.id().table_id)); + continue; + } + _ => { + return Err(ErrorCode::InvalidInputSyntax(format!( + "{table_name} is not a table", + )) + .into()); + } + }; + } + Err(CatalogError::NotFound("table", _)) => { + let (view, _) = reader + .get_view_by_name(db_name, schema_path, &table_name) + .map_err(|_| CatalogError::NotFound("table", table_name))?; + grant_objs.push(PbObject::ViewId(view.id)); + } + Err(e) => { + return Err(e.into()); } } - grant_objs.push(PbObject::TableId(table.id().table_id)); } } GrantObjects::Sources(sources) => { @@ -138,7 +153,7 @@ fn make_prost_privilege( for schema in schemas { let schema_name = Binder::resolve_schema_name(schema)?; let schema = reader.get_schema_by_name(session.database(), &schema_name)?; - grant_objs.push(PbObject::AllDmlTablesSchemaId(schema.id())); + grant_objs.push(PbObject::AllDmlRelationsSchemaId(schema.id())); } } o => { diff --git a/src/frontend/src/user/user_catalog.rs b/src/frontend/src/user/user_catalog.rs index 319d60fc56ae2..905348d37dfa6 100644 --- a/src/frontend/src/user/user_catalog.rs +++ b/src/frontend/src/user/user_catalog.rs @@ -189,7 +189,6 @@ impl UserCatalog { for (&key, found) in &mut action_map { let (required_action, required_grant_option) = *key; - if action == required_action && (!required_grant_option | with_grant_option) { *found = true; } diff --git a/src/frontend/src/user/user_privilege.rs b/src/frontend/src/user/user_privilege.rs index 2b474bda0554e..d4ad71e64120d 100644 --- a/src/frontend/src/user/user_privilege.rs +++ b/src/frontend/src/user/user_privilege.rs @@ -94,7 +94,7 @@ pub fn available_prost_privilege(object: PbObject, for_dml_table: bool) -> PbGra &acl::ALL_AVAILABLE_MVIEW_MODES } } - PbObject::ViewId(_) => &acl::ALL_AVAILABLE_VIEW_MODES, + PbObject::ViewId(_) => &acl::ALL_AVAILABLE_TABLE_MODES, PbObject::SinkId(_) => &acl::ALL_AVAILABLE_SINK_MODES, PbObject::SubscriptionId(_) => &acl::ALL_AVAILABLE_SUBSCRIPTION_MODES, PbObject::FunctionId(_) => &acl::ALL_AVAILABLE_FUNCTION_MODES, diff --git a/src/meta/service/src/user_service.rs b/src/meta/service/src/user_service.rs index d7dd4f5b4de4b..cab1b29228055 100644 --- a/src/meta/service/src/user_service.rs +++ b/src/meta/service/src/user_service.rs @@ -76,7 +76,7 @@ impl UserServiceImpl { } expanded_privileges.push(privilege); } - } else if let Some(Object::AllDmlTablesSchemaId(schema_id)) = &privilege.object { + } else if let Some(Object::AllDmlRelationsSchemaId(schema_id)) = &privilege.object { let tables = match &self.metadata_manager { MetadataManager::V1(mgr) => { mgr.catalog_manager.list_dml_table_ids(*schema_id).await @@ -89,6 +89,16 @@ impl UserServiceImpl { .map(|id| id as _) .collect(), }; + let views = match &self.metadata_manager { + MetadataManager::V1(mgr) => mgr.catalog_manager.list_view_ids(*schema_id).await, + MetadataManager::V2(mgr) => mgr + .catalog_controller + .list_view_ids(*schema_id as _) + .await? + .into_iter() + .map(|id| id as _) + .collect(), + }; for table_id in tables { let mut privilege = privilege.clone(); privilege.object = Some(Object::TableId(table_id)); @@ -100,6 +110,17 @@ impl UserServiceImpl { } expanded_privileges.push(privilege); } + for view_id in views { + let mut privilege = privilege.clone(); + privilege.object = Some(Object::ViewId(view_id)); + if let Some(true) = with_grant_option { + privilege + .action_with_opts + .iter_mut() + .for_each(|p| p.with_grant_option = true); + } + expanded_privileges.push(privilege); + } } else if let Some(Object::AllSourcesSchemaId(schema_id)) = &privilege.object { let sources = match &self.metadata_manager { MetadataManager::V1(mgr) => { diff --git a/src/meta/src/controller/catalog.rs b/src/meta/src/controller/catalog.rs index 036015dc13a72..1994e9ecac7b7 100644 --- a/src/meta/src/controller/catalog.rs +++ b/src/meta/src/controller/catalog.rs @@ -31,7 +31,7 @@ use risingwave_meta_model_v2::{ ActorUpstreamActors, ColumnCatalogArray, ConnectionId, CreateType, DatabaseId, FragmentId, FunctionId, I32Array, IndexId, JobStatus, ObjectId, PrivateLinkService, Property, SchemaId, SinkId, SourceId, StreamNode, StreamSourceInfo, StreamingParallelism, SubscriptionId, TableId, - UserId, + UserId, ViewId, }; use risingwave_pb::catalog::subscription::SubscriptionState; use risingwave_pb::catalog::table::PbTableType; @@ -2423,6 +2423,19 @@ impl CatalogController { Ok(table_ids) } + pub async fn list_view_ids(&self, schema_id: SchemaId) -> MetaResult> { + let inner = self.inner.read().await; + let view_ids: Vec = View::find() + .select_only() + .column(view::Column::ViewId) + .join(JoinType::InnerJoin, view::Relation::Object.def()) + .filter(object::Column::SchemaId.eq(schema_id)) + .into_tuple() + .all(&inner.db) + .await?; + Ok(view_ids) + } + pub async fn list_tables_by_type(&self, table_type: TableType) -> MetaResult> { let inner = self.inner.read().await; let table_objs = Table::find() diff --git a/src/meta/src/controller/utils.rs b/src/meta/src/controller/utils.rs index 42ccc97e6637f..7d8f4769e8264 100644 --- a/src/meta/src/controller/utils.rs +++ b/src/meta/src/controller/utils.rs @@ -754,12 +754,11 @@ where let obj = match object.obj_type { ObjectType::Database => PbObject::DatabaseId(oid), ObjectType::Schema => PbObject::SchemaId(oid), - ObjectType::Table => PbObject::TableId(oid), + ObjectType::Table | ObjectType::Index => PbObject::TableId(oid), ObjectType::Source => PbObject::SourceId(oid), ObjectType::Sink => PbObject::SinkId(oid), ObjectType::View => PbObject::ViewId(oid), ObjectType::Function => PbObject::FunctionId(oid), - ObjectType::Index => unreachable!("index is not supported yet"), ObjectType::Connection => unreachable!("connection is not supported yet"), ObjectType::Subscription => PbObject::SubscriptionId(oid), }; diff --git a/src/meta/src/manager/catalog/database.rs b/src/meta/src/manager/catalog/database.rs index ed128ee3e1ecb..00d54267861c8 100644 --- a/src/meta/src/manager/catalog/database.rs +++ b/src/meta/src/manager/catalog/database.rs @@ -359,6 +359,14 @@ impl DatabaseManager { .collect_vec() } + pub fn list_view_ids(&self, schema_id: SchemaId) -> Vec { + self.views + .values() + .filter(|view| view.schema_id == schema_id) + .map(|view| view.id) + .collect_vec() + } + pub fn list_sources(&self) -> Vec { self.sources.values().cloned().collect_vec() } diff --git a/src/meta/src/manager/catalog/mod.rs b/src/meta/src/manager/catalog/mod.rs index 665eb45fc3c37..e1c2e1cb2d1d4 100644 --- a/src/meta/src/manager/catalog/mod.rs +++ b/src/meta/src/manager/catalog/mod.rs @@ -3604,6 +3604,10 @@ impl CatalogManager { .list_dml_table_ids(schema_id) } + pub async fn list_view_ids(&self, schema_id: SchemaId) -> Vec { + self.core.lock().await.database.list_view_ids(schema_id) + } + pub async fn list_sources(&self) -> Vec { self.core.lock().await.database.list_sources() }