From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 352AD275C2 for ; Thu, 26 Jul 2018 16:15:55 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 09mrqgVJ2Gap for ; Thu, 26 Jul 2018 16:15:55 -0400 (EDT) Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id D1C9A275FE for ; Thu, 26 Jul 2018 16:15:53 -0400 (EDT) Date: Thu, 26 Jul 2018 23:15:52 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH] Add entities user, role to access control. Message-ID: <20180726201552.GB26714@chai> References: <20180712152932.60750-1-sergepetrenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180712152932.60750-1-sergepetrenko@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Serge Petrenko * Serge Petrenko [18/07/12 20:54]: > + /* Do not allow changes for system users and roles. */ > + struct credentials *cr = effective_user(); > + if ((uid <= (uint32_t) BOX_SYSTEM_USER_ID_MAX || uid == SUPER) && > + cr->uid != ADMIN) { > + struct user *current_user = user_find_xc(cr->uid); > + tnt_raise(AccessDeniedError, "alter", "user or role", > + old_user->def->name, current_user->def->name); > + } It should be possible to change these system objects, provided the person knows what they are doing, please remove this check, esp. since it's outside the scope of this bug fix. > case SC_ROLE: > { > - struct user *role = user_by_id(priv->object_id); > - if (role == NULL || role->def->type != SC_ROLE) { > - tnt_raise(ClientError, ER_NO_SUCH_ROLE, > - role ? role->def->name : > - int2str(priv->object_id)); > - } > - /* > - * Only the creator of the role can grant or revoke it. > - * Everyone can grant 'PUBLIC' role. > - */ > - if (role->def->owner != grantor->def->uid && > - grantor->def->uid != ADMIN && > - (role->def->uid != PUBLIC || priv->access != PRIV_X)) { > + if (priv->object_id != 0) { > + struct user *role = user_by_id(priv->object_id); > + if (role == NULL || role->def->type != SC_ROLE) { > + tnt_raise(ClientError, ER_NO_SUCH_ROLE, > + role ? role->def->name : > + int2str(priv->object_id)); > + } > + /* > + * Only the creator of the role can grant or revoke it. > + * Everyone can grant 'PUBLIC' role. > + */ > + if (role->def->owner != grantor->def->uid && > + grantor->def->uid != ADMIN && > + (role->def->uid != PUBLIC || priv->access != PRIV_X)) { > + tnt_raise(AccessDeniedError, > + priv_name(priv_type), > + schema_object_name(SC_ROLE), name, > + grantor->def->name); > + } > + /* Not necessary to do during revoke, but who cares. */ > + role_check(grantee, role); > + } else if (grantor->def->uid != ADMIN) { > + /* only admin may grant privileges on an entire entity. */ > tnt_raise(AccessDeniedError, > priv_name(priv_type), > schema_object_name(SC_ROLE), name, > grantor->def->name); > } > - /* Not necessary to do during revoke, but who cares. */ > - role_check(grantee, role); Why no 'break;'? Instead of changing indent level you could have added a break. > + case SC_USER: > + { > + /* > + * user ID 0 is shared between user 'guest' and granting > + * privileges upon whole entity user. This is not a problem, > + * since we don't want to grant privileges on any system user, > + * including 'guest'. > + */ > + if(priv->object_id == 0) { > + access = entity_access.user; > + break; > + } Hm, does it mean that now it's not possible to grant some privileges to 'guest'? Looks like we should be using a different identifier for entity-level grants then? Are you testing this at all? (I found no test for this). -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov