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

[n.pettik]

> On 26 Mar 2019, at 13:59, Kirill Shcherbatov <kshcherbatov at tarantool.org> wrote:
> 
>> 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.

  1913	static void
  1914	on_replace_dd_index(struct trigger * /* trigger */, void *event)
  1915	{
		...
  1930		enum priv_type priv_type = new_tuple ? PRIV_C : PRIV_D;
  1931		if (old_tuple && new_tuple)
  1932			priv_type = PRIV_A;
  1933		access_check_ddl(old_space->def->name, old_space->def->id,
  1934				 old_space->def->uid, SC_SPACE, priv_type);

For indexes we check different privileges for drop, create and alter cases.

> As for _trigger, _fk_constraint regular access check everything is ok already.

I look at our docs: https://tarantool.io/en/doc/2.1/book/box/authentication/

It says:

‘’’
To create objects, users need the ‘create’ privilege and at least ‘read’ and
‘write’ privileges on the system space with a similar name
’''

Moreover, I see that we already have PRIV_TRIGGER in the list of object types.
AFAIK, ANSI states that user must have that privilege to create or drop triggers.
Please, consult other members of core team what check is expected here.
Personally I intuitively think that we need PRIV_C/PRIV_D/PRIV_A + PRIV_TRIGGER.
If PRIV_A is enough for all cases, add docbot request explaining that to create FK/TRIGGER
user should add alter privilege to space(s).

> 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.
> 
> 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);

Could you move this check a bit higher, like in DROP trigger code?
We don’t need to compile trigger body to check access privilege.

> 
> 		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.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.

Nit: _fk_constraint

> +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;'}})

Please, add three separate cases: trigger creation, trigger drop and
trigger replace. The same for FK constraints.