[tarantool-patches] [PATCH 1/4] Make access_check_ddl check for entity privileges.

[Serge Petrenko <sergepetrenko@tarantool.org>]

Function access_check_ddl checked only for universal access, thus
granting entity or singe object access to a user would have no effect in
scope of this function.
Fix this by adding entity access checks.

Also attaching an existing sequence to a space checked for
create privilege on both space (instead of alter) and sequence
(instead of read + write). Fixed both and changed the tests accordingly.

Now creating an index demands alter on a space, because checking for
create would lead to anyone with create access to entity space be able
to create indices in other users spaces. Also checking for alter or drop
on dropping an index, since any operations on space indices are altering
a space, and dropping a space requires dropping all indices, so have to
check for alter (in case of just dropping an index) or drop (in case of
dropping the space with all its indices, to not require additional alter
privilege).

Closes #3516
---
 src/box/alter.cc           |  53 +++++++++++++++----
 test/box/access.result     |  29 ++++++++--
 test/box/access.test.lua   |  21 ++++++--
 test/box/sequence.result   | 128 ++++++++++++++++++++++++++++++++++++++++-----
 test/box/sequence.test.lua |  58 +++++++++++++++-----
 5 files changed, 245 insertions(+), 44 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index b74369321..bd50e457a 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -85,6 +85,13 @@ access_check_ddl(const char *name, uint32_t owner_uid,
 	user_access_t access = ((PRIV_U | (user_access_t) priv_type) &
 				~has_access);
 	bool is_owner = owner_uid == cr->uid || cr->uid == ADMIN;
+	if (access == 0)
+		return; /* Access granted. */
+	/* Check for specific entity access. */
+	struct access *object = entity_access_get(type);
+	if (object) {
+		access &= ~object[cr->auth_token].effective;
+	}
 	/*
 	 * Only the owner of the object or someone who has
 	 * specific DDL privilege on the object can execute
@@ -94,7 +101,7 @@ access_check_ddl(const char *name, uint32_t owner_uid,
 	 * the owner of the object, but this should be ignored --
 	 * CREATE privilege is required.
 	 */
-	if (access == 0 || (is_owner && !(access & (PRIV_U|PRIV_C))))
+	if (access == 0 || (is_owner && !(access & (PRIV_U | PRIV_C))))
 		return; /* Access granted. */
 
 	/* Create a meaningful error message. */
@@ -1764,11 +1771,20 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 	uint32_t iid = tuple_field_u32_xc(old_tuple ? old_tuple : new_tuple,
 					  BOX_INDEX_FIELD_ID);
 	struct space *old_space = space_cache_find_xc(id);
-	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);
+	bool have_alter = true;
+	try {
+		access_check_ddl(old_space->def->name, old_space->def->uid, SC_SPACE,
+				 PRIV_A, true);
+	} catch(AccessDeniedError *e) {
+		/*
+		 * We need alter access on space in case of creating or
+		 * altering an index. But we can have alter OR drop on
+		 * space in case of dropping an index.
+		 */
+		have_alter = false;
+		if(new_tuple)
+			throw;
+	}
 	struct index *old_index = space_index(old_space, iid);
 
 	/*
@@ -1826,6 +1842,13 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 	 */
 	/* Case 1: drop the index, if it is dropped. */
 	if (old_index != NULL && new_tuple == NULL) {
+		/*
+		 * In case we don't have alter on space when
+		 * dropping an index, check for drop on space.
+		 */
+		if (!have_alter)
+		    access_check_ddl(old_space->def->name, old_space->def->uid,
+				     SC_SPACE, PRIV_D, true);
 		alter_space_move_indexes(alter, 0, iid);
 		(void) new DropIndex(alter, old_index->def);
 	}
@@ -3032,7 +3055,7 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event)
 						      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);
+				 PRIV_C, false);
 		sequence_cache_replace(new_def);
 		alter->new_def = new_def;
 	} else if (old_tuple != NULL && new_tuple == NULL) {	/* DELETE */
@@ -3135,10 +3158,20 @@ on_replace_dd_space_sequence(struct trigger * /* trigger */, void *event)
 	enum priv_type priv_type = stmt->new_tuple ? PRIV_C : PRIV_D;
 	if (stmt->new_tuple && stmt->old_tuple)
 		priv_type = PRIV_A;
-
 	/* Check we have the correct access type on the sequence.  * */
-	access_check_ddl(seq->def->name, seq->def->uid, SC_SEQUENCE, priv_type,
-			 false);
+	if (is_generated || !stmt->new_tuple) {
+		access_check_ddl(seq->def->name, 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);
+	}
 	/** Check we have alter access on space. */
 	access_check_ddl(space->def->name, space->def->uid, SC_SPACE, PRIV_A,
 			 false);
diff --git a/test/box/access.result b/test/box/access.result
index a1f3e996a..f4669a4a3 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -875,7 +875,12 @@ session = box.session
 box.schema.user.create('test')
 ---
 ...
-box.schema.user.grant('test', 'read,write,create', 'universe')
+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')
@@ -1389,7 +1394,10 @@ box.schema.func.create('test_func')
 box.session.su("admin")
 ---
 ...
-box.schema.user.grant("tester", "read", "universe")
+box.schema.user.grant("tester", "read", "space", "_user")
+---
+...
+box.schema.user.grant("tester", "read", "space", "_func")
 ---
 ...
 -- failed create
@@ -1416,7 +1424,20 @@ box.session.su("admin")
 -- explicitly since we still use process_rw to write to system
 -- tables from ddl
 --
-box.schema.user.grant("tester", "create,write", "universe")
+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', 'read', 'space', '_sequence')
 ---
 ...
 box.session.su("tester")
@@ -1824,7 +1845,7 @@ _  = box.schema.sequence.create('test_sequence')
 box.session.su('admin')
 ---
 ...
-box.schema.user.grant('tester', 'create', 'universe')
+box.schema.user.grant('tester', 'create', 'sequence')
 ---
 ...
 box.session.su('tester')
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index fb8f744e8..9ae0e1114 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -340,7 +340,10 @@ c:close()
 
 session = box.session
 box.schema.user.create('test')
-box.schema.user.grant('test', 'read,write,create', 'universe')
+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')
 session.su('admin')
@@ -520,7 +523,8 @@ box.schema.space.create("test_space")
 box.schema.user.create('test_user')
 box.schema.func.create('test_func')
 box.session.su("admin")
-box.schema.user.grant("tester", "read", "universe")
+box.schema.user.grant("tester", "read", "space", "_user")
+box.schema.user.grant("tester", "read", "space", "_func")
 -- failed create
 box.session.su("tester")
 box.schema.space.create("test_space")
@@ -533,7 +537,16 @@ box.session.su("admin")
 -- explicitly since we still use process_rw to write to system
 -- tables from ddl
 --
-box.schema.user.grant("tester", "create,write", "universe")
+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', 'read', 'space', '_sequence')
 box.session.su("tester")
 -- successful create
 s1 = box.schema.space.create("test_space")
@@ -712,7 +725,7 @@ box.schema.user.grant('tester', 'read,write', 'space', '_sequence')
 box.session.su('tester')
 _  = box.schema.sequence.create('test_sequence')
 box.session.su('admin')
-box.schema.user.grant('tester', 'create', 'universe')
+box.schema.user.grant('tester', 'create', 'sequence')
 box.session.su('tester')
 _ = box.schema.sequence.create('test_sequence')
 box.session.su('admin')
diff --git a/test/box/sequence.result b/test/box/sequence.result
index cbbd45080..75d5ea1e6 100644
--- a/test/box/sequence.result
+++ b/test/box/sequence.result
@@ -1471,8 +1471,17 @@ box.session.su('admin')
 sq:drop()
 ---
 ...
+box.schema.user.revoke('user', 'read,write', 'universe')
+---
+...
 -- A user can alter/use sequences that he owns.
-box.schema.user.grant('user', 'create', 'universe')
+box.schema.user.grant('user', 'create', 'sequence')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_sequence')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_sequence_data')
 ---
 ...
 box.session.su('user')
@@ -1493,7 +1502,13 @@ sq = box.schema.sequence.create('seq')
 box.session.su('admin')
 ---
 ...
-box.schema.user.revoke('user', 'read,write,create', 'universe')
+box.schema.user.revoke('user', 'create', 'sequence')
+---
+...
+box.schema.user.revoke('user', 'write', 'space', '_sequence')
+---
+...
+box.schema.user.revoke('user', 'write', 'space', '_sequence_data')
 ---
 ...
 box.session.su('user')
@@ -1515,7 +1530,8 @@ box.session.su('admin')
 sq:drop()
 ---
 ...
--- A sequence can be attached to a space only if the user owns both.
+-- A sequence can be attached to a space only if the user has
+-- alter privilege on space and read/write on sequence.
 sq1 = box.schema.sequence.create('seq1')
 ---
 ...
@@ -1525,10 +1541,22 @@ s1 = box.schema.space.create('space1')
 _ = s1:create_index('pk')
 ---
 ...
-box.schema.user.grant('user', 'read,write', 'universe')
+box.schema.user.grant('user', 'write', 'space', '_sequence')
 ---
 ...
-box.schema.user.grant('user', 'create', 'universe')
+box.schema.user.grant('user', 'write', 'space', '_sequence_data')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_schema')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_space')
+---
+...
+box.schema.user.grant('user', 'create', 'space')
+---
+...
+box.schema.user.grant('user', 'create', 'sequence')
 ---
 ...
 box.session.su('user')
@@ -1540,17 +1568,53 @@ sq2 = box.schema.sequence.create('seq2')
 s2 = box.schema.space.create('space2')
 ---
 ...
--- fixme: no error on using another user's sequence
-_ = s2:create_index('pk', {sequence = 'seq1'})
+box.session.su('admin')
+---
+...
+box.schema.user.revoke('user', 'create', 'space')
+---
+...
+box.schema.user.revoke('user', 'create', 'sequence')
+---
+...
+box.schema.user.revoke('user', 'write', 'space', '_sequence')
+---
+...
+box.schema.user.revoke('user', 'write', 'space', '_sequence_data')
+---
+...
+box.schema.user.revoke('user', 'write', 'space', '_schema')
+---
+...
+box.schema.user.revoke('user', 'write', 'space', '_space')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_index')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_space_sequence')
 ---
 ...
+box.schema.user.grant('user', 'read', 'space', '_index')
+---
+...
+box.schema.user.grant('user', 'read', 'space', '_space_sequence')
+---
+...
+box.session.su('user')
+---
+...
+_ = s2:create_index('pk', {sequence = 'seq1'}) -- error
+---
+- error: Read access to sequence 'seq1' is denied for user 'user'
+...
 s1.index.pk:alter({sequence = 'seq1'}) -- error
 ---
 - error: Alter access to space 'space1' is denied for user 'user'
 ...
 box.space._space_sequence:replace{s1.id, sq1.id, false} -- error
 ---
-- error: Alter access to space 'space1' is denied for user 'user'
+- error: Read access to sequence 'seq1' is denied for user 'user'
 ...
 box.space._space_sequence:replace{s1.id, sq2.id, false} -- error
 ---
@@ -1558,7 +1622,7 @@ box.space._space_sequence:replace{s1.id, sq2.id, false} -- error
 ...
 box.space._space_sequence:replace{s2.id, sq1.id, false} -- error
 ---
-- error: Alter access to sequence 'seq1' is denied for user 'user'
+- error: Read access to sequence 'seq1' is denied for user 'user'
 ...
 s2.index.pk:alter({sequence = 'seq2'}) -- ok
 ---
@@ -1569,10 +1633,22 @@ box.session.su('admin')
 -- If the user owns a sequence attached to a space,
 -- it can use it for auto increment, otherwise it
 -- needs privileges.
-box.schema.user.revoke('user', 'read,write', 'universe')
+box.schema.user.revoke('user', 'write', 'space', '_index')
+---
+...
+box.schema.user.revoke('user', 'write', 'space', '_space_sequence')
+---
+...
+box.schema.user.revoke('user', 'read', 'space', '_space')
+---
+...
+box.schema.user.revoke('user', 'read', 'space', '_sequence')
+---
+...
+box.schema.user.revoke('user', 'read', 'space', '_index')
 ---
 ...
-box.schema.user.revoke('user', 'create', 'universe')
+box.schema.user.revoke('user', 'read', 'space', '_space_sequence')
 ---
 ...
 box.session.su('user')
@@ -1680,7 +1756,16 @@ s:drop()
 ---
 ...
 -- When a user is dropped, all his sequences are dropped as well.
-box.schema.user.grant('user', 'read,write,create', 'universe')
+box.schema.user.grant('user', 'write', 'space', '_sequence')
+---
+...
+box.schema.user.grant('user', 'read', 'space', '_sequence')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_space_sequence')
+---
+...
+box.schema.user.grant('user', 'create', 'sequence')
 ---
 ...
 box.session.su('user')
@@ -1710,10 +1795,25 @@ box.schema.user.create('user1')
 box.schema.user.create('user2')
 ---
 ...
-box.schema.user.grant('user1', 'read,write,create', 'universe')
+box.schema.user.grant('user1', 'create', 'sequence')
+---
+...
+box.schema.user.grant('user1', 'write', 'space', '_sequence')
+---
+...
+box.schema.user.grant('user1', 'read', 'space', '_sequence')
+---
+...
+box.schema.user.grant('user1', 'read', 'space', '_user')
+---
+...
+box.schema.user.grant('user1', 'write', 'space', '_sequence_data')
+---
+...
+box.schema.user.grant('user1', 'write', 'space', '_priv')
 ---
 ...
-box.schema.user.grant('user2', 'read,write,create', 'universe')
+box.schema.user.grant('user2', 'read,write', 'universe')
 ---
 ...
 box.session.su('user1')
diff --git a/test/box/sequence.test.lua b/test/box/sequence.test.lua
index c119459b3..00261e397 100644
--- a/test/box/sequence.test.lua
+++ b/test/box/sequence.test.lua
@@ -488,16 +488,21 @@ sq:alter{step = 2} -- error
 sq:drop() -- error
 box.session.su('admin')
 sq:drop()
+box.schema.user.revoke('user', 'read,write', 'universe')
 
 -- A user can alter/use sequences that he owns.
-box.schema.user.grant('user', 'create', 'universe')
+box.schema.user.grant('user', 'create', 'sequence')
+box.schema.user.grant('user', 'write', 'space', '_sequence')
+box.schema.user.grant('user', 'write', 'space', '_sequence_data')
 box.session.su('user')
 sq = box.schema.sequence.create('seq')
 sq:alter{step = 2} -- ok
 sq:drop() -- ok
 sq = box.schema.sequence.create('seq')
 box.session.su('admin')
-box.schema.user.revoke('user', 'read,write,create', 'universe')
+box.schema.user.revoke('user', 'create', 'sequence')
+box.schema.user.revoke('user', 'write', 'space', '_sequence')
+box.schema.user.revoke('user', 'write', 'space', '_sequence_data')
 box.session.su('user')
 sq:set(100) -- ok - user owns the sequence
 sq:next() -- ok
@@ -505,17 +510,34 @@ sq:reset() -- ok
 box.session.su('admin')
 sq:drop()
 
--- A sequence can be attached to a space only if the user owns both.
+-- A sequence can be attached to a space only if the user has
+-- alter privilege on space and read/write on sequence.
 sq1 = box.schema.sequence.create('seq1')
 s1 = box.schema.space.create('space1')
 _ = s1:create_index('pk')
-box.schema.user.grant('user', 'read,write', 'universe')
-box.schema.user.grant('user', 'create', 'universe')
+box.schema.user.grant('user', 'write', 'space', '_sequence')
+box.schema.user.grant('user', 'write', 'space', '_sequence_data')
+box.schema.user.grant('user', 'write', 'space', '_schema')
+box.schema.user.grant('user', 'write', 'space', '_space')
+box.schema.user.grant('user', 'create', 'space')
+box.schema.user.grant('user', 'create', 'sequence')
 box.session.su('user')
 sq2 = box.schema.sequence.create('seq2')
 s2 = box.schema.space.create('space2')
--- fixme: no error on using another user's sequence
-_ = s2:create_index('pk', {sequence = 'seq1'})
+
+box.session.su('admin')
+box.schema.user.revoke('user', 'create', 'space')
+box.schema.user.revoke('user', 'create', 'sequence')
+box.schema.user.revoke('user', 'write', 'space', '_sequence')
+box.schema.user.revoke('user', 'write', 'space', '_sequence_data')
+box.schema.user.revoke('user', 'write', 'space', '_schema')
+box.schema.user.revoke('user', 'write', 'space', '_space')
+box.schema.user.grant('user', 'write', 'space', '_index')
+box.schema.user.grant('user', 'write', 'space', '_space_sequence')
+box.schema.user.grant('user', 'read', 'space', '_index')
+box.schema.user.grant('user', 'read', 'space', '_space_sequence')
+box.session.su('user')
+_ = s2:create_index('pk', {sequence = 'seq1'}) -- error
 s1.index.pk:alter({sequence = 'seq1'}) -- error
 box.space._space_sequence:replace{s1.id, sq1.id, false} -- error
 box.space._space_sequence:replace{s1.id, sq2.id, false} -- error
@@ -526,8 +548,12 @@ box.session.su('admin')
 -- If the user owns a sequence attached to a space,
 -- it can use it for auto increment, otherwise it
 -- needs privileges.
-box.schema.user.revoke('user', 'read,write', 'universe')
-box.schema.user.revoke('user', 'create', 'universe')
+box.schema.user.revoke('user', 'write', 'space', '_index')
+box.schema.user.revoke('user', 'write', 'space', '_space_sequence')
+box.schema.user.revoke('user', 'read', 'space', '_space')
+box.schema.user.revoke('user', 'read', 'space', '_sequence')
+box.schema.user.revoke('user', 'read', 'space', '_index')
+box.schema.user.revoke('user', 'read', 'space', '_space_sequence')
 box.session.su('user')
 s2:insert{nil, 1} -- ok: {1, 1}
 box.session.su('admin')
@@ -563,7 +589,10 @@ box.session.su('admin')
 s:drop()
 
 -- When a user is dropped, all his sequences are dropped as well.
-box.schema.user.grant('user', 'read,write,create', 'universe')
+box.schema.user.grant('user', 'write', 'space', '_sequence')
+box.schema.user.grant('user', 'read', 'space', '_sequence')
+box.schema.user.grant('user', 'write', 'space', '_space_sequence')
+box.schema.user.grant('user', 'create', 'sequence')
 box.session.su('user')
 _ = box.schema.sequence.create('test1')
 _ = box.schema.sequence.create('test2')
@@ -575,8 +604,13 @@ box.sequence
 -- to a sequence.
 box.schema.user.create('user1')
 box.schema.user.create('user2')
-box.schema.user.grant('user1', 'read,write,create', 'universe')
-box.schema.user.grant('user2', 'read,write,create', 'universe')
+box.schema.user.grant('user1', 'create', 'sequence')
+box.schema.user.grant('user1', 'write', 'space', '_sequence')
+box.schema.user.grant('user1', 'read', 'space', '_sequence')
+box.schema.user.grant('user1', 'read', 'space', '_user')
+box.schema.user.grant('user1', 'write', 'space', '_sequence_data')
+box.schema.user.grant('user1', 'write', 'space', '_priv')
+box.schema.user.grant('user2', 'read,write', 'universe')
 box.session.su('user1')
 sq = box.schema.sequence.create('test')
 box.session.su('user2')
-- 
2.15.2 (Apple Git-101.1)