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

[Sergey Petrenko <sergepetrenko@tarantool.org>]

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)