[tarantool-patches] [PATCH v3] Make access_check_ddl check for entity privileges.
Serge Petrenko
sergepetrenko at tarantool.org
Thu Jul 19 22:35:33 MSK 2018
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 and sequence
(instead of read + write on sequence). Fixed it and changed the tests
accordingly.
Closes #3516
---
https://github.com/tarantool/tarantool/issues/3516
https://github.com/tarantool/tarantool/tree/sergepetrenko/gh-3516-entity-access-checks
Changes in v3
- creating / altering / dropping an index
checks for create / alter / drop privilege
on space respectively.
Adding correct index privileges is moved to
a separate patch.
Changes in v2
- rebased to the latest 1.10
src/box/alter.cc | 26 ++++++++--
test/box/access.result | 29 +++++++++--
test/box/access.test.lua | 21 ++++++--
test/box/sequence.result | 125 ++++++++++++++++++++++++++++++++++++++++-----
test/box/sequence.test.lua | 57 ++++++++++++++++-----
5 files changed, 220 insertions(+), 38 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index b74369321..3007a131d 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. */
@@ -3032,7 +3039,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 */
@@ -3137,8 +3144,19 @@ on_replace_dd_space_sequence(struct trigger * /* trigger */, void *event)
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..a2a1a60ea 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
+-- create 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', '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', 'universe')
+box.schema.user.grant('user', 'create', 'space')
+---
+...
+box.schema.user.grant('user', 'create', 'sequence')
---
...
box.session.su('user')
@@ -1540,17 +1568,50 @@ 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', '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 +1619,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 +1630,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 +1753,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 +1792,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..96297d6f2 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,33 @@ 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
+-- create 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', '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 +547,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 +588,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 +603,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)
More information about the Tarantool-patches
mailing list