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

[Konstantin Osipov <kostja@tarantool.org>]

* Serge Petrenko <sergepetrenko@tarantool.org> [18/07/17 21:31]:

This patch is generally LGTM, but it's based on entity access, so
I can't push it, since I requested some changes in entity access
patch. A few questions below.

> access_check_ddl() didn't check for single object privileges, e.g. user
> with alter access on a space couldn't create an index in this space. It
> would only succeed if it had alter on entire entity space.
> Fix this by adding single object privilege checks to access_check_ddl and
> adding access cache to struct user, to hold other users' privileges on it.
> 
> Also checking for single object privilege made it possible to grant
> every user alter privilege on itself, so that a user may change its own
> password (previously it was possible because of a hack). Removed the
> hack, and added grant alter to itself upon user creation.
> Modified tests accordingly, and added a couple of test cases.

What hack did you remove? The problem I have with self-granting
'alter' is that old deployments will break, since they don't have
a self-grant. Maybe we leave the hack in place for a while, begin
self-granting 'alter', and only remove the hack in 2.0? We
shouldn't forget to grant everyone an 'alter' on self in 2.0 upgrade script.
But to begin with, I can't find where this hack was in the code,
and where you removed it.

> Closes #3530
> ---
>  src/box/alter.cc         | 123 ++++++++++++++++++++++----------
>  src/box/lua/schema.lua   |   5 +-
>  src/box/user.cc          |  10 ++-
>  src/box/user.h           |   2 +
>  test/box/access.result   | 182 +++++++++++++++++++++++++++++++++++++++++++++++
>  test/box/access.test.lua |  53 ++++++++++++++
>  test/box/role.result     |   9 +++
>  test/box/sequence.result |   3 +
>  8 files changed, 347 insertions(+), 40 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 6293dcc50..54a09664b 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,
>  		 enum schema_object_type type,
>  		 enum priv_type priv_type,
>  		 bool is_17_compat_mode)
> @@ -103,7 +104,48 @@ 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))) {
> +		/* Check for privileges on a single object. */

Please avoid this deep nesting, use early returns from the
function for example, or create a separate function,
access_check_ddl_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;
> +		}

OK.

>  		 * check that it has read and write access.
>  		 */
> -		access_check_ddl(seq->def->name, seq->def->uid, SC_SEQUENCE,
> -				 PRIV_R, false);
> -		access_check_ddl(seq->def->name, seq->def->uid, SC_SEQUENCE,
> -				 PRIV_W, false);
> +		access_check_ddl(seq->def->name, seq->def->id, seq->def->uid,
> +				 SC_SEQUENCE, PRIV_R, false);
> +		access_check_ddl(seq->def->name, seq->def->id, seq->def->uid,
> +				 SC_SEQUENCE, PRIV_W, false);
>  	}
>  	/** Check we have alter access on space. */
> -	access_check_ddl(space->def->name, space->def->uid, SC_SPACE, PRIV_A,
> -			 false);
> +	access_check_ddl(space->def->name, space->def->id, space->def->uid,
> +			 SC_SPACE, PRIV_A, false);
>  
>  	struct trigger *on_commit =
>  		txn_alter_trigger_new(on_commit_dd_space_sequence, space);
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 4b7a14411..a098e44fe 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -1740,7 +1740,7 @@ local priv_object_combo = {
>      ["role"]     = bit.bor(box.priv.X, box.priv.U,
>                             box.priv.C, box.priv.D),
>      ["user"]	 = bit.bor(box.priv.C, box.priv.U,
> -                           box.priv.D),
> +                           box.priv.A, box.priv.D),
>  }
>      -- grant role 'public' to the user
>      box.schema.user.grant(uid, 'public')
> +    -- grant user 'alter' on itself, so it can
> +    -- change its password or username.
> +    box.schema.user.grant(uid, 'alter', 'user', uid)

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov