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

Chris Sosnin k.sosnin at tarantool.org
Wed Dec 11 12:50:08 MSK 2019


Thank you for the review!
See fixes for your comments below:

> Sequences also can have grants. And sequence.drop removes them.
> I propose you to drop sequence grants in a second commit.

For this purpose I rewrote the function so it can be used not only for space
objects but for sequences too. Diff:

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

And it is used like this:
+	vdbe_emit_revoke_object(parse_context, "space", space->def->id,
+				space->access);

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

Not needed anymore.

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

Done:
+	struct Vdbe *v = sqlGetVdbe(parser);
+	assert(v != NULL);

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

Done, you can see it in the first diff above.

> 4. Box.cfg{} is already called by that time, so you
> don't need to call it again.

Done.

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

Done. Also added sequence grants in tests:
+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')

New version of the patch below:

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
@@ -56,6 +56,7 @@
 #include "box/schema.h"
 #include "box/tuple_format.h"
 #include "box/coll_id_cache.h"
+#include "box/user.h"

 void
 sql_finish_coding(struct Parse *parse_context)
@@ -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);
+	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,
@@ -1538,6 +1575,12 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 {
 	struct Vdbe *v = sqlGetVdbe(parse_context);
 	assert(v != NULL);
+	/*
+	 * Remove all grants associated with
+	 * with the table being dropped.
+	 */
+	vdbe_emit_revoke_object(parse_context, "space", space->def->id,
+				space->access);
 	/*
 	 * Drop all triggers associated with the table being
 	 * dropped. Code is generated to remove entries from
@@ -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);
 		/* Delete entry from _sequence_data. */
 		int sequence_id_reg = ++parse_context->nMem;
 		sqlVdbeAddOp2(v, OP_Integer, space->sequence->def->id,
diff --git a/src/box/user.cc b/src/box/user.cc
index a012cb196..fe0555886 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -517,6 +517,13 @@ user_find(uint32_t uid)
 	return user;
 }

+/* Find a user by authentication token. */
+struct user *
+user_find_by_token(uint8_t auth_token)
+{
+    return &users[auth_token];
+}
+
 /** Find user by name. */
 struct user *
 user_find_by_name(const char *name, uint32_t len)
diff --git a/src/box/user.h b/src/box/user.h
index 545401d5c..ccfc59346 100644
--- a/src/box/user.h
+++ b/src/box/user.h
@@ -112,6 +112,10 @@ user_find_by_name(const char *name, uint32_t len);
 struct user *
 user_find(uint32_t uid);

+/* Find a user by authentication token. */
+struct user *
+user_find_by_token(uint8_t auth_token);
+
 /** Create a cache of user's privileges in @a cr. */
 void
 credentials_create(struct credentials *cr, struct user *user);
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..ca95ed0f9
--- /dev/null
+++ b/test/sql/gh-4546-sql-drop-grants.result
@@ -0,0 +1,40 @@
+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.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;]])
+---
+- row_count: 1
+...
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')
+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
+
+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;]])
--
2.21.0 (Apple Git-122.2)


More information about the Tarantool-patches mailing list