Re: [tarantool-patches] [PATCH 2/4] Add entities user, role to access control.

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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.