Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Petrenko <sergepetrenko@tarantool.org>
To: Konstantin Osipov <kostja@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH v2 3/4] Add single object privilege checks to access_check_ddl.
Date: Mon, 30 Jul 2018 11:37:01 +0300	[thread overview]
Message-ID: <D79F007B-84B4-4C04-A72E-440B041A2FFB@tarantool.org> (raw)
In-Reply-To: <20180726203731.GA32000@chai>

Hi, I rebased this patch on top of the new one regarding #3524, and fixed
your comments. Please see 2 my comments and the new diff below.

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

 Here’s the hack I was talking about: in function on_replace_dd_user in alter.cc
-		access_check_ddl(user->name, user->uid, SC_USER, PRIV_A,
-				 true);
We pass user as its own owner, which enables it to change its own password/username.

I leave the hack in place for now, and also add grant of alter on itself upon user creation:

+		access_check_ddl(user->name, user->uid, user->uid, SC_USER,
+				 PRIV_A, true);

schema.lua: function box.schema.user.create():
+    -- grant user 'alter' on itself, so it can
+    -- change its password or username.
+    box.schema.user.grant(uid, 'alter', 'user', uid)

> 
>> 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()?
Fixed. Added a jump to error case.

> 
>> +		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
 
src/box/alter.cc         | 121 +++++++++++++++++++++---------
 src/box/lua/schema.lua   |   3 +
 src/box/user.cc          |   8 +-
 src/box/user.h           |   2 +
 test/box/access.result   | 186 +++++++++++++++++++++++++++++++++++++++++++++++
 test/box/access.test.lua |  57 +++++++++++++++
 test/box/role.result     |   9 +++
 test/box/sequence.result |   3 +
 8 files changed, 351 insertions(+), 38 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index d4545f31c..35e4d0999 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,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;
+	/* 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;
+	}
+	if (access == 0)
+	    return; /* Access granted. */
 
 	/* 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) {
@@ -1590,7 +1633,8 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 		struct space_def *def =
 			space_def_new_from_tuple(new_tuple, ER_CREATE_SPACE,
 						 region);
-		access_check_ddl(def->name, def->uid, SC_SPACE, PRIV_C, true);
+		access_check_ddl(def->name, def->id, def->uid, SC_SPACE,
+				 PRIV_C, true);
 		auto def_guard =
 			make_scoped_guard([=] { space_def_delete(def); });
 		RLIST_HEAD(empty_list);
@@ -1623,8 +1667,8 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 			txn_alter_trigger_new(on_create_space_rollback, space);
 		txn_on_rollback(txn, on_rollback);
 	} else if (new_tuple == NULL) { /* DELETE */
-		access_check_ddl(old_space->def->name, old_space->def->uid,
-				 SC_SPACE, PRIV_D, true);
+		access_check_ddl(old_space->def->name, old_space->def->id,
+				 old_space->def->uid, SC_SPACE, PRIV_D, true);
 		/* Verify that the space is empty (has no indexes) */
 		if (old_space->index_count) {
 			tnt_raise(ClientError, ER_DROP_SPACE,
@@ -1669,7 +1713,8 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 		struct space_def *def =
 			space_def_new_from_tuple(new_tuple, ER_ALTER_SPACE,
 						 region);
-		access_check_ddl(def->name, def->uid, SC_SPACE, PRIV_A, true);
+		access_check_ddl(def->name, def->id, def->uid, SC_SPACE,
+				 PRIV_A, true);
 		auto def_guard =
 			make_scoped_guard([=] { space_def_delete(def); });
 		if (def->id != space_id(old_space))
@@ -1774,8 +1819,8 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 	enum priv_type priv_type = new_tuple ? PRIV_C : PRIV_D;
 	if (old_tuple && new_tuple)
 		priv_type = PRIV_A;
-	access_check_ddl(old_space->def->name, old_space->def->uid, SC_SPACE,
-			 priv_type, true);
+	access_check_ddl(old_space->def->name, old_space->def->id,
+			 old_space->def->uid, SC_SPACE, priv_type, true);
 	struct index *old_index = space_index(old_space, iid);
 
 	/*
@@ -2170,7 +2215,8 @@ 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, user->type, PRIV_C, true);
+		access_check_ddl(user->name, user->uid, 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;
@@ -2178,7 +2224,8 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
 			txn_alter_trigger_new(user_cache_remove_user, NULL);
 		txn_on_rollback(txn, on_rollback);
 	} else if (new_tuple == NULL) { /* DELETE */
-		access_check_ddl(old_user->def->name, old_user->def->owner,
+		access_check_ddl(old_user->def->name, old_user->def->uid,
+				 old_user->def->owner,
 				 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) {
@@ -2205,8 +2252,8 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
 		 * correct.
 		 */
 		struct user_def *user = user_def_new_from_tuple(new_tuple);
-		access_check_ddl(user->name, user->uid, SC_USER, PRIV_A,
-				 true);
+		access_check_ddl(user->name, user->uid, user->uid, SC_USER,
+				 PRIV_A, true);
 		auto def_guard = make_scoped_guard([=] { free(user); });
 		struct trigger *on_commit =
 			txn_alter_trigger_new(user_cache_alter_user, NULL);
@@ -2308,7 +2355,8 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
 	struct func *old_func = func_by_id(fid);
 	if (new_tuple != NULL && old_func == NULL) { /* INSERT */
 		struct func_def *def = func_def_new_from_tuple(new_tuple);
-		access_check_ddl(def->name, def->uid, SC_FUNCTION, PRIV_C, true);
+		access_check_ddl(def->name, def->fid, def->uid, SC_FUNCTION,
+				 PRIV_C, true);
 		auto def_guard = make_scoped_guard([=] { free(def); });
 		func_cache_replace(def);
 		def_guard.is_active = false;
@@ -2322,7 +2370,7 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
 		 * Can only delete func if you're the one
 		 * who created it or a superuser.
 		 */
-		access_check_ddl(old_func->def->name, uid, SC_FUNCTION,
+		access_check_ddl(old_func->def->name, fid, uid, SC_FUNCTION,
 				 PRIV_D, true);
 		/* Can only delete func if it has no grants. */
 		if (schema_find_grants("function", old_func->def->fid)) {
@@ -2336,8 +2384,8 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event)
 	} else {                                /* UPDATE, REPLACE */
 		struct func_def *def = func_def_new_from_tuple(new_tuple);
 		auto def_guard = make_scoped_guard([=] { free(def); });
-		access_check_ddl(def->name, def->uid, SC_FUNCTION, PRIV_A,
-				 true);
+		access_check_ddl(def->name, def->fid, def->uid,
+				 SC_FUNCTION, PRIV_A, true);
 		struct trigger *on_commit =
 			txn_alter_trigger_new(func_cache_replace_func, NULL);
 		txn_on_commit(txn, on_commit);
@@ -2493,8 +2541,9 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
 						    BOX_COLLATION_FIELD_ID);
 		struct coll_id *old_coll_id = coll_by_id(old_id);
 		assert(old_coll_id != NULL);
-		access_check_ddl(old_coll_id->name, old_coll_id->owner_id,
-				 SC_COLLATION, PRIV_D, false);
+		access_check_ddl(old_coll_id->name, old_id,
+				 old_coll_id->owner_id, SC_COLLATION, PRIV_D,
+				 false);
 		/*
 		 * Set on_commit/on_rollback triggers after
 		 * deletion from the cache to make trigger logic
@@ -2509,8 +2558,8 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
 		/* INSERT */
 		struct coll_id_def new_def;
 		coll_id_def_new_from_tuple(new_tuple, &new_def);
-		access_check_ddl(new_def.name, new_def.owner_id, SC_COLLATION,
-				 PRIV_C, false);
+		access_check_ddl(new_def.name, new_def.id, new_def.owner_id,
+				 SC_COLLATION, PRIV_C, false);
 		struct coll_id *new_coll_id = coll_id_new(&new_def);
 		if (new_coll_id == NULL)
 			diag_raise();
@@ -2569,8 +2618,8 @@ priv_def_check(struct priv_def *priv, enum priv_type priv_type)
 			  int2str(priv->grantee_id));
 	}
 	const char *name = schema_find_name(priv->object_type, priv->object_id);
-	access_check_ddl(name, grantor->def->uid, priv->object_type, priv_type,
-			 false);
+	access_check_ddl(name, priv->object_id, grantor->def->uid,
+			 priv->object_type, priv_type, false);
 	switch (priv->object_type) {
 	case SC_UNIVERSE:
 		if (grantor->def->uid != ADMIN) {
@@ -3075,8 +3124,8 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event)
 		new_def = sequence_def_new_from_tuple(new_tuple,
 						      ER_CREATE_SEQUENCE);
 		assert(sequence_by_id(new_def->id) == NULL);
-		access_check_ddl(new_def->name, new_def->uid, SC_SEQUENCE,
-				 PRIV_C, false);
+		access_check_ddl(new_def->name, new_def->id, new_def->uid,
+				 SC_SEQUENCE, PRIV_C, false);
 		sequence_cache_replace(new_def);
 		alter->new_def = new_def;
 	} else if (old_tuple != NULL && new_tuple == NULL) {	/* DELETE */
@@ -3084,8 +3133,8 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event)
 						 BOX_SEQUENCE_DATA_FIELD_ID);
 		struct sequence *seq = sequence_by_id(id);
 		assert(seq != NULL);
-		access_check_ddl(seq->def->name, seq->def->uid, SC_SEQUENCE,
-				 PRIV_D, false);
+		access_check_ddl(seq->def->name, seq->def->id, seq->def->uid,
+				 SC_SEQUENCE, PRIV_D, false);
 		if (space_has_data(BOX_SEQUENCE_DATA_ID, 0, id))
 			tnt_raise(ClientError, ER_DROP_SEQUENCE,
 				  seq->def->name, "the sequence has data");
@@ -3101,8 +3150,8 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event)
 						      ER_ALTER_SEQUENCE);
 		struct sequence *seq = sequence_by_id(new_def->id);
 		assert(seq != NULL);
-		access_check_ddl(seq->def->name, seq->def->uid, SC_SEQUENCE,
-				 PRIV_A, false);
+		access_check_ddl(seq->def->name, seq->def->id, seq->def->uid,
+				 SC_SEQUENCE, PRIV_A, false);
 		alter->old_def = seq->def;
 		alter->new_def = new_def;
 	}
@@ -3182,21 +3231,21 @@ on_replace_dd_space_sequence(struct trigger * /* trigger */, void *event)
 
 	/* Check we have the correct access type on the sequence.  * */
 	if (is_generated || !stmt->new_tuple) {
-		access_check_ddl(seq->def->name, seq->def->uid, SC_SEQUENCE,
-				 priv_type, false);
+		access_check_ddl(seq->def->name, seq->def->id, seq->def->uid,
+				 SC_SEQUENCE, priv_type, false);
 	} else {
 		/*
 		 * In case user wants to attach an existing sequence,
 		 * 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 28f68cbda..350da210a 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -2103,6 +2103,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
+    -- 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
@@ -248,12 +248,16 @@ access_find(struct priv_def *priv)
 	}
 	case SC_USER:
 	{
-		/* No grants on a single object user yet. */
+		struct user *user = user_by_id(priv->object_id);
+		if (user)
+			access = user->access;
 		break;
 	}
 	case SC_ROLE:
 	{
-		/* No grants on a single object role yet. */
+		struct user *role = user_by_id(priv->object_id);
+		if (role)
+			access = role->access;
 		break;
 	}
 	case SC_SEQUENCE:
diff --git a/src/box/user.h b/src/box/user.h
index 07c4dc504..069d9b77e 100644
--- a/src/box/user.h
+++ b/src/box/user.h
@@ -88,6 +88,8 @@ struct user
 	bool is_dirty;
 	/** Memory pool for privs */
 	struct region pool;
+	/** Cached runtime access imformation. */
+	struct access access[BOX_USER_MAX];
 };
 
 /** Find user by id. */
diff --git a/test/box/access.result b/test/box/access.result
index 7acd6fa43..819d8c471 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -136,6 +136,9 @@ box.schema.user.revoke('rich', 'read,write', 'universe')
 box.schema.user.revoke('rich', 'public')
 ---
 ...
+box.schema.user.revoke('rich', 'alter', 'user', 'rich')
+---
+...
 box.schema.user.disable("rich")
 ---
 ...
@@ -501,6 +504,7 @@ box.space._priv:select{id}
 ---
 - - [1, 32, 'role', 2, 4]
   - [1, 32, 'universe', 0, 27]
+  - [1, 32, 'user', 32, 128]
 ...
 box.schema.user.grant('user', 'read', 'universe')
 ---
@@ -510,6 +514,7 @@ box.space._priv:select{id}
 ---
 - - [1, 32, 'role', 2, 4]
   - [1, 32, 'universe', 0, 27]
+  - [1, 32, 'user', 32, 128]
 ...
 box.schema.user.revoke('user', 'write', 'universe')
 ---
@@ -518,6 +523,7 @@ box.space._priv:select{id}
 ---
 - - [1, 32, 'role', 2, 4]
   - [1, 32, 'universe', 0, 25]
+  - [1, 32, 'user', 32, 128]
 ...
 box.schema.user.revoke('user', 'read', 'universe')
 ---
@@ -526,6 +532,7 @@ box.space._priv:select{id}
 ---
 - - [1, 32, 'role', 2, 4]
   - [1, 32, 'universe', 0, 24]
+  - [1, 32, 'user', 32, 128]
 ...
 box.schema.user.grant('user', 'write', 'universe')
 ---
@@ -534,6 +541,7 @@ box.space._priv:select{id}
 ---
 - - [1, 32, 'role', 2, 4]
   - [1, 32, 'universe', 0, 26]
+  - [1, 32, 'user', 32, 128]
 ...
 box.schema.user.grant('user', 'read', 'universe')
 ---
@@ -542,6 +550,7 @@ box.space._priv:select{id}
 ---
 - - [1, 32, 'role', 2, 4]
   - [1, 32, 'universe', 0, 27]
+  - [1, 32, 'user', 32, 128]
 ...
 box.schema.user.drop('user')
 ---
@@ -965,6 +974,9 @@ box.schema.user.info('test_user')
   - - session,usage
     - universe
     - 
+  - - alter
+    - user
+    - test_user
 ...
 box.schema.role.info('test_role')
 ---
@@ -995,6 +1007,9 @@ box.schema.user.info('test_user')
   - - session,usage
     - universe
     - 
+  - - alter
+    - user
+    - test_user
 ...
 box.schema.role.info('test_role')
 ---
@@ -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.
+--
+box.schema.user.create("test")
+---
+...
+_ = box.schema.space.create("space1")
+---
+...
+box.schema.user.grant("test", "read", "space", "space1")
+---
+...
+box.schema.user.grant("test", "write", "space", "_index")
+---
+...
+box.session.su("test")
+---
+...
+box.space.space1:create_index("pk")
+---
+- error: Create access to space 'space1' is denied for user 'test'
+...
+box.session.su("admin")
+---
+...
+box.space.space1.index[0] == nil
+---
+- true
+...
+-- fixme: cannot grant create on a single space
+-- this is because when checking for create
+-- access_check_ddl ignores space privileges,
+-- assuming that there is no space yet.
+box.schema.user.grant("test", "create", "space")
+---
+...
+box.session.su("test")
+---
+...
+_ = box.space.space1:create_index("pk")
+---
+...
+box.space.space1:insert{5}
+---
+- error: Write access to space 'space1' is denied for user 'test'
+...
+box.session.su("admin")
+---
+...
+box.space.space1.index[0] ~= nil
+---
+- true
+...
+box.space.space1:select{}
+---
+- []
+...
+box.schema.user.grant("test", "write", "space", "space1")
+---
+...
+box.session.su("test")
+---
+...
+box.space.space1:insert{5}
+---
+- [5]
+...
+box.session.su("admin")
+---
+...
+box.space.space1:select{}
+---
+- - [5]
+...
+box.schema.user.drop("test")
+---
+...
+box.space.space1:drop()
+---
+...
+--
+-- test that it is possible to grant privileges on a single user.
+box.schema.user.create("user1")
+---
+...
+box.schema.user.create("user2")
+---
+...
+box.schema.user.create("user3")
+---
+...
+box.schema.user.grant("user1", "write", "space", "_user")
+---
+...
+box.schema.user.grant("user1", "read", "space", "_user")
+---
+...
+box.space._user:select{}
+---
+- - [0, 1, 'guest', 'user', {'chap-sha1': 'vhvewKp0tNyweZQ+cFKAlsyphfg='}]
+  - [1, 1, 'admin', 'user', {}]
+  - [2, 1, 'public', 'role', {}]
+  - [3, 1, 'replication', 'role', {}]
+  - [31, 1, 'super', 'role', {}]
+  - [32, 1, 'user1', 'user', {}]
+  - [33, 1, 'user2', 'user', {}]
+  - [34, 1, 'user3', 'user', {}]
+...
+box.session.su("user1")
+---
+...
+-- can alter itself, but can't alter others without privileges.
+box.schema.user.passwd("user1", "abcd")
+---
+...
+box.schema.user.passwd("user2", "abcd")
+---
+- error: Alter access to user 'user2' is denied for user 'user1'
+...
+box.session.su("admin")
+---
+...
+box.space._user:select{}
+---
+- - [0, 1, 'guest', 'user', {'chap-sha1': 'vhvewKp0tNyweZQ+cFKAlsyphfg='}]
+  - [1, 1, 'admin', 'user', {}]
+  - [2, 1, 'public', 'role', {}]
+  - [3, 1, 'replication', 'role', {}]
+  - [31, 1, 'super', 'role', {}]
+  - [32, 1, 'user1', 'user', {'chap-sha1': 'oVTFJWXp5/lL/Aih/nAmJO2O/9o='}]
+  - [33, 1, 'user2', 'user', {}]
+  - [34, 1, 'user3', 'user', {}]
+...
+box.schema.user.grant("user1", "alter", "user", "user2")
+---
+...
+box.session.su("user1")
+---
+...
+box.schema.user.passwd("user2", "abcd")
+---
+...
+-- still fails
+box.schema.user.passwd("user3", "qewr")
+---
+- error: Alter access to user 'user3' is denied for user 'user1'
+...
+box.session.su("admin")
+---
+...
+box.space._user:select{}
+---
+- - [0, 1, 'guest', 'user', {'chap-sha1': 'vhvewKp0tNyweZQ+cFKAlsyphfg='}]
+  - [1, 1, 'admin', 'user', {}]
+  - [2, 1, 'public', 'role', {}]
+  - [3, 1, 'replication', 'role', {}]
+  - [31, 1, 'super', 'role', {}]
+  - [32, 1, 'user1', 'user', {'chap-sha1': 'oVTFJWXp5/lL/Aih/nAmJO2O/9o='}]
+  - [33, 1, 'user2', 'user', {'chap-sha1': 'oVTFJWXp5/lL/Aih/nAmJO2O/9o='}]
+  - [34, 1, 'user3', 'user', {}]
+...
+box.schema.user.drop("user1")
+---
+...
+box.schema.user.drop("user2")
+---
+...
+box.schema.user.drop("user3")
+---
+...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 9b7510e64..991ddf6ba 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -60,6 +60,7 @@ box.schema.func.drop('dummy')
 box.space['_user']:delete{uid}
 box.schema.user.revoke('rich', 'read,write', 'universe')
 box.schema.user.revoke('rich', 'public')
+box.schema.user.revoke('rich', 'alter', 'user', 'rich')
 box.schema.user.disable("rich")
 -- test double disable is a no op
 box.schema.user.disable("rich")
@@ -726,3 +727,59 @@ _ = box.schema.sequence.create('test_sequence')
 box.session.su('admin')
 box.schema.user.drop('tester')
 
+
+--
+-- test case for 3530: do not ignore single object privileges in
+-- access_check_ddl.
+--
+box.schema.user.create("test")
+_ = box.schema.space.create("space1")
+box.schema.user.grant("test", "read", "space", "space1")
+box.schema.user.grant("test", "write", "space", "_index")
+box.session.su("test")
+box.space.space1:create_index("pk")
+box.session.su("admin")
+box.space.space1.index[0] == nil
+-- fixme: cannot grant create on a single space
+-- this is because when checking for create
+-- access_check_ddl ignores space privileges,
+-- assuming that there is no space yet.
+box.schema.user.grant("test", "create", "space")
+box.session.su("test")
+_ = box.space.space1:create_index("pk")
+box.space.space1:insert{5}
+box.session.su("admin")
+box.space.space1.index[0] ~= nil
+box.space.space1:select{}
+box.schema.user.grant("test", "write", "space", "space1")
+box.session.su("test")
+box.space.space1:insert{5}
+box.session.su("admin")
+box.space.space1:select{}
+box.schema.user.drop("test")
+box.space.space1:drop()
+
+--
+-- test that it is possible to grant privileges on a single user.
+box.schema.user.create("user1")
+box.schema.user.create("user2")
+box.schema.user.create("user3")
+box.schema.user.grant("user1", "write", "space", "_user")
+box.schema.user.grant("user1", "read", "space", "_user")
+box.space._user:select{}
+box.session.su("user1")
+-- can alter itself, but can't alter others without privileges.
+box.schema.user.passwd("user1", "abcd")
+box.schema.user.passwd("user2", "abcd")
+box.session.su("admin")
+box.space._user:select{}
+box.schema.user.grant("user1", "alter", "user", "user2")
+box.session.su("user1")
+box.schema.user.passwd("user2", "abcd")
+-- still fails
+box.schema.user.passwd("user3", "qewr")
+box.session.su("admin")
+box.space._user:select{}
+box.schema.user.drop("user1")
+box.schema.user.drop("user2")
+box.schema.user.drop("user3")
diff --git a/test/box/role.result b/test/box/role.result
index 243c7bc6c..5666f7ef7 100644
--- a/test/box/role.result
+++ b/test/box/role.result
@@ -49,6 +49,9 @@ box.schema.user.info('tester')
   - - session,usage
     - universe
     - 
+  - - alter
+    - user
+    - tester
 ...
 box.schema.user.grant('tester', 'execute', 'role', 'iddqd')
 ---
@@ -64,6 +67,9 @@ box.schema.user.info('tester')
   - - session,usage
     - universe
     - 
+  - - alter
+    - user
+    - tester
 ...
 -- test granting user to a user
 box.schema.user.grant('tester', 'execute', 'role', 'tester')
@@ -956,6 +962,9 @@ box.schema.user.info('test_user')
   - - session,usage
     - universe
     - 
+  - - alter
+    - user
+    - test_user
 ...
 box.schema.role.info('test_user')
 ---
diff --git a/test/box/sequence.result b/test/box/sequence.result
index a2a1a60ea..b3907659f 100644
--- a/test/box/sequence.result
+++ b/test/box/sequence.result
@@ -1362,6 +1362,9 @@ box.schema.user.info()
   - - session,usage
     - universe
     - 
+  - - alter
+    - user
+    - user
 ...
 sq:set(100) -- ok
 ---
-- 
2.15.2 (Apple Git-101.1)

  reply	other threads:[~2018-07-30  8:37 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-17 15:47 [tarantool-patches] [PATCH 0/4] Fixes in access control and privileges Serge Petrenko
2018-07-17 15:47 ` [tarantool-patches] [PATCH 1/4] Make access_check_ddl check for entity privileges Serge Petrenko
2018-07-17 15:47 ` [tarantool-patches] [PATCH 2/4] Add entities user, role to access control Serge Petrenko
2018-07-17 15:47 ` [tarantool-patches] [PATCH 3/4] Add single object privilege checks to access_check_ddl Serge Petrenko
2018-07-17 15:47 ` [tarantool-patches] [PATCH 4/4] Add a privilege upgrade script and update tests Serge Petrenko
     [not found] ` <cover.1531843622.git.sergepetrenko@tarantool.org>
2018-07-17 16:08   ` [tarantool-patches] [PATCH v2 1/4] Make access_check_ddl check for entity privileges Serge Petrenko
2018-07-17 16:08   ` [tarantool-patches] [PATCH v2 2/4] Add entities user, role to access control Serge Petrenko
2018-07-17 16:08   ` [tarantool-patches] [PATCH v2 3/4] Add single object privilege checks to access_check_ddl Serge Petrenko
2018-07-26 20:37     ` [tarantool-patches] " Konstantin Osipov
2018-07-30  8:37       ` Sergey Petrenko [this message]
2018-07-17 16:08   ` [tarantool-patches] [PATCH v2 4/4] Add a privilege upgrade script and update tests Serge Petrenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=D79F007B-84B4-4C04-A72E-440B041A2FFB@tarantool.org \
    --to=sergepetrenko@tarantool.org \
    --cc=kostja@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v2 3/4] Add single object privilege checks to access_check_ddl.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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