[Tarantool-patches] [PATCH] sql: remove grants associated with the table
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Dec 11 02:45:24 MSK 2019
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.
More information about the Tarantool-patches
mailing list