From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 1874E46970E for ; Wed, 18 Dec 2019 02:13:14 +0300 (MSK) References: <20191211095008.90358-1-k.sosnin@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 18 Dec 2019 00:13:12 +0100 MIME-Version: 1.0 In-Reply-To: <20191211095008.90358-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 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 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 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