From 53dbcc699bffaacdd695945959067cc9d0603bef Mon Sep 17 00:00:00 2001
From: Dmitry Rodionov <d.rodionov@picodata.io>
Date: Wed, 29 Nov 2023 20:03:28 +0300
Subject: [PATCH] refactor: make fields of PrivilegeDef private
This allows future constructors to enforce invariants.
Additionally this commit removes redundant set_schema_version on Op by
passing schema version to corresponding Op constructors.
---
src/access_control.rs | 35 +++++------
src/schema.rs | 82 ++++++++++++++++++++------
src/sql.rs | 64 +++++++++-----------
src/storage.rs | 133 ++++++++++++++++++++++--------------------
src/traft/op.rs | 65 +++++++--------------
5 files changed, 201 insertions(+), 178 deletions(-)
diff --git a/src/access_control.rs b/src/access_control.rs
index 0600f49fbc..374f89b131 100644
--- a/src/access_control.rs
+++ b/src/access_control.rs
@@ -226,7 +226,7 @@ fn access_check_grant_revoke(
if grantor_id != ADMIN_USER_ID {
return Err(make_access_denied(
access_name,
- priv_def.object_type,
+ priv_def.object_type(),
"",
grantor.name,
));
@@ -236,7 +236,7 @@ fn access_check_grant_revoke(
Some(object_id) => object_id,
};
- match priv_def.object_type {
+ match priv_def.object_type() {
PicoSchemaObjectType::Universe => {
// Only admin can grant on universe
if grantor_id != ADMIN_USER_ID {
@@ -284,7 +284,8 @@ fn access_check_grant_revoke(
// Note that having a role means having execute privilege on it.
if sys_role_meta.owner_id != grantor_id
&& grantor_id != ADMIN_USER_ID
- && !(sys_role_meta.name == "public" && priv_def.privilege == PrivilegeType::Execute)
+ && !(sys_role_meta.name == "public"
+ && priv_def.privilege() == PrivilegeType::Execute)
{
return Err(make_access_denied(
access_name,
@@ -524,20 +525,20 @@ mod tests {
grantee_id: UserId,
grantor_id: Option<UserId>,
) {
- let priv_def = PrivilegeDef {
- grantor_id: grantor_id.unwrap_or(session::uid().unwrap()),
- grantee_id,
+ let priv_def = PrivilegeDef::new(
+ privilege,
object_type,
object_id,
- privilege,
- schema_version: 0,
- };
+ grantee_id,
+ grantor_id.unwrap_or(session::uid().unwrap()),
+ 0,
+ );
access_check_op(
&Op::Acl(Acl::GrantPrivilege {
priv_def: priv_def.clone(),
}),
- priv_def.grantor_id,
+ priv_def.grantor_id(),
)
.unwrap();
@@ -551,20 +552,20 @@ mod tests {
object_type: SchemaObjectType,
object_id: i64,
) {
- let priv_def = PrivilegeDef {
- grantor_id: session::uid().unwrap(),
- grantee_id,
+ let priv_def = PrivilegeDef::new(
+ privilege,
object_type,
object_id,
- privilege,
- schema_version: 0,
- };
+ grantee_id,
+ session::uid().unwrap(),
+ 0,
+ );
access_check_op(
&Op::Acl(Acl::RevokePrivilege {
priv_def: priv_def.clone(),
}),
- priv_def.grantor_id,
+ priv_def.grantor_id(),
)
.unwrap();
on_master_revoke_privilege(&priv_def).unwrap()
diff --git a/src/schema.rs b/src/schema.rs
index 10d9cbfb98..a841dabed7 100644
--- a/src/schema.rs
+++ b/src/schema.rs
@@ -389,8 +389,8 @@ tarantool::define_str_enum! {
/// Privilege definition.
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Hash)]
pub struct PrivilegeDef {
- pub privilege: PrivilegeType,
- pub object_type: SchemaObjectType,
+ privilege: PrivilegeType,
+ object_type: SchemaObjectType,
/// `-1` denotes an absense of a target object.
/// Other values should be >= 0 and denote an existing target object.
/// When working with `object_type` `universe` it might seem that it does
@@ -398,19 +398,78 @@ pub struct PrivilegeDef {
/// universe has a target object with `object_id == 0`.
///
/// To get the value of this field as `Option<u32>` see [`Self::object_id`]
- pub object_id: i64,
+ object_id: i64,
/// Id of the user or role to whom the privilege is granted.
///
/// In tarantool users and roles are stored in the same space, which means a
/// role and a user cannot have the same id or name.
- pub grantee_id: UserId,
- pub grantor_id: UserId,
- pub schema_version: u64,
+ grantee_id: UserId,
+ grantor_id: UserId,
+ schema_version: u64,
}
impl Encode for PrivilegeDef {}
impl PrivilegeDef {
+ pub fn new(
+ privilege: PrivilegeType,
+ object_type: SchemaObjectType,
+ object_id: i64,
+ grantee_id: UserId,
+ grantor_id: UserId,
+ schema_version: u64,
+ ) -> PrivilegeDef {
+ PrivilegeDef {
+ privilege,
+ object_type,
+ object_id,
+ grantee_id,
+ grantor_id,
+ schema_version,
+ }
+ }
+
+ #[inline(always)]
+ pub fn privilege(&self) -> PrivilegeType {
+ self.privilege
+ }
+
+ #[inline(always)]
+ pub fn object_type(&self) -> SchemaObjectType {
+ self.object_type
+ }
+
+ /// Get `object_id` field interpreting `-1` as `None`.
+ #[inline(always)]
+ pub fn object_id(&self) -> Option<u32> {
+ if self.object_id >= 0 {
+ Some(self.object_id as _)
+ } else {
+ debug_assert_eq!(self.object_id, -1, "object_id should be >= -1");
+ None
+ }
+ }
+
+ #[inline(always)]
+ pub fn object_id_raw(&self) -> i64 {
+ self.object_id
+ }
+
+ #[inline(always)]
+ pub fn grantee_id(&self) -> UserId {
+ self.grantee_id
+ }
+
+ #[inline(always)]
+ pub fn grantor_id(&self) -> UserId {
+ self.grantor_id
+ }
+
+ #[inline(always)]
+ pub fn schema_version(&self) -> u64 {
+ self.schema_version
+ }
+
/// Format of the _pico_privilege global table.
#[inline(always)]
pub fn format() -> Vec<tarantool::space::Field> {
@@ -438,17 +497,6 @@ impl PrivilegeDef {
}
}
- /// Get `object_id` field interpreting `-1` as `None`.
- #[inline(always)]
- pub fn object_id(&self) -> Option<u32> {
- if self.object_id >= 0 {
- Some(self.object_id as _)
- } else {
- debug_assert_eq!(self.object_id, -1, "object_id should be >= -1");
- None
- }
- }
-
pub fn get_default_privileges() -> &'static Vec<PrivilegeDef> {
static DEFAULT_PRIVILEGES: OnceCell<Vec<PrivilegeDef>> = OnceCell::new();
DEFAULT_PRIVILEGES.get_or_init(|| {
diff --git a/src/sql.rs b/src/sql.rs
index 173cffeca0..407152b392 100644
--- a/src/sql.rs
+++ b/src/sql.rs
@@ -718,8 +718,10 @@ fn reenterable_schema_change_request(
continue 'retry;
}
+ let schema_version = storage.properties.next_schema_version()?;
+
// Check for conflicts and make the op
- let mut op = match ¶ms {
+ let op = match ¶ms {
Params::CreateTable(params) => {
if params.space_exists()? {
// Space already exists, no op needed
@@ -736,8 +738,7 @@ fn reenterable_schema_change_request(
params.test_create_space(storage)?;
let ddl = params.into_ddl()?;
Op::DdlPrepare {
- // This field will be updated later.
- schema_version: 0,
+ schema_version,
ddl,
}
}
@@ -748,8 +749,7 @@ fn reenterable_schema_change_request(
};
let ddl = OpDdl::DropTable { id: space_def.id };
Op::DdlPrepare {
- // This field will be updated later.
- schema_version: 0,
+ schema_version,
ddl,
}
}
@@ -770,8 +770,7 @@ fn reenterable_schema_change_request(
let user_def = UserDef {
id,
name: name.clone(),
- // This field will be updated later.
- schema_version: 0,
+ schema_version,
auth: auth.clone(),
owner: current_user,
};
@@ -794,15 +793,14 @@ fn reenterable_schema_change_request(
let object_type = SchemaObjectType::Universe;
let object_id = 0;
let privilege = PrivilegeType::Session;
- let priv_def = PrivilegeDef {
- grantor_id,
- grantee_id,
+ let priv_def = PrivilegeDef::new(
+ privilege,
object_type,
object_id,
- privilege,
- // This field will be updated later.
- schema_version: 0,
- };
+ grantee_id,
+ grantor_id,
+ schema_version,
+ );
match alter_option_param {
AlterOptionParam::ChangePassword(auth) => {
@@ -813,8 +811,7 @@ fn reenterable_schema_change_request(
Op::Acl(OpAcl::ChangeAuth {
user_id: user_def.id,
auth: auth.clone(),
- // This field will be updated later.
- schema_version: 0,
+ schema_version,
})
}
@@ -853,8 +850,7 @@ fn reenterable_schema_change_request(
};
Op::Acl(OpAcl::DropUser {
user_id: user_def.id,
- // This field will be updated later.
- schema_version: 0,
+ schema_version,
})
}
Params::CreateRole(name) => {
@@ -878,7 +874,7 @@ fn reenterable_schema_change_request(
id,
name: name.clone(),
// This field will be updated later.
- schema_version: 0,
+ schema_version,
owner: current_user,
};
Op::Acl(OpAcl::CreateRole { role_def })
@@ -890,8 +886,7 @@ fn reenterable_schema_change_request(
};
Op::Acl(OpAcl::DropRole {
role_id: role_def.id,
- // This field will be updated later.
- schema_version: 0,
+ schema_version,
})
}
Params::GrantPrivilege(grant_type, grantee_name) => {
@@ -910,15 +905,14 @@ fn reenterable_schema_change_request(
return Ok(ConsumerResult { row_count: 0 });
}
Op::Acl(OpAcl::GrantPrivilege {
- priv_def: PrivilegeDef {
- grantor_id,
- grantee_id,
+ priv_def: PrivilegeDef::new(
+ privilege,
object_type,
object_id,
- privilege,
- // This field will be updated later.
- schema_version: 0,
- },
+ grantee_id,
+ grantor_id,
+ schema_version,
+ ),
})
}
Params::RevokePrivilege(revoke_type, grantee_name) => {
@@ -938,19 +932,17 @@ fn reenterable_schema_change_request(
}
Op::Acl(OpAcl::RevokePrivilege {
- priv_def: PrivilegeDef {
- grantor_id,
- grantee_id,
+ priv_def: PrivilegeDef::new(
+ privilege,
object_type,
object_id,
- privilege,
- // This field will be updated later.
- schema_version: 0,
- },
+ grantee_id,
+ grantor_id,
+ schema_version,
+ ),
})
}
};
- op.set_schema_version(storage.properties.next_schema_version()?);
let is_ddl_prepare = matches!(op, Op::DdlPrepare { .. });
let term = raft::Storage::term(&node.raft_storage, index)?;
diff --git a/src/storage.rs b/src/storage.rs
index a9d313b613..fb89dbade1 100644
--- a/src/storage.rs
+++ b/src/storage.rs
@@ -669,7 +669,7 @@ impl Clusterwide {
old_role_versions.insert(def.id, def.schema_version);
}
for def in self.privileges.iter()? {
- let schema_version = def.schema_version;
+ let schema_version = def.schema_version();
old_priv_versions.insert(def, schema_version);
}
@@ -2465,10 +2465,10 @@ impl Privileges {
pub fn delete_all_by_grantee_id(&self, grantee_id: UserId) -> tarantool::Result<()> {
for priv_def in self.by_grantee_id(grantee_id)? {
self.delete(
- priv_def.grantee_id,
- &priv_def.object_type,
- priv_def.object_id,
- &priv_def.privilege,
+ priv_def.grantee_id(),
+ &priv_def.object_type(),
+ priv_def.object_id_raw(),
+ &priv_def.privilege(),
)?;
}
Ok(())
@@ -2478,15 +2478,15 @@ impl Privileges {
#[inline]
pub fn delete_all_by_granted_role(&self, role_id: u32) -> Result<()> {
for priv_def in self.iter()? {
- if priv_def.privilege == PrivilegeType::Execute
- && priv_def.object_type == SchemaObjectType::Role
- && priv_def.object_id == role_id as i64
+ if priv_def.privilege() == PrivilegeType::Execute
+ && priv_def.object_type() == SchemaObjectType::Role
+ && priv_def.object_id() == Some(role_id)
{
self.delete(
- priv_def.grantee_id,
- &priv_def.object_type,
- priv_def.object_id,
- &priv_def.privilege,
+ priv_def.grantee_id(),
+ &priv_def.object_type(),
+ priv_def.object_id_raw(),
+ &priv_def.privilege(),
)?;
}
}
@@ -2501,11 +2501,12 @@ impl Privileges {
object_id: i64,
) -> tarantool::Result<()> {
for priv_def in self.by_object(object_type, object_id)? {
+ debug_assert_eq!(priv_def.object_id_raw(), object_id);
self.delete(
- priv_def.grantee_id,
- &priv_def.object_type,
- priv_def.object_id,
- &priv_def.privilege,
+ priv_def.grantee_id(),
+ &priv_def.object_type(),
+ object_id,
+ &priv_def.privilege(),
)?;
}
Ok(())
@@ -2729,7 +2730,9 @@ impl SchemaDef for PrivilegeDef {
#[inline(always)]
fn schema_version(&self) -> u64 {
- self.schema_version
+ // 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()
}
#[inline(always)]
@@ -2767,8 +2770,8 @@ pub mod acl {
&self,
storage: &Clusterwide,
) -> tarantool::Result<(&'static str, String)> {
- let role_def = storage.roles.by_id(self.grantee_id)?;
- let user_def = storage.users.by_id(self.grantee_id)?;
+ let role_def = storage.roles.by_id(self.grantee_id())?;
+ let user_def = storage.users.by_id(self.grantee_id())?;
match (role_def, user_def) {
(Some(role_def), None) => Ok(("role", role_def.name)),
(None, Some(user_def)) => Ok(("user", user_def.name)),
@@ -2785,41 +2788,45 @@ pub mod acl {
) -> [PrivilegeDef; 4] {
[
// SQL: GRANT 'public' TO <user_name>
- PrivilegeDef {
- privilege: PrivilegeType::Execute,
- object_type: SchemaObjectType::Role,
- object_id: PUBLIC_ID as _,
- grantee_id: user_def.id,
+ PrivilegeDef::new(
+ PrivilegeType::Execute,
+ SchemaObjectType::Role,
+ PUBLIC_ID as _,
+ user_def.id,
grantor_id,
- schema_version: user_def.schema_version,
- },
+ user_def.schema_version,
+ )
+ .expect("valid"),
// ALTER USER <user_name> LOGIN
- PrivilegeDef {
- privilege: PrivilegeType::Session,
- object_type: SchemaObjectType::Universe,
- object_id: UNIVERSE_ID,
- grantee_id: user_def.id,
- grantor_id: ADMIN_ID,
- schema_version: user_def.schema_version,
- },
+ PrivilegeDef::new(
+ PrivilegeType::Session,
+ SchemaObjectType::Universe,
+ UNIVERSE_ID,
+ user_def.id,
+ ADMIN_ID,
+ user_def.schema_version,
+ )
+ .expect("valid"),
// ALTER USER <user_name> ENABLE
- PrivilegeDef {
- privilege: PrivilegeType::Usage,
- object_type: SchemaObjectType::Universe,
- object_id: UNIVERSE_ID,
- grantee_id: user_def.id,
- grantor_id: ADMIN_ID,
- schema_version: user_def.schema_version,
- },
+ PrivilegeDef::new(
+ PrivilegeType::Usage,
+ SchemaObjectType::Universe,
+ UNIVERSE_ID,
+ user_def.id,
+ ADMIN_ID,
+ user_def.schema_version,
+ )
+ .expect("valid"),
// SQL: GRANT ALTER ON <user_name> TO <user_name>
- PrivilegeDef {
- privilege: PrivilegeType::Alter,
- object_type: SchemaObjectType::User,
- object_id: user_def.id as _,
- grantee_id: user_def.id,
+ PrivilegeDef::new(
+ PrivilegeType::Alter,
+ SchemaObjectType::User,
+ user_def.id as _,
+ user_def.id,
grantor_id,
- schema_version: user_def.schema_version,
- },
+ user_def.schema_version,
+ )
+ .expect("valid"),
]
}
@@ -2927,8 +2934,8 @@ pub mod acl {
) -> tarantool::Result<()> {
storage.privileges.insert(priv_def)?;
- let privilege = &priv_def.privilege;
- let (object, object_type) = (priv_def.object_id(), &priv_def.object_type);
+ let privilege = priv_def.privilege();
+ let (object, object_type) = (priv_def.object_id(), priv_def.object_type());
let (grantee_type, grantee) = priv_def.grantee_type_and_name(storage)?;
match (privilege.as_str(), object_type.as_str()) {
@@ -2972,14 +2979,14 @@ pub mod acl {
) -> tarantool::Result<()> {
// FIXME: currently there's no way to revoke a default privilege
storage.privileges.delete(
- priv_def.grantee_id,
- &priv_def.object_type,
- priv_def.object_id,
- &priv_def.privilege,
+ priv_def.grantee_id(),
+ &priv_def.object_type(),
+ priv_def.object_id_raw(),
+ &priv_def.privilege(),
)?;
- let privilege = &priv_def.privilege;
- let (object, object_type) = (priv_def.object_id(), &priv_def.object_type);
+ let privilege = priv_def.privilege();
+ let (object, object_type) = (priv_def.object_id(), &priv_def.object_type());
let (grantee_type, grantee) = priv_def.grantee_type_and_name(storage)?;
match (privilege.as_str(), object_type.as_str()) {
@@ -3131,9 +3138,9 @@ pub mod acl {
box.schema.role.grant(grantee_id, privilege, object_type, object_id)
end",
(
- priv_def.grantee_id,
- &priv_def.privilege,
- &priv_def.object_type.into_tarantool(),
+ priv_def.grantee_id(),
+ &priv_def.privilege(),
+ &priv_def.object_type().into_tarantool(),
priv_def.object_id(),
),
)
@@ -3158,9 +3165,9 @@ pub mod acl {
box.schema.role.revoke(grantee_id, privilege, object_type, object_id)
end",
(
- priv_def.grantee_id,
- &priv_def.privilege,
- &priv_def.object_type.into_tarantool(),
+ priv_def.grantee_id(),
+ &priv_def.privilege(),
+ &priv_def.object_type().into_tarantool(),
priv_def.object_id(),
),
)
diff --git a/src/traft/op.rs b/src/traft/op.rs
index 3cbaf2972d..ea6842b963 100644
--- a/src/traft/op.rs
+++ b/src/traft/op.rs
@@ -153,27 +153,27 @@ impl std::fmt::Display for Op {
}
Self::Acl(Acl::GrantPrivilege { priv_def }) => {
let object_id = priv_def.object_id();
- let PrivilegeDef {
- grantee_id,
- grantor_id,
- object_type,
- privilege,
- schema_version,
- ..
- } = priv_def;
- write!(f, "GrantPrivilege({schema_version}, {grantor_id}, {grantee_id}, {object_type}, {object_id:?}, {privilege})")
+
+ write!(
+ f,
+ "GrantPrivilege({schema_version}, {grantor_id}, {grantee_id}, {object_type}, {object_id:?}, {privilege})",
+ schema_version = priv_def.schema_version(),
+ grantor_id = priv_def.grantor_id(),
+ grantee_id = priv_def.grantee_id(),
+ object_type = priv_def.object_type(),
+ privilege = priv_def.privilege(),
+ )
}
Self::Acl(Acl::RevokePrivilege { priv_def }) => {
let object_id = priv_def.object_id();
- let PrivilegeDef {
- grantee_id,
- grantor_id,
- object_type,
- privilege,
- schema_version,
- ..
- } = priv_def;
- write!(f, "RevokePrivilege({schema_version}, {grantor_id}, {grantee_id}, {object_type}, {object_id:?}, {privilege})")
+ write!(
+ f,
+ "RevokePrivilege({schema_version}, {grantor_id}, {grantee_id}, {object_type}, {object_id:?}, {privilege})",
+ schema_version = priv_def.schema_version(),
+ grantor_id = priv_def.grantor_id(),
+ grantee_id = priv_def.grantee_id(),
+ object_type = priv_def.object_type(),
+ privilege = priv_def.privilege(),)
}
};
@@ -270,19 +270,6 @@ impl Op {
Self::DdlPrepare { .. } | Self::Acl(_) => true,
}
}
-
- #[inline]
- pub fn set_schema_version(&mut self, new_schema_version: u64) {
- match self {
- Self::Nop | Self::Dml(_) | Self::DdlAbort | Self::DdlCommit => {}
- Self::DdlPrepare { schema_version, .. } => {
- *schema_version = new_schema_version;
- }
- Self::Acl(acl) => {
- acl.set_schema_version(new_schema_version);
- }
- }
- }
}
// TODO: remove this
@@ -576,20 +563,8 @@ impl Acl {
Self::DropUser { schema_version, .. } => *schema_version,
Self::CreateRole { role_def, .. } => role_def.schema_version,
Self::DropRole { schema_version, .. } => *schema_version,
- Self::GrantPrivilege { priv_def } => priv_def.schema_version,
- Self::RevokePrivilege { priv_def } => priv_def.schema_version,
- }
- }
-
- pub fn set_schema_version(&mut self, new_schema_version: u64) {
- match self {
- Self::CreateUser { user_def } => user_def.schema_version = new_schema_version,
- Self::ChangeAuth { schema_version, .. } => *schema_version = new_schema_version,
- Self::DropUser { schema_version, .. } => *schema_version = new_schema_version,
- Self::CreateRole { role_def, .. } => role_def.schema_version = new_schema_version,
- Self::DropRole { schema_version, .. } => *schema_version = new_schema_version,
- Self::GrantPrivilege { priv_def } => priv_def.schema_version = new_schema_version,
- Self::RevokePrivilege { priv_def } => priv_def.schema_version = new_schema_version,
+ Self::GrantPrivilege { priv_def } => priv_def.schema_version(),
+ Self::RevokePrivilege { priv_def } => priv_def.schema_version(),
}
}
}
--
GitLab