Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Chris Sosnin <k.sosnin@tarantool.org>,
	tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] sql: remove grants associated with the table
Date: Wed, 11 Dec 2019 00:45:24 +0100	[thread overview]
Message-ID: <dde57ceb-8555-d404-fd16-2e175cda1a58@tarantool.org> (raw)
In-Reply-To: <20191206125308.79432-1-k.sosnin@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.

  parent reply	other threads:[~2019-12-10 23:45 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-06 12:53 Chris Sosnin
2019-12-07 10:29 ` Chris Sosnin
2019-12-10 23:45 ` Vladislav Shpilevoy [this message]
2019-12-11  9:50   ` Chris Sosnin
2019-12-17 23:13     ` Vladislav Shpilevoy
2019-12-17 23:13       ` Vladislav Shpilevoy
2019-12-18 11:00       ` [Tarantool-patches] [PATCH 0/2] sql: revoke table privileges on drop Chris Sosnin
2019-12-18 11:00         ` [Tarantool-patches] [PATCH 1/2] sql: remove grants associated with the table Chris Sosnin
2019-12-24  1:37           ` Nikita Pettik
2019-12-24 16:47             ` Vladislav Shpilevoy
2019-12-18 11:00         ` [Tarantool-patches] [PATCH 2/2] sql: drop only generated sequence in DROP TABLE Chris Sosnin
2019-12-24  1:23           ` Nikita Pettik
2019-12-24 16:26             ` Vladislav Shpilevoy
2019-12-24 23:19               ` Nikita Pettik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=dde57ceb-8555-d404-fd16-2e175cda1a58@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=k.sosnin@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] sql: remove grants associated with the table' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox