* [Tarantool-patches] [PATCH] sql: remove grants associated with the table @ 2019-12-06 12:53 Chris Sosnin 2019-12-07 10:29 ` Chris Sosnin 2019-12-10 23:45 ` Vladislav Shpilevoy 0 siblings, 2 replies; 14+ messages in thread From: Chris Sosnin @ 2019-12-06 12:53 UTC (permalink / raw) To: tarantool-patches 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 @@ -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,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) +{ + struct Vdbe *v = sqlGetVdbe(parser); + if (v == NULL) + return; + /* + * 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); + } + } + VdbeComment((v, "Remove table grants")); + sqlReleaseTempRange(parser, key_reg, 3); +} + /** * Generate code to drop a table. * This routine includes dropping triggers, sequences, @@ -1538,6 +1576,11 @@ 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_drop_grants(parse_context, space); /* * Drop all triggers associated with the table being * dropped. Code is generated to remove entries from 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..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{} +--- +... +box.schema.user.create('test_user1') +--- +... +box.schema.user.create('test_user2') +--- +... +test_space = box.schema.create_space('T') +--- +... +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..6aeca4ce0 --- /dev/null +++ b/test/sql/gh-4546-sql-drop-grants.test.lua @@ -0,0 +1,15 @@ +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.cfg{} + +box.schema.user.create('test_user1') +box.schema.user.create('test_user2') +test_space = box.schema.create_space('T') +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) ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Tarantool-patches] [PATCH] sql: remove grants associated with the table 2019-12-06 12:53 [Tarantool-patches] [PATCH] sql: remove grants associated with the table Chris Sosnin @ 2019-12-07 10:29 ` Chris Sosnin 2019-12-10 23:45 ` Vladislav Shpilevoy 1 sibling, 0 replies; 14+ messages in thread From: Chris Sosnin @ 2019-12-07 10:29 UTC (permalink / raw) To: tarantool-patches There was a typo, I fixed it: diff --git a/src/box/sql/build.c b/src/box/sql/build.c + sqlReleaseTempRange(parser, key_reg, 4); -- 2.21.0 (Apple Git-122.2) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: remove grants associated with the table 2019-12-06 12:53 [Tarantool-patches] [PATCH] sql: remove grants associated with the table Chris Sosnin 2019-12-07 10:29 ` Chris Sosnin @ 2019-12-10 23:45 ` Vladislav Shpilevoy 2019-12-11 9:50 ` Chris Sosnin 1 sibling, 1 reply; 14+ messages in thread From: Vladislav Shpilevoy @ 2019-12-10 23:45 UTC (permalink / raw) To: Chris Sosnin, tarantool-patches 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Tarantool-patches] [PATCH] sql: remove grants associated with the table 2019-12-10 23:45 ` Vladislav Shpilevoy @ 2019-12-11 9:50 ` Chris Sosnin 2019-12-17 23:13 ` Vladislav Shpilevoy 0 siblings, 1 reply; 14+ messages in thread From: Chris Sosnin @ 2019-12-11 9:50 UTC (permalink / raw) To: tarantool-patches, v.shpilevoy 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) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: remove grants associated with the table 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 0 siblings, 2 replies; 14+ messages in thread From: Vladislav Shpilevoy @ 2019-12-17 23:13 UTC (permalink / raw) To: Chris Sosnin, tarantool-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: remove grants associated with the table 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 1 sibling, 0 replies; 14+ messages in thread From: Vladislav Shpilevoy @ 2019-12-17 23:13 UTC (permalink / raw) To: Chris Sosnin, tarantool-patches On 18/12/2019 00:13, Vladislav Shpilevoy wrote: > Hi! Thanks for the fixes! > > See N comments below, 2 my commits in the end, and on > top of your branch. Sorry, N = 5. Five comments. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Tarantool-patches] [PATCH 0/2] sql: revoke table privileges on drop 2019-12-17 23:13 ` Vladislav Shpilevoy 2019-12-17 23:13 ` Vladislav Shpilevoy @ 2019-12-18 11:00 ` Chris Sosnin 2019-12-18 11:00 ` [Tarantool-patches] [PATCH 1/2] sql: remove grants associated with the table Chris Sosnin 2019-12-18 11:00 ` [Tarantool-patches] [PATCH 2/2] sql: drop only generated sequence in DROP TABLE Chris Sosnin 1 sibling, 2 replies; 14+ messages in thread From: Chris Sosnin @ 2019-12-18 11:00 UTC (permalink / raw) To: tarantool-patches, v.shpilevoy, korablev Hi! Thank you for the review and for your fixes! > +box.schema.user.drop('test_user1') > +--- > +... > +box.schema.user.drop('test_user2') > +--- > +... > +box.space._sequence:select{} > +--- > +- [] > +... I moved sequence tests to the second commit, firstly, because here you don't do seq:drop() (I believe because after my initial patch sequences are getting deleted anyways, but after yours seq remains in _sequence space) and, secondly, because this way we can better isolate these two patches from each other. I agree with the rest, especially with your catch about sequences. Thus, I'm sending this patchset to Nikita. branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4546-sql-drop-grants issue: https://github.com/tarantool/tarantool/issues/4546 Chris Sosnin (2): sql: remove grants associated with the table sql: drop only generated sequence in DROP TABLE src/box/sql/build.c | 81 +++++++++++++++++++---- src/box/user.cc | 7 ++ src/box/user.h | 4 ++ test/sql/autoincrement.result | 80 ++++++++++++++++++++++ test/sql/autoincrement.test.lua | 33 +++++++++ test/sql/gh-4546-sql-drop-grants.result | 54 +++++++++++++++ test/sql/gh-4546-sql-drop-grants.test.lua | 24 +++++++ 7 files changed, 269 insertions(+), 14 deletions(-) create mode 100644 test/sql/autoincrement.result create mode 100644 test/sql/autoincrement.test.lua create mode 100644 test/sql/gh-4546-sql-drop-grants.result create mode 100644 test/sql/gh-4546-sql-drop-grants.test.lua -- 2.21.0 (Apple Git-122.2) ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Tarantool-patches] [PATCH 1/2] sql: remove grants associated with the table 2019-12-18 11:00 ` [Tarantool-patches] [PATCH 0/2] sql: revoke table privileges on drop Chris Sosnin @ 2019-12-18 11:00 ` Chris Sosnin 2019-12-24 1:37 ` Nikita Pettik 2019-12-18 11:00 ` [Tarantool-patches] [PATCH 2/2] sql: drop only generated sequence in DROP TABLE Chris Sosnin 1 sibling, 1 reply; 14+ messages in thread From: Chris Sosnin @ 2019-12-18 11:00 UTC (permalink / raw) To: tarantool-patches, v.shpilevoy, korablev 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 | 46 +++++++++++++++++++++++ src/box/user.cc | 7 ++++ src/box/user.h | 4 ++ test/sql/gh-4546-sql-drop-grants.result | 38 +++++++++++++++++++ test/sql/gh-4546-sql-drop-grants.test.lua | 19 ++++++++++ 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..f1433645a 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,45 @@ 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); + 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, + 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); + } + if (had_grants) + 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 +1578,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 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..1079d2ab4 --- /dev/null +++ b/test/sql/gh-4546-sql-drop-grants.result @@ -0,0 +1,38 @@ +test_run = require('test_run').new() +--- +... +engine = test_run:get_cfg('engine') +--- +... +-- +-- 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', { \ + engine = engine, \ + format = {{'i', 'integer'}} \ +}) +--- +... +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 +... +box.schema.user.drop('test_user1') +--- +... +box.schema.user.drop('test_user2') +--- +... 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..09cfeda19 --- /dev/null +++ b/test/sql/gh-4546-sql-drop-grants.test.lua @@ -0,0 +1,19 @@ +test_run = require('test_run').new() +engine = test_run:get_cfg('engine') + +-- +-- 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', { \ + engine = engine, \ + format = {{'i', 'integer'}} \ +}) +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') -- 2.21.0 (Apple Git-122.2) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] sql: remove grants associated with the table 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 0 siblings, 1 reply; 14+ messages in thread From: Nikita Pettik @ 2019-12-24 1:37 UTC (permalink / raw) To: Chris Sosnin; +Cc: tarantool-patches, v.shpilevoy On 18 Dec 14:00, Chris Sosnin wrote: > Dropping table with sql removes everything > associated with it but grants, which is > inconsistent. Generating code for it fixes this bug. Feel free to use up to 77 chars in commit message. You can enable auto-formatting in vim: au FileType gitcommit setlocal tw=72 Or you can manually highlight commit message and format with :gq > Closes #4546 > --- > branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4546-sql-drop-grants > issue: https://github.com/tarantool/tarantool/issues/4546 You don't have to put these links in each patch, they are required only in cover letter. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 51cd7ce63..f1433645a 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > +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); > + bool had_grants = false; As a rule we use present time: has_grants. > + for (uint8_t token = 0; token < BOX_USER_MAX; ++token) { > + if (!access[token].granted) > + continue; > + had_grants = true; Personally I wouldn't bother with separate variable solely to display comment. Let's keep it tho. > + 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); > + } > + if (had_grants) > + 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 +1578,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. Nit: double with. The rest is OK. LGTM and pushed to master. > + */ > + vdbe_emit_revoke_object(parse_context, "space", space->def->id, > + space->access); > /* ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] sql: remove grants associated with the table 2019-12-24 1:37 ` Nikita Pettik @ 2019-12-24 16:47 ` Vladislav Shpilevoy 0 siblings, 0 replies; 14+ messages in thread From: Vladislav Shpilevoy @ 2019-12-24 16:47 UTC (permalink / raw) To: Nikita Pettik, Chris Sosnin; +Cc: tarantool-patches >> + for (uint8_t token = 0; token < BOX_USER_MAX; ++token) { >> + if (!access[token].granted) >> + continue; >> + had_grants = true; > > Personally I wouldn't bother with separate variable solely to > display comment. Let's keep it tho. > We wouldn't either, but appears that VdbeComment asserts when there is no operation before it. So we must ensure, that the comment is added only in case at least one opcode is generated. >> + 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); >> + } >> + if (had_grants) >> + VdbeComment((v, "Remove %s grants", object_type)); >> + sqlReleaseTempRange(parser, key_reg, 4); >> +} >> + ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Tarantool-patches] [PATCH 2/2] sql: drop only generated sequence in DROP TABLE 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-18 11:00 ` Chris Sosnin 2019-12-24 1:23 ` Nikita Pettik 1 sibling, 1 reply; 14+ messages in thread From: Chris Sosnin @ 2019-12-18 11:00 UTC (permalink / raw) To: tarantool-patches, v.shpilevoy, korablev 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 --- 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 | 35 ++++++---- test/sql/autoincrement.result | 80 +++++++++++++++++++++++ test/sql/autoincrement.test.lua | 33 ++++++++++ test/sql/gh-4546-sql-drop-grants.result | 16 +++++ test/sql/gh-4546-sql-drop-grants.test.lua | 5 ++ 5 files changed, 155 insertions(+), 14 deletions(-) create mode 100644 test/sql/autoincrement.result create mode 100644 test/sql/autoincrement.test.lua 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 1079d2ab4..9376c367d 100644 --- a/test/sql/gh-4546-sql-drop-grants.result +++ b/test/sql/gh-4546-sql-drop-grants.result @@ -20,6 +20,12 @@ test_space = box.schema.create_space('T', { \ }) --- ... +seq = box.schema.sequence.create('S') +--- +... +ind = test_space:create_index('I', {sequence = 'S'}) +--- +... box.schema.user.grant('test_user1', 'read', 'space', 'T') --- ... @@ -36,3 +42,13 @@ box.schema.user.drop('test_user1') box.schema.user.drop('test_user2') --- ... +seqs = box.space._sequence:select{} +--- +... +#seqs == 1 and seqs[1].name == seq.name or seqs +--- +- true +... +seq:drop() +--- +... diff --git a/test/sql/gh-4546-sql-drop-grants.test.lua b/test/sql/gh-4546-sql-drop-grants.test.lua index 09cfeda19..ddb8cb0bb 100644 --- a/test/sql/gh-4546-sql-drop-grants.test.lua +++ b/test/sql/gh-4546-sql-drop-grants.test.lua @@ -12,8 +12,13 @@ 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', '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') +seqs = box.space._sequence:select{} +#seqs == 1 and seqs[1].name == seq.name or seqs +seq:drop() -- 2.21.0 (Apple Git-122.2) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] sql: drop only generated sequence in DROP TABLE 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 0 siblings, 1 reply; 14+ messages in thread From: Nikita Pettik @ 2019-12-24 1:23 UTC (permalink / raw) To: Chris Sosnin; +Cc: tarantool-patches, v.shpilevoy On 18 Dec 14:00, Chris Sosnin wrote: > create mode 100644 test/sql/autoincrement.result > create mode 100644 test/sql/autoincrement.test.lua > > diff --git a/test/sql/autoincrement.test.lua b/test/sql/autoincrement.test.lua Patch itself is LGTM (as obvious). But I am not a big fan of placing each test in separate file (at least when there's no specific issue for that). Moreover, in commit message it is said that this patch is follow-up to #4546, so could you move this test to gh-4546-sql-drop-grants.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 1079d2ab4..9376c367d 100644 > --- a/test/sql/gh-4546-sql-drop-grants.result > +++ b/test/sql/gh-4546-sql-drop-grants.result > @@ -20,6 +20,12 @@ test_space = box.schema.create_space('T', { \ > }) > --- > ... > +seq = box.schema.sequence.create('S') > +--- > +... > +ind = test_space:create_index('I', {sequence = 'S'}) > +--- > +... > box.schema.user.grant('test_user1', 'read', 'space', 'T') > --- > ... > @@ -36,3 +42,13 @@ box.schema.user.drop('test_user1') > box.schema.user.drop('test_user2') > --- > ... > +seqs = box.space._sequence:select{} > +--- > +... > +#seqs == 1 and seqs[1].name == seq.name or seqs > +--- > +- true > +... > +seq:drop() > +--- > +... > diff --git a/test/sql/gh-4546-sql-drop-grants.test.lua b/test/sql/gh-4546-sql-drop-grants.test.lua > index 09cfeda19..ddb8cb0bb 100644 > --- a/test/sql/gh-4546-sql-drop-grants.test.lua > +++ b/test/sql/gh-4546-sql-drop-grants.test.lua > @@ -12,8 +12,13 @@ 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', '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') > +seqs = box.space._sequence:select{} > +#seqs == 1 and seqs[1].name == seq.name or seqs > +seq:drop() > -- > 2.21.0 (Apple Git-122.2) > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] sql: drop only generated sequence in DROP TABLE 2019-12-24 1:23 ` Nikita Pettik @ 2019-12-24 16:26 ` Vladislav Shpilevoy 2019-12-24 23:19 ` Nikita Pettik 0 siblings, 1 reply; 14+ messages in thread From: Vladislav Shpilevoy @ 2019-12-24 16:26 UTC (permalink / raw) To: Nikita Pettik, Chris Sosnin; +Cc: tarantool-patches Thanks for the review! On 24/12/2019 02:23, Nikita Pettik wrote: > On 18 Dec 14:00, Chris Sosnin wrote: >> create mode 100644 test/sql/autoincrement.result >> create mode 100644 test/sql/autoincrement.test.lua >> >> diff --git a/test/sql/autoincrement.test.lua b/test/sql/autoincrement.test.lua > > Patch itself is LGTM (as obvious). But I am not a big fan of placing each > test in separate file (at least when there's no specific issue for that). As you know, me neither. I added this new file to put here all autoincrement stuff now and in future. We have sql-tap/autoinc, but sql-tap tests are really hard to write and fix. > Moreover, in commit message it is said that this patch is follow-up to > #4546, so could you move this test to gh-4546-sql-drop-grants.test.lua ? 4546 was about table grants. The test in autoincrement file about a different bug - drop of sequence. It is kinda related to 4546 in that sequence grants also should be dropped, if their sequence is deleted. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] sql: drop only generated sequence in DROP TABLE 2019-12-24 16:26 ` Vladislav Shpilevoy @ 2019-12-24 23:19 ` Nikita Pettik 0 siblings, 0 replies; 14+ messages in thread From: Nikita Pettik @ 2019-12-24 23:19 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches On 24 Dec 17:26, Vladislav Shpilevoy wrote: > Thanks for the review! > > On 24/12/2019 02:23, Nikita Pettik wrote: > > On 18 Dec 14:00, Chris Sosnin wrote: > >> create mode 100644 test/sql/autoincrement.result > >> create mode 100644 test/sql/autoincrement.test.lua > >> > >> diff --git a/test/sql/autoincrement.test.lua b/test/sql/autoincrement.test.lua > > > > Patch itself is LGTM (as obvious). But I am not a big fan of placing each > > test in separate file (at least when there's no specific issue for that). > > As you know, me neither. I added this new file to put here all > autoincrement stuff now and in future. We have sql-tap/autoinc, > but sql-tap tests are really hard to write and fix. It's hard in comparison with sql/ suite. But in general, sql tap tests are quite simple and easy to write (IMHO). > > Moreover, in commit message it is said that this patch is follow-up to > > #4546, so could you move this test to gh-4546-sql-drop-grants.test.lua ? > > 4546 was about table grants. The test in autoincrement file about a > different bug - drop of sequence. It is kinda related to 4546 in that > sequence grants also should be dropped, if their sequence is deleted. Ok, then I will simply remove 'follow-up' remark and push it to master. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-12-24 23:19 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-06 12:53 [Tarantool-patches] [PATCH] sql: remove grants associated with the table Chris Sosnin 2019-12-07 10:29 ` Chris Sosnin 2019-12-10 23:45 ` Vladislav Shpilevoy 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox