[tarantool-patches] Re: [PATCH v2 2/9] box: fix _trigger and _ck_constraint access check

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> Why did you check only alter privilege? On drop we should check
> drop privilege, on alter - alter privilege and on creation - creation
> privilege.
Hi! Here I check that we have alter privilege on corresponding "space" because replace
in _trigger, _fk_constraint (and in further patches _ck_constaint)  cause space *
object change. 
This is very similar with access checks for _index space. 
As for _trigger, _fk_constraint regular access check everything is ok already.

Look:
tarantool> box.schema.user.grant('guest', 'write', 'space', '_trigger')
---
...
tarantool> c.space._trigger:replace({'TR1', 512, {sql='CREATE TRIGGER tr1 AFTER DELETE ON t5 BEGIN DELETE FROM t5; END;'}})
---
- error: Alter access to space 'T5' is denied for user 'guest'
...

> Please, add tests on these cases.
Done.
=========================================================================

Due to the fact that the insert in _fk_constraint and _trigger
leads to a change in object space * need to check the right to
change corresponding space before creating triggers or foreign
key.
---
 src/box/alter.cc          | 17 +++++++++++++++++
 test/box/net.box.result   | 32 ++++++++++++++++++++++++++++++++
 test/box/net.box.test.lua | 12 ++++++++++++
 3 files changed, 61 insertions(+)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 080a72b9f..fb668aa4c 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3514,6 +3514,11 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 		uint32_t space_id =
 			tuple_field_u32_xc(old_tuple,
 					   BOX_TRIGGER_FIELD_SPACE_ID);
+		struct space *space = space_by_id(space_id);
+		assert(space != NULL);
+		access_check_ddl(space->def->name, space->def->id,
+				 space->def->uid, SC_SPACE, PRIV_A);
+
 		char *trigger_name =
 			(char *)region_alloc_xc(&fiber()->gc,
 						trigger_name_len + 1);
@@ -3567,6 +3572,10 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 				  "trigger space_id does not match the value "
 				  "resolved on AST building from SQL");
 		}
+		struct space *space = space_by_id(space_id);
+		assert(space != NULL);
+		access_check_ddl(space->def->name, space->def->id,
+				 space->def->uid, SC_SPACE, PRIV_A);
 
 		struct sql_trigger *old_trigger;
 		if (sql_trigger_replace(trigger_name,
@@ -3897,6 +3906,8 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
 				  fk_def->name,
 				  "referencing space can't be VIEW");
 		}
+		access_check_ddl(child_space->def->name, child_space->def->id,
+				 child_space->def->uid, SC_SPACE, PRIV_A);
 		struct space *parent_space =
 			space_cache_find_xc(fk_def->parent_id);
 		if (parent_space->def->opts.is_view) {
@@ -3904,6 +3915,8 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
 				  fk_def->name,
 				  "referenced space can't be VIEW");
 		}
+		access_check_ddl(parent_space->def->name, parent_space->def->id,
+				 parent_space->def->uid, SC_SPACE, PRIV_A);
 		/*
 		 * FIXME: until SQL triggers are completely
 		 * integrated into server (i.e. we are able to
@@ -4031,6 +4044,10 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
 			space_cache_find_xc(fk_def->child_id);
 		struct space *parent_space =
 			space_cache_find_xc(fk_def->parent_id);
+		access_check_ddl(child_space->def->name, child_space->def->id,
+				 child_space->def->uid, SC_SPACE, PRIV_A);
+		access_check_ddl(parent_space->def->name, parent_space->def->id,
+				 parent_space->def->uid, SC_SPACE, PRIV_A);
 		struct fk_constraint *old_fk=
 			fk_constraint_remove(&child_space->child_fk_constraint,
 					     fk_def->name);
diff --git a/test/box/net.box.result b/test/box/net.box.result
index aecaf9436..f488bc030 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -3526,3 +3526,35 @@ s:drop()
 box.cfg{readahead = readahead}
 ---
 ...
+-- Test alter privilege for space that is modified by insertion in
+-- _trigger, _ck_constraint space.
+box.sql.execute("CREATE TABLE t5(x  INT primary key, y INT, CHECK( x + y < 2 ));")
+---
+...
+box.sql.execute("CREATE TRIGGER tr1 AFTER DELETE ON t5 BEGIN DELETE FROM t5; END;")
+---
+...
+box.schema.user.grant('guest','create,read,write','universe')
+---
+...
+c = net.connect(box.cfg.listen)
+---
+...
+c.space._trigger:replace({'TR1', box.space.T5.id, {sql='CREATE TRIGGER tr1 AFTER DELETE ON t5 BEGIN DELETE FROM t5; END;'}})
+---
+- error: Alter access to space 'T5' is denied for user 'guest'
+...
+c:execute('CREATE TABLE t6(x INT PRIMARY KEY REFERENCES t5 ON DELETE RESTRICT);')
+---
+- error: 'Failed to execute SQL statement: Alter access to space ''T5'' is denied
+    for user ''guest'''
+...
+c:close()
+---
+...
+box.schema.user.revoke('guest','create,read,write','universe')
+---
+...
+box.space.T5:drop()
+---
+...
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index 04d6c1903..074c1a0fe 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -1435,3 +1435,15 @@ test_run:wait_log('default', 'readahead limit is reached', 1024, 0.1)
 
 s:drop()
 box.cfg{readahead = readahead}
+
+-- Test alter privilege for space that is modified by insertion in
+-- _trigger, _ck_constraint space.
+box.sql.execute("CREATE TABLE t5(x  INT primary key, y INT, CHECK( x + y < 2 ));")
+box.sql.execute("CREATE TRIGGER tr1 AFTER DELETE ON t5 BEGIN DELETE FROM t5; END;")
+box.schema.user.grant('guest','create,read,write','universe')
+c = net.connect(box.cfg.listen)
+c.space._trigger:replace({'TR1', box.space.T5.id, {sql='CREATE TRIGGER tr1 AFTER DELETE ON t5 BEGIN DELETE FROM t5; END;'}})
+c:execute('CREATE TABLE t6(x INT PRIMARY KEY REFERENCES t5 ON DELETE RESTRICT);')
+c:close()
+box.schema.user.revoke('guest','create,read,write','universe')
+box.space.T5:drop()
-- 
2.21.0