From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 22 Aug 2018 14:58:06 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH 3/4] Add single object privilege checks to access_check_ddl. Message-ID: <20180822115806.rjb76wbg5g6nsxap@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Serge Petrenko Cc: kostja@tarantool.org, tarantool-patches@freelists.org List-ID: On Mon, Aug 20, 2018 at 11:10:07AM +0300, Serge Petrenko wrote: > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 436827a6d..b58832612 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -62,7 +62,8 @@ > /* {{{ Auxiliary functions and methods. */ > > static void > -access_check_ddl(const char *name, uint32_t owner_uid, > +access_check_ddl(const char *name, uint32_t object_id, > + uint32_t owner_uid, Nit: this could fit in one line. > enum schema_object_type type, > enum priv_type priv_type, This too. > bool is_17_compat_mode) > @@ -103,9 +104,51 @@ access_check_ddl(const char *name, uint32_t owner_uid, > */ > if (access == 0 || (is_owner && !(access & (PRIV_U | PRIV_C)))) > return; /* Access granted. */ > + /* > + * You can't grant CREATE privilege to a non-existing object. > + * USAGE can be granted only globally. > + */ > + if (access & (PRIV_U | PRIV_C)) > + goto error; AFAIU in case of index creation you need to check PRIV_C on the space, no? > + /* Check for privileges on a single object. */ > + switch (type) { > + case SC_SPACE: > + { > + struct space *space = space_by_id(object_id); > + if (space) > + access &= ~space->access[cr->auth_token].effective; > + break; > + } > + case SC_FUNCTION: > + { > + struct func *func = func_by_id(object_id); > + if (func) > + access &= ~func->access[cr->auth_token].effective; > + break; > + } > + case SC_USER: > + case SC_ROLE: > + { > + struct user *user_or_role = user_by_id(object_id); > + if (user_or_role) > + access &= ~user_or_role->access[cr->auth_token].effective; > + break; > + } > + case SC_SEQUENCE: > + { > + struct sequence *seq = sequence_by_id(object_id); > + if (seq) > + access &= ~seq->access[cr->auth_token].effective; > + break; > + } > + default: > + break; > + } Why don't you use access_find() here? That would reduce the amount of code so that you wouldn't need to use the error label. > + if (access == 0) > + return; /* Access granted. */ Nit: spaces instead of a tab. > > /* Create a meaningful error message. */ > - struct user *user = user_find_xc(cr->uid); > +error: struct user *user = user_find_xc(cr->uid); > const char *object_name; > const char *pname; > if (access & PRIV_U) { > diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua > index bad35b680..1c254fe1d 100644 > --- a/src/box/lua/schema.lua > +++ b/src/box/lua/schema.lua > @@ -2060,6 +2060,9 @@ box.schema.user.create = function(name, opts) > uid = _user:auto_increment{session.euid(), name, 'user', auth_mech_list}[1] > -- grant role 'public' to the user > box.schema.user.grant(uid, 'public') > + -- grant user 'alter' on itself, so it can Nit: -- Grant privilege 'alter to itself so that it can > + -- change its password or username. > + box.schema.user.grant(uid, 'alter', 'user', uid) > -- we have to grant global privileges from setuid function, since > -- only admin has the ownership over universe and we don't have > -- grant option > diff --git a/src/box/user.cc b/src/box/user.cc > index b4fb65a59..83d07f7b3 100644 > --- a/src/box/user.cc > +++ b/src/box/user.cc > @@ -1860,3 +1875,174 @@ box.session.su('admin') > box.schema.user.drop('tester') > --- > ... > +-- > +-- test case for 3530: do not ignore single object privileges in > +-- access_check_ddl. Please don't mention core function names in the tests, because we can rename them any time, which will make it more difficult to understand the tests.