feat(rbac): User only support select or drop system_history (#18281)

TCeason · web-flow · commit c8938606640e · 2025-07-03T12:12:54.000+08:00
* feat(rbac): User only support select or drop system_history

* add select privilege test
diff --git a/src/meta/app/src/principal/mod.rs b/src/meta/app/src/principal/mod.rs
@@ -103,6 +103,7 @@ pub use user_defined_function::UserDefinedFunction;
 pub use user_grant::GrantEntry;
 pub use user_grant::GrantObject;
 pub use user_grant::UserGrantSet;
+pub use user_grant::SENSITIVE_SYSTEM_RESOURCE;
 pub use user_grant::SYSTEM_TABLES_ALLOW_LIST;
 pub use user_identity::UserIdentity;
 pub use user_info::UserInfo;
diff --git a/src/meta/app/src/principal/user_grant.rs b/src/meta/app/src/principal/user_grant.rs
@@ -48,6 +48,8 @@ pub const SYSTEM_TABLES_ALLOW_LIST: [&str; 21] = [
     "indexes",
 ];
 
+pub const SENSITIVE_SYSTEM_RESOURCE: &str = "system_history";
+
 #[derive(serde::Serialize, serde::Deserialize, Clone, Debug, Eq, PartialEq, Hash)]
 pub enum GrantObject {
     Global,
diff --git a/src/query/service/src/history_tables/alter_table.rs b/src/query/service/src/history_tables/alter_table.rs
@@ -178,7 +178,7 @@ mod tests {
 
         #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
         async fn test_get_alter_table_sql() -> Result<(), ErrorCode> {
-            let test_fixture = TestFixture::setup().await?;
+            let test_fixture = TestFixture::setup_with_history_log().await?;
             let ctx = test_fixture.new_query_ctx().await?;
             let create_db = "CREATE DATABASE system_history";
             let create_table = "CREATE TABLE system_history.test_table_alter (id INT, name String)";
@@ -214,7 +214,7 @@ mod tests {
             // - The node with the older schema version is restarted.
             // - Upon restart, the older node should not attempt to alter the table again.
 
-            let test_fixture = TestFixture::setup().await?;
+            let test_fixture = TestFixture::setup_with_history_log().await?;
             let ctx = test_fixture.new_query_ctx().await?;
             let create_db = "CREATE DATABASE system_history";
             let create_table =
@@ -246,7 +246,7 @@ mod tests {
         #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
         async fn test_should_reset_table_not_exists() -> Result<(), ErrorCode> {
             // Test: When log_history table doesn't exist, should_reset should return false
-            let test_fixture = TestFixture::setup().await?;
+            let test_fixture = TestFixture::setup_with_history_log().await?;
             let ctx = test_fixture.new_query_ctx().await?;
 
             // Don't create the table, it should not exist
@@ -260,7 +260,7 @@ mod tests {
         #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
         async fn test_should_reset_both_inner() -> Result<(), ErrorCode> {
             // Test: When log_history table is internal, config also internal, should_reset should return false
-            let test_fixture = TestFixture::setup().await?;
+            let test_fixture = TestFixture::setup_with_history_log().await?;
             let ctx = test_fixture.new_query_ctx().await?;
 
             // Create system_history database and internal log_history table
@@ -279,7 +279,7 @@ mod tests {
         #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
         async fn test_should_reset_inner_to_external() -> Result<(), ErrorCode> {
             // Test Condition: Internal -> External conversion should trigger reset
-            let test_fixture = TestFixture::setup().await?;
+            let test_fixture = TestFixture::setup_with_history_log().await?;
             let ctx = test_fixture.new_query_ctx().await?;
 
             // Create system_history database and internal log_history table
diff --git a/src/query/service/src/history_tables/mod.rs b/src/query/service/src/history_tables/mod.rs
@@ -16,6 +16,6 @@ mod alter_table;
 mod external;
 mod global_history_log;
 mod meta;
-mod session;
+pub mod session;
 
 pub use global_history_log::GlobalHistoryLog;
diff --git a/src/query/service/src/history_tables/session.rs b/src/query/service/src/history_tables/session.rs
@@ -16,25 +16,18 @@ use std::sync::Arc;
 
 use databend_common_catalog::session_type::SessionType;
 use databend_common_exception::Result;
-use databend_common_meta_app::principal::GrantObject;
 use databend_common_meta_app::principal::UserInfo;
-use databend_common_meta_app::principal::UserPrivilegeType;
 use databend_common_users::BUILTIN_ROLE_ACCOUNT_ADMIN;
 
 use crate::sessions::Session;
 use crate::sessions::SessionManager;
 
 /// Create a user for history log with necessary privileges
 pub fn get_history_log_user(tenant_id: &str, cluster_id: &str) -> UserInfo {
-    let mut user = UserInfo::new_no_auth(
+    UserInfo::new_no_auth(
         format!("{}-{}-history-log", tenant_id, cluster_id).as_str(),
         "0.0.0.0",
-    );
-    user.grants.grant_privileges(
-        &GrantObject::Global,
-        UserPrivilegeType::CreateDatabase.into(),
-    );
-    user
+    )
 }
 
 /// Create a dummy session for history log
diff --git a/src/query/service/src/interpreters/access/privilege_access.rs b/src/query/service/src/interpreters/access/privilege_access.rs
@@ -17,8 +17,10 @@ use std::sync::Arc;
 
 use databend_common_base::base::GlobalInstance;
 use databend_common_catalog::catalog::Catalog;
+use databend_common_catalog::catalog::CATALOG_DEFAULT;
 use databend_common_catalog::plan::DataSourceInfo;
 use databend_common_catalog::table_context::TableContext;
+use databend_common_config::GlobalConfig;
 use databend_common_exception::ErrorCode;
 use databend_common_exception::Result;
 use databend_common_management::RoleApi;
@@ -31,6 +33,7 @@ use databend_common_meta_app::principal::StageType;
 use databend_common_meta_app::principal::UserGrantSet;
 use databend_common_meta_app::principal::UserPrivilegeSet;
 use databend_common_meta_app::principal::UserPrivilegeType;
+use databend_common_meta_app::principal::SENSITIVE_SYSTEM_RESOURCE;
 use databend_common_meta_app::principal::SYSTEM_TABLES_ALLOW_LIST;
 use databend_common_meta_app::tenant::Tenant;
 use databend_common_meta_types::seq_value::SeqV;
@@ -46,6 +49,7 @@ use databend_common_users::UserApiProvider;
 use databend_enterprise_resources_management::ResourcesManagement;
 use databend_storages_common_table_meta::table::OPT_KEY_TEMP_PREFIX;
 
+use crate::history_tables::session::get_history_log_user;
 use crate::interpreters::access::AccessChecker;
 use crate::sessions::QueryContext;
 use crate::sessions::Session;
@@ -159,13 +163,44 @@ impl PrivilegeAccess {
         Ok(Some(object))
     }
 
+    fn access_system_history(
+        &self,
+        catalog_name: &str,
+        db_name: &str,
+        privilege: UserPrivilegeType,
+    ) -> Result<()> {
+        let cluster_id = GlobalConfig::instance().query.cluster_id.clone();
+        let tenant_id = GlobalConfig::instance().query.tenant_id.clone();
+        if get_history_log_user(tenant_id.tenant_name(), &cluster_id).identity()
+            == self.ctx.get_current_user()?.identity()
+        {
+            return Ok(());
+        }
+        if catalog_name == CATALOG_DEFAULT
+            && db_name.eq_ignore_ascii_case(SENSITIVE_SYSTEM_RESOURCE)
+            && !matches!(
+                privilege,
+                UserPrivilegeType::Select | UserPrivilegeType::Drop
+            )
+        {
+            return Err(ErrorCode::PermissionDenied(
+                format!(
+                    "Permission Denied: Operation '{:?}' on database 'default.system_history' is not allowed. This sensitive system resource only supports 'SELECT' and 'DROP'",
+                    privilege
+                ),
+            ));
+        }
+        Ok(())
+    }
+
     async fn validate_db_access(
         &self,
         catalog_name: &str,
         db_name: &str,
         privileges: UserPrivilegeType,
         if_exists: bool,
     ) -> Result<()> {
+        self.access_system_history(catalog_name, db_name, privileges)?;
         let tenant = self.ctx.get_tenant();
         let check_current_role_only = match privileges {
             // create table/stream need check db's Create Privilege
@@ -270,6 +305,8 @@ impl PrivilegeAccess {
             return Ok(());
         }
 
+        self.access_system_history(catalog_name, db_name, privilege)?;
+
         if self.ctx.is_temp_table(catalog_name, db_name, table_name) {
             return Ok(());
         }
diff --git a/src/query/service/src/interpreters/interpreter_privilege_grant.rs b/src/query/service/src/interpreters/interpreter_privilege_grant.rs
@@ -32,6 +32,7 @@ use log::error;
 use log::info;
 
 use crate::interpreters::common::validate_grant_object_exists;
+use crate::interpreters::util::check_system_history;
 use crate::interpreters::Interpreter;
 use crate::pipelines::PipelineBuildResult;
 use crate::sessions::QueryContext;
@@ -58,6 +59,7 @@ impl GrantPrivilegeInterpreter {
             GrantObject::Database(_, db_name) => {
                 let catalog_name = catalog_name.unwrap();
                 let catalog = self.ctx.get_catalog(&catalog_name).await?;
+                check_system_history(&catalog, db_name)?;
                 let db_id = catalog
                     .get_database(tenant, db_name)
                     .await?
@@ -72,6 +74,7 @@ impl GrantPrivilegeInterpreter {
             GrantObject::Table(_, db_name, table_name) => {
                 let catalog_name = catalog_name.unwrap();
                 let catalog = self.ctx.get_catalog(&catalog_name).await?;
+                check_system_history(&catalog, db_name)?;
                 let db_id = catalog
                     .get_database(tenant, db_name)
                     .await?
@@ -88,15 +91,25 @@ impl GrantPrivilegeInterpreter {
                     table_id,
                 })
             }
-            GrantObject::TableById(_, db_id, table_id) => Ok(OwnershipObject::Table {
-                catalog_name: catalog_name.unwrap(),
-                db_id: *db_id,
-                table_id: *table_id,
-            }),
-            GrantObject::DatabaseById(_, db_id) => Ok(OwnershipObject::Database {
-                catalog_name: catalog_name.unwrap(),
-                db_id: *db_id,
-            }),
+            GrantObject::TableById(_, db_id, table_id) => {
+                let catalog_name = catalog_name.unwrap();
+                let catalog = self.ctx.get_catalog(&catalog_name).await?;
+                check_system_history(&catalog, &catalog.get_db_name_by_id(*db_id).await?)?;
+                Ok(OwnershipObject::Table {
+                    catalog_name,
+                    db_id: *db_id,
+                    table_id: *table_id,
+                })
+            },
+            GrantObject::DatabaseById(_, db_id) => {
+                let catalog_name = catalog_name.unwrap();
+                let catalog = self.ctx.get_catalog(&catalog_name).await?;
+                check_system_history(&catalog, &catalog.get_db_name_by_id(*db_id).await?)?;
+                Ok(OwnershipObject::Database {
+                    catalog_name,
+                    db_id: *db_id,
+                })
+            },
             GrantObject::Stage(name) => Ok(OwnershipObject::Stage {
                 name: name.to_string(),
             }),
diff --git a/src/query/service/src/interpreters/interpreter_privilege_revoke.rs b/src/query/service/src/interpreters/interpreter_privilege_revoke.rs
@@ -87,7 +87,7 @@ impl Interpreter for RevokePrivilegeInterpreter {
                         .revoke_privileges_from_role(&tenant, &role, object, plan.priv_types)
                         .await?;
                 }
-                // grant_ownership and grant_privileges_to_role will modify the kv in meta.
+                // revoke_privileges_from_role will modify the kv in meta.
                 // So we need invalidate the role cache.
                 RoleCacheManager::instance().invalidate_cache(&tenant);
             }
diff --git a/src/query/service/src/interpreters/util.rs b/src/query/service/src/interpreters/util.rs
@@ -18,13 +18,15 @@ use databend_common_ast::ast::Expr;
 use databend_common_ast::parser::parse_expr;
 use databend_common_ast::parser::tokenize_sql;
 use databend_common_ast::parser::Dialect;
+use databend_common_catalog::catalog::Catalog;
 use databend_common_exception::ErrorCode;
 use databend_common_expression::ComputedExpr;
 use databend_common_expression::DataBlock;
 use databend_common_expression::DataSchemaRef;
 use databend_common_expression::Scalar;
 use databend_common_expression::TableSchemaRef;
 use databend_common_meta_app::principal::UserInfo;
+use databend_common_meta_app::principal::SENSITIVE_SYSTEM_RESOURCE;
 use databend_common_script::ir::ColumnAccess;
 use databend_common_script::Client;
 use databend_common_sql::Planner;
@@ -34,6 +36,18 @@ use itertools::Itertools;
 use crate::interpreters::InterpreterFactory;
 use crate::sessions::QueryContext;
 
+pub fn check_system_history(
+    catalog: &Arc<dyn Catalog>,
+    db_name: &str,
+) -> databend_common_exception::Result<()> {
+    if !catalog.is_external() && db_name.eq_ignore_ascii_case(SENSITIVE_SYSTEM_RESOURCE) {
+        return Err(ErrorCode::IllegalGrant(
+            "Can not modify system_history ownership",
+        ));
+    }
+    Ok(())
+}
+
 #[allow(clippy::type_complexity)]
 pub fn generate_desc_schema(
     schema: TableSchemaRef,
diff --git a/src/query/service/src/test_kits/fixture.rs b/src/query/service/src/test_kits/fixture.rs
@@ -147,6 +147,28 @@ impl TestFixture {
         Self::setup_with_custom(OSSSetup { config }).await
     }
 
+    /// Create a new TestFixture with default config.
+    pub async fn setup_with_history_log() -> Result<TestFixture> {
+        let config = ConfigBuilder::create().config();
+        let conf = OSSSetup { config }.setup().await?;
+
+        use crate::history_tables::session::create_session;
+        let default_session =
+            create_session(conf.query.tenant_id.tenant_name(), &conf.query.cluster_id).await?;
+        let default_ctx = default_session.create_query_context().await?;
+
+        let random_prefix: String = Uuid::new_v4().simple().to_string();
+        let thread_name = std::thread::current().name().unwrap().to_string();
+        let guard = TestGuard::new(thread_name.clone());
+        Ok(Self {
+            default_ctx,
+            default_session,
+            conf,
+            prefix: random_prefix,
+            _guard: guard,
+        })
+    }
+
     pub async fn setup_with_segment_cache_bytes(segment_bytes: u64) -> Result<TestFixture> {
         let config = ConfigBuilder::create()
             .enable_table_meta_cache()
diff --git a/tests/logging/check_logs_table.sh b/tests/logging/check_logs_table.sh
@@ -93,3 +93,76 @@ check_query_log "t-3" "$select_query_id" "settings (timezone='Asia/Shanghai') SE
 check_query_log "t-4" null "settings (timezone='Asia/Shanghai') SELECT DATE_DIFF(hour, event_time, now()) FROM system_history.login_history limit 1" "0"
 
 check_query_log "t-5" "$select_query_id" "settings (timezone='Asia/Shanghai') SELECT DATE_DIFF(hour, query_start, now()) FROM system_history.access_history WHERE" "0"
+
+response=$(curl -s -u root: -XPOST "http://localhost:8000/v1/query" -H 'Content-Type: application/json' -d '{"sql": "drop user if exists a"}')
+response=$(curl -s -u root: -XPOST "http://localhost:8000/v1/query" -H 'Content-Type: application/json' -d '{"sql": "drop role if exists ra"}')
+response=$(curl -s -u root: -XPOST "http://localhost:8000/v1/query" -H 'Content-Type: application/json' -d "{\"sql\": \"create user a identified by '123' with default_role='ra'\"}")
+response=$(curl -s -u root: -XPOST "http://localhost:8000/v1/query" -H 'Content-Type: application/json' -d '{"sql": "create role ra"}')
+response=$(curl -s -u root: -XPOST "http://localhost:8000/v1/query" -H 'Content-Type: application/json' -d '{"sql": "grant select , drop on system_history.* to role ra"}')
+response=$(curl -s -u root: -XPOST "http://localhost:8000/v1/query" -H 'Content-Type: application/json' -d '{"sql": "grant alter on system_history.* to role ra"}')
+response=$(curl -s -u root: -XPOST "http://localhost:8000/v1/query" -H 'Content-Type: application/json' -d '{"sql": "grant role ra to a"}')
+
+execute_and_verify() {
+    local cmd_description="$1"
+    local user_cred="$2"
+    local json_payload="$3"
+    local jq_expression="$4"
+    local result
+
+    echo -n "Executing: $cmd_description ... "
+
+    result=$(curl -s -u "${user_cred}" -XPOST "http://localhost:8000/v1/query" -H 'Content-Type: application/json' -d "${json_payload}" | jq -r "${jq_expression}")
+
+    if [[ "$result" != "true" ]]; then
+        echo "Failed! Expected result: true, actual result: $result."
+        echo "$cmd_description failed."
+        exit 1 # Exit script immediately if it fails
+    fi
+}
+
+check_system_history_permissions() {
+    # Command 1: User 'a:123' attempts to create a table in 'system_history' (expected to fail)
+    execute_and_verify \
+        "User 'a:123' attempts to create a table in 'system_history'" \
+        "a:123" \
+        '{"sql": "create table system_history.t (a INT)"}' \
+        '.state == "Failed"'
+
+    # Command 2: User 'root:' attempts to modify 'system_history.log_history' table (expected to fail)
+    execute_and_verify \
+        "User 'root:' attempts to modify 'system_history.log_history' table" \
+        "root:" \
+        '{"sql": "alter table system_history.log_history add column id int"}' \
+        '.state == "Failed"'
+
+    # Command 3: User 'a:123' attempts to drop 'system_history.access_history' table (expected to succeed)
+    execute_and_verify \
+        "User 'a:123' attempts to drop 'system_history.access_history' table" \
+        "a:123" \
+        '{"sql": "drop table system_history.access_history"}' \
+        '.state == "Succeeded"'
+
+    # Command 4: User 'root:' attempts to grant ownership on 'system_history.*' (expected to fail)
+    execute_and_verify \
+        "User 'root:' attempts to grant ownership on 'system_history.*'" \
+        "root:" \
+        '{"sql": "grant ownership on system_history.* to role ra"}' \
+        '.state == "Failed"'
+
+    # Command 5: User 'root:' attempts to grant ownership on 'system_history.query_history' (expected to fail)
+    execute_and_verify \
+        "User 'root:' attempts to grant ownership on 'system_history.query_history'" \
+        "root:" \
+        '{"sql": "grant ownership on system_history.query_history to role ra"}' \
+        '.state == "Failed"'
+    # Command 6: User 'a:123' attempts to select 'system_history.query_history' table (expected to succeed)
+        execute_and_verify \
+            "User 'a:123' attempts to query 'system_history.query_history' table" \
+            "a:123" \
+            '{"sql": "select count() from system_history.query_history"}' \
+            '.state == "Succeeded"'
+}
+
+check_system_history_permissions
+response=$(curl -s -u root: -XPOST "http://localhost:8000/v1/query" -H 'Content-Type: application/json' -d '{"sql": "drop user if exists a"}')
+response=$(curl -s -u root: -XPOST "http://localhost:8000/v1/query" -H 'Content-Type: application/json' -d '{"sql": "drop role if exists ra"}')

Original file line number	Diff line number	Diff line change
`@@ -87,7 +87,7 @@ impl Interpreter for RevokePrivilegeInterpreter {`
`87`	`87`	`.revoke_privileges_from_role(&tenant, &role, object, plan.priv_types)`
`88`	`88`	`.await?;`
`89`	`89`	`}`
`90`		`- // grant_ownership and grant_privileges_to_role will modify the kv in meta.`
	`90`	`+ // revoke_privileges_from_role will modify the kv in meta.`
`91`	`91`	`// So we need invalidate the role cache.`
`92`	`92`	`RoleCacheManager::instance().invalidate_cache(&tenant);`
`93`	`93`	`}`