Skip to content

Commit

Permalink
feat(rbac): treat the empty ownership as owned by ACCOUNT_ADMIN inste…
Browse files Browse the repository at this point in the history
…ad of PUBLIC (databendlabs#14112)

* default as account_admin

* fix cargo check

* allow list in system tables

* tune comments

* remove verify_ownership parameter in validate_access_db and _table

* fix typo

* fix lint

* add test for rewritten

* add stateless tests

* fix stateless error

* fix set_role

* add result file

* rename validate_ownership to has_ownership

* fix typo
  • Loading branch information
flaneur2020 authored Dec 28, 2023
1 parent a53315b commit 3fdf353
Show file tree
Hide file tree
Showing 8 changed files with 139 additions and 140 deletions.
197 changes: 85 additions & 112 deletions src/query/service/src/interpreters/access/privilege_access.rs

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions src/query/service/src/sessions/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -260,11 +260,11 @@ impl Session {
}

#[async_backtrace::framed]
pub async fn validate_ownership(self: &Arc<Self>, object: &GrantObjectByID) -> Result<()> {
pub async fn has_ownership(self: &Arc<Self>, object: &GrantObjectByID) -> Result<bool> {
if matches!(self.get_type(), SessionType::Local) {
return Ok(());
return Ok(true);
}
self.privilege_mgr.validate_ownership(object).await
self.privilege_mgr.has_ownership(object).await
}

#[async_backtrace::framed]
Expand Down
24 changes: 10 additions & 14 deletions src/query/service/src/sessions/session_privilege_mgr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use databend_common_meta_app::principal::UserInfo;
use databend_common_meta_app::principal::UserPrivilegeType;
use databend_common_users::GrantObjectVisibilityChecker;
use databend_common_users::RoleCacheManager;
use databend_common_users::BUILTIN_ROLE_ACCOUNT_ADMIN;
use databend_common_users::BUILTIN_ROLE_PUBLIC;

use crate::sessions::SessionContext;
Expand Down Expand Up @@ -63,7 +64,7 @@ pub trait SessionPrivilegeManager {
privilege: Vec<UserPrivilegeType>,
) -> Result<()>;

async fn validate_ownership(&self, object: &GrantObjectByID) -> Result<()>;
async fn has_ownership(&self, object: &GrantObjectByID) -> Result<bool>;

async fn validate_available_role(&self, role_name: &str) -> Result<RoleInfo>;

Expand Down Expand Up @@ -265,25 +266,20 @@ impl SessionPrivilegeManager for SessionPrivilegeManagerImpl {
}

#[async_backtrace::framed]
async fn validate_ownership(&self, object: &GrantObjectByID) -> Result<()> {
async fn has_ownership(&self, object: &GrantObjectByID) -> Result<bool> {
let role_mgr = RoleCacheManager::instance();
let tenant = self.session_ctx.get_current_tenant();

// if the object is not owned by any role, then considered as PUBLIC, which is always true
let owner_role = match role_mgr.find_object_owner(&tenant, object).await? {
Some(owner_role) => owner_role,
None => return Ok(()),
// if the object is not owned by any role, then considered as ACCOUNT_ADMIN, the normal users
// can not access it unless ACCOUNT_ADMIN grant relevant privileges to them.
let owner_role_name = match role_mgr.find_object_owner(&tenant, object).await? {
Some(owner_role) => owner_role.name,
None => BUILTIN_ROLE_ACCOUNT_ADMIN.to_string(),
};

let available_roles = self.get_all_available_roles().await?;
if !available_roles.iter().any(|r| r.name == owner_role.name) {
return Err(ErrorCode::PermissionDenied(
"Permission denied, current session do not have the ownership of this object"
.to_string(),
));
}

Ok(())
let exists = available_roles.iter().any(|r| r.name == owner_role_name);
return Ok(exists);
}

#[async_backtrace::framed]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ test -- select
1
1
1
1
test -- select view
Error: APIError: ResponseError with 1063: Permission denied, privilege [Select] is required on 'default'.'default2'.'v_t20_0012' for user 'test-user'@'%' with roles [public,test-role1,test-role2]
1
test -- clustering_information
true
Expand All @@ -30,8 +31,6 @@ GRANT SELECT ON 'default'.'grant_db'.'t' TO 'a'@'%'
default
grant_db
information_schema
test -- show tables
test_t
test -- show tables from system
Error: APIError: ResponseError with 1063: Permission denied: User 'a'@'%' does not have the required privileges for database 'system'.
test -- show tables from grant_db
Expand Down
13 changes: 7 additions & 6 deletions tests/suites/0_stateless/18_rbac/20_0012_privilege_access.sh
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,16 @@ echo "GRANT SELECT ON default.t20_0012_b TO 'test-user'" | $BENDSQL_CLIENT_CONNE
echo "select * from default.t20_0012_b order by c" | $TEST_USER_CONNECT

## Create view table
## TODO(liyz): view is not covered with ownership yet, so the created views are owned by PUBLIC, which
## is accessible by all users. This need change.
## View is not covered with ownership yet, the privilge checks is bound to the database
## the view table is created in.
echo "select 'test -- select view'" | $TEST_USER_CONNECT
echo "create database default2" | $BENDSQL_CLIENT_CONNECT

echo "create view default2.v_t20_0012 as select * from default.t20_0012_a" | $BENDSQL_CLIENT_CONNECT
## Verify view table privilege
echo "select * from default2.v_t20_0012" | $TEST_USER_CONNECT
## Only grant privilege for view table
## Only grant privilege for view table, now this user can access the view under default2 db,
## but can not access the tables under the `default` database, stil raises permission error
## on SELECT default2.v_t20_0012
echo "GRANT SELECT ON default2.v_t20_0012 TO 'test-user'" | $BENDSQL_CLIENT_CONNECT
echo "REVOKE SELECT ON default.t20_0012_a FROM 'test-user'" | $BENDSQL_CLIENT_CONNECT
echo "REVOKE SELECT ON default.t20_0012_b FROM 'test-user'" | $BENDSQL_CLIENT_CONNECT
Expand Down Expand Up @@ -140,8 +143,6 @@ echo "drop table if exists default.test_t" | $BENDSQL_CLIENT_CONNECT
echo "create table default.test_t(id int not null)" | $BENDSQL_CLIENT_CONNECT
echo "show grants for a" | $BENDSQL_CLIENT_CONNECT
echo "show databases" | $USER_A_CONNECT
echo "select 'test -- show tables'" | $BENDSQL_CLIENT_CONNECT
echo "show tables" | $USER_A_CONNECT
echo "select 'test -- show tables from system'" | $BENDSQL_CLIENT_CONNECT
echo "show tables from system" | $USER_A_CONNECT
echo "select 'test -- show tables from grant_db'" | $BENDSQL_CLIENT_CONNECT
Expand Down
8 changes: 6 additions & 2 deletions tests/suites/0_stateless/18_rbac/20_0015_set_role.sh
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ echo '-- reset user, roles, and tables'
echo "DROP USER IF EXISTS 'testuser1'" | $BENDSQL_CLIENT_CONNECT
echo "DROP ROLE IF EXISTS 'testrole1'" | $BENDSQL_CLIENT_CONNECT
echo "DROP ROLE IF EXISTS 'testrole2'" | $BENDSQL_CLIENT_CONNECT
echo "DROP ROLE IF EXISTS 'testrole3'" | $BENDSQL_CLIENT_CONNECT
echo "DROP ROLE IF EXISTS 'testrole4'" | $BENDSQL_CLIENT_CONNECT
echo "DROP TABLE IF EXISTS t20_0015_table1" | $BENDSQL_CLIENT_CONNECT
echo "DROP TABLE IF EXISTS t20_0015_table2" | $BENDSQL_CLIENT_CONNECT

echo '-- prepare user, roles, and tables for tests'
echo "CREATE USER 'testuser1' IDENTIFIED BY '$TEST_USER_PASSWORD'" | $BENDSQL_CLIENT_CONNECT
Expand All @@ -21,8 +25,8 @@ echo 'GRANT ROLE testrole2 to ROLE testrole3' | $BENDSQL_CLIENT_CONNECT
echo 'GRANT ROLE testrole1 to testuser1' | $BENDSQL_CLIENT_CONNECT
echo 'GRANT ROLE testrole2 to testuser1' | $BENDSQL_CLIENT_CONNECT
echo 'GRANT ROLE testrole3 to testuser1' | $BENDSQL_CLIENT_CONNECT
echo "CREATE TABLE t20_0015_table1(c int not null)" | $BENDSQL_CLIENT_CONNECT
echo "CREATE TABLE t20_0015_table2(c int not null)" | $BENDSQL_CLIENT_CONNECT
echo "CREATE TABLE t20_0015_table1(c int not null) ENGINE = MEMORY" | $BENDSQL_CLIENT_CONNECT
echo "CREATE TABLE t20_0015_table2(c int not null) ENGINE = MEMORY" | $BENDSQL_CLIENT_CONNECT

echo '-- grant privilege to roles'
echo 'GRANT SELECT, INSERT ON default.t20_0015_table1 TO ROLE testrole1' | $BENDSQL_CLIENT_CONNECT
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
-- reset users
-- prepare user and tables for tests
-- ensure the statements not break with PUBLIC role
23 changes: 23 additions & 0 deletions tests/suites/0_stateless/18_rbac/20_0016_rewrite_statements.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#!/usr/bin/env bash

CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
. "$CURDIR"/../../../shell_env.sh

export TEST_USER_PASSWORD="password"
export TEST_USER_CONNECT="bendsql --user=testuser1 --password=password --host=${QUERY_MYSQL_HANDLER_HOST} --port ${QUERY_HTTP_HANDLER_PORT}"

echo '-- reset users'
echo "DROP USER IF EXISTS 'testuser1'" | $BENDSQL_CLIENT_CONNECT

echo '-- prepare user and tables for tests'
echo "CREATE USER 'testuser1' IDENTIFIED BY '$TEST_USER_PASSWORD'" | $BENDSQL_CLIENT_CONNECT
echo "CREATE TABLE IF NOT EXISTS t20_0016_table1(c int not null)" | $BENDSQL_CLIENT_CONNECT
echo "GRANT SELECT ON default.t20_0016_table1 TO testuser1" | $BENDSQL_CLIENT_CONNECT

echo '-- ensure the statements not break with PUBLIC role'
echo "SHOW TABLES;" | $TEST_USER_CONNECT > /dev/null
echo "SHOW DATABASES;" | $TEST_USER_CONNECT > /dev/null
echo "SHOW USERS;" | $TEST_USER_CONNECT > /dev/null
echo "SHOW ROLES;" | $TEST_USER_CONNECT > /dev/null
echo "SHOW STAGES;" | $TEST_USER_CONNECT > /dev/null
echo "SHOW PROCESSLIST;" | $TEST_USER_CONNECT > /dev/null

0 comments on commit 3fdf353

Please sign in to comment.