From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 7215D46970E for ; Tue, 24 Dec 2019 04:37:32 +0300 (MSK) Date: Tue, 24 Dec 2019 04:37:31 +0300 From: Nikita Pettik Message-ID: <20191224013731.GE41539@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH 1/2] 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 Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org 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); > /*