* [tarantool-patches] Re: [PATCH] Make access_check_ddl check for entity privileges.
[not found] <c734dd77-57dd-b0a3-af26-bf38937b1725@tarantool.org>
@ 2018-07-19 7:48 ` Sergey Petrenko
0 siblings, 0 replies; 4+ messages in thread
From: Sergey Petrenko @ 2018-07-19 7:48 UTC (permalink / raw)
To: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 3963 bytes --]
Sorry, forgot to send to the mailing list.
-------- Перенаправленное сообщение --------
Тема: Re: [tarantool-patches] Re: [PATCH] Make access_check_ddl check
for entity privileges.
Дата: Thu, 19 Jul 2018 10:46:27 +0300
От: Sergey Petrenko <sergepetrenko@tarantool.org>
Кому: Konstantin Osipov <kostja@tarantool.org>
18.07.2018 9:07, Konstantin Osipov пишет:
> * Sergey Petrenko <sergepetrenko@tarantool.org> [18/07/12 11:55]:
>
>>>> - 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);
>>>> + enum priv_type priv_type = new_tuple ? PRIV_A : PRIV_D;
>>>> + access_check_ddl(old_space->def->name, old_space->def->id,
>>>> + old_space->def->uid, SC_SPACE, priv_type, true);
>>> As far as I understand, you changed it because creating an index
>>> is technically altering a space, not creating it. But in this case
>>> dropping an index is also technically altering a space.
>>> In SQL, CREATE/DROP/ALTER match SQL statements CREATE/DROP/ALTER
>>> respectively. Since in NoSQL in Tarantool we don't have these
>>> statements, instead, we create each index with a separate Lua
>>> command, let's keep the old check: use CREATE access to space
>>> in order to permit CREATING an index, ALTER - to permit update,
>>> and DROP - to permit drop.
>> Checking for create privilege ignores ownership, since when creating an
>> object there can't be a create privilege on the object itself.
> INDEX is not a separate object, it's a part of the space.
> A user who has created the space should be able to
> CREATE/DROP/ALTER any index in the space based on the definer
> rule (the owner of the object should be able to do anything with
> it).
You misunderstood me, I meant the space object, since we used to
check for CREATE privilege on space itself to create an index previously.
Checking for ALTER on space, would allow exactly what you want with
CREATE/ALTER/DROP of an index. The owner has every privilege, including
ALTER, so he is able to do anything with the indices.
If someone else created an index on your space, and you want to drop it,
it is no problem. This is why we check for ALTER OR DROP on space in case
of dropping an index.
> I imagine if an index has an independent owner, one would not be
> able to drop their own space if some other user created an index
> on it.
>
> Let's try to avoid this. Oracle also has entity access. How does
> it work there? Who is set as the definer of the index if user b
> creates an index on space created by user a? Let's bring this up
> with Peter Gulutzan, he may have an educated opinion on the
> subject.
What Oracle does is they have separate entities INDEX and TABLE,
and in order to create an index, you need to have an INDEX privilege
on a space OR you may have a CREATE ANY INDEX privilege.
So, technically, non-owners can create indices in your
tables, but again, if you want to drop a table, it is dropped together
with all the indices no matter who their owner is. (The same thing we
may achieve with privilege checks that I and Georgy propose).
>
> We also have an option of separating INDEX and SPACE as entities,
> and introducing INDEX entity. But then again a user who created a
> space should be able to create/drop/alter any index in that space
> - the opposite seems counter-intuitive.
IMO it would unnecessarily complicate access control, cos we would
need to grant all the needed privileges on an index to space owner,
and we also would have to make it so that theese privileges cannot be
revoked.
OR we can set space owner as the definer of any index created on that space,
but again, this seems strange. If you are a non-owner of the space,
you create an index and immediately lose control over it.
We may also consult with Peter Gulutzan. Do you want me to write him?
[-- Attachment #2: Type: text/html, Size: 5395 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] [PATCH] Make access_check_ddl check for entity privileges.
@ 2018-07-11 16:40 Serge Petrenko
2018-07-11 18:33 ` [tarantool-patches] " Konstantin Osipov
0 siblings, 1 reply; 4+ messages in thread
From: Serge Petrenko @ 2018-07-11 16:40 UTC (permalink / raw)
To: tarantool-patches; +Cc: Serge Petrenko
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 and also start implementing
single object access checks.
Also there was no entity access array for entity user. Added one. Now it
is possible to grant a specific privilege on an entire entity user.
Modified box/access.test to grant only the needed privileges.
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 #3516
---
Please note, therre are currently no entity access arrays for role and
collation. Need to discuss whether they need to be added.
https://github.com/tarantool/tarantool/compare/sergepetrenko/gh-3516-entity-access-checks
https://github.com/tarantool/tarantool/issues/3516
src/box/alter.cc | 129 +++++++++++++++++++++++++++++++--------------
src/box/lua/schema.lua | 8 +++
src/box/schema.h | 3 ++
src/box/user.cc | 8 +++
test/box/access.result | 31 +++++++++--
test/box/access.test.lua | 17 ++++--
test/box/sequence.result | 128 +++++++++++++++++++++++++++++++++++++++-----
test/box/sequence.test.lua | 58 +++++++++++++++-----
8 files changed, 309 insertions(+), 73 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index bb1fabfcc..97b03f9c3 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -62,10 +62,9 @@
/* {{{ Auxiliary functions and methods. */
static void
-access_check_ddl(const char *name, uint32_t owner_uid,
- enum schema_object_type type,
- enum priv_type priv_type,
- bool is_17_compat_mode)
+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)
{
struct credentials *cr = effective_user();
user_access_t has_access = cr->universal_access;
@@ -85,6 +84,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
@@ -96,6 +102,40 @@ access_check_ddl(const char *name, uint32_t owner_uid,
*/
if (access == 0 || (is_owner && !(access & (PRIV_U|PRIV_C))))
return; /* Access granted. */
+ int rc = -1;
+ if (!(access & (PRIV_U | PRIV_C))) {
+ /*
+ * If no global or entity privileges found, check for
+ * specific object privilege. Ignore universe and unknown
+ * types here, since universe is already handled, and what
+ * to do with unknown is unknown.
+ *
+ * Currently no specific privileges to a single role, user,
+ * collation.
+ */
+ switch (type) {
+ case SC_SPACE:
+ rc = access_check_space(
+ space_cache_find_xc(object_id),
+ access);
+ break;
+ case SC_SEQUENCE:
+ if (priv_type == PRIV_W) {
+ rc = access_check_sequence(
+ sequence_cache_find(object_id));
+ break;
+ }
+ case SC_FUNCTION:
+ case SC_USER:
+ case SC_ROLE:
+ case SC_COLLATION:
+ default:
+ rc = -1;
+ break;
+ }
+ }
+ if (rc == 0)
+ return; /* Access granted. */
/* Create a meaningful error message. */
struct user *user = user_find_xc(cr->uid);
@@ -1574,7 +1614,7 @@ 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);
@@ -1607,8 +1647,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,
@@ -1653,7 +1693,7 @@ 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))
@@ -1751,11 +1791,9 @@ 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);
+ enum priv_type priv_type = new_tuple ? PRIV_A : PRIV_D;
+ 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);
/*
@@ -2150,7 +2188,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, SC_USER, PRIV_C, true);
+ access_check_ddl(user->name, user->uid, user->owner,
+ SC_USER, PRIV_C, true);
auto def_guard = make_scoped_guard([=] { free(user); });
(void) user_cache_replace(user);
def_guard.is_active = false;
@@ -2158,8 +2197,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,
- SC_USER, PRIV_D, true);
+ access_check_ddl(old_user->def->name, old_user->def->uid,
+ old_user->def->owner, SC_USER, PRIV_D, true);
/* Can't drop guest or super user */
if (uid <= (uint32_t) BOX_SYSTEM_USER_ID_MAX || uid == SUPER) {
tnt_raise(ClientError, ER_DROP_USER,
@@ -2185,8 +2224,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);
@@ -2288,7 +2327,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;
@@ -2302,8 +2342,8 @@ 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,
- PRIV_D, true);
+ access_check_ddl(old_func->def->name, old_func->def->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)) {
tnt_raise(ClientError, ER_DROP_FUNCTION,
@@ -2316,8 +2356,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);
@@ -2473,7 +2513,8 @@ 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,
+ 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
@@ -2489,8 +2530,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();
@@ -2549,8 +2590,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) {
@@ -3018,8 +3059,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 */
@@ -3027,8 +3068,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");
@@ -3044,8 +3085,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;
}
@@ -3122,13 +3163,23 @@ 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->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->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 dc18f71b2..f52d201e5 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1737,6 +1737,7 @@ local priv_object_combo = {
box.priv.C, box.priv.D),
["role"] = bit.bor(box.priv.X, box.priv.U,
box.priv.C, box.priv.D),
+ ["user"] = bit.bor(box.priv.C, box.priv.D),
}
--
@@ -1840,6 +1841,13 @@ local function object_resolve(object_type, object_name)
end
return seq
end
+ if object_type == 'user' then
+ if object_name == nil or object_name == 0 then
+ return 0
+ end
+ -- otherwise some error. Don't know which one yet.
+ box.error(box.error.NO_SUCH_USER, object_name)
+ end
if object_type == 'role' then
local _vuser = box.space[box.schema.VUSER_ID]
local role
diff --git a/src/box/schema.h b/src/box/schema.h
index 0822262d0..767954e5b 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -240,6 +240,7 @@ struct on_access_denied_ctx {
struct entity_access {
struct access space[BOX_USER_MAX];
struct access function[BOX_USER_MAX];
+ struct access user[BOX_USER_MAX];
struct access sequence[BOX_USER_MAX];
};
@@ -255,6 +256,8 @@ entity_access_get(enum schema_object_type type)
return entity_access.space;
case SC_FUNCTION:
return entity_access.function;
+ case SC_USER:
+ return entity_access.user;
case SC_SEQUENCE:
return entity_access.sequence;
default:
diff --git a/src/box/user.cc b/src/box/user.cc
index fbf06566a..358183971 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -229,6 +229,14 @@ access_find(struct priv_def *priv)
access = func->access;
break;
}
+ case SC_USER:
+ {
+ if (priv->object_id == 0) {
+ access = entity_access.user;
+ break;
+ }
+ /* No privileges on a specific user currently. */
+ }
case SC_SEQUENCE:
{
if (priv->object_id == 0) {
diff --git a/test/box/access.result b/test/box/access.result
index a1f3e996a..dbe591839 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,22 @@ 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')
+---
+...
+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 +1847,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..c90fe0a1a 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,12 @@ 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')
+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 +721,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)
^ permalink raw reply [flat|nested] 4+ messages in thread* [tarantool-patches] Re: [PATCH] Make access_check_ddl check for entity privileges.
2018-07-11 16:40 [tarantool-patches] " Serge Petrenko
@ 2018-07-11 18:33 ` Konstantin Osipov
2018-07-12 8:52 ` Sergey Petrenko
0 siblings, 1 reply; 4+ messages in thread
From: Konstantin Osipov @ 2018-07-11 18:33 UTC (permalink / raw)
To: tarantool-patches; +Cc: Serge Petrenko
* Serge Petrenko <sergepetrenko@tarantool.org> [18/07/11 19:42]:
> /*
> * Only the owner of the object or someone who has
> * specific DDL privilege on the object can execute
> @@ -96,6 +102,40 @@ access_check_ddl(const char *name, uint32_t owner_uid,
> */
> if (access == 0 || (is_owner && !(access & (PRIV_U|PRIV_C))))
> return; /* Access granted. */
> + int rc = -1;
> + if (!(access & (PRIV_U | PRIV_C))) {
You can't grant CREATE privilege to a non-existing object.
USAGE can be granted only globally.
This comment explains this if statement. Please add it.
I
> + * Ignore universe and unknown
> + * types here, since universe is already handled, and what
> + * to do with unknown is unknown.
I don't understand this comment, please rephrase or remove.
> + *
> + * Currently no specific privileges to a single role, user,
> + * collation.
> + */
> + switch (type) {
> + case SC_SPACE:
> + rc = access_check_space(
> + space_cache_find_xc(object_id),
> + access);
> + break;
This switch statement is not aligned right. You should use
lisp-style alignment for function arguments.
Throwing ER_NO_SUCH_SPACE is a security breach, you should not
reveal that the space does not exist, instead simply say that
the user has no requested access to space (ER_ACCESS_DENIED).
> + case SC_SEQUENCE:
> + if (priv_type == PRIV_W) {
> + rc = access_check_sequence(
> + sequence_cache_find(object_id));
> + break;
> + }
Wrong alignment, same argument about exceptions.
> + case SC_FUNCTION:
> + case SC_USER:
> + case SC_ROLE:
> + case SC_COLLATION:
What about these? Can't you grant DROP/ALTER on a specific user or
role?
> @@ -1751,11 +1791,9 @@ 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);
> + enum priv_type priv_type = new_tuple ? PRIV_A : PRIV_D;
> + access_check_ddl(old_space->def->name, old_space->def->id,
> + old_space->def->uid, SC_SPACE, priv_type, true);
As far as I understand, you changed it because creating an index
is technically altering a space, not creating it. But in this case
dropping an index is also technically altering a space.
In SQL, CREATE/DROP/ALTER match SQL statements CREATE/DROP/ALTER
respectively. Since in NoSQL in Tarantool we don't have these
statements, instead, we create each index with a separate Lua
command, let's keep the old check: use CREATE access to space
in order to permit CREATING an index, ALTER - to permit update,
and DROP - to permit drop.
> @@ -2185,8 +2224,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);
Why did you write user->uid here for the owner?
> end
> + if object_type == 'user' then
> + if object_name == nil or object_name == 0 then
> + return 0
> + end
> + -- otherwise some error. Don't know which one yet.
> + box.error(box.error.NO_SUCH_USER, object_name)
> + end
It would be better if you extract new entities into a separate
patch. The bigger the patch gets the less changes for it to get in
- there always is something that can be improved.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 4+ messages in thread* [tarantool-patches] Re: [PATCH] Make access_check_ddl check for entity privileges.
2018-07-11 18:33 ` [tarantool-patches] " Konstantin Osipov
@ 2018-07-12 8:52 ` Sergey Petrenko
2018-07-18 6:07 ` Konstantin Osipov
0 siblings, 1 reply; 4+ messages in thread
From: Sergey Petrenko @ 2018-07-12 8:52 UTC (permalink / raw)
To: Konstantin Osipov, tarantool-patches
Hi, thank you for review!
Please see my comments and the new diff below.
11.07.2018 21:33, Konstantin Osipov пишет:
> * Serge Petrenko <sergepetrenko@tarantool.org> [18/07/11 19:42]:
>
>> /*
>> * Only the owner of the object or someone who has
>> * specific DDL privilege on the object can execute
>> @@ -96,6 +102,40 @@ access_check_ddl(const char *name, uint32_t owner_uid,
>> */
>> if (access == 0 || (is_owner && !(access & (PRIV_U|PRIV_C))))
>> return; /* Access granted. */
>> + int rc = -1;
>> + if (!(access & (PRIV_U | PRIV_C))) {
> You can't grant CREATE privilege to a non-existing object.
> USAGE can be granted only globally.
>
> This comment explains this if statement. Please add it.
>
> I
>> + * Ignore universe and unknown
>> + * types here, since universe is already handled, and what
>> + * to do with unknown is unknown.
> I don't understand this comment, please rephrase or remove.
>
>> + *
>> + * Currently no specific privileges to a single role, user,
>> + * collation.
>> + */
>> + switch (type) {
>> + case SC_SPACE:
>> + rc = access_check_space(
>> + space_cache_find_xc(object_id),
>> + access);
>> + break;
> This switch statement is not aligned right. You should use
> lisp-style alignment for function arguments.
>
> Throwing ER_NO_SUCH_SPACE is a security breach, you should not
> reveal that the space does not exist, instead simply say that
> the user has no requested access to space (ER_ACCESS_DENIED).
>
>> + case SC_SEQUENCE:
>> + if (priv_type == PRIV_W) {
>> + rc = access_check_sequence(
>> + sequence_cache_find(object_id));
>> + break;
>> + }
> Wrong alignment, same argument about exceptions.
>
>> + case SC_FUNCTION:
>> + case SC_USER:
>> + case SC_ROLE:
>> + case SC_COLLATION:
> What about these? Can't you grant DROP/ALTER on a specific user or
> role?
>
I dropped all the object checks from the patch since there is still a lot
to be done. I will finish with the checks taking your comments into
account and send a separate patch.
>> @@ -1751,11 +1791,9 @@ 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);
>> + enum priv_type priv_type = new_tuple ? PRIV_A : PRIV_D;
>> + access_check_ddl(old_space->def->name, old_space->def->id,
>> + old_space->def->uid, SC_SPACE, priv_type, true);
> As far as I understand, you changed it because creating an index
> is technically altering a space, not creating it. But in this case
> dropping an index is also technically altering a space.
> In SQL, CREATE/DROP/ALTER match SQL statements CREATE/DROP/ALTER
> respectively. Since in NoSQL in Tarantool we don't have these
> statements, instead, we create each index with a separate Lua
> command, let's keep the old check: use CREATE access to space
> in order to permit CREATING an index, ALTER - to permit update,
> and DROP - to permit drop.
Checking for create privilege ignores ownership, since when creating an
object there can't be a create privilege on the object itself. So you
may only
create indices (even in your own spaces!) only if you have create
privilege on
universe or on entity space. Which, in turn, leads to you being able to
create
indices in any space (even in one you don't own).
I discussed it whith Georgy and we decided to check for alter privilege
on index
creation, and check for alter OR drop privilege on dropping an index,
since dropping an index is altering a space, and dropping a space
(which requires dropping all its indices) should only require a drop
privilege.
>> @@ -2185,8 +2224,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);
> Why did you write user->uid here for the owner?
AFAIU, it was added here to let user change his own name and password.
I left the old variant unnchanged for now, but we can grant the user alter
privilege upon itself on its creation, as soon as single object access
checks
are implemented.
>
>> end
>> + if object_type == 'user' then
>> + if object_name == nil or object_name == 0 then
>> + return 0
>> + end
>> + -- otherwise some error. Don't know which one yet.
>> + box.error(box.error.NO_SUCH_USER, object_name)
>> + end
> It would be better if you extract new entities into a separate
> patch. The bigger the patch gets the less changes for it to get in
> - there always is something that can be improved.
>
Removed this part from the patch, will add lacking entities (role,
collation)
and send a separate patch.
---
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 bb1fabfcc..ef8b1acff 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. */
@@ -1751,11 +1758,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);
/*
@@ -1813,6 +1829,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);
}
@@ -3019,7 +3042,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 */
@@ -3122,10 +3145,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)
^ permalink raw reply [flat|nested] 4+ messages in thread* [tarantool-patches] Re: [PATCH] Make access_check_ddl check for entity privileges.
2018-07-12 8:52 ` Sergey Petrenko
@ 2018-07-18 6:07 ` Konstantin Osipov
0 siblings, 0 replies; 4+ messages in thread
From: Konstantin Osipov @ 2018-07-18 6:07 UTC (permalink / raw)
To: Sergey Petrenko; +Cc: tarantool-patches
* Sergey Petrenko <sergepetrenko@tarantool.org> [18/07/12 11:55]:
> > > - 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);
> > > + enum priv_type priv_type = new_tuple ? PRIV_A : PRIV_D;
> > > + access_check_ddl(old_space->def->name, old_space->def->id,
> > > + old_space->def->uid, SC_SPACE, priv_type, true);
> > As far as I understand, you changed it because creating an index
> > is technically altering a space, not creating it. But in this case
> > dropping an index is also technically altering a space.
> > In SQL, CREATE/DROP/ALTER match SQL statements CREATE/DROP/ALTER
> > respectively. Since in NoSQL in Tarantool we don't have these
> > statements, instead, we create each index with a separate Lua
> > command, let's keep the old check: use CREATE access to space
> > in order to permit CREATING an index, ALTER - to permit update,
> > and DROP - to permit drop.
> Checking for create privilege ignores ownership, since when creating an
> object there can't be a create privilege on the object itself.
INDEX is not a separate object, it's a part of the space.
A user who has created the space should be able to
CREATE/DROP/ALTER any index in the space based on the definer
rule (the owner of the object should be able to do anything with
it).
I imagine if an index has an independent owner, one would not be
able to drop their own space if some other user created an index
on it.
Let's try to avoid this. Oracle also has entity access. How does
it work there? Who is set as the definer of the index if user b
creates an index on space created by user a? Let's bring this up
with Peter Gulutzan, he may have an educated opinion on the
subject.
We also have an option of separating INDEX and SPACE as entities,
and introducing INDEX entity. But then again a user who created a
space should be able to create/drop/alter any index in that space
- the opposite seems counter-intuitive.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-07-19 7:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <c734dd77-57dd-b0a3-af26-bf38937b1725@tarantool.org>
2018-07-19 7:48 ` [tarantool-patches] Re: [PATCH] Make access_check_ddl check for entity privileges Sergey Petrenko
2018-07-11 16:40 [tarantool-patches] " Serge Petrenko
2018-07-11 18:33 ` [tarantool-patches] " Konstantin Osipov
2018-07-12 8:52 ` Sergey Petrenko
2018-07-18 6:07 ` Konstantin Osipov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox