[tarantool-patches] [PATCH v2] Fix sequence privilege checks

[tarantool-patches] [PATCH v2] Fix sequence privilege checks

[Serge Petrenko]

There was no check for create privilege when creating a sequence.
Added one, and modified the tests accordingly.
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.

Closes #3512
---
https://github.com/tarantool/tarantool/tree/sergepetrenko/access-checks
https://github.com/tarantool/tarantool/issues/3512

Changes in v2:
  - fixed privilege checks on attaching a sequence
    to a space.

 src/box/alter.cc           | 22 +++++++++++++++------
 test/box/access.result     | 18 +++++++++++++++--
 test/box/access.test.lua   |  9 +++++++--
 test/box/sequence.result   | 49 ++++++++++++++++++++++++++++++++++------------
 test/box/sequence.test.lua | 29 +++++++++++++++++----------
 5 files changed, 95 insertions(+), 32 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 509e4b7e3..58eb3ab82 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1716,9 +1716,7 @@ 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;
+	enum priv_type priv_type = new_tuple ? PRIV_A : PRIV_D;
 	access_check_ddl(old_space->def->name, old_space->def->uid, SC_SPACE,
 			 priv_type, true);
 	struct index *old_index = space_index(old_space, iid);
@@ -2981,6 +2979,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);
 		sequence_cache_replace(new_def);
 		alter->new_def = new_def;
 	} else if (old_tuple != NULL && new_tuple == NULL) {	/* DELETE */
@@ -3083,10 +3083,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 7e070e6d5..928c3fcd4 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -1740,8 +1740,9 @@ c:close()
 ---
 ...
 --
--- A user with read/write access to sequence was able
--- to create a sequence
+-- A user with read/write access to sequence shouldn't
+-- be able to create a sequence. It also needs a create privilege
+-- on universe.
 --
 box.schema.user.create('tester')
 ---
@@ -1754,6 +1755,19 @@ box.session.su('tester')
 ...
 _  = box.schema.sequence.create('test_sequence')
 ---
+- error: Create access to sequence 'test_sequence' is denied for user 'tester'
+...
+box.session.su('admin')
+---
+...
+box.schema.user.grant('tester', 'create', 'universe')
+---
+...
+box.session.su('tester')
+---
+...
+_ = box.schema.sequence.create('test_sequence')
+---
 ...
 box.session.su('admin')
 ---
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index a2988c4c0..7dc92ba52 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -670,13 +670,18 @@ box.schema.func.drop("func")
 c:close()
 
 --
--- A user with read/write access to sequence was able
--- to create a sequence
+-- A user with read/write access to sequence shouldn't
+-- be able to create a sequence. It also needs a create privilege
+-- on universe.
 --
 box.schema.user.create('tester')
 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.session.su('tester')
+_ = box.schema.sequence.create('test_sequence')
+box.session.su('admin')
 box.schema.user.drop('tester')
 
diff --git a/test/box/sequence.result b/test/box/sequence.result
index 0c9951d8b..95c6d623d 100644
--- a/test/box/sequence.result
+++ b/test/box/sequence.result
@@ -1472,6 +1472,9 @@ sq:drop()
 ---
 ...
 -- A user can alter/use sequences that he owns.
+box.schema.user.grant('user', 'create', 'universe')
+---
+...
 box.session.su('user')
 ---
 ...
@@ -1490,13 +1493,13 @@ sq = box.schema.sequence.create('seq')
 box.session.su('admin')
 ---
 ...
-box.schema.user.revoke('user', 'read,write', 'universe')
+box.schema.user.revoke('user', 'read,write,create', 'universe')
 ---
 ...
 box.session.su('user')
 ---
 ...
-sq:set(100) -- ok
+sq:set(100) -- ok - user owns the sequence
 ---
 ...
 sq:next() -- ok
@@ -1512,7 +1515,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')
 ---
 ...
@@ -1537,17 +1541,35 @@ 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', 'read,write,create', 'universe')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_index')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_space_sequence')
+---
+...
+box.schema.user.grant('user', 'read', 'universe')
 ---
 ...
+box.session.su('user')
+---
+...
+_ = s2:create_index('pk', {sequence = 'seq1'}) -- error
+---
+- error: Write 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: Write access to sequence 'seq1' is denied for user 'user'
 ...
 box.space._space_sequence:replace{s1.id, sq2.id, false} -- error
 ---
@@ -1555,7 +1577,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: Write access to sequence 'seq1' is denied for user 'user'
 ...
 s2.index.pk:alter({sequence = 'seq2'}) -- ok
 ---
@@ -1566,10 +1588,13 @@ 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', 'create', 'universe')
+box.schema.user.revoke('user', 'write', 'space', '_space_sequence')
+---
+...
+box.schema.user.revoke('user', 'read', 'universe')
 ---
 ...
 box.session.su('user')
@@ -1677,7 +1702,7 @@ s:drop()
 ---
 ...
 -- When a user is dropped, all his sequences are dropped as well.
-box.schema.user.grant('user', 'read,write', 'universe')
+box.schema.user.grant('user', 'read,write,create', 'universe')
 ---
 ...
 box.session.su('user')
@@ -1707,10 +1732,10 @@ box.schema.user.create('user1')
 box.schema.user.create('user2')
 ---
 ...
-box.schema.user.grant('user1', 'read,write', 'universe')
+box.schema.user.grant('user1', 'read,write,create', 'universe')
 ---
 ...
-box.schema.user.grant('user2', 'read,write', 'universe')
+box.schema.user.grant('user2', 'read,write,create', 'universe')
 ---
 ...
 box.session.su('user1')
diff --git a/test/box/sequence.test.lua b/test/box/sequence.test.lua
index 1bcb91a9c..4d481c4f0 100644
--- a/test/box/sequence.test.lua
+++ b/test/box/sequence.test.lua
@@ -490,21 +490,23 @@ box.session.su('admin')
 sq:drop()
 
 -- A user can alter/use sequences that he owns.
+box.schema.user.grant('user', 'create', 'universe')
 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', 'universe')
+box.schema.user.revoke('user', 'read,write,create', 'universe')
 box.session.su('user')
-sq:set(100) -- ok
+sq:set(100) -- ok - user owns the sequence
 sq:next() -- ok
 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')
@@ -513,8 +515,14 @@ box.schema.user.grant('user', 'create', 'universe')
 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', 'read,write,create', 'universe')
+box.schema.user.grant('user', 'write', 'space', '_index')
+box.schema.user.grant('user', 'write', 'space', '_space_sequence')
+box.schema.user.grant('user', 'read', 'universe')
+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
@@ -525,8 +533,9 @@ 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', 'universe')
 box.session.su('user')
 s2:insert{nil, 1} -- ok: {1, 1}
 box.session.su('admin')
@@ -562,7 +571,7 @@ 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', 'universe')
+box.schema.user.grant('user', 'read,write,create', 'universe')
 box.session.su('user')
 _ = box.schema.sequence.create('test1')
 _ = box.schema.sequence.create('test2')
@@ -574,8 +583,8 @@ box.sequence
 -- to a sequence.
 box.schema.user.create('user1')
 box.schema.user.create('user2')
-box.schema.user.grant('user1', 'read,write', 'universe')
-box.schema.user.grant('user2', 'read,write', 'universe')
+box.schema.user.grant('user1', 'read,write,create', 'universe')
+box.schema.user.grant('user2', 'read,write,create', 'universe')
 box.session.su('user1')
 sq = box.schema.sequence.create('test')
 box.session.su('user2')
-- 
2.15.2 (Apple Git-101.1)