Re: [tarantool-patches] [PATCH 3/4] Add single object privilege checks to access_check_ddl.

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

On Mon, Aug 20, 2018 at 11:10:07AM +0300, Serge Petrenko wrote:
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 436827a6d..b58832612 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -62,7 +62,8 @@
>  /* {{{ Auxiliary functions and methods. */
>  
>  static void
> -access_check_ddl(const char *name, uint32_t owner_uid,
> +access_check_ddl(const char *name, uint32_t object_id,
> +		 uint32_t owner_uid,

Nit: this could fit in one line.

>  		 enum schema_object_type type,
>  		 enum priv_type priv_type,

This too.

>  		 bool is_17_compat_mode)
> @@ -103,9 +104,51 @@ access_check_ddl(const char *name, uint32_t owner_uid,
>  	 */
>  	if (access == 0 || (is_owner && !(access & (PRIV_U | PRIV_C))))
>  		return; /* Access granted. */
> +	/*
> +	 * You can't grant CREATE privilege to a non-existing object.
> +	 * USAGE can be granted only globally.
> +	 */
> +	if (access & (PRIV_U | PRIV_C))
> +		goto error;

AFAIU in case of index creation you need to check PRIV_C on the space,
no?

> +	/* Check for privileges on a single object. */
> +	switch (type) {
> +	case SC_SPACE:
> +	{
> +		struct space *space = space_by_id(object_id);
> +		if (space)
> +			access &= ~space->access[cr->auth_token].effective;
> +		break;
> +	}
> +	case SC_FUNCTION:
> +	{
> +		struct func *func = func_by_id(object_id);
> +		if (func)
> +			access &= ~func->access[cr->auth_token].effective;
> +		break;
> +	}
> +	case SC_USER:
> +	case SC_ROLE:
> +	{
> +		struct user *user_or_role = user_by_id(object_id);
> +		if (user_or_role)
> +			access &= ~user_or_role->access[cr->auth_token].effective;
> +		break;
> +	}
> +	case SC_SEQUENCE:
> +	{
> +		struct sequence *seq = sequence_by_id(object_id);
> +		if (seq)
> +			access &= ~seq->access[cr->auth_token].effective;
> +		break;
> +	}
> +	default:
> +		break;
> +	}

Why don't you use access_find() here? That would reduce the amount of
code so that you wouldn't need to use the error label.

> +	if (access == 0)
> +	    return; /* Access granted. */

Nit: spaces instead of a tab.

>  
>  	/* Create a meaningful error message. */
> -	struct user *user = user_find_xc(cr->uid);
> +error:	struct user *user = user_find_xc(cr->uid);
>  	const char *object_name;
>  	const char *pname;
>  	if (access & PRIV_U) {
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index bad35b680..1c254fe1d 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -2060,6 +2060,9 @@ box.schema.user.create = function(name, opts)
>      uid = _user:auto_increment{session.euid(), name, 'user', auth_mech_list}[1]
>      -- grant role 'public' to the user
>      box.schema.user.grant(uid, 'public')
> +    -- grant user 'alter' on itself, so it can

Nit:

       -- Grant privilege 'alter to itself so that it can

> +    -- change its password or username.
> +    box.schema.user.grant(uid, 'alter', 'user', uid)
>      -- we have to grant global privileges from setuid function, since
>      -- only admin has the ownership over universe and we don't have
>      -- grant option
> diff --git a/src/box/user.cc b/src/box/user.cc
> index b4fb65a59..83d07f7b3 100644
> --- a/src/box/user.cc
> +++ b/src/box/user.cc
> @@ -1860,3 +1875,174 @@ box.session.su('admin')
>  box.schema.user.drop('tester')
>  ---
>  ...
> +--
> +-- test case for 3530: do not ignore single object privileges in
> +-- access_check_ddl.

Please don't mention core function names in the tests, because we can
rename them any time, which will make it more difficult to understand
the tests.