Re: [Tarantool-patches] [PATCH] sql: remove grants associated with the table

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

I realized, that grants problem also affects
sequence. Take a look at

    if sequence_tuple ~= nil and sequence_tuple.is_generated == true then
        -- Delete automatically generated sequence.
        box.schema.sequence.drop(sequence_tuple.sequence_id)
    end

In schema.lua in box.schema.space.drop.

Sequences also can have grants. And sequence.drop removes them.
I propose you to drop sequence grants in a second commit. The
problematic place is

    if (space->sequence != NULL) {

in sql_code_drop_table(). It should drop sequence grants too.

See 5 comments below.

On 06/12/2019 13:53, Chris Sosnin wrote:
> Dropping table with sql removes everything
> associated with it but grants, which is
> inconsistent. Generating code for it fixes this bug.
> 
> Closes #4546
> ---
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4546-sql-drop-grants
> issue: https://github.com/tarantool/tarantool/issues/4546
> 
>  src/box/sql/build.c                       | 43 +++++++++++++++++++++++
>  src/box/user.cc                           |  7 ++++
>  src/box/user.h                            |  4 +++
>  test/sql/gh-4546-sql-drop-grants.result   | 34 ++++++++++++++++++
>  test/sql/gh-4546-sql-drop-grants.test.lua | 15 ++++++++
>  5 files changed, 103 insertions(+)
>  create mode 100644 test/sql/gh-4546-sql-drop-grants.result
>  create mode 100644 test/sql/gh-4546-sql-drop-grants.test.lua
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 51cd7ce63..d700ec5e2 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1523,6 +1524,43 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
>  	sqlReleaseTempRange(parser, key_reg, 3);
>  }
> 
> +/**
> + * Generate VDBE program to remove table grants.
> + *
> + * @param parser Parsing context.
> + * @param space Space whose grants will be deleted.
> + */
> +static void
> +vdbe_emit_drop_grants(struct Parse *parser, struct space *space)

1. Seems like the space is not changed here, so can be made
'const struct space *'.

> +{
> +	struct Vdbe *v = sqlGetVdbe(parser);
> +	if (v == NULL)
> +		return;

2. Lets assert that v is never NULL. By the time this
function is called, Vdbe is always created.

> +	/*
> +	 * Get uid of users through space->access
> +	 * and generate code to delete corresponding
> +	 * entries from _priv
> +	 */
> +	int key_reg = sqlGetTempRange(parser, 4);
> +	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
> +		if (space->access[token].granted) {
> +			struct user *user = user_find_by_token(token);
> +			sqlVdbeAddOp2(v, OP_Integer, user->def->uid,
> +				 	  key_reg);
> +			sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
> +				 	  "space", P4_STATIC);
> +			sqlVdbeAddOp2(v, OP_Integer, space->def->id,
> +					  key_reg + 2);
> +			sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3,
> +				 	  key_reg + 3);
> +			sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID,
> +					  key_reg + 3);

3. The indentation is broken, some calls fit one line,
and there are dangling whitespaces right below 'Add' on
each line.

Consider this refactoring:

=========================================================================
@@ -1543,19 +1543,15 @@ vdbe_emit_drop_grants(struct Parse *parser, struct space *space)
 	 */
 	int key_reg = sqlGetTempRange(parser, 4);
 	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
-		if (space->access[token].granted) {
-			struct user *user = user_find_by_token(token);
-			sqlVdbeAddOp2(v, OP_Integer, user->def->uid,
-				 	  key_reg);
-			sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
-				 	  "space", P4_STATIC);
-			sqlVdbeAddOp2(v, OP_Integer, space->def->id,
-					  key_reg + 2);
-			sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3,
-				 	  key_reg + 3);
-			sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID,
-					  key_reg + 3);
-		}
+		if (! space->access[token].granted)
+			continue;
+		struct user *user = user_find_by_token(token);
+		sqlVdbeAddOp2(v, OP_Integer, user->def->uid, key_reg);
+		sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0, "space",
+			      P4_STATIC);
+		sqlVdbeAddOp2(v, OP_Integer, space->def->id, key_reg + 2);
+		sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3, key_reg + 3);
+		sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID, key_reg + 3);
 	}
 	VdbeComment((v, "Remove table grants"));
 	sqlReleaseTempRange(parser, key_reg, 4);
=========================================================================

> diff --git a/test/sql/gh-4546-sql-drop-grants.result b/test/sql/gh-4546-sql-drop-grants.result
> new file mode 100644
> index 000000000..915a265bd
> --- /dev/null
> +++ b/test/sql/gh-4546-sql-drop-grants.result
> @@ -0,0 +1,34 @@
> +test_run = require('test_run').new()
> +---
> +...
> +engine = test_run:get_cfg('engine')
> +---
> +...
> +box.execute('pragma sql_default_engine=\''..engine..'\'')
> +---
> +- row_count: 0
> +...
> +-- If we drop the table with sql, all associated
> +-- grants must be deleted so we don't recieve an error
> +box.cfg{}

4. Box.cfg{} is already called by that time, so you
don't need to call it again. All test folders have a
file 'suite.ini'. Here you can find a name of the
script with is executed before executing tests in that
folder. For example, sql/suite.ini says:

    script = app.lua

It means, that app.lua is executed before all the other
tests in the folder. And it does box.cfg.

> +---
> +...
> +box.schema.user.create('test_user1')
> +---
> +...
> +box.schema.user.create('test_user2')
> +---
> +...
> +test_space = box.schema.create_space('T')

5. Lets add a minimal format. Just one field type/name.
Because generally SQL does not work with spaces not
having a format.