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

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

Hi! Thanks for the fixes!

See N comments below, 2 my commits in the end, and on
top of your branch.

> 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                       | 47 +++++++++++++++++++++++
>  src/box/user.cc                           |  7 ++++
>  src/box/user.h                            |  4 ++
>  test/sql/gh-4546-sql-drop-grants.result   | 40 +++++++++++++++++++
>  test/sql/gh-4546-sql-drop-grants.test.lua | 16 ++++++++
>  5 files changed, 114 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..af937de3b 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1523,6 +1524,42 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
>  	sqlReleaseTempRange(parser, key_reg, 3);
>  }
> 
> +/**
> + * Generate VDBE program to revoke all
> + * privileges associated with the given object.
> + *
> + * @param parser Parsing context.
> + * @param object_type Object type
> + * @param object_id Object id
> + * @param access Access array associated with an object
> + */
> +static void
> +vdbe_emit_revoke_object(struct Parse *parser, const char *object_type,
> +			uint32_t object_id, struct access access[])
> +{
> +	struct Vdbe *v = sqlGetVdbe(parser);
> +	assert(v != NULL);
> +	/*
> +	 * Get uid of users through access array
> +	 * and generate code to delete corresponding
> +	 * entries from _priv
> +	 */
> +	int key_reg = sqlGetTempRange(parser, 4);

1. I run some tests and appeared, that VdbeComment
below asserts, when the object didn't have grants.

I've fixed it in my commit.

> +	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
> +		if (!access[token].granted)
> +			continue;
> +		const 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,
> +			      object_type, P4_STATIC);
> +		sqlVdbeAddOp2(v, OP_Integer, object_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 %s grants", object_type));
> +	sqlReleaseTempRange(parser, key_reg, 4);
> +}
> +
>  /**
>   * Generate code to drop a table.
>   * This routine includes dropping triggers, sequences,
> @@ -1565,6 +1608,10 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
>  	sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
>  	sqlVdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);
>  	if (space->sequence != NULL) {
> +		/* Delete entries from _priv */
> +		vdbe_emit_revoke_object(parse_context, "sequence",
> +					space->sequence->def->id,
> +					space->sequence->access);

2. I took a look at this code again, and realized, that
sequence problem is more severe. This code deletes a
sequence even if it does not belong to the space, i.e.
was not generated automatically.

I've fixed that in a separate commit.

>  		/* Delete entry from _sequence_data. */
>  		int sequence_id_reg = ++parse_context->nMem;
>  		sqlVdbeAddOp2(v, OP_Integer, space->sequence->def->id,
> diff --git a/test/sql/gh-4546-sql-drop-grants.test.lua b/test/sql/gh-4546-sql-drop-grants.test.lua
> new file mode 100644
> index 000000000..5ac4b5701
> --- /dev/null
> +++ b/test/sql/gh-4546-sql-drop-grants.test.lua
> @@ -0,0 +1,16 @@
> +test_run = require('test_run').new()
> +engine = test_run:get_cfg('engine')

3. Engine is unused. To actually use it you need to
give it to create_space() arguments. I've fixed it in
my commit.

> +box.execute('pragma sql_default_engine=\''..engine..'\'')

4. Since this test does not create spaces via SQL, this
setting becomes not needed. I've drop it.

> +
> +-- If we drop the table with sql, all associated
> +-- grants must be deleted so we don't recieve an error
> +
> +box.schema.user.create('test_user1')
> +box.schema.user.create('test_user2')
> +test_space = box.schema.create_space('T', {format = {{'i', 'integer'}}})
> +seq = box.schema.sequence.create('S')
> +ind = test_space:create_index('I', {sequence = 'S'})
> +box.schema.user.grant('test_user1', 'write', 'sequence', 'S')
> +box.schema.user.grant('test_user1', 'read', 'space', 'T')
> +box.schema.user.grant('test_user2', 'write', 'space', 'T')
> +box.execute([[DROP TABLE T;]])

5. Test users are not dropped in the end of the test. I've
fixed it in my commit.

> --
> 2.21.0 (Apple Git-122.2)
> 

See my 2 commits below. First is review fixes for your commit.
Please, review it and squash if you agree with the fixes.

Second commit is independent one. Please, review it, but do
not squash. Instead, change author of the commit to yourself.
If you have comments on the second commit - bring them up and
lets discuss.

If you agree with both commits, and you've done squash and the
author change, then send for a second review to Nikita.

First commit:

===================================================================

commit 717ad6bbc382a798ef94d6a233e3b6464adc066b
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Tue Dec 17 23:13:40 2019 +0100

    Review fixes

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index af937de3b..f1433645a 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1529,13 +1529,13 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
  * privileges associated with the given object.
  *
  * @param parser Parsing context.
- * @param object_type Object type
- * @param object_id Object id
- * @param access Access array associated with an object
+ * @param object_type Object type.
+ * @param object_id Object id.
+ * @param access Access array associated with an object.
  */
 static void
 vdbe_emit_revoke_object(struct Parse *parser, const char *object_type,
-			uint32_t object_id, struct access access[])
+			uint32_t object_id, struct access *access)
 {
 	struct Vdbe *v = sqlGetVdbe(parser);
 	assert(v != NULL);
@@ -1545,9 +1545,11 @@ vdbe_emit_revoke_object(struct Parse *parser, const char *object_type,
 	 * entries from _priv
 	 */
 	int key_reg = sqlGetTempRange(parser, 4);
+	bool had_grants = false;
 	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
 		if (!access[token].granted)
 			continue;
+		had_grants = true;
 		const 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,
@@ -1556,7 +1558,8 @@ vdbe_emit_revoke_object(struct Parse *parser, const char *object_type,
 		sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3, key_reg + 3);
 		sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID, key_reg + 3);
 	}
-	VdbeComment((v, "Remove %s grants", object_type));
+	if (had_grants)
+		VdbeComment((v, "Remove %s grants", object_type));
 	sqlReleaseTempRange(parser, key_reg, 4);
 }
 
@@ -1608,10 +1611,6 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 	sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
 	sqlVdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);
 	if (space->sequence != NULL) {
-		/* Delete entries from _priv */
-		vdbe_emit_revoke_object(parse_context, "sequence",
-					space->sequence->def->id,
-					space->sequence->access);
 		/* Delete entry from _sequence_data. */
 		int sequence_id_reg = ++parse_context->nMem;
 		sqlVdbeAddOp2(v, OP_Integer, space->sequence->def->id,
diff --git a/test/sql/gh-4546-sql-drop-grants.result b/test/sql/gh-4546-sql-drop-grants.result
index ca95ed0f9..04d9ef172 100644
--- a/test/sql/gh-4546-sql-drop-grants.result
+++ b/test/sql/gh-4546-sql-drop-grants.result
@@ -4,19 +4,20 @@ 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
+--
+-- gh-4546: DROP TABLE must delete all privileges given on that
+-- table to any user.
+--
 box.schema.user.create('test_user1')
 ---
 ...
 box.schema.user.create('test_user2')
 ---
 ...
-test_space = box.schema.create_space('T', {format = {{'i', 'integer'}}})
+test_space = box.schema.create_space('T', {		\
+	engine = engine,				\
+	format = {{'i', 'integer'}}			\
+})
 ---
 ...
 seq = box.schema.sequence.create('S')
@@ -25,9 +26,6 @@ seq = box.schema.sequence.create('S')
 ind = test_space:create_index('I', {sequence = 'S'})
 ---
 ...
-box.schema.user.grant('test_user1', 'write', 'sequence', 'S')
----
-...
 box.schema.user.grant('test_user1', 'read', 'space', 'T')
 ---
 ...
@@ -38,3 +36,13 @@ box.execute([[DROP TABLE T;]])
 ---
 - row_count: 1
 ...
+box.schema.user.drop('test_user1')
+---
+...
+box.schema.user.drop('test_user2')
+---
+...
+box.space._sequence:select{}
+---
+- []
+...
diff --git a/test/sql/gh-4546-sql-drop-grants.test.lua b/test/sql/gh-4546-sql-drop-grants.test.lua
index 5ac4b5701..966733718 100644
--- a/test/sql/gh-4546-sql-drop-grants.test.lua
+++ b/test/sql/gh-4546-sql-drop-grants.test.lua
@@ -1,16 +1,22 @@
 test_run = require('test_run').new()
 engine = test_run:get_cfg('engine')
-box.execute('pragma sql_default_engine=\''..engine..'\'')
 
--- If we drop the table with sql, all associated
--- grants must be deleted so we don't recieve an error
+--
+-- gh-4546: DROP TABLE must delete all privileges given on that
+-- table to any user.
+--
 
 box.schema.user.create('test_user1')
 box.schema.user.create('test_user2')
-test_space = box.schema.create_space('T', {format = {{'i', 'integer'}}})
+test_space = box.schema.create_space('T', {		\
+	engine = engine,				\
+	format = {{'i', 'integer'}}			\
+})
 seq = box.schema.sequence.create('S')
 ind = test_space:create_index('I', {sequence = 'S'})
-box.schema.user.grant('test_user1', 'write', 'sequence', 'S')
 box.schema.user.grant('test_user1', 'read', 'space', 'T')
 box.schema.user.grant('test_user2', 'write', 'space', 'T')
 box.execute([[DROP TABLE T;]])
+box.schema.user.drop('test_user1')
+box.schema.user.drop('test_user2')
+box.space._sequence:select{}

===================================================================

Second commit:

===================================================================

commit 6550d9034dc5cce16f824379b28ce5abd138c6aa
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Tue Dec 17 23:59:00 2019 +0100

    sql: drop only generated sequence in DROP TABLE
    
    It is possible to create a sequence manually, and give it to a
    newly created index as a source of unique identifiers. Such
    sequences are not owned by a space, and therefore shouldn't be
    deleted when the space is dropped. They are not dropped when
    space:drop() in Lua is called, but were dropped in SQL
    'DROP TABLE' before this patch. Now Lua and SQL are consistent in
    that case.
    
    Follow up #4546

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index f1433645a..93aae0894 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1611,26 +1611,33 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 	sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
 	sqlVdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);
 	if (space->sequence != NULL) {
-		/* Delete entry from _sequence_data. */
-		int sequence_id_reg = ++parse_context->nMem;
-		sqlVdbeAddOp2(v, OP_Integer, space->sequence->def->id,
-				  sequence_id_reg);
-		sqlVdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
-				  idx_rec_reg);
-		sqlVdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_DATA_ID,
-				  idx_rec_reg);
-		VdbeComment((v, "Delete entry from _sequence_data"));
 		/* Delete entry from _space_sequence. */
 		sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1,
 				  idx_rec_reg);
 		sqlVdbeAddOp2(v, OP_SDelete, BOX_SPACE_SEQUENCE_ID,
 				  idx_rec_reg);
 		VdbeComment((v, "Delete entry from _space_sequence"));
-		/* Delete entry by id from _sequence. */
-		sqlVdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
-				  idx_rec_reg);
-		sqlVdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_ID, idx_rec_reg);
-		VdbeComment((v, "Delete entry from _sequence"));
+		if (space->sequence->is_generated) {
+			/* Delete entry from _sequence_data. */
+			int sequence_id_reg = ++parse_context->nMem;
+			sqlVdbeAddOp2(v, OP_Integer, space->sequence->def->id,
+				      sequence_id_reg);
+			sqlVdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
+				      idx_rec_reg);
+			sqlVdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_DATA_ID,
+				      idx_rec_reg);
+			VdbeComment((v, "Delete entry from _sequence_data"));
+			/* Delete entries from _priv */
+		        vdbe_emit_revoke_object(parse_context, "sequence",
+						space->sequence->def->id,
+						space->sequence->access);
+			/* Delete entry by id from _sequence. */
+			sqlVdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
+				      idx_rec_reg);
+			sqlVdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_ID,
+				      idx_rec_reg);
+			VdbeComment((v, "Delete entry from _sequence"));
+		}
 	}
 	/* Delete all child FK constraints. */
 	struct fk_constraint *child_fk;
diff --git a/test/sql/autoincrement.result b/test/sql/autoincrement.result
new file mode 100644
index 000000000..ca3d6f3cf
--- /dev/null
+++ b/test/sql/autoincrement.result
@@ -0,0 +1,80 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+engine = test_run:get_cfg('engine')
+ | ---
+ | ...
+
+--
+-- Not automatic sequence should not be removed by DROP TABLE.
+--
+box.schema.user.create('user1')
+ | ---
+ | ...
+test_space = box.schema.create_space('T', {         \
+    engine = engine,                                \
+    format = {{'i', 'integer'}}                     \
+})
+ | ---
+ | ...
+seq = box.schema.sequence.create('S')
+ | ---
+ | ...
+ind = test_space:create_index('I', {sequence = 'S'})
+ | ---
+ | ...
+box.schema.user.grant('user1', 'write', 'sequence', 'S')
+ | ---
+ | ...
+box.execute('DROP TABLE t');
+ | ---
+ | - row_count: 1
+ | ...
+seqs = box.space._sequence:select{}
+ | ---
+ | ...
+#seqs == 1 and seqs[1].name == seq.name or seqs
+ | ---
+ | - true
+ | ...
+seq:drop()
+ | ---
+ | ...
+
+--
+-- Automatic sequence should be removed at DROP TABLE, together
+-- with all the grants.
+--
+box.execute('CREATE TABLE t (id INTEGER PRIMARY KEY AUTOINCREMENT)')
+ | ---
+ | - row_count: 1
+ | ...
+seqs = box.space._sequence:select{}
+ | ---
+ | ...
+#seqs == 1 or seqs
+ | ---
+ | - true
+ | ...
+seq = seqs[1].name
+ | ---
+ | ...
+box.schema.user.grant('user1', 'write', 'sequence', seq)
+ | ---
+ | ...
+box.execute('DROP TABLE t')
+ | ---
+ | - row_count: 1
+ | ...
+seqs = box.space._sequence:select{}
+ | ---
+ | ...
+#seqs == 0 or seqs
+ | ---
+ | - true
+ | ...
+
+box.schema.user.drop('user1')
+ | ---
+ | ...
diff --git a/test/sql/autoincrement.test.lua b/test/sql/autoincrement.test.lua
new file mode 100644
index 000000000..63a902aea
--- /dev/null
+++ b/test/sql/autoincrement.test.lua
@@ -0,0 +1,33 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+
+--
+-- Not automatic sequence should not be removed by DROP TABLE.
+--
+box.schema.user.create('user1')
+test_space = box.schema.create_space('T', {         \
+    engine = engine,                                \
+    format = {{'i', 'integer'}}                     \
+})
+seq = box.schema.sequence.create('S')
+ind = test_space:create_index('I', {sequence = 'S'})
+box.schema.user.grant('user1', 'write', 'sequence', 'S')
+box.execute('DROP TABLE t');
+seqs = box.space._sequence:select{}
+#seqs == 1 and seqs[1].name == seq.name or seqs
+seq:drop()
+
+--
+-- Automatic sequence should be removed at DROP TABLE, together
+-- with all the grants.
+--
+box.execute('CREATE TABLE t (id INTEGER PRIMARY KEY AUTOINCREMENT)')
+seqs = box.space._sequence:select{}
+#seqs == 1 or seqs
+seq = seqs[1].name
+box.schema.user.grant('user1', 'write', 'sequence', seq)
+box.execute('DROP TABLE t')
+seqs = box.space._sequence:select{}
+#seqs == 0 or seqs
+
+box.schema.user.drop('user1')
diff --git a/test/sql/gh-4546-sql-drop-grants.result b/test/sql/gh-4546-sql-drop-grants.result
index 04d9ef172..3ba26e380 100644
--- a/test/sql/gh-4546-sql-drop-grants.result
+++ b/test/sql/gh-4546-sql-drop-grants.result
@@ -42,7 +42,10 @@ box.schema.user.drop('test_user1')
 box.schema.user.drop('test_user2')
 ---
 ...
-box.space._sequence:select{}
+seqs = box.space._sequence:select{}
 ---
-- []
+...
+#seqs == 1 and seqs[1].name == seq.name or seqs
+---
+- true
 ...
diff --git a/test/sql/gh-4546-sql-drop-grants.test.lua b/test/sql/gh-4546-sql-drop-grants.test.lua
index 966733718..fc2c27e08 100644
--- a/test/sql/gh-4546-sql-drop-grants.test.lua
+++ b/test/sql/gh-4546-sql-drop-grants.test.lua
@@ -19,4 +19,5 @@ box.schema.user.grant('test_user2', 'write', 'space', 'T')
 box.execute([[DROP TABLE T;]])
 box.schema.user.drop('test_user1')
 box.schema.user.drop('test_user2')
-box.space._sequence:select{}
+seqs = box.space._sequence:select{}
+#seqs == 1 and seqs[1].name == seq.name or seqs