From: Konstantin Osipov <kostja@tarantool.org> To: tarantool-patches@freelists.org Cc: Serge Petrenko <sergepetrenko@tarantool.org> Subject: [tarantool-patches] Re: [PATCH] Make access_check_ddl check for entity privileges. Date: Wed, 11 Jul 2018 21:33:58 +0300 [thread overview] Message-ID: <20180711183358.GA31667@chai> (raw) In-Reply-To: <20180711164036.33604-1-sergepetrenko@tarantool.org> * Serge Petrenko <sergepetrenko@tarantool.org> [18/07/11 19:42]: > /* > * Only the owner of the object or someone who has > * specific DDL privilege on the object can execute > @@ -96,6 +102,40 @@ access_check_ddl(const char *name, uint32_t owner_uid, > */ > if (access == 0 || (is_owner && !(access & (PRIV_U|PRIV_C)))) > return; /* Access granted. */ > + int rc = -1; > + if (!(access & (PRIV_U | PRIV_C))) { You can't grant CREATE privilege to a non-existing object. USAGE can be granted only globally. This comment explains this if statement. Please add it. I > + * Ignore universe and unknown > + * types here, since universe is already handled, and what > + * to do with unknown is unknown. I don't understand this comment, please rephrase or remove. > + * > + * Currently no specific privileges to a single role, user, > + * collation. > + */ > + switch (type) { > + case SC_SPACE: > + rc = access_check_space( > + space_cache_find_xc(object_id), > + access); > + break; This switch statement is not aligned right. You should use lisp-style alignment for function arguments. Throwing ER_NO_SUCH_SPACE is a security breach, you should not reveal that the space does not exist, instead simply say that the user has no requested access to space (ER_ACCESS_DENIED). > + case SC_SEQUENCE: > + if (priv_type == PRIV_W) { > + rc = access_check_sequence( > + sequence_cache_find(object_id)); > + break; > + } Wrong alignment, same argument about exceptions. > + case SC_FUNCTION: > + case SC_USER: > + case SC_ROLE: > + case SC_COLLATION: What about these? Can't you grant DROP/ALTER on a specific user or role? > @@ -1751,11 +1791,9 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) > uint32_t iid = tuple_field_u32_xc(old_tuple ? old_tuple : new_tuple, > BOX_INDEX_FIELD_ID); > struct space *old_space = space_cache_find_xc(id); > - enum priv_type priv_type = new_tuple ? PRIV_C : PRIV_D; > - if (old_tuple && new_tuple) > - priv_type = PRIV_A; > - access_check_ddl(old_space->def->name, old_space->def->uid, SC_SPACE, > - priv_type, true); > + enum priv_type priv_type = new_tuple ? PRIV_A : PRIV_D; > + access_check_ddl(old_space->def->name, old_space->def->id, > + old_space->def->uid, SC_SPACE, priv_type, true); As far as I understand, you changed it because creating an index is technically altering a space, not creating it. But in this case dropping an index is also technically altering a space. In SQL, CREATE/DROP/ALTER match SQL statements CREATE/DROP/ALTER respectively. Since in NoSQL in Tarantool we don't have these statements, instead, we create each index with a separate Lua command, let's keep the old check: use CREATE access to space in order to permit CREATING an index, ALTER - to permit update, and DROP - to permit drop. > @@ -2185,8 +2224,8 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event) > * correct. > */ > struct user_def *user = user_def_new_from_tuple(new_tuple); > - access_check_ddl(user->name, user->uid, SC_USER, PRIV_A, > - true); > + access_check_ddl(user->name, user->uid, user->uid, SC_USER, > + PRIV_A, true); Why did you write user->uid here for the owner? > end > + if object_type == 'user' then > + if object_name == nil or object_name == 0 then > + return 0 > + end > + -- otherwise some error. Don't know which one yet. > + box.error(box.error.NO_SUCH_USER, object_name) > + end It would be better if you extract new entities into a separate patch. The bigger the patch gets the less changes for it to get in - there always is something that can be improved. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov
next prev parent reply other threads:[~2018-07-11 18:34 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-11 16:40 [tarantool-patches] " Serge Petrenko 2018-07-11 18:33 ` Konstantin Osipov [this message] 2018-07-12 8:52 ` [tarantool-patches] " Sergey Petrenko 2018-07-18 6:07 ` Konstantin Osipov [not found] <c734dd77-57dd-b0a3-af26-bf38937b1725@tarantool.org> 2018-07-19 7:48 ` Sergey Petrenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180711183358.GA31667@chai \ --to=kostja@tarantool.org \ --cc=sergepetrenko@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH] Make access_check_ddl check for entity privileges.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox