From f3029b7fec34b001912543e5c9dc9ddbe497d423 Mon Sep 17 00:00:00 2001
From: Egor Ivkov <e.o.ivkov@gmail.com>
Date: Tue, 23 Jan 2024 20:55:00 +0300
Subject: [PATCH] feat: make alter and revoke privilege operations idempotent
---
src/sql.rs | 22 ++------------------
src/storage.rs | 49 ++++++++++++++++++++++++++++++++++----------
test/int/test_sql.py | 8 ++++----
3 files changed, 44 insertions(+), 35 deletions(-)
diff --git a/src/sql.rs b/src/sql.rs
index a271239772..fada1eb4fc 100644
--- a/src/sql.rs
+++ b/src/sql.rs
@@ -825,29 +825,11 @@ fn reenterable_schema_change_request(
}
AlterOptionParam::Login => {
- if check_privilege_already_granted(
- node,
- grantee_id,
- &object_type,
- object_id,
- &privilege,
- )? {
- // Login is already granted, no op needed.
- return Ok(ConsumerResult { row_count: 0 });
- }
+ // It will be checked at a later stage whether login is already granted
Op::Acl(OpAcl::GrantPrivilege { priv_def })
}
AlterOptionParam::NoLogin => {
- if !check_privilege_already_granted(
- node,
- grantee_id,
- &object_type,
- object_id,
- &privilege,
- )? {
- // Login is not granted yet, no op needed.
- return Ok(ConsumerResult { row_count: 0 });
- }
+ // It will be checked at a later stage whether login was not granted
Op::Acl(OpAcl::RevokePrivilege {
priv_def,
initiator: current_user,
diff --git a/src/storage.rs b/src/storage.rs
index 579158267d..64eac060f4 100644
--- a/src/storage.rs
+++ b/src/storage.rs
@@ -2504,12 +2504,27 @@ impl Privileges {
Ok(())
}
+ /// If `if_not_exists == true` will skip insertion if this privilege already exists.
#[inline(always)]
- pub fn insert(&self, priv_def: &PrivilegeDef) -> tarantool::Result<()> {
+ pub fn insert(&self, priv_def: &PrivilegeDef, if_not_exists: bool) -> tarantool::Result<()> {
+ if if_not_exists
+ && self
+ .space
+ .get(&(
+ priv_def.grantee_id(),
+ priv_def.object_type(),
+ priv_def.object_id_raw(),
+ priv_def.privilege(),
+ ))?
+ .is_some()
+ {
+ return Ok(());
+ }
self.space.insert(priv_def)?;
Ok(())
}
+ /// If `if_exists == true` will skip deletion if this privilege does not exist.
#[inline(always)]
pub fn delete(
&self,
@@ -2517,7 +2532,16 @@ impl Privileges {
object_type: &str,
object_id: i64,
privilege: &str,
+ if_exists: bool,
) -> tarantool::Result<()> {
+ if if_exists
+ && self
+ .space
+ .get(&(grantee_id, object_type, object_id, privilege))?
+ .is_none()
+ {
+ return Ok(());
+ }
self.space
.delete(&(grantee_id, object_type, object_id, privilege))?;
Ok(())
@@ -2532,6 +2556,7 @@ impl Privileges {
&priv_def.object_type(),
priv_def.object_id_raw(),
&priv_def.privilege(),
+ false,
)?;
}
Ok(())
@@ -2551,6 +2576,7 @@ impl Privileges {
&priv_def.object_type(),
priv_def.object_id_raw(),
&priv_def.privilege(),
+ false,
)?;
}
}
@@ -2571,6 +2597,7 @@ impl Privileges {
&priv_def.object_type(),
object_id,
&priv_def.privilege(),
+ false,
)?;
}
Ok(())
@@ -3029,16 +3056,14 @@ pub mod acl {
storage: &Clusterwide,
priv_def: &PrivilegeDef,
) -> tarantool::Result<()> {
- storage.privileges.insert(priv_def)?;
+ storage.privileges.insert(priv_def, true)?;
let privilege = &priv_def.privilege();
let object = priv_def
.resolve_object_name(storage)
.expect("target object should exist");
let object_type = &priv_def.object_type();
-
let (grantee_type, grantee) = priv_def.grantee_type_and_name(storage)?;
-
let initiator_def = user_by_id(priv_def.grantor_id())?;
match (privilege.as_str(), object_type.as_str()) {
@@ -3089,6 +3114,7 @@ pub mod acl {
&priv_def.object_type(),
priv_def.object_id_raw(),
&priv_def.privilege(),
+ true,
)?;
let privilege = &priv_def.privilege();
@@ -3097,7 +3123,6 @@ pub mod acl {
.expect("target object should exist");
let object_type = &priv_def.object_type();
let (grantee_type, grantee) = priv_def.grantee_type_and_name(storage)?;
-
let initiator_def = user_by_id(initiator)?;
match (privilege.as_str(), object_type.as_str()) {
@@ -3239,16 +3264,17 @@ pub mod acl {
Ok(())
}
- /// Grant a tarantool user some privilege defined by `priv_def`.
+ /// Grant a tarantool user or role the privilege defined by `priv_def`.
+ /// Is idempotent: will not return an error even if the privilege is already granted.
pub fn on_master_grant_privilege(priv_def: &PrivilegeDef) -> tarantool::Result<()> {
let lua = ::tarantool::lua_state();
lua.exec_with(
"local grantee_id, privilege, object_type, object_id = ...
local grantee_def = box.space._user:get(grantee_id)
if grantee_def.type == 'user' then
- box.schema.user.grant(grantee_id, privilege, object_type, object_id)
+ box.schema.user.grant(grantee_id, privilege, object_type, object_id, {if_not_exists=true})
else
- box.schema.role.grant(grantee_id, privilege, object_type, object_id)
+ box.schema.role.grant(grantee_id, privilege, object_type, object_id, {if_not_exists=true})
end",
(
priv_def.grantee_id(),
@@ -3262,7 +3288,8 @@ pub mod acl {
Ok(())
}
- /// Revoke a privilege from a tarantool user.
+ /// Revoke a privilege from a tarantool user or role.
+ /// Is idempotent: will not return an error even if the privilege was not granted.
pub fn on_master_revoke_privilege(priv_def: &PrivilegeDef) -> tarantool::Result<()> {
let lua = ::tarantool::lua_state();
lua.exec_with(
@@ -3273,9 +3300,9 @@ pub mod acl {
return
end
if grantee_def.type == 'user' then
- box.schema.user.revoke(grantee_id, privilege, object_type, object_id)
+ box.schema.user.revoke(grantee_id, privilege, object_type, object_id, {if_exists=true})
else
- box.schema.role.revoke(grantee_id, privilege, object_type, object_id)
+ box.schema.role.revoke(grantee_id, privilege, object_type, object_id, {if_exists=true})
end",
(
priv_def.grantee_id(),
diff --git a/test/int/test_sql.py b/test/int/test_sql.py
index 9c1a0a33f4..90f8f30b5b 100644
--- a/test/int/test_sql.py
+++ b/test/int/test_sql.py
@@ -1478,15 +1478,15 @@ def test_sql_alter_login(cluster: Cluster):
acl = i1.sudo_sql(f"create user {username} with password '{password}'")
assert acl["row_count"] == 1
- # Alter user with LOGIN option do nothing.
+ # Alter user with LOGIN option - opertaion is idempotent.
acl = i1.sudo_sql(f""" alter user {username} with login """)
- assert acl["row_count"] == 0
+ assert acl["row_count"] == 1
# * Alter user with NOLOGIN option.
acl = i1.sudo_sql(f""" alter user {username} with nologin """)
assert acl["row_count"] == 1
- # * Alter user with NOLOGIN again do nothing.
+ # * Alter user with NOLOGIN again - operation is idempotent.
acl = i1.sudo_sql(f""" alter user {username} with nologin """)
- assert acl["row_count"] == 0
+ assert acl["row_count"] == 1
# * TODO: Check SESSION privilege is removed.
--
GitLab