From 8e2e1991c9df4356e5959e36bc701092243aa36b Mon Sep 17 00:00:00 2001
From: Egor Ivkov <e.ivkov@picodata.io>
Date: Fri, 17 May 2024 21:10:21 +0300
Subject: [PATCH] feat: allow user owner to grant and revoke login permission
(cherry picked from commit 9c462099ff126506001f70c3153057003463d0e3)
Conflicts: src/access_control.rs
---
src/access_control.rs | 172 +++++++++++++++++++++++++++++++++++++++---
src/schema.rs | 2 +-
src/sql.rs | 2 +-
test/int/test_sql.py | 66 ++++++++++++++--
4 files changed, 224 insertions(+), 18 deletions(-)
diff --git a/src/access_control.rs b/src/access_control.rs
index 440c75774d..a4540b93e6 100644
--- a/src/access_control.rs
+++ b/src/access_control.rs
@@ -352,26 +352,49 @@ fn access_check_grant_revoke(
Some(object_id) => object_id,
};
+ // Nobody should be able to revoke privileges from pico_service
+ // As it would break the cluster permanently.
+ if access == PrivType::Revoke && priv_def.grantee_id() == PICO_SERVICE_ID {
+ tarantool::set_error!(
+ tarantool::error::TarantoolErrorCode::AccessDenied,
+ "Revoke '{}' from '{}' is denied for all users",
+ priv_def.privilege(),
+ PICO_SERVICE_USER_NAME,
+ );
+ return Err(tarantool::error::TarantoolError::last().into());
+ }
+
match priv_def.object_type() {
PicoSchemaObjectType::Universe => {
- // Only admin can grant on universe
- if grantor_id != ADMIN_ID {
+ if priv_def.privilege() != PrivilegeType::Login {
+ // This assumption is used in the following checks
+ crate::warn_or_panic!(
+ "Only Login privilege can be granted on Universe, got {}",
+ priv_def.privilege()
+ );
return Err(make_access_denied(
access_name,
PicoSchemaObjectType::Universe,
- "",
+ "Only Login privilege can be granted on Universe",
grantor.name,
));
}
- // Even admin should not be able to revoke privileges from pico_service
- // As it would break the cluster permanently.
- if access == PrivType::Revoke && priv_def.grantee_id() == PICO_SERVICE_ID {
+ // Following checks are assuming that it's a grant or revoke of Login privilege
+
+ let target_sys_user = user_by_id(priv_def.grantee_id())?;
+ if target_sys_user.ty != UserMetadataKind::User {
+ return Err(make_no_such_user(&target_sys_user.name));
+ }
+
+ // Only owner or admin can grant login on user
+ if target_sys_user.owner_id != grantor_id && grantor_id != ADMIN_ID {
tarantool::set_error!(
tarantool::error::TarantoolErrorCode::AccessDenied,
- "Revoke '{}' from '{}' is denied for all users",
- priv_def.privilege(),
- PICO_SERVICE_USER_NAME,
+ "{} Login from '{}' is denied for {}",
+ access_name,
+ target_sys_user.name,
+ grantor.name
);
return Err(tarantool::error::TarantoolError::last().into());
}
@@ -615,7 +638,7 @@ mod tests {
access_control::{access_check_op, UserMetadataKind},
schema::{
auth_for_role_definition, Distribution, PrivilegeDef, PrivilegeType, SchemaObjectType,
- UserDef, ADMIN_ID,
+ UserDef, ADMIN_ID, UNIVERSE_ID,
},
storage::{
acl::{
@@ -1585,4 +1608,133 @@ mod tests {
);
}
}
+
+ #[tarantool::test]
+ fn alter_user_login() {
+ let storage = Clusterwide::for_tests();
+
+ let owner_name = "test_owner";
+ let user_name = "test_user";
+
+ let owner_id = make_user(owner_name, None);
+ let user_id = make_user(user_name, Some(owner_id));
+
+ // User that is not an owner or admin cannot grant/revoke login on user
+ let e = access_check_acl(
+ &storage,
+ &Acl::GrantPrivilege {
+ priv_def: PrivilegeDef::new(
+ PrivilegeType::Login,
+ SchemaObjectType::Universe,
+ UNIVERSE_ID,
+ user_id,
+ user_id,
+ 0,
+ )
+ .unwrap(),
+ },
+ user_id,
+ )
+ .unwrap_err();
+ assert_eq!(
+ e.to_string(),
+ format!(
+ "box error: AccessDenied: Grant Login from '{user_name}' is denied for test_user"
+ )
+ );
+
+ let e = access_check_acl(
+ &storage,
+ &Acl::RevokePrivilege {
+ priv_def: PrivilegeDef::new(
+ PrivilegeType::Login,
+ SchemaObjectType::Universe,
+ UNIVERSE_ID,
+ user_id,
+ user_id,
+ 0,
+ )
+ .unwrap(),
+ initiator: user_id,
+ },
+ user_id,
+ )
+ .unwrap_err();
+ assert_eq!(
+ e.to_string(),
+ format!(
+ "box error: AccessDenied: Revoke Login from '{user_name}' is denied for test_user"
+ )
+ );
+
+ // Owner can grant/revoke login on user
+ access_check_acl(
+ &storage,
+ &Acl::GrantPrivilege {
+ priv_def: PrivilegeDef::new(
+ PrivilegeType::Login,
+ SchemaObjectType::Universe,
+ UNIVERSE_ID,
+ user_id,
+ owner_id,
+ 0,
+ )
+ .unwrap(),
+ },
+ owner_id,
+ )
+ .unwrap();
+ access_check_acl(
+ &storage,
+ &Acl::RevokePrivilege {
+ priv_def: PrivilegeDef::new(
+ PrivilegeType::Login,
+ SchemaObjectType::Universe,
+ UNIVERSE_ID,
+ user_id,
+ owner_id,
+ 0,
+ )
+ .unwrap(),
+ initiator: owner_id,
+ },
+ owner_id,
+ )
+ .unwrap();
+
+ // Admin can grant/revoke login on user
+ access_check_acl(
+ &storage,
+ &Acl::GrantPrivilege {
+ priv_def: PrivilegeDef::new(
+ PrivilegeType::Login,
+ SchemaObjectType::Universe,
+ UNIVERSE_ID,
+ user_id,
+ ADMIN_ID,
+ 0,
+ )
+ .unwrap(),
+ },
+ ADMIN_ID,
+ )
+ .unwrap();
+ access_check_acl(
+ &storage,
+ &Acl::RevokePrivilege {
+ priv_def: PrivilegeDef::new(
+ PrivilegeType::Login,
+ SchemaObjectType::Universe,
+ UNIVERSE_ID,
+ user_id,
+ ADMIN_ID,
+ 0,
+ )
+ .unwrap(),
+ initiator: ADMIN_ID,
+ },
+ ADMIN_ID,
+ )
+ .unwrap();
+ }
}
diff --git a/src/schema.rs b/src/schema.rs
index a86f04f937..b7bbace3f3 100644
--- a/src/schema.rs
+++ b/src/schema.rs
@@ -474,7 +474,7 @@ impl PrivilegeType {
/// user Id of the vanilla role super we keep it as a placeholder. Aside from that
/// vanilla super role has some privileges that cannot be represented in picodata,
/// namely privileges on universe. This hole opens possibility for unwanted edge cases.
-/// 2) Only Session and Usage can be granted on Universe.
+/// 2) Only Login can be granted on Universe in picodata.
/// Note that validation is not performed in Deserialize. We assume that for untrusted data
/// the object is created via constructor. In other cases validation was already performed
/// prior to serialization thus deserialization always creates a valid object.
diff --git a/src/sql.rs b/src/sql.rs
index 3ca028f8dc..76c84d26e7 100644
--- a/src/sql.rs
+++ b/src/sql.rs
@@ -1228,7 +1228,7 @@ fn reenterable_schema_change_request(
};
// For ALTER Login/NoLogin.
- let grantor_id = session::euid()?;
+ let grantor_id = current_user;
let grantee_id = get_grantee_id(storage, name)?;
let object_type = SchemaObjectType::Universe;
let object_id = 0;
diff --git a/test/int/test_sql.py b/test/int/test_sql.py
index 3c49931afb..6a7170af12 100644
--- a/test/int/test_sql.py
+++ b/test/int/test_sql.py
@@ -1738,20 +1738,34 @@ def test_sql_alter_login(cluster: Cluster):
cluster.deploy(instance_count=2)
i1, i2 = cluster.instances
+ # Create the owner of `USER`
+ owner_username = "OWNER_USER"
+ owner_password = "PA5sWORD"
+
+ acl = i1.sudo_sql(f"create user {owner_username} with password '{owner_password}'")
+ assert acl["row_count"] == 1
+
+ acl = i1.sudo_sql(f"grant create user to {owner_username}")
+ assert acl["row_count"] == 1
+
username = "USER"
password = "PA5sWORD"
# Create user.
- acl = i1.sudo_sql(f"create user {username} with password '{password}'")
+ acl = i1.sql(
+ f"create user {username} with password '{password}'",
+ user=owner_username,
+ password=owner_password,
+ )
assert acl["row_count"] == 1
# Alter user with LOGIN option - opertaion is idempotent.
- acl = i1.sudo_sql(f""" alter user {username} with login """)
+ acl = i1.sudo_sql(f"alter user {username} with login")
assert acl["row_count"] == 1
# Alter user with NOLOGIN option.
- acl = i1.sudo_sql(f""" alter user {username} with nologin """)
+ acl = i1.sudo_sql(f"alter user {username} with nologin")
assert acl["row_count"] == 1
# Alter user with NOLOGIN again - operation is idempotent.
- acl = i1.sudo_sql(f""" alter user {username} with nologin """)
+ acl = i1.sudo_sql(f"alter user {username} with nologin")
assert acl["row_count"] == 1
# Login privilege is removed
with pytest.raises(
@@ -1760,6 +1774,47 @@ def test_sql_alter_login(cluster: Cluster):
):
i1.sql("insert into t values(2);", user=username, password=password)
+ # Alter user with LOGIN option again - `USER` owner can also modify LOGIN privilege.
+ acl = i1.sql(
+ f"alter user {username} with login",
+ user=owner_username,
+ password=owner_password,
+ )
+ assert acl["row_count"] == 1
+
+ # Alter user with NOLOGIN option again - `USER` owner can also modify LOGIN privilege.
+ acl = i1.sql(
+ f"alter user {username} with nologin",
+ user=owner_username,
+ password=owner_password,
+ )
+ assert acl["row_count"] == 1
+
+ # Login privilege cannot be granted or removed by any user
+ # other than admin or user account owner
+ other_username = "OTHER_USER"
+ other_password = "PA5sWORD"
+ acl = i1.sudo_sql(f"create user {other_username} with password '{other_password}'")
+ assert acl["row_count"] == 1
+ with pytest.raises(
+ Exception,
+ match=f"AccessDenied: Grant Login from '{username}' is denied for {other_username}",
+ ):
+ acl = i1.sql(
+ f"alter user {username} with login",
+ user=other_username,
+ password=other_password,
+ )
+ with pytest.raises(
+ Exception,
+ match=f"AccessDenied: Revoke Login from '{username}' is denied for {other_username}",
+ ):
+ acl = i1.sql(
+ f"alter user {username} with nologin",
+ user=other_username,
+ password=other_password,
+ )
+
# Login privilege cannot be removed from pico_service even by admin.
with pytest.raises(
Exception,
@@ -2170,7 +2225,7 @@ def test_sql_privileges(cluster: Cluster):
username = "alice"
alice_pwd = "Pa55sword"
- # Create user with execute on universe privilege
+ # Create user
acl = i1.sql(
f"""
create user "{username}" with password '{alice_pwd}'
@@ -2178,7 +2233,6 @@ def test_sql_privileges(cluster: Cluster):
"""
)
assert acl["row_count"] == 1
- i1.eval(f""" pico.grant_privilege("{username}", "execute", "universe") """)
# ------------------------
# Check SQL read privilege
--
GitLab