Tarantool development patches archive
 help / color / mirror / Atom feed
* [tarantool-patches] [PATCH] Add entities user, role to access control.
@ 2018-07-12 15:29 Serge Petrenko
  2018-07-26 20:15 ` [tarantool-patches] " Konstantin Osipov
  0 siblings, 1 reply; 3+ messages in thread
From: Serge Petrenko @ 2018-07-12 15:29 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Serge Petrenko

Previously the only existing entities in access control were space,
funciton and sequence. Added user and role entities, so it is now
possible to create users or roles without create privilege on universe.
Also added all the needed checks and modified tests accordingly.

Closes #3524
---
Please first see the patch regarding #3516. This patch is based on it.

https://github.com/tarantool/tarantool/tree/sergepetrenko/gh-3524-entity-access-grants
https://github.com/tarantool/tarantool/issues/3524
 src/box/alter.cc            | 61 ++++++++++++++++++++++++++++++++-------------
 src/box/lua/schema.lua      | 26 +++++++++++--------
 src/box/schema.h            |  6 +++++
 src/box/user.cc             | 25 ++++++++++++++++++-
 test/box/access.result      | 20 +++++++--------
 test/box/access.test.lua    | 15 ++++-------
 test/box/access_misc.result |  2 +-
 test/box/role.result        | 25 +++++++++++++++++--
 test/box/role.test.lua      | 12 +++++++--
 9 files changed, 138 insertions(+), 54 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index ef8b1acff..c215064d7 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2173,7 +2173,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;
@@ -2182,7 +2182,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,
@@ -2208,6 +2208,14 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
 		 * correct.
 		 */
 		struct user_def *user = user_def_new_from_tuple(new_tuple);
+		/* 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);
+		}
 		access_check_ddl(user->name, user->uid, SC_USER, PRIV_A,
 				 true);
 		auto def_guard = make_scoped_guard([=] { free(user); });
@@ -2628,26 +2636,38 @@ priv_def_check(struct priv_def *priv, enum priv_type priv_type)
 	}
 	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);
+	}
+	case SC_USER:
+	{
+		if (priv->object_id != 0
 	}
 	default:
 		break;
@@ -2668,7 +2688,12 @@ grant_or_revoke(struct priv_def *priv)
 	struct user *grantee = user_by_id(priv->grantee_id);
 	if (grantee == NULL)
 		return;
-	if (priv->object_type == SC_ROLE) {
+	/*
+	 * Grant a role to a user only when privilege type is 'execute'
+	 * and the role is specified.
+	 */
+	if (priv->object_type == SC_ROLE && !(priv->access & ~PRIV_X) &&
+	    priv->object_id != 0) {
 		struct user *role = user_by_id(priv->object_id);
 		if (role == NULL || role->def->type != SC_ROLE)
 			return;
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index dc18f71b2..3b1f237b0 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1737,6 +1737,8 @@ local priv_object_combo = {
                            box.priv.C, box.priv.D),
     ["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),
 }
 
 --
@@ -1840,21 +1842,25 @@ 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
         local _vuser = box.space[box.schema.VUSER_ID]
-        local role
+        local role_or_user
+        if object_name == nil or object_name == 0 then
+            return 0
+        end
         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
     end
-
     box.error(box.error.UNKNOWN_SCHEMA_OBJECT, object_type)
 end
 
@@ -2100,7 +2106,7 @@ local function grant(uid, name, privilege, object_type,
     if privilege_hex ~= old_privilege then
         _priv:replace{options.grantor, uid, object_type, oid, privilege_hex}
     elseif not options.if_not_exists then
-            if object_type == 'role' then
+            if object_type == 'role' and (object_name ~= nil or object_name ~= 0) then
                 box.error(box.error.ROLE_GRANTED, name, object_name)
             else
                 box.error(box.error.PRIV_GRANTED, name, privilege,
diff --git a/src/box/schema.h b/src/box/schema.h
index 0822262d0..f78ea43cc 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -240,6 +240,8 @@ struct on_access_denied_ctx {
 struct entity_access {
        struct access space[BOX_USER_MAX];
        struct access function[BOX_USER_MAX];
+       struct access user[BOX_USER_MAX];
+       struct access role[BOX_USER_MAX];
        struct access sequence[BOX_USER_MAX];
 };
 
@@ -255,6 +257,10 @@ entity_access_get(enum schema_object_type type)
                return entity_access.space;
        case SC_FUNCTION:
                return entity_access.function;
+       case SC_USER:
+	       return entity_access.user;
+       case SC_ROLE:
+	       return entity_access.role;
        case SC_SEQUENCE:
                return entity_access.sequence;
        default:
diff --git a/src/box/user.cc b/src/box/user.cc
index fbf06566a..4edef1d5f 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -229,6 +229,29 @@ access_find(struct priv_def *priv)
 			access = func->access;
 		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;
+		}
+		/* No grants on a single object user yet. */
+	}
+	case SC_ROLE:
+	{
+		/* Tha same remark as in case SC_USER applies. */
+		if (priv->object_id == 0) {
+			access = entity_access.role;
+			break;
+		}
+		/* No grants on a single object role yet. */
+	}
 	case SC_SEQUENCE:
 	{
 		if (priv->object_id == 0) {
@@ -315,7 +338,7 @@ user_reload_privs(struct user *user)
 			 * Skip role grants, we're only
 			 * interested in real objects.
 			 */
-			if (priv.object_type != SC_ROLE)
+			if (priv.object_type != SC_ROLE || !(priv.access & PRIV_X))
 				user_grant_priv(user, &priv);
 		}
 	}
diff --git a/test/box/access.result b/test/box/access.result
index f4669a4a3..31095aec2 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -878,8 +878,6 @@ box.schema.user.create('test')
 box.schema.user.grant('test', 'read', 'space', '_collation')
 ---
 ...
---box.schema.user.grant('test', 'write', 'space', '_collation')
--- FIXME: granting create on 'collation' only doesn't work
 box.schema.user.grant('test', 'create', 'universe')
 ---
 ...
@@ -1427,16 +1425,18 @@ box.session.su("admin")
 box.schema.user.grant('tester', 'write', 'universe')
 ---
 ...
--- no entity user currently, so have to grant create
--- on universe in order to create a user.
-box.schema.user.grant('tester', 'create', 'universe')
+box.schema.user.grant('tester', 'create', 'user')
+---
+...
+box.schema.user.grant('tester', 'create', 'space')
+---
+...
+box.schema.user.grant('tester', 'create', 'function')
+---
+...
+box.schema.user.grant('tester', 'create' , 'sequence')
 ---
 ...
--- this should work instead:
---box.schema.user.grant('tester', 'create', 'user')
---box.schema.user.grant('tester', 'create', 'space')
---box.schema.user.grant('tester', 'create', 'function')
---box.schema.user.grant('tester', 'create' , 'sequence')
 box.schema.user.grant('tester', 'read', 'space', '_sequence')
 ---
 ...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 9ae0e1114..9b7510e64 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -341,8 +341,7 @@ c:close()
 session = box.session
 box.schema.user.create('test')
 box.schema.user.grant('test', 'read', 'space', '_collation')
---box.schema.user.grant('test', 'write', 'space', '_collation')
--- FIXME: granting create on 'collation' only doesn't work
+
 box.schema.user.grant('test', 'create', 'universe')
 session.su('test')
 box.internal.collation.create('test', 'ICU', 'ru_RU')
@@ -538,14 +537,10 @@ box.session.su("admin")
 -- tables from ddl
 --
 box.schema.user.grant('tester', 'write', 'universe')
--- no entity user currently, so have to grant create
--- on universe in order to create a user.
-box.schema.user.grant('tester', 'create', 'universe')
--- this should work instead:
---box.schema.user.grant('tester', 'create', 'user')
---box.schema.user.grant('tester', 'create', 'space')
---box.schema.user.grant('tester', 'create', 'function')
---box.schema.user.grant('tester', 'create' , 'sequence')
+box.schema.user.grant('tester', 'create', 'user')
+box.schema.user.grant('tester', 'create', 'space')
+box.schema.user.grant('tester', 'create', 'function')
+box.schema.user.grant('tester', 'create' , 'sequence')
 box.schema.user.grant('tester', 'read', 'space', '_sequence')
 box.session.su("tester")
 -- successful create
diff --git a/test/box/access_misc.result b/test/box/access_misc.result
index 839b576ac..0f4892533 100644
--- a/test/box/access_misc.result
+++ b/test/box/access_misc.result
@@ -361,7 +361,7 @@ testuser_uid = session.uid()
 ...
 _ = box.space._user:delete(2)
 ---
-- error: Drop access to user 'public' is denied for user 'testuser'
+- error: Drop access to role 'public' is denied for user 'testuser'
 ...
 box.space._user:select(1)
 ---
diff --git a/test/box/role.result b/test/box/role.result
index 806cea90b..243c7bc6c 100644
--- a/test/box/role.result
+++ b/test/box/role.result
@@ -214,7 +214,22 @@ box.schema.role.drop('test')
 box.schema.user.grant('grantee', 'liaison')
 ---
 ...
-box.schema.user.grant('test', 'read,write,create', 'universe')
+box.schema.user.grant('test', 'read,write', 'space', '_priv')
+---
+...
+box.schema.user.grant('test', 'write', 'space', '_schema')
+---
+...
+box.schema.user.grant('test', 'create', 'space')
+---
+...
+box.schema.user.grant('test', 'read,write', 'space', '_space')
+---
+...
+box.schema.user.grant('test', 'write', 'space', '_index')
+---
+...
+box.schema.user.grant('test', 'read', 'space', '_user')
 ---
 ...
 box.session.su('test')
@@ -635,7 +650,13 @@ box.schema.user.create('user')
 box.schema.user.create('grantee')
 ---
 ...
-box.schema.user.grant('user', 'read,write,execute,create', 'universe')
+box.schema.user.grant('user', 'read,write', 'space', '_user')
+---
+...
+box.schema.user.grant('user', 'read,write', 'space', '_priv')
+---
+...
+box.schema.user.grant('user', 'create', 'role')
 ---
 ...
 box.session.su('user')
diff --git a/test/box/role.test.lua b/test/box/role.test.lua
index e97339f49..9845f4c4c 100644
--- a/test/box/role.test.lua
+++ b/test/box/role.test.lua
@@ -69,7 +69,13 @@ box.schema.role.revoke('test', 'liaison')
 box.schema.role.drop('test')
 
 box.schema.user.grant('grantee', 'liaison')
-box.schema.user.grant('test', 'read,write,create', 'universe')
+
+box.schema.user.grant('test', 'read,write', 'space', '_priv')
+box.schema.user.grant('test', 'write', 'space', '_schema')
+box.schema.user.grant('test', 'create', 'space')
+box.schema.user.grant('test', 'read,write', 'space', '_space')
+box.schema.user.grant('test', 'write', 'space', '_index')
+box.schema.user.grant('test', 'read', 'space', '_user')
 box.session.su('test')
 s = box.schema.space.create('test')
 _ = s:create_index('i1')
@@ -248,7 +254,9 @@ box.schema.role.drop("role10")
 box.schema.user.create('user')
 box.schema.user.create('grantee')
 
-box.schema.user.grant('user', 'read,write,execute,create', 'universe')
+box.schema.user.grant('user', 'read,write', 'space', '_user')
+box.schema.user.grant('user', 'read,write', 'space', '_priv')
+box.schema.user.grant('user', 'create', 'role')
 box.session.su('user')
 box.schema.role.create('role')
 box.session.su('admin')
-- 
2.15.2 (Apple Git-101.1)

^ permalink raw reply	[flat|nested] 3+ messages in thread

* [tarantool-patches] Re: [PATCH] Add entities user, role to access control.
  2018-07-12 15:29 [tarantool-patches] [PATCH] Add entities user, role to access control Serge Petrenko
@ 2018-07-26 20:15 ` Konstantin Osipov
  2018-07-30  7:52   ` Sergey Petrenko
  0 siblings, 1 reply; 3+ messages in thread
From: Konstantin Osipov @ 2018-07-26 20:15 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Serge Petrenko

* 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* [tarantool-patches] Re: [PATCH] Add entities user, role to access control.
  2018-07-26 20:15 ` [tarantool-patches] " Konstantin Osipov
@ 2018-07-30  7:52   ` Sergey Petrenko
  0 siblings, 0 replies; 3+ messages in thread
From: Sergey Petrenko @ 2018-07-30  7:52 UTC (permalink / raw)
  To: Konstantin Osipov; +Cc: tarantool-patches

Hi! I made a separate patch introducing new schema object types to be used
under the hood when granting privileges on an entire entity. I also rebased this
patch on top of the latter one and fixed your comments. Please see the new diff below.

> 26 июля 2018 г., в 23:15, 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.
Removed this check.
> 
>> 	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.

Revorked this part, it is not so bulky anymore, see the new diff below.

>> +	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).

Revorked this part as you requested.

 src/box/alter.cc            | 43 ++++++++++++++++++++++++++++++++++++++++---
 src/box/lua/schema.lua      | 35 +++++++++++++++++++++++++----------
 src/box/schema.h            |  8 ++++++++
 src/box/user.cc             | 22 +++++++++++++++++++++-
 test/box/access.result      | 20 ++++++++++----------
 test/box/access.test.lua    | 15 +++++----------
 test/box/access_misc.result |  2 +-
 test/box/role.result        | 25 +++++++++++++++++++++++--
 test/box/role.test.lua      | 12 ++++++++++--
 9 files changed, 143 insertions(+), 39 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 3007a131d..d4545f31c 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,
@@ -2645,6 +2645,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);
+		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);
+		}
+		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);
+		}
 	}
 	default:
 		break;
@@ -2665,7 +2697,12 @@ grant_or_revoke(struct priv_def *priv)
 	struct user *grantee = user_by_id(priv->grantee_id);
 	if (grantee == NULL)
 		return;
-	if (priv->object_type == SC_ROLE) {
+	/*
+	 * Grant a role to a user only when privilege type is 'execute'
+	 * and the role is specified.
+	 */
+	if (priv->object_type == SC_ROLE && !(priv->access & ~PRIV_X) &&
+	    priv->object_id != 0) {
 		struct user *role = user_by_id(priv->object_id);
 		if (role == NULL || role->def->type != SC_ROLE)
 			return;
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index eb984b1de..28f68cbda 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1746,6 +1746,9 @@ local priv_object_combo = {
     ["role"]         = bit.bor(box.priv.X, box.priv.U,
                                box.priv.D),
     ["all roles"]     = bit.bor(box.priv.C, box.priv.D),
+    ["user"]          = bit.bor(box.priv.A, box.priv.D),
+    ["all users"]     = bit.bor(box.priv.C, box.priv.A,
+                                box.priv.D),
 }
 
 --
@@ -1839,6 +1842,12 @@ local function object_resolve(object_type, object_name)
         end
         return 0
     end
+    if object_type == 'all users' then
+        if object_name ~= nil and object_name ~= 0 then
+            box.error(box.error.ILLEGAL_PARAMS, "no object name allowed")
+        end
+        return 0
+    end
     if object_type == 'space' then
 	if object_name == nil then
 	    return nil
@@ -1876,22 +1885,27 @@ local function object_resolve(object_type, object_name)
         end
         return seq
     end
-    if object_type == 'role' then
-	if object_name == nil then
+    if object_type == 'role' or object_type == 'user' then
+        if object_name == nil then
 	    return nil
 	end
         local _vuser = box.space[box.schema.VUSER_ID]
-        local role
+        local role_or_user
+        if object_name == nil or object_name == 0 then
+            return 0
+        end
         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
     end
 
     box.error(box.error.UNKNOWN_SCHEMA_OBJECT, object_type)
@@ -2146,7 +2160,8 @@ local function grant(uid, name, privilege, object_type,
     if privilege_hex ~= old_privilege then
         _priv:replace{options.grantor, uid, object_type, oid, privilege_hex}
     elseif not options.if_not_exists then
-            if object_type == 'role' then
+            if object_type == 'role' and object_name ~= nil and
+	       object_name ~= 0 and privilege == 'execute' then
                 box.error(box.error.ROLE_GRANTED, name, object_name)
             else
                 box.error(box.error.PRIV_GRANTED, name, privilege,
diff --git a/src/box/schema.h b/src/box/schema.h
index f1735ff34..c343650de 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -240,6 +240,8 @@ struct on_access_denied_ctx {
 struct entity_access {
        struct access space[BOX_USER_MAX];
        struct access function[BOX_USER_MAX];
+       struct access user[BOX_USER_MAX];
+       struct access role[BOX_USER_MAX];
        struct access sequence[BOX_USER_MAX];
 };
 
@@ -257,6 +259,12 @@ entity_access_get(enum schema_object_type type)
 	case SC_FUNCTION:
 	case SC_ENTITY_FUNCTION:
 		return entity_access.function;
+	case SC_USER:
+	case SC_ENTITY_USER:
+		return entity_access.user;
+	case SC_ROLE:
+	case SC_ENTITY_ROLE:
+		return entity_access.role;
 	case SC_SEQUENCE:
 	case SC_ENTITY_SEQUENCE:
 		return entity_access.sequence;
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
@@ -217,6 +217,16 @@ access_find(struct priv_def *priv)
 		access = entity_access.function;
 		break;
 	}
+	case SC_ENTITY_USER:
+	{
+		access = entity_access.user;
+		break;
+	}
+	case SC_ENTITY_ROLE:
+	{
+		access = entity_access.role;
+		break;
+	}
 	case SC_ENTITY_SEQUENCE:
 	{
 		access = entity_access.sequence;
@@ -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;
+	}
 	case SC_SEQUENCE:
 	{
 		struct sequence *seq = sequence_by_id(priv->object_id);
@@ -318,7 +338,7 @@ user_reload_privs(struct user *user)
 			 * Skip role grants, we're only
 			 * interested in real objects.
 			 */
-			if (priv.object_type != SC_ROLE)
+			if (priv.object_type != SC_ROLE || !(priv.access & PRIV_X))
 				user_grant_priv(user, &priv);
 		}
 	}
diff --git a/test/box/access.result b/test/box/access.result
index 30fcb6455..7acd6fa43 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -878,8 +878,6 @@ box.schema.user.create('test')
 box.schema.user.grant('test', 'read', 'space', '_collation')
 ---
 ...
---box.schema.user.grant('test', 'write', 'space', '_collation')
--- FIXME: granting create on 'collation' only doesn't work
 box.schema.user.grant('test', 'create', 'universe')
 ---
 ...
@@ -1427,16 +1425,18 @@ box.session.su("admin")
 box.schema.user.grant('tester', 'write', 'universe')
 ---
 ...
--- no entity user currently, so have to grant create
--- on universe in order to create a user.
-box.schema.user.grant('tester', 'create', 'universe')
+box.schema.user.grant('tester', 'create', 'user')
+---
+...
+box.schema.user.grant('tester', 'create', 'space')
+---
+...
+box.schema.user.grant('tester', 'create', 'function')
+---
+...
+box.schema.user.grant('tester', 'create' , 'sequence')
 ---
 ...
--- this should work instead:
---box.schema.user.grant('tester', 'create', 'user')
---box.schema.user.grant('tester', 'create', 'space')
---box.schema.user.grant('tester', 'create', 'function')
---box.schema.user.grant('tester', 'create' , 'sequence')
 box.schema.user.grant('tester', 'read', 'space', '_sequence')
 ---
 ...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 9ae0e1114..9b7510e64 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -341,8 +341,7 @@ c:close()
 session = box.session
 box.schema.user.create('test')
 box.schema.user.grant('test', 'read', 'space', '_collation')
---box.schema.user.grant('test', 'write', 'space', '_collation')
--- FIXME: granting create on 'collation' only doesn't work
+
 box.schema.user.grant('test', 'create', 'universe')
 session.su('test')
 box.internal.collation.create('test', 'ICU', 'ru_RU')
@@ -538,14 +537,10 @@ box.session.su("admin")
 -- tables from ddl
 --
 box.schema.user.grant('tester', 'write', 'universe')
--- no entity user currently, so have to grant create
--- on universe in order to create a user.
-box.schema.user.grant('tester', 'create', 'universe')
--- this should work instead:
---box.schema.user.grant('tester', 'create', 'user')
---box.schema.user.grant('tester', 'create', 'space')
---box.schema.user.grant('tester', 'create', 'function')
---box.schema.user.grant('tester', 'create' , 'sequence')
+box.schema.user.grant('tester', 'create', 'user')
+box.schema.user.grant('tester', 'create', 'space')
+box.schema.user.grant('tester', 'create', 'function')
+box.schema.user.grant('tester', 'create' , 'sequence')
 box.schema.user.grant('tester', 'read', 'space', '_sequence')
 box.session.su("tester")
 -- successful create
diff --git a/test/box/access_misc.result b/test/box/access_misc.result
index 2d87fa2d5..b7f0b7031 100644
--- a/test/box/access_misc.result
+++ b/test/box/access_misc.result
@@ -361,7 +361,7 @@ testuser_uid = session.uid()
 ...
 _ = box.space._user:delete(2)
 ---
-- error: Drop access to user 'public' is denied for user 'testuser'
+- error: Drop access to role 'public' is denied for user 'testuser'
 ...
 box.space._user:select(1)
 ---
diff --git a/test/box/role.result b/test/box/role.result
index 806cea90b..243c7bc6c 100644
--- a/test/box/role.result
+++ b/test/box/role.result
@@ -214,7 +214,22 @@ box.schema.role.drop('test')
 box.schema.user.grant('grantee', 'liaison')
 ---
 ...
-box.schema.user.grant('test', 'read,write,create', 'universe')
+box.schema.user.grant('test', 'read,write', 'space', '_priv')
+---
+...
+box.schema.user.grant('test', 'write', 'space', '_schema')
+---
+...
+box.schema.user.grant('test', 'create', 'space')
+---
+...
+box.schema.user.grant('test', 'read,write', 'space', '_space')
+---
+...
+box.schema.user.grant('test', 'write', 'space', '_index')
+---
+...
+box.schema.user.grant('test', 'read', 'space', '_user')
 ---
 ...
 box.session.su('test')
@@ -635,7 +650,13 @@ box.schema.user.create('user')
 box.schema.user.create('grantee')
 ---
 ...
-box.schema.user.grant('user', 'read,write,execute,create', 'universe')
+box.schema.user.grant('user', 'read,write', 'space', '_user')
+---
+...
+box.schema.user.grant('user', 'read,write', 'space', '_priv')
+---
+...
+box.schema.user.grant('user', 'create', 'role')
 ---
 ...
 box.session.su('user')
diff --git a/test/box/role.test.lua b/test/box/role.test.lua
index e97339f49..9845f4c4c 100644
--- a/test/box/role.test.lua
+++ b/test/box/role.test.lua
@@ -69,7 +69,13 @@ box.schema.role.revoke('test', 'liaison')
 box.schema.role.drop('test')
 
 box.schema.user.grant('grantee', 'liaison')
-box.schema.user.grant('test', 'read,write,create', 'universe')
+
+box.schema.user.grant('test', 'read,write', 'space', '_priv')
+box.schema.user.grant('test', 'write', 'space', '_schema')
+box.schema.user.grant('test', 'create', 'space')
+box.schema.user.grant('test', 'read,write', 'space', '_space')
+box.schema.user.grant('test', 'write', 'space', '_index')
+box.schema.user.grant('test', 'read', 'space', '_user')
 box.session.su('test')
 s = box.schema.space.create('test')
 _ = s:create_index('i1')
@@ -248,7 +254,9 @@ box.schema.role.drop("role10")
 box.schema.user.create('user')
 box.schema.user.create('grantee')
 
-box.schema.user.grant('user', 'read,write,execute,create', 'universe')
+box.schema.user.grant('user', 'read,write', 'space', '_user')
+box.schema.user.grant('user', 'read,write', 'space', '_priv')
+box.schema.user.grant('user', 'create', 'role')
 box.session.su('user')
 box.schema.role.create('role')
 box.session.su('admin')
-- 
2.15.2 (Apple Git-101.1)

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-07-30  7:52 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-12 15:29 [tarantool-patches] [PATCH] Add entities user, role to access control Serge Petrenko
2018-07-26 20:15 ` [tarantool-patches] " Konstantin Osipov
2018-07-30  7:52   ` Sergey Petrenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox