diff --git a/src/cas.rs b/src/cas.rs
index facfd485bf1577d4aa72c4788e576eb942d4b972..523849b2c47b9976480cb83f51dca90cb57b8b15 100644
--- a/src/cas.rs
+++ b/src/cas.rs
@@ -159,7 +159,7 @@ fn proc_cas_local(req: Request) -> Result<Response> {
});
}
if status.leader_id != Some(node.raft_id()) {
- // Nearly impossible error indicating invalid request.
+ // Invalid request. This node is not a leader at this moment.
return Err(TraftError::NotALeader);
}
@@ -274,23 +274,31 @@ fn proc_cas_local(req: Request) -> Result<Response> {
req.predicate.check_entry(entry.index, &op, storage)?;
}
- if let Op::Dml(dml) = &req.op {
- // Check if the requested dml is applicable to the local storage.
- // This will run the required on_replace triggers which will check among
- // other things conformity to space format, user defined constraints etc.
- //
- // FIXME: this works for explicit constraints which would be directly
- // violated by the operation, but there are still some cases where an
- // invalid dml operation would be proposed to the raft log, which would
- // fail to apply, e.g. if there's a number of dml operations in flight
- // which conflict via the secondary key.
- // To fix these cases we would need to implement the so-called "limbo".
- // See https://git.picodata.io/picodata/picodata/picodata/-/issues/368
- transaction::begin()?;
- let res = storage.do_dml(dml);
- transaction::rollback().expect("can't fail");
- // Return the error if it happened. Ignore the tuple if there was one.
- _ = res?;
+ // Performs preliminary checks on an operation so that it will not fail when applied.
+ match &req.op {
+ Op::Dml(dml) => {
+ // Check if the requested dml is applicable to the local storage.
+ // This will run the required on_replace triggers which will check among
+ // other things conformity to space format, user defined constraints etc.
+ //
+ // FIXME: this works for explicit constraints which would be directly
+ // violated by the operation, but there are still some cases where an
+ // invalid dml operation would be proposed to the raft log, which would
+ // fail to apply, e.g. if there's a number of dml operations in flight
+ // which conflict via the secondary key.
+ // To fix these cases we would need to implement the so-called "limbo".
+ // See https://git.picodata.io/picodata/picodata/picodata/-/issues/368
+ transaction::begin()?;
+ let res = storage.do_dml(dml);
+ transaction::rollback().expect("can't fail");
+ // Return the error if it happened. Ignore the tuple if there was one.
+ _ = res?;
+ }
+ // We can't use a `do_acl` inside of a transaction here similarly to `do_dml`,
+ // as acl should be applied only on replicaset leader. Raft leader is not always
+ // a replicaset leader.
+ Op::Acl(acl) => acl.validate()?,
+ _ => (),
}
// Don't wait for the proposal to be accepted, instead return the index
diff --git a/src/traft/op.rs b/src/traft/op.rs
index 9d99d5cd9c657bc7fddb119aa38e15912da103e1..5dd9fe48666556d0881e2a1ab3364c0f11fc2b9a 100644
--- a/src/traft/op.rs
+++ b/src/traft/op.rs
@@ -1,6 +1,9 @@
-use crate::schema::{Distribution, PrivilegeDef, RoleDef, UserDef};
+use crate::schema::{
+ Distribution, PrivilegeDef, RoleDef, UserDef, ADMIN_ID, GUEST_ID, PUBLIC_ID, SUPER_ID,
+};
use crate::storage::space_by_name;
use crate::storage::Clusterwide;
+use crate::traft::error::Error as TRaftError;
use ::tarantool::auth::AuthDef;
use ::tarantool::index::{IndexId, Part};
use ::tarantool::space::{Field, SpaceId};
@@ -593,6 +596,40 @@ impl Acl {
Self::RevokePrivilege { priv_def, .. } => priv_def.schema_version(),
}
}
+
+ /// Performs preliminary checks on acl so that it will not fail when applied.
+ /// These checks do not include authorization checks, which are done separately in
+ /// [`crate::access_control::access_check_op`].
+ pub fn validate(&self) -> Result<(), TRaftError> {
+ // THOUGHT: should we move access_check_* fns here as it's done in tarantool?
+ match self {
+ Self::ChangeAuth { user_id, .. } => {
+ // See https://git.picodata.io/picodata/tarantool/-/blob/da5ad0fa3ab8940f524cfa9bf3d582347c01fc4a/src/box/alter.cc#L2925
+ if *user_id == GUEST_ID {
+ return Err(TRaftError::other(
+ "altering guest user's password is not allowed",
+ ));
+ }
+ }
+ Self::DropUser { user_id, .. } => {
+ // See https://git.picodata.io/picodata/tarantool/-/blob/da5ad0fa3ab8940f524cfa9bf3d582347c01fc4a/src/box/alter.cc#L3080
+ if *user_id == GUEST_ID || *user_id == ADMIN_ID {
+ return Err(TRaftError::other("dropping system user is not allowed"));
+ }
+ // user_has_data will be successful in any case https://git.picodata.io/picodata/tarantool/-/blob/da5ad0fa3ab8940f524cfa9bf3d582347c01fc4a/src/box/alter.cc#L2846
+ // as box.schema.user.drop(..) deletes all the related spaces/priveleges/etc.
+ }
+
+ Self::DropRole { role_id, .. } => {
+ // See https://git.picodata.io/picodata/tarantool/-/blob/da5ad0fa3ab8940f524cfa9bf3d582347c01fc4a/src/box/alter.cc#L3080
+ if *role_id == PUBLIC_ID || *role_id == SUPER_ID {
+ return Err(TRaftError::other("dropping system role is not allowed"));
+ }
+ }
+ _ => (),
+ }
+ Ok(())
+ }
}
mod vec_of_raw_byte_buf {
diff --git a/test/int/test_acl.py b/test/int/test_acl.py
index 2182fe67a39222e8c7bd6f46e79b1989cba28e94..b09153f432d39fd668d900e3862db1662097adb0 100644
--- a/test/int/test_acl.py
+++ b/test/int/test_acl.py
@@ -1053,5 +1053,27 @@ def test_circular_grants_for_role(cluster: Cluster):
throws_on_grant("Son", "Son")
+def test_alter_system_user(cluster: Cluster):
+ i1, *_ = cluster.deploy(instance_count=1)
+
+ with pytest.raises(
+ ReturnError,
+ match="altering guest user's password is not allowed",
+ ):
+ i1.sql("alter user \"guest\" with password '12345678'")
+
+ with pytest.raises(
+ ReturnError,
+ match="dropping system user is not allowed",
+ ):
+ i1.sql('drop user "guest"')
+
+ with pytest.raises(
+ ReturnError,
+ match="dropping system role is not allowed",
+ ):
+ i1.sql('drop role "public"')
+
+
# TODO: test acl get denied when there's an unfinished ddl
# TODO: check various retryable cas outcomes when doing schema change requests