diff --git a/src/access_control.rs b/src/access_control.rs
index 374f89b13101b7271410b08047cb1876d7c3dccb..b009a680f13e2f8a7ba3f3d9ad06055e6bef43b5 100644
--- a/src/access_control.rs
+++ b/src/access_control.rs
@@ -532,7 +532,8 @@ mod tests {
grantee_id,
grantor_id.unwrap_or(session::uid().unwrap()),
0,
- );
+ )
+ .expect("must be valid");
access_check_op(
&Op::Acl(Acl::GrantPrivilege {
@@ -559,7 +560,8 @@ mod tests {
grantee_id,
session::uid().unwrap(),
0,
- );
+ )
+ .expect("must be valid");
access_check_op(
&Op::Acl(Acl::RevokePrivilege {
diff --git a/src/schema.rs b/src/schema.rs
index a841dabed7e6db2eeb142358f6830cd26e96ef61..7988c8aff51900e2be33ae7ba41e89306ee408bd 100644
--- a/src/schema.rs
+++ b/src/schema.rs
@@ -1,6 +1,7 @@
use once_cell::sync::OnceCell;
use std::borrow::Cow;
use std::collections::{BTreeMap, HashSet};
+use std::fmt::Display;
use std::time::Duration;
use tarantool::auth::AuthDef;
@@ -379,14 +380,20 @@ tarantool::define_str_enum! {
Drop = "drop",
/// ALTER
Alter = "alter",
- /// This is never granted, but used internally.
- Grant = "grant",
- /// Never granted, but used internally.
- Revoke = "revoke",
}
}
/// Privilege definition.
+/// Note the differences between picodata privileges and vanilla tarantool ones.
+/// 1) Super role in picodata is a placeholder. It exists because picodata reuses user
+/// ids from vanilla and super occupies an id. So to avoid assigning some piciodata
+/// 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.
+/// 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.
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Hash)]
pub struct PrivilegeDef {
privilege: PrivilegeType,
@@ -410,6 +417,22 @@ pub struct PrivilegeDef {
impl Encode for PrivilegeDef {}
+#[derive(Debug, thiserror::Error, PartialEq, Eq)]
+pub struct InvalidPrivilegeError {
+ object_type: SchemaObjectType,
+ unsupported: PrivilegeType,
+ expected_one_of: &'static [PrivilegeType],
+}
+
+impl Display for InvalidPrivilegeError {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ f.write_fmt(format_args!(
+ "Unsupported {} privilege '{}', expected one of {:?}",
+ self.object_type, self.unsupported, self.expected_one_of,
+ ))
+ }
+}
+
impl PrivilegeDef {
pub fn new(
privilege: PrivilegeType,
@@ -418,15 +441,43 @@ impl PrivilegeDef {
grantee_id: UserId,
grantor_id: UserId,
schema_version: u64,
- ) -> PrivilegeDef {
- PrivilegeDef {
+ ) -> Result<PrivilegeDef, InvalidPrivilegeError> {
+ let privilege_def = PrivilegeDef {
privilege,
object_type,
object_id,
grantee_id,
grantor_id,
schema_version,
+ };
+
+ use PrivilegeType::*;
+
+ let valid_privileges: &[PrivilegeType] = match privilege_def.object_type {
+ SchemaObjectType::Table => match privilege_def.object_id() {
+ Some(_) => &[Read, Write, Alter, Drop],
+ None => &[Read, Write, Create, Alter, Drop],
+ },
+ SchemaObjectType::Role => match privilege_def.object_id() {
+ Some(_) => &[Execute, Drop],
+ None => &[Create, Drop],
+ },
+ SchemaObjectType::User => match privilege_def.object_id() {
+ Some(_) => &[Alter, Drop],
+ None => &[Create, Alter, Drop],
+ },
+ SchemaObjectType::Universe => &[Session, Usage],
+ };
+
+ if !valid_privileges.contains(&privilege) {
+ return Err(InvalidPrivilegeError {
+ object_type,
+ unsupported: privilege,
+ expected_one_of: valid_privileges,
+ });
}
+
+ Ok(privilege_def)
}
#[inline(always)]
@@ -503,15 +554,18 @@ impl PrivilegeDef {
let mut v = Vec::new();
// SQL: GRANT <'usage', 'session'> ON 'universe' TO 'guest'
- for privilege in [PrivilegeType::Usage, PrivilegeType::Session] {
- v.push(PrivilegeDef {
- grantor_id: ADMIN_ID,
- grantee_id: GUEST_ID,
- object_type: SchemaObjectType::Universe,
- object_id: UNIVERSE_ID,
- privilege,
- schema_version: 0,
- });
+ // SQL: GRANT <'usage', 'session'> ON 'universe' TO 'admin'
+ for user in [GUEST_ID, ADMIN_ID] {
+ for privilege in [PrivilegeType::Usage, PrivilegeType::Session] {
+ v.push(PrivilegeDef {
+ grantor_id: ADMIN_ID,
+ grantee_id: user,
+ object_type: SchemaObjectType::Universe,
+ object_id: UNIVERSE_ID,
+ privilege,
+ schema_version: 0,
+ });
+ }
}
// SQL: GRANT 'public' TO 'guest'
@@ -524,18 +578,6 @@ impl PrivilegeDef {
schema_version: 0,
});
- // SQL: GRANT 'all privileges' ON 'universe' TO 'admin'
- for privilege in PrivilegeType::VARIANTS {
- v.push(PrivilegeDef {
- grantor_id: ADMIN_ID,
- grantee_id: ADMIN_ID,
- object_type: SchemaObjectType::Universe,
- object_id: UNIVERSE_ID,
- privilege: *privilege,
- schema_version: 0,
- });
- }
-
v
})
}
@@ -1266,7 +1308,7 @@ mod tests {
#[cfg(test)]
mod test {
use super::*;
- use tarantool::tuple::ToTupleBuffer;
+ use tarantool::tuple::{Decode, ToTupleBuffer};
#[test]
#[rustfmt::skip]
@@ -1318,4 +1360,61 @@ mod test {
let format = PrivilegeDef::format();
crate::util::check_tuple_matches_format(tuple_data.as_ref(), &format, "PrivilegeDef::format");
}
+
+ #[track_caller]
+ fn check_object_privilege(
+ object_type: SchemaObjectType,
+ valid_privileges: &'static [PrivilegeType],
+ object_id: i64,
+ ) {
+ for privilege in PrivilegeType::VARIANTS {
+ let res = PrivilegeDef::new(*privilege, object_type, object_id, 1, 1, 1);
+
+ if valid_privileges.contains(privilege) {
+ assert!(res.is_ok())
+ } else {
+ assert_eq!(
+ res.unwrap_err(),
+ InvalidPrivilegeError {
+ object_type,
+ unsupported: *privilege,
+ expected_one_of: valid_privileges,
+ }
+ )
+ }
+ }
+ }
+
+ #[test]
+ fn privilege_def_validation() {
+ use PrivilegeType::*;
+
+ // table
+ let valid = &[Read, Write, Create, Alter, Drop];
+ check_object_privilege(SchemaObjectType::Table, valid, -1);
+
+ // particular table
+ let valid = &[Read, Write, Alter, Drop];
+ check_object_privilege(SchemaObjectType::Table, valid, 42);
+
+ // user
+ let valid = &[Create, Alter, Drop];
+ check_object_privilege(SchemaObjectType::User, valid, -1);
+
+ // particular user
+ let valid = &[Alter, Drop];
+ check_object_privilege(SchemaObjectType::User, valid, 42);
+
+ // role
+ let valid = &[Create, Drop];
+ check_object_privilege(SchemaObjectType::Role, valid, -1);
+
+ // particular role
+ let valid = &[Execute, Drop];
+ check_object_privilege(SchemaObjectType::Role, valid, 42);
+
+ // universe
+ let valid = &[Session, Usage];
+ check_object_privilege(SchemaObjectType::Universe, valid, 0);
+ }
}
diff --git a/src/sql.rs b/src/sql.rs
index 407152b392758a63ec7813ffdbb4d2e6f1f5ce3f..764136a8a1103aeb0e42f68d0f6033cb993f890d 100644
--- a/src/sql.rs
+++ b/src/sql.rs
@@ -800,7 +800,8 @@ fn reenterable_schema_change_request(
grantee_id,
grantor_id,
schema_version,
- );
+ )
+ .map_err(Error::other)?;
match alter_option_param {
AlterOptionParam::ChangePassword(auth) => {
@@ -912,7 +913,8 @@ fn reenterable_schema_change_request(
grantee_id,
grantor_id,
schema_version,
- ),
+ )
+ .map_err(Error::other)?,
})
}
Params::RevokePrivilege(revoke_type, grantee_name) => {
@@ -939,7 +941,8 @@ fn reenterable_schema_change_request(
grantee_id,
grantor_id,
schema_version,
- ),
+ )
+ .map_err(Error::other)?,
})
}
};
diff --git a/src/storage.rs b/src/storage.rs
index fb89dbade15e7944da28d48e2e673bace1c0be88..823d43fca20f025523a89ed88b7bc8e7f76a81e5 100644
--- a/src/storage.rs
+++ b/src/storage.rs
@@ -2477,6 +2477,7 @@ impl Privileges {
/// Remove any privilege definitions assigning the given role.
#[inline]
pub fn delete_all_by_granted_role(&self, role_id: u32) -> Result<()> {
+ // here we failed to deserialize the privilege that has alter on universe
for priv_def in self.iter()? {
if priv_def.privilege() == PrivilegeType::Execute
&& priv_def.object_type() == SchemaObjectType::Role
@@ -2730,9 +2731,7 @@ impl SchemaDef for PrivilegeDef {
#[inline(always)]
fn schema_version(&self) -> u64 {
- // Note: this doesnt lead to recursion because of the
- // method resolution order: https://doc.rust-lang.org/reference/expressions/method-call-expr.html
- self.schema_version()
+ PrivilegeDef::schema_version(self)
}
#[inline(always)]
diff --git a/test/conftest.py b/test/conftest.py
index 6fd0c643828b04046cf76ff3e0e25754b0d8df9a..0402a2f72652e1bca0bd608c9869463efb24fe30 100644
--- a/test/conftest.py
+++ b/test/conftest.py
@@ -1354,6 +1354,32 @@ class Cluster:
eprint(f"CaS:\n {predicate=}\n {dml=}")
return instance.call("pico.cas", dml, predicate, user=user, password=password)
+ def masters(self) -> List[Instance]:
+ ret = []
+ for instance in self.instances:
+ if not instance.eval("return box.info.ro"):
+ ret.append(instance)
+
+ return ret
+
+ def grant_box_privilege(
+ self, user, privilege: str, object_type: str, object_name: Optional[str] = None
+ ):
+ """
+ Sometimes in our tests we go beyond picodata privilege model and need
+ to grant priveleges on something that is not part of the picodata access control model.
+ For example execute access on universe mainly needed to invoke functions.
+ """
+ for instance in self.masters():
+ instance.eval(
+ """
+ box.session.su("admin")
+ user, privilege, object_type, object_name = ...
+ return box.schema.user.grant(user, privilege, object_type, object_name)
+ """,
+ [user, privilege, object_type, object_name],
+ )
+
@dataclass
class PortalStorage:
diff --git a/test/int/test_acl.py b/test/int/test_acl.py
index 3c80452266411dd14bc0ab784f1ad05e32f5bf11..4279464498c159c2180415450b7f6b6784d2746e 100644
--- a/test/int/test_acl.py
+++ b/test/int/test_acl.py
@@ -202,9 +202,9 @@ def test_acl_lua_api(cluster: Cluster):
i1.call(
f"pico.{f}",
"User is not found",
- "execute",
- "universe",
- None,
+ "read",
+ "table",
+ "_pico_property",
)
# No such privilege -> error.
@@ -219,7 +219,7 @@ def test_acl_lua_api(cluster: Cluster):
ReturnError,
match=rf"unsupported privilege 'read,write', see pico.help\('{f}'\) for details",
):
- i1.call(f"pico.{f}", "Dave", "read,write", "universe", None)
+ i1.call(f"pico.{f}", "Dave", "read,write", "role", None)
# No object_type -> error.
with pytest.raises(ReturnError, match="object_type should be a string"):
@@ -463,9 +463,7 @@ def test_acl_basic(cluster: Cluster):
# Grant some privileges.
# Doing anything via remote function execution requires execute access
# to the "universe"
- index = i1.grant_privilege(user, "execute", "universe")
- cluster.raft_wait_index(index)
- v += 1
+ cluster.grant_box_privilege(user, "execute", "universe")
index = i1.grant_privilege(user, "read", "table", "money")
cluster.raft_wait_index(index)
@@ -592,8 +590,7 @@ def test_acl_roles_basic(cluster: Cluster):
# Doing anything via remote function execution requires execute access
# to the "universe"
- index = i1.grant_privilege(user, "execute", "universe", None)
- cluster.raft_wait_index(index)
+ cluster.grant_box_privilege(user, "execute", "universe", None)
# Try reading from table on behalf of the user.
for i in cluster.instances:
@@ -757,8 +754,8 @@ def test_acl_from_snapshot(cluster: Cluster):
("read", "space", "_pico_table"),
}
- with pytest.raises(TarantoolError, match="Role 'Executor' is not found"):
- i.call("box.schema.role.info", "Executor")
+ with pytest.raises(TarantoolError, match="Role 'Writer' is not found"):
+ i.call("box.schema.role.info", "Writer")
#
# These will be catching up by snapshot.
@@ -791,13 +788,13 @@ def test_acl_from_snapshot(cluster: Cluster):
)
cluster.raft_wait_index(index)
- index = i1.call("pico.create_role", "Executor")
+ index = i1.call("pico.create_role", "Writer")
cluster.raft_wait_index(index)
- index = i1.grant_privilege("Executor", "execute", "universe", None)
+ index = i1.grant_privilege("Writer", "write", "table", None)
cluster.raft_wait_index(index)
- index = i1.grant_privilege("Blam", "execute", "role", "Executor")
+ index = i1.grant_privilege("Blam", "execute", "role", "Writer")
cluster.raft_wait_index(index)
# Compact log to trigger snapshot generation.
@@ -822,7 +819,7 @@ def test_acl_from_snapshot(cluster: Cluster):
assert to_set_of_tuples(i.call("box.schema.user.info", "Blam")) == {
("execute", "role", "public"),
- ("execute", "role", "Executor"),
+ ("execute", "role", "Writer"),
("session,usage", "universe", ""),
("alter", "user", "Blam"),
}
@@ -831,8 +828,8 @@ def test_acl_from_snapshot(cluster: Cluster):
("read", "space", "_pico_instance"),
}
- assert to_set_of_tuples(i.call("box.schema.role.info", "Executor")) == {
- ("execute", "universe", ""),
+ assert to_set_of_tuples(i.call("box.schema.role.info", "Writer")) == {
+ ("write", "space", ""),
}
@@ -886,16 +883,18 @@ def test_builtin_users_and_roles(cluster: Cluster):
"_pico_property",
)
+ index = i1.call("pico.create_user", "Dave", VALID_PASSWORD)
+ i1.raft_wait_index(index)
+
# granting already granted privilege does not raise an error
index = i1.call("pico.raft_get_index")
new_index = i1.grant_privilege(
- "admin",
- "write",
+ "Dave",
+ "session",
"universe",
)
assert index == new_index
- index = i1.call("pico.create_user", "Dave", VALID_PASSWORD)
new_index = i1.grant_privilege(
"Dave",
"execute",
diff --git a/test/int/test_basics.py b/test/int/test_basics.py
index 5516edd052b4575385775456a30211b5f89231ec..e3011a8c5cc2997343bce6601a2d655313f151aa 100644
--- a/test/int/test_basics.py
+++ b/test/int/test_basics.py
@@ -257,27 +257,19 @@ def test_raft_log(instance: Instance):
| 15 | 1 |1.0.15|Insert({_pico_role}, [31,"super",0,1])|
| 16 | 1 |1.0.16|Insert({_pico_privilege}, ["usage","universe",0,0,1,0])|
| 17 | 1 |1.0.17|Insert({_pico_privilege}, ["session","universe",0,0,1,0])|
-| 18 | 1 |1.0.18|Insert({_pico_privilege}, ["execute","role",2,0,1,0])|
-| 19 | 1 |1.0.19|Insert({_pico_privilege}, ["read","universe",0,1,1,0])|
-| 20 | 1 |1.0.20|Insert({_pico_privilege}, ["write","universe",0,1,1,0])|
-| 21 | 1 |1.0.21|Insert({_pico_privilege}, ["execute","universe",0,1,1,0])|
-| 22 | 1 |1.0.22|Insert({_pico_privilege}, ["session","universe",0,1,1,0])|
-| 23 | 1 |1.0.23|Insert({_pico_privilege}, ["usage","universe",0,1,1,0])|
-| 24 | 1 |1.0.24|Insert({_pico_privilege}, ["create","universe",0,1,1,0])|
-| 25 | 1 |1.0.25|Insert({_pico_privilege}, ["drop","universe",0,1,1,0])|
-| 26 | 1 |1.0.26|Insert({_pico_privilege}, ["alter","universe",0,1,1,0])|
-| 27 | 1 |1.0.27|Insert({_pico_privilege}, ["grant","universe",0,1,1,0])|
-| 28 | 1 |1.0.28|Insert({_pico_privilege}, ["revoke","universe",0,1,1,0])|
-| 29 | 1 | |AddNode(1)|
-| 30 | 2 | |-|
-| 31 | 2 |1.1.1|Replace({_pico_instance}, ["i1","68d4a766-4144-3248-aeb4-e212356716e4",1,"r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07",["Offline",0],["Online",1],{b},"default"])|
-| 32 | 2 |1.1.2|Insert({_pico_replicaset}, ["r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07","i1","default",0.0,"auto","not-ready"])|
-| 33 | 2 |1.1.3|Replace({_pico_instance}, ["i1","68d4a766-4144-3248-aeb4-e212356716e4",1,"r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07",["Replicated",1],["Online",1],{b},"default"])|
-| 34 | 2 |1.1.4|Update({_pico_replicaset}, ["r1"], [["=","weight",1.0], ["=","state","ready"]])|
-| 35 | 2 |1.1.5|Replace({_pico_property}, ["target_vshard_config",[{{"e0df68c5-e7f9-395f-86b3-30ad9e1b7b07":[{{"68d4a766-4144-3248-aeb4-e212356716e4":["guest:@127.0.0.1:{p}","i1",true]}},1.0]}},"on"]])|
-| 36 | 2 |1.1.6|Replace({_pico_property}, ["current_vshard_config",[{{"e0df68c5-e7f9-395f-86b3-30ad9e1b7b07":[{{"68d4a766-4144-3248-aeb4-e212356716e4":["guest:@127.0.0.1:{p}","i1",true]}},1.0]}},"on"]])|
-| 37 | 2 |1.1.7|Replace({_pico_property}, ["vshard_bootstrapped",true])|
-| 38 | 2 |1.1.8|Replace({_pico_instance}, ["i1","68d4a766-4144-3248-aeb4-e212356716e4",1,"r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07",["Online",1],["Online",1],{b},"default"])|
+| 18 | 1 |1.0.18|Insert({_pico_privilege}, ["usage","universe",0,1,1,0])|
+| 19 | 1 |1.0.19|Insert({_pico_privilege}, ["session","universe",0,1,1,0])|
+| 20 | 1 |1.0.20|Insert({_pico_privilege}, ["execute","role",2,0,1,0])|
+| 21 | 1 | |AddNode(1)|
+| 22 | 2 | |-|
+| 23 | 2 |1.1.1|Replace({_pico_instance}, ["i1","68d4a766-4144-3248-aeb4-e212356716e4",1,"r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07",["Offline",0],["Online",1],{b},"default"])|
+| 24 | 2 |1.1.2|Insert({_pico_replicaset}, ["r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07","i1","default",0.0,"auto","not-ready"])|
+| 25 | 2 |1.1.3|Replace({_pico_instance}, ["i1","68d4a766-4144-3248-aeb4-e212356716e4",1,"r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07",["Replicated",1],["Online",1],{b},"default"])|
+| 26 | 2 |1.1.4|Update({_pico_replicaset}, ["r1"], [["=","weight",1.0], ["=","state","ready"]])|
+| 27 | 2 |1.1.5|Replace({_pico_property}, ["target_vshard_config",[{{"e0df68c5-e7f9-395f-86b3-30ad9e1b7b07":[{{"68d4a766-4144-3248-aeb4-e212356716e4":["guest:@127.0.0.1:{p}","i1",true]}},1.0]}},"on"]])|
+| 28 | 2 |1.1.6|Replace({_pico_property}, ["current_vshard_config",[{{"e0df68c5-e7f9-395f-86b3-30ad9e1b7b07":[{{"68d4a766-4144-3248-aeb4-e212356716e4":["guest:@127.0.0.1:{p}","i1",true]}},1.0]}},"on"]])|
+| 29 | 2 |1.1.7|Replace({_pico_property}, ["vshard_bootstrapped",true])|
+| 30 | 2 |1.1.8|Replace({_pico_instance}, ["i1","68d4a766-4144-3248-aeb4-e212356716e4",1,"r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07",["Online",1],["Online",1],{b},"default"])|
+-----+----+-----+--------+
""".format( # noqa: E501
p=instance.port,
@@ -291,7 +283,7 @@ def test_raft_log(instance: Instance):
_pico_user=space_id("_pico_user"),
_pico_role=space_id("_pico_role"),
)
- assert strip_spaces(expected) == strip_spaces(raft_log)
+ assert strip_spaces(raft_log) == strip_spaces(expected)
def test_governor_notices_restarts(instance: Instance):
diff --git a/test/int/test_sql.py b/test/int/test_sql.py
index 960982c2b609eba67c70588cdb33c406982a1275..528192325c636df74ba753279c3cf328ded4f212 100644
--- a/test/int/test_sql.py
+++ b/test/int/test_sql.py
@@ -936,8 +936,6 @@ def test_sql_alter_login(cluster: Cluster):
# * TODO: Check SESSION privilege is removed.
-# TODO: replace all Lua `i1.call`s to SQL `iq.sql`.
-# See https://git.picodata.io/picodata/picodata/sbroad/-/issues/492.
def test_sql_acl_privileges(cluster: Cluster):
cluster.deploy(instance_count=2)
i1, i2 = cluster.instances
@@ -977,9 +975,6 @@ def test_sql_acl_privileges(cluster: Cluster):
)
assert ddl["row_count"] == 1
- # Grant remote functions call.
- i1.grant_privilege(username, "execute", "universe", None)
-
# Remember number of default privileges.
default_privileges_number = len(
i1.sql(""" select * from "_pico_privilege" """)["rows"]