From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7ACD546971A for ; Wed, 11 Dec 2019 02:45:26 +0300 (MSK) References: <20191206125308.79432-1-k.sosnin@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 11 Dec 2019 00:45:24 +0100 MIME-Version: 1.0 In-Reply-To: <20191206125308.79432-1-k.sosnin@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] sql: remove grants associated with the table List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chris Sosnin , tarantool-patches@dev.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.