[tarantool-patches] [PATCH 2/4] Add entities user, role to access control.
Vladimir Davydov
vdavydov.dev at gmail.com
Wed Aug 22 13:37:43 MSK 2018
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.
More information about the Tarantool-patches
mailing list