From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 22 Aug 2018 13:37:43 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH 2/4] Add entities user, role to access control. Message-ID: <20180822103743.72y5glc2fuuhamyj@esperanza> References: <4edfd1024c84ab0bfd1a752e9d84ef0356036b48.1534751862.git.sergepetrenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4edfd1024c84ab0bfd1a752e9d84ef0356036b48.1534751862.git.sergepetrenko@tarantool.org> To: Serge Petrenko Cc: kostja@tarantool.org, tarantool-patches@freelists.org List-ID: On Mon, Aug 20, 2018 at 11:10:06AM +0300, Serge Petrenko wrote: > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 42136b7df..436827a6d 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -2170,7 +2170,7 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event) > struct user *old_user = user_by_id(uid); > if (new_tuple != NULL && old_user == NULL) { /* INSERT */ > struct user_def *user = user_def_new_from_tuple(new_tuple); > - access_check_ddl(user->name, user->owner, SC_USER, PRIV_C, true); > + access_check_ddl(user->name, user->owner, user->type, PRIV_C, true); > auto def_guard = make_scoped_guard([=] { free(user); }); > (void) user_cache_replace(user); > def_guard.is_active = false; > @@ -2179,7 +2179,7 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event) > txn_on_rollback(txn, on_rollback); > } else if (new_tuple == NULL) { /* DELETE */ > access_check_ddl(old_user->def->name, old_user->def->owner, > - SC_USER, PRIV_D, true); > + old_user->def->type, PRIV_D, true); > /* Can't drop guest or super user */ > if (uid <= (uint32_t) BOX_SYSTEM_USER_ID_MAX || uid == SUPER) { > tnt_raise(ClientError, ER_DROP_USER, There's one more call to access_check_ddl() in this funciton, in the UPDATE case. I guess you should replace SC_USER with old_user->type there too. > @@ -2670,6 +2670,38 @@ priv_def_check(struct priv_def *priv, enum priv_type priv_type) > } > /* Not necessary to do during revoke, but who cares. */ > role_check(grantee, role); > + break; > + } > + case SC_USER: > + { > + struct user *user = NULL; > + user = user_by_id(priv->object_id); Nit: struct user *user = user_by_id(priv->object_id); > + if (user == NULL || user->def->type != SC_USER) { > + tnt_raise(ClientError, ER_NO_SUCH_USER, > + user ? user->def->name : > + int2str(priv->object_id)); > + } > + if (user->def->owner != grantor->def->uid && > + grantor->def->uid != ADMIN) { > + tnt_raise(AccessDeniedError, > + priv_name(priv_type), > + schema_object_name(SC_USER), name, > + grantor->def->name); > + } I think you could painlessly merge this condition with SC_ROLE above. Not sure if it's worth it though. > + break; > + } > + case SC_ENTITY_SPACE: > + case SC_ENTITY_FUNCTION: > + case SC_ENTITY_SEQUENCE: > + case SC_ENTITY_ROLE: > + case SC_ENTITY_USER: > + { > + /* Only amdin may grant privileges on an entire entity. */ > + if (grantor->def->uid != ADMIN) { > + tnt_raise(AccessDeniedError, priv_name(priv_type), > + schema_object_name(priv->object_type), name, > + grantor->def->name); > + } This should go to patch 1. This patch should only add SC_ENTITY_USER/ROLE here. > } > default: > break; > @@ -1845,19 +1847,24 @@ local function object_resolve(object_type, object_name) > end > return seq > end > - if object_type == 'role' then > + if object_type == 'role' or object_type == 'user' then > + if object_name == '' then > + return '' > + end > local _vuser = box.space[box.schema.VUSER_ID] > - local role > + local role_or_user > if type(object_name) == 'string' then > - role = _vuser.index.name:get{object_name} > + role_or_user = _vuser.index.name:get{object_name} > else > - role = _vuser:get{object_name} > + role_or_user = _vuser:get{object_name} > end > - if role and role[4] == 'role' then > - return role[1] > - else > + if role_or_user and role_or_user[4] == object_type then > + return role_or_user[1] > + elseif object_type == 'role' then > box.error(box.error.NO_SUCH_ROLE, object_name) > - end > + else > + box.error(box.error.NO_SUCH_USER, object_name) > + end Nit: tab instead of spaces. > diff --git a/src/box/user.cc b/src/box/user.cc > index eec785652..b4fb65a59 100644 > --- a/src/box/user.cc > +++ b/src/box/user.cc > @@ -236,6 +246,16 @@ access_find(struct priv_def *priv) > access = func->access; > break; > } > + case SC_USER: > + { > + /* No grants on a single object user yet. */ > + break; > + } > + case SC_ROLE: > + { > + /* No grants on a single object role yet. */ > + break; > + } Again, these two could be painlessly merged, I guess. And it wouldn't hurt patch 3 IMO. Up to you.