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

[Konstantin Osipov <kostja@tarantool.org>]

* Serge Petrenko <sergepetrenko@tarantool.org> [18/07/12 20:54]:
> +		/* Do not allow changes for system users and roles. */
> +		struct credentials *cr = effective_user();
> +		if ((uid <= (uint32_t) BOX_SYSTEM_USER_ID_MAX || uid == SUPER) &&
> +		    cr->uid != ADMIN) {
> +			struct user *current_user = user_find_xc(cr->uid);
> +			tnt_raise(AccessDeniedError, "alter", "user or role",
> +				  old_user->def->name, current_user->def->name);
> +		}

It should be possible to change these system objects, provided the
person knows what they are doing, please remove this check, esp.
since it's outside the scope of this bug fix.

>  	case SC_ROLE:
>  	{
> -		struct user *role = user_by_id(priv->object_id);
> -		if (role == NULL || role->def->type != SC_ROLE) {
> -			tnt_raise(ClientError, ER_NO_SUCH_ROLE,
> -				  role ? role->def->name :
> -				  int2str(priv->object_id));
> -		}
> -		/*
> -		 * Only the creator of the role can grant or revoke it.
> -		 * Everyone can grant 'PUBLIC' role.
> -		 */
> -		if (role->def->owner != grantor->def->uid &&
> -		    grantor->def->uid != ADMIN &&
> -		    (role->def->uid != PUBLIC || priv->access != PRIV_X)) {
> +		if (priv->object_id != 0) {
> +			struct user *role = user_by_id(priv->object_id);
> +			if (role == NULL || role->def->type != SC_ROLE) {
> +				tnt_raise(ClientError, ER_NO_SUCH_ROLE,
> +					  role ? role->def->name :
> +					  int2str(priv->object_id));
> +			}
> +			/*
> +			 * Only the creator of the role can grant or revoke it.
> +			 * Everyone can grant 'PUBLIC' role.
> +			 */
> +			if (role->def->owner != grantor->def->uid &&
> +			    grantor->def->uid != ADMIN &&
> +			    (role->def->uid != PUBLIC || priv->access != PRIV_X)) {
> +				tnt_raise(AccessDeniedError,
> +					  priv_name(priv_type),
> +					  schema_object_name(SC_ROLE), name,
> +					  grantor->def->name);
> +			}
> +			/* Not necessary to do during revoke, but who cares. */
> +			role_check(grantee, role);
> +		} else if (grantor->def->uid != ADMIN) {
> +			/* only admin may grant privileges on an entire entity. */

>  			tnt_raise(AccessDeniedError,
>  				  priv_name(priv_type),
>  				  schema_object_name(SC_ROLE), name,
>  				  grantor->def->name);
>  		}
> -		/* Not necessary to do during revoke, but who cares. */
> -		role_check(grantee, role);

Why no 'break;'?

Instead of changing indent level you could have added a break.

> +	case SC_USER:
> +	{
> +		/*
> +		 * user ID 0 is shared between user 'guest' and granting
> +		 * privileges upon whole entity user. This is not a problem,
> +		 * since we don't want to grant privileges on any system user,
> +		 * including 'guest'.
> +		 */
> +		if(priv->object_id == 0) {
> +			access = entity_access.user;
> +			break;
> +		}

Hm, does it mean that now it's not possible to grant some
privileges to 'guest'? Looks like we should be using a different
identifier for entity-level grants then?
Are you testing this at all? (I found no test for this).

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