[tarantool-patches] Re: [PATCH] Add entities user, role to access control.
Sergey Petrenko
sergepetrenko at tarantool.org
Mon Jul 30 10:52:08 MSK 2018
Hi! I made a separate patch introducing new schema object types to be used
under the hood when granting privileges on an entire entity. I also rebased this
patch on top of the latter one and fixed your comments. Please see the new diff below.
> 26 июля 2018 г., в 23:15, Konstantin Osipov <kostja at tarantool.org> написал(а):
>
> * Serge Petrenko <sergepetrenko at tarantool.org> [18/07/12 20:54]:
>> + /* Do not allow changes for system users and roles. */
>> + struct credentials *cr = effective_user();
>> + if ((uid <= (uint32_t) BOX_SYSTEM_USER_ID_MAX || uid == SUPER) &&
>> + cr->uid != ADMIN) {
>> + struct user *current_user = user_find_xc(cr->uid);
>> + tnt_raise(AccessDeniedError, "alter", "user or role",
>> + old_user->def->name, current_user->def->name);
>> + }
>
> It should be possible to change these system objects, provided the
> person knows what they are doing, please remove this check, esp.
> since it's outside the scope of this bug fix.
Removed this check.
>
>> case SC_ROLE:
>> {
>> - struct user *role = user_by_id(priv->object_id);
>> - if (role == NULL || role->def->type != SC_ROLE) {
>> - tnt_raise(ClientError, ER_NO_SUCH_ROLE,
>> - role ? role->def->name :
>> - int2str(priv->object_id));
>> - }
>> - /*
>> - * Only the creator of the role can grant or revoke it.
>> - * Everyone can grant 'PUBLIC' role.
>> - */
>> - if (role->def->owner != grantor->def->uid &&
>> - grantor->def->uid != ADMIN &&
>> - (role->def->uid != PUBLIC || priv->access != PRIV_X)) {
>> + if (priv->object_id != 0) {
>> + struct user *role = user_by_id(priv->object_id);
>> + if (role == NULL || role->def->type != SC_ROLE) {
>> + tnt_raise(ClientError, ER_NO_SUCH_ROLE,
>> + role ? role->def->name :
>> + int2str(priv->object_id));
>> + }
>> + /*
>> + * Only the creator of the role can grant or revoke it.
>> + * Everyone can grant 'PUBLIC' role.
>> + */
>> + if (role->def->owner != grantor->def->uid &&
>> + grantor->def->uid != ADMIN &&
>> + (role->def->uid != PUBLIC || priv->access != PRIV_X)) {
>> + tnt_raise(AccessDeniedError,
>> + priv_name(priv_type),
>> + schema_object_name(SC_ROLE), name,
>> + grantor->def->name);
>> + }
>> + /* Not necessary to do during revoke, but who cares. */
>> + role_check(grantee, role);
>> + } else if (grantor->def->uid != ADMIN) {
>> + /* only admin may grant privileges on an entire entity. */
>
>> tnt_raise(AccessDeniedError,
>> priv_name(priv_type),
>> schema_object_name(SC_ROLE), name,
>> grantor->def->name);
>> }
>> - /* Not necessary to do during revoke, but who cares. */
>> - role_check(grantee, role);
>
> Why no 'break;'?
>
> Instead of changing indent level you could have added a break.
Revorked this part, it is not so bulky anymore, see the new diff below.
>> + case SC_USER:
>> + {
>> + /*
>> + * user ID 0 is shared between user 'guest' and granting
>> + * privileges upon whole entity user. This is not a problem,
>> + * since we don't want to grant privileges on any system user,
>> + * including 'guest'.
>> + */
>> + if(priv->object_id == 0) {
>> + access = entity_access.user;
>> + break;
>> + }
>
> Hm, does it mean that now it's not possible to grant some
> privileges to 'guest'? Looks like we should be using a different
> identifier for entity-level grants then?
> Are you testing this at all? (I found no test for this).
Revorked this part as you requested.
src/box/alter.cc | 43 ++++++++++++++++++++++++++++++++++++++++---
src/box/lua/schema.lua | 35 +++++++++++++++++++++++++----------
src/box/schema.h | 8 ++++++++
src/box/user.cc | 22 +++++++++++++++++++++-
test/box/access.result | 20 ++++++++++----------
test/box/access.test.lua | 15 +++++----------
test/box/access_misc.result | 2 +-
test/box/role.result | 25 +++++++++++++++++++++++--
test/box/role.test.lua | 12 ++++++++++--
9 files changed, 143 insertions(+), 39 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 3007a131d..d4545f31c 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2170,7 +2170,7 @@ 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->owner, user->type, PRIV_C, true);
auto def_guard = make_scoped_guard([=] { free(user); });
(void) user_cache_replace(user);
def_guard.is_active = false;
@@ -2179,7 +2179,7 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
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);
+ old_user->def->type, 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,
@@ -2645,6 +2645,38 @@ priv_def_check(struct priv_def *priv, enum priv_type priv_type)
}
/* Not necessary to do during revoke, but who cares. */
role_check(grantee, role);
+ break;
+ }
+ case SC_USER:
+ {
+ struct user *user = NULL;
+ user = user_by_id(priv->object_id);
+ if (user == NULL || user->def->type != SC_USER) {
+ tnt_raise(ClientError, ER_NO_SUCH_USER,
+ user ? user->def->name :
+ int2str(priv->object_id));
+ }
+ if (user->def->owner != grantor->def->uid &&
+ grantor->def->uid != ADMIN) {
+ tnt_raise(AccessDeniedError,
+ priv_name(priv_type),
+ schema_object_name(SC_USER), name,
+ grantor->def->name);
+ }
+ break;
+ }
+ case SC_ENTITY_SPACE:
+ case SC_ENTITY_FUNCTION:
+ case SC_ENTITY_SEQUENCE:
+ case SC_ENTITY_ROLE:
+ case SC_ENTITY_USER:
+ {
+ /* Only amdin may grant privileges on an entire entity. */
+ if (grantor->def->uid != ADMIN) {
+ tnt_raise(AccessDeniedError, priv_name(priv_type),
+ schema_object_name(priv->object_type), name,
+ grantor->def->name);
+ }
}
default:
break;
@@ -2665,7 +2697,12 @@ grant_or_revoke(struct priv_def *priv)
struct user *grantee = user_by_id(priv->grantee_id);
if (grantee == NULL)
return;
- if (priv->object_type == SC_ROLE) {
+ /*
+ * Grant a role to a user only when privilege type is 'execute'
+ * and the role is specified.
+ */
+ if (priv->object_type == SC_ROLE && !(priv->access & ~PRIV_X) &&
+ priv->object_id != 0) {
struct user *role = user_by_id(priv->object_id);
if (role == NULL || role->def->type != SC_ROLE)
return;
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index eb984b1de..28f68cbda 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1746,6 +1746,9 @@ local priv_object_combo = {
["role"] = bit.bor(box.priv.X, box.priv.U,
box.priv.D),
["all roles"] = bit.bor(box.priv.C, box.priv.D),
+ ["user"] = bit.bor(box.priv.A, box.priv.D),
+ ["all users"] = bit.bor(box.priv.C, box.priv.A,
+ box.priv.D),
}
--
@@ -1839,6 +1842,12 @@ local function object_resolve(object_type, object_name)
end
return 0
end
+ if object_type == 'all users' then
+ if object_name ~= nil and object_name ~= 0 then
+ box.error(box.error.ILLEGAL_PARAMS, "no object name allowed")
+ end
+ return 0
+ end
if object_type == 'space' then
if object_name == nil then
return nil
@@ -1876,22 +1885,27 @@ local function object_resolve(object_type, object_name)
end
return seq
end
- if object_type == 'role' then
- if object_name == nil then
+ if object_type == 'role' or object_type == 'user' then
+ if object_name == nil then
return nil
end
local _vuser = box.space[box.schema.VUSER_ID]
- local role
+ local role_or_user
+ if object_name == nil or object_name == 0 then
+ return 0
+ end
if type(object_name) == 'string' then
- role = _vuser.index.name:get{object_name}
+ role_or_user = _vuser.index.name:get{object_name}
else
- role = _vuser:get{object_name}
+ role_or_user = _vuser:get{object_name}
end
- if role and role[4] == 'role' then
- return role[1]
- else
+ if role_or_user and role_or_user[4] == object_type then
+ return role_or_user[1]
+ elseif object_type == 'role' then
box.error(box.error.NO_SUCH_ROLE, object_name)
- end
+ else
+ box.error(box.error.NO_SUCH_USER, object_name)
+ end
end
box.error(box.error.UNKNOWN_SCHEMA_OBJECT, object_type)
@@ -2146,7 +2160,8 @@ local function grant(uid, name, privilege, object_type,
if privilege_hex ~= old_privilege then
_priv:replace{options.grantor, uid, object_type, oid, privilege_hex}
elseif not options.if_not_exists then
- if object_type == 'role' then
+ if object_type == 'role' and object_name ~= nil and
+ object_name ~= 0 and privilege == 'execute' then
box.error(box.error.ROLE_GRANTED, name, object_name)
else
box.error(box.error.PRIV_GRANTED, name, privilege,
diff --git a/src/box/schema.h b/src/box/schema.h
index f1735ff34..c343650de 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -240,6 +240,8 @@ 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 role[BOX_USER_MAX];
struct access sequence[BOX_USER_MAX];
};
@@ -257,6 +259,12 @@ entity_access_get(enum schema_object_type type)
case SC_FUNCTION:
case SC_ENTITY_FUNCTION:
return entity_access.function;
+ case SC_USER:
+ case SC_ENTITY_USER:
+ return entity_access.user;
+ case SC_ROLE:
+ case SC_ENTITY_ROLE:
+ return entity_access.role;
case SC_SEQUENCE:
case SC_ENTITY_SEQUENCE:
return entity_access.sequence;
diff --git a/src/box/user.cc b/src/box/user.cc
index eec785652..b4fb65a59 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -217,6 +217,16 @@ access_find(struct priv_def *priv)
access = entity_access.function;
break;
}
+ case SC_ENTITY_USER:
+ {
+ access = entity_access.user;
+ break;
+ }
+ case SC_ENTITY_ROLE:
+ {
+ access = entity_access.role;
+ break;
+ }
case SC_ENTITY_SEQUENCE:
{
access = entity_access.sequence;
@@ -236,6 +246,16 @@ access_find(struct priv_def *priv)
access = func->access;
break;
}
+ case SC_USER:
+ {
+ /* No grants on a single object user yet. */
+ break;
+ }
+ case SC_ROLE:
+ {
+ /* No grants on a single object role yet. */
+ break;
+ }
case SC_SEQUENCE:
{
struct sequence *seq = sequence_by_id(priv->object_id);
@@ -318,7 +338,7 @@ user_reload_privs(struct user *user)
* Skip role grants, we're only
* interested in real objects.
*/
- if (priv.object_type != SC_ROLE)
+ if (priv.object_type != SC_ROLE || !(priv.access & PRIV_X))
user_grant_priv(user, &priv);
}
}
diff --git a/test/box/access.result b/test/box/access.result
index 30fcb6455..7acd6fa43 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -878,8 +878,6 @@ box.schema.user.create('test')
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')
---
...
@@ -1427,16 +1425,18 @@ box.session.su("admin")
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')
+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')
---
...
--- 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')
---
...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 9ae0e1114..9b7510e64 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -341,8 +341,7 @@ c:close()
session = box.session
box.schema.user.create('test')
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')
@@ -538,14 +537,10 @@ box.session.su("admin")
-- tables from ddl
--
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', '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
diff --git a/test/box/access_misc.result b/test/box/access_misc.result
index 2d87fa2d5..b7f0b7031 100644
--- a/test/box/access_misc.result
+++ b/test/box/access_misc.result
@@ -361,7 +361,7 @@ testuser_uid = session.uid()
...
_ = box.space._user:delete(2)
---
-- error: Drop access to user 'public' is denied for user 'testuser'
+- error: Drop access to role 'public' is denied for user 'testuser'
...
box.space._user:select(1)
---
diff --git a/test/box/role.result b/test/box/role.result
index 806cea90b..243c7bc6c 100644
--- a/test/box/role.result
+++ b/test/box/role.result
@@ -214,7 +214,22 @@ box.schema.role.drop('test')
box.schema.user.grant('grantee', 'liaison')
---
...
-box.schema.user.grant('test', 'read,write,create', 'universe')
+box.schema.user.grant('test', 'read,write', 'space', '_priv')
+---
+...
+box.schema.user.grant('test', 'write', 'space', '_schema')
+---
+...
+box.schema.user.grant('test', 'create', 'space')
+---
+...
+box.schema.user.grant('test', 'read,write', 'space', '_space')
+---
+...
+box.schema.user.grant('test', 'write', 'space', '_index')
+---
+...
+box.schema.user.grant('test', 'read', 'space', '_user')
---
...
box.session.su('test')
@@ -635,7 +650,13 @@ box.schema.user.create('user')
box.schema.user.create('grantee')
---
...
-box.schema.user.grant('user', 'read,write,execute,create', 'universe')
+box.schema.user.grant('user', 'read,write', 'space', '_user')
+---
+...
+box.schema.user.grant('user', 'read,write', 'space', '_priv')
+---
+...
+box.schema.user.grant('user', 'create', 'role')
---
...
box.session.su('user')
diff --git a/test/box/role.test.lua b/test/box/role.test.lua
index e97339f49..9845f4c4c 100644
--- a/test/box/role.test.lua
+++ b/test/box/role.test.lua
@@ -69,7 +69,13 @@ box.schema.role.revoke('test', 'liaison')
box.schema.role.drop('test')
box.schema.user.grant('grantee', 'liaison')
-box.schema.user.grant('test', 'read,write,create', 'universe')
+
+box.schema.user.grant('test', 'read,write', 'space', '_priv')
+box.schema.user.grant('test', 'write', 'space', '_schema')
+box.schema.user.grant('test', 'create', 'space')
+box.schema.user.grant('test', 'read,write', 'space', '_space')
+box.schema.user.grant('test', 'write', 'space', '_index')
+box.schema.user.grant('test', 'read', 'space', '_user')
box.session.su('test')
s = box.schema.space.create('test')
_ = s:create_index('i1')
@@ -248,7 +254,9 @@ box.schema.role.drop("role10")
box.schema.user.create('user')
box.schema.user.create('grantee')
-box.schema.user.grant('user', 'read,write,execute,create', 'universe')
+box.schema.user.grant('user', 'read,write', 'space', '_user')
+box.schema.user.grant('user', 'read,write', 'space', '_priv')
+box.schema.user.grant('user', 'create', 'role')
box.session.su('user')
box.schema.role.create('role')
box.session.su('admin')
--
2.15.2 (Apple Git-101.1)
More information about the Tarantool-patches
mailing list