diff --git a/src/access_control.rs b/src/access_control.rs
index 425d0170366bb293b2669e376a40a4dd321fa8c9..3d298427d9f3cb0ae8fcfe6c428fd44909a36d56 100644
--- a/src/access_control.rs
+++ b/src/access_control.rs
@@ -43,7 +43,7 @@ use tarantool::{
tuple::Encode,
};
-use crate::storage::ClusterwideTable;
+use crate::storage::{ClusterwideTable, SPACE_ID_INTERNAL_MAX};
use crate::traft::error::Error;
use crate::traft::op::Dml;
use crate::{
@@ -221,8 +221,21 @@ fn box_access_check_ddl_as_user(
}
fn access_check_dml(dml: &Dml, as_user: UserId) -> tarantool::Result<()> {
+ let space_id = dml.space();
+ if space_id <= SPACE_ID_INTERNAL_MAX && as_user != ADMIN_ID && as_user != PICO_SERVICE_ID {
+ let table_name = ClusterwideTable::try_from(space_id)
+ .map_or(format!("id={}", space_id), |table| table.name().to_string());
+
+ tarantool::set_error!(
+ tarantool::error::TarantoolErrorCode::AccessDenied,
+ "Write access to table '{}' is denied for all users",
+ table_name
+ );
+ return Err(tarantool::error::TarantoolError::last().into());
+ }
+
let _su = session::su(as_user)?;
- box_access_check_space(dml.space(), PrivType::Write)
+ box_access_check_space(space_id, PrivType::Write)
}
/// This function performs access control checks that are identical to ones performed in
@@ -672,33 +685,6 @@ fn access_check_acl(
}
}
-// Non-admin users are prohibited from performing DML operations on system tables
-// because of the shared namespace for user and catalog tables in Tarantool.
-// Allowing write access to any table via 'GRANT WRITE TABLE TO ..'
-// also grants access to catalog tables. Such changes can cause security issues,
-// and without this check would also grant access to catalog tables.
-// Admin users have limited write access to some system tables.
-// However, most tables are allowed because Picodata runs under admin to manipulate with the catalog
-// See PROHIBITED_TABLES in cas.rs for details.
-pub fn check_system_tables_access(dml: &Dml, as_user: UserId) -> tarantool::Result<()> {
- let space = dml.space();
- let Ok(table) = &ClusterwideTable::try_from(space) else {
- return Ok(());
- };
-
- let sys_user = user_by_id(as_user)?;
- if as_user != ADMIN_ID && as_user != PICO_SERVICE_ID {
- return Err(make_access_denied(
- "Write access",
- PicoSchemaObjectType::Table,
- table,
- sys_user.name,
- ));
- }
-
- Ok(())
-}
-
pub(super) fn access_check_op(
storage: &Clusterwide,
op: &Op,
@@ -708,13 +694,11 @@ pub(super) fn access_check_op(
Op::Nop => Ok(()),
Op::Dml(dml) => {
access_check_dml(dml, as_user)?;
- check_system_tables_access(dml, as_user)?;
Ok(())
}
Op::BatchDml { ops } => {
for op in ops {
access_check_dml(op, as_user)?;
- check_system_tables_access(op, as_user)?;
}
Ok(())
}
@@ -889,7 +873,11 @@ mod tests {
// space
let space_name = "test_box_access_check_ddl";
- let space = Space::create(space_name, &SpaceCreateOptions::default()).unwrap();
+ let opts = SpaceCreateOptions {
+ id: Some(1025),
+ ..Default::default()
+ };
+ let space = Space::create(space_name, &opts).unwrap();
// create space
{
diff --git a/src/cas.rs b/src/cas.rs
index 11911d99efd2c6813abae203e6ce8eb37682e01a..140bbcb1e7ce77eb833e33dfeab5427f80e722f3 100644
--- a/src/cas.rs
+++ b/src/cas.rs
@@ -17,7 +17,6 @@ use ::raft::Error as RaftError;
use ::raft::GetEntriesContext;
use ::raft::StorageError;
-use crate::schema::ADMIN_ID;
use tarantool::error::Error as TntError;
use tarantool::error::TarantoolErrorCode;
use tarantool::fiber;
@@ -40,12 +39,12 @@ const PROHIBITED_TABLES: &[ClusterwideTable] = &[
ClusterwideTable::Routine,
];
-pub fn check_admin_dml_prohibited(dml: &Dml, as_user: UserId) -> traft::Result<()> {
+pub fn check_dml_prohibited(dml: &Dml) -> traft::Result<()> {
let space = dml.space();
let Ok(table) = &ClusterwideTable::try_from(space) else {
return Ok(());
};
- if as_user == ADMIN_ID && PROHIBITED_TABLES.contains(table) {
+ if PROHIBITED_TABLES.contains(table) {
return Err(Error::TableNotAllowed {
table: table.name().into(),
}
@@ -252,11 +251,11 @@ fn proc_cas_local(req: &Request) -> Result<Response> {
match &req.op {
Op::Dml(dml) => {
- check_admin_dml_prohibited(dml, req.as_user)?;
+ check_dml_prohibited(dml)?;
}
Op::BatchDml { ops: dmls } => {
for dml in dmls {
- check_admin_dml_prohibited(dml, req.as_user)?;
+ check_dml_prohibited(dml)?;
}
}
_ => {}
@@ -450,10 +449,7 @@ pub enum Error {
actual_term: RaftTerm,
},
- #[error(
- "TableNotAllowed: table {table} cannot be modified directly, \
- please refer to available SQL commands"
- )]
+ #[error("TableNotAllowed: table {table} cannot be modified by DML Raft Operation directly")]
TableNotAllowed { table: String },
/// An error related to `key_def` operation arised from tarantool
diff --git a/test/int/test_acl.py b/test/int/test_acl.py
index bcda2eeb498330b7c9a82fcf5ccc8ab8f9e46ec8..7996d1b69698fa4a11786db6534cc6ed619668ed 100644
--- a/test/int/test_acl.py
+++ b/test/int/test_acl.py
@@ -349,7 +349,7 @@ def test_cas_permissions(cluster: Cluster):
with pytest.raises(
Exception,
- match="Write access to space '_pico_property' is denied for user 'Steven'",
+ match="Write access to table '_pico_property' is denied for all users",
):
cluster.cas(
"replace",
@@ -365,8 +365,7 @@ def test_cas_permissions(cluster: Cluster):
with pytest.raises(
Exception,
- match="box error: "
- "AccessDenied: Write access to table '_pico_property' is denied for user 'Steven'",
+ match="Write access to table '_pico_property' is denied for all users",
):
cluster.cas(
"replace",
diff --git a/test/int/test_cas.py b/test/int/test_cas.py
index e76cb038d02709e939d07251d2636546fbceaf90..b3bb201d7830b7788175be1ddc015a83a91189c1 100644
--- a/test/int/test_cas.py
+++ b/test/int/test_cas.py
@@ -85,7 +85,7 @@ def test_cas_errors(instance: Instance):
instance.cas("insert", table, [0], ranges=[CasRange(eq=0)], user=1)
assert e5.value.args[:2] == (
ErrorCode.CasTableNotAllowed,
- f"TableNotAllowed: table {table} cannot be modified directly, please refer to available SQL commands", # noqa: E501
+ f"TableNotAllowed: table {table} cannot be modified by DML Raft Operation directly",
)
# Field type error