[tarantool-patches] [PATCH v2] Fix sequence privilege checks
Serge Petrenko
sergepetrenko at tarantool.org
Mon Jul 9 18:04:14 MSK 2018
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)
More information about the Tarantool-patches
mailing list