From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Wed, 22 Aug 2018 13:28:45 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [tarantool-patches] [PATCH 1/4] Introduce separate entity object
 types for entity privileges.
Message-ID: <20180822102845.obibc7ren43m4lmq@esperanza>
References: <cover.1534751862.git.sergepetrenko@tarantool.org>
 <8730529d7aefb43da1dd5fde8f01a6b927f8430e.1534751862.git.sergepetrenko@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8730529d7aefb43da1dd5fde8f01a6b927f8430e.1534751862.git.sergepetrenko@tarantool.org>
To: Serge Petrenko <sergepetrenko@tarantool.org>
Cc: kostja@tarantool.org, tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Mon, Aug 20, 2018 at 11:10:05AM +0300, Serge Petrenko wrote:
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 3007a131d..42136b7df 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -2537,10 +2537,35 @@ priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple)
>  {
>  	priv->grantor_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_ID);
>  	priv->grantee_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_UID);
> +
>  	const char *object_type =
>  		tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_TYPE);
> -	priv->object_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_OBJECT_ID);
>  	priv->object_type = schema_object_type(object_type);
> +
> +	const char *data = tuple_field(tuple, BOX_PRIV_FIELD_OBJECT_ID);
> +	if (data == NULL) {
> +		tnt_raise(ClientError, ER_NO_SUCH_FIELD,
> +		      BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE);
> +	}
> +	/*
> +	 * When granting or revoking privileges on a whole entity
> +	 * we pass empty string ('') to object_id to indicate
> +	 * grant on every object of that entity.
> +	 * So check for that first.
> +	 */
> +	switch (mp_typeof(*data)) {
> +	case MP_STR:
> +		if (mp_decode_strl(&data) == 0) {
> +			/* Entity-wide privilege. */
> +			priv->object_id = 0;
> +			priv->object_type = schema_entity_type(priv->object_type);
> +			break;
> +		}
> +		FALLTHROUGH;
> +	default:
> +		priv->object_id = tuple_field_u32_xc(tuple,
> +						     BOX_PRIV_FIELD_OBJECT_ID);
> +	}
>  	if (priv->object_type == SC_UNKNOWN) {
>  		tnt_raise(ClientError, ER_UNKNOWN_SCHEMA_OBJECT,
>  			  object_type);

> +enum schema_object_type
> +schema_entity_type(enum schema_object_type type)
> +{
> +	switch (type) {
> +	case SC_SPACE:
> +		return SC_ENTITY_SPACE;
> +	case SC_FUNCTION:
> +		return SC_ENTITY_FUNCTION;
> +	case SC_USER:
> +		return SC_ENTITY_USER;
> +	case SC_ROLE:
> +		return SC_ENTITY_ROLE;
> +	case SC_SEQUENCE:
> +		return SC_ENTITY_SEQUENCE;
> +	case SC_COLLATION:
> +		return SC_ENTITY_COLLATION;
> +	default:
> +		unreachable();
> +	}
> +}

You'll reach this unreachable() if you insert something like this into
_priv table

  box.space._priv:insert{1, 1, 'abc', '', 1}

I guess you should return SC_UNKNOWN instead.

Also, after looking at patch 2, I'm convinced SC_ENTITY_USER and
SC_ENTITY_ROLE should be defined there, not in this patch. Please
rebase.

Also, there's priv_def_check(), which I missed by doing review last
time. This function handles object_id == 0 for all object types, but
there's no need to anymore, because you have entity types now. Please
remove. You should add handling of entity types to that function in
this patch (you do it in patch 2 for some reason).