7 авг. 2018 г., в 19:38, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):

On Thu, Aug 02, 2018 at 01:55:58PM +0300, Serge Petrenko wrote:

When granting or revoking a privilege on an entire entity, id 0 was used
to indicate the fact that we don't grant a privilege on a single object,
but on a whole entity. This caused confusion, because for entity USER,
for example, id 0 is a valid object id (user 'guest' uses it).
Any non-zero id dedicated to this cause obviously may be confused as well.
Fix this by creating separate schema_object_types for entities:
SC_ENTITY_SPACE, SC_ENTITY_USER, etc.

Closes: #3574
Prerequisite: #3524
---
https://github.com/tarantool/tarantool/tree/sergepetrenko/gh-3574-whole-entity-types
https://github.com/tarantool/tarantool/issues/3574

Changes in v2: 
 - keep only old syntax for granting access
   to all entities
 - add an upgrade script to alter indices of spaces
   _priv and _vpriv to store 'scalar' in object_id field,
   and use an asterisk ('*') in object_id to indicate
   granting on an entire entity.

Hmm, I wonder what happens if someone creates an object (say space) and
names it "*» …

 - keep the new entity types in priv_def and use them
   internally.

src/box/alter.cc             |  27 ++++++++++++++++++++++++++-
src/box/bootstrap.snap       | Bin 1540 -> 1557 bytes
src/box/lua/schema.lua       |  41 ++++++++++++++++++++++++-----------------
src/box/lua/upgrade.lua      |  37 +++++++++++++++++++++++++++++++++++++
src/box/schema.cc            |  18 ++++++++++++++++++
src/box/schema.h             |  23 +++++++++++++----------
src/box/schema_def.c         |  39 +++++++++++++++++++++++++++++++--------
src/box/schema_def.h         |  28 ++++++++++++++++++++++------
src/box/user.cc              |  27 +++++++++++++++------------
test/box-py/bootstrap.result |  14 +++++++-------
test/box/access.result       |   6 +++---
test/box/access_misc.result  |   8 ++++----
test/box/alter.result        |   8 ++++----
test/xlog/upgrade.result     |  14 +++++++-------
14 files changed, 211 insertions(+), 79 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 3007a131d..6057b66c9 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2529,6 +2529,31 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
}
}

+void
+priv_def_try_fill(struct priv_def *priv, struct tuple *tuple)

This function should be static. Anyway, I don't think it's worth
factoring this code out, because it's pretty straightforward and
executed only in priv_def_create_form_tuple().

+{
+ const char *object_id = tuple_field(tuple, BOX_PRIV_FIELD_OBJECT_ID);
+ if (object_id == NULL) {
+ tnt_raise(ClientError, ER_NO_SUCH_FIELD,
+   BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE);
+ }

Please use tuple_field_u32() and tuple_field_str() instead.

+ /*
+  * When granting or revoking privileges on a whole entity we
+  * pass an asterisk ('*') to object_id to indicate grant on every
+  * object of that entity. So check for that first.
+  */
+ if (mp_typeof(*object_id) == MP_STR) {

Shouldn't you check that the string is actually "*»?

+ priv->object_id = 0;
+ priv->object_type = schema_entity_type(priv->object_type);

This assumes that priv->object_type is initialized before
priv_def_try_fill(). IMO this makes the function protocol
rather abstruse. Another reason not to factor it out.

+ } else if (mp_typeof(*object_id) == MP_UINT) {
+ priv->object_id = mp_decode_uint(&object_id);
+ } else {
+ tnt_raise(ClientError, ER_FIELD_TYPE,
+   BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE,
+   field_type_strs[FIELD_TYPE_UNSIGNED]);
+ }
+}
+
/**
 * Create a privilege definition from tuple.
 */
@@ -2539,8 +2564,8 @@ priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple)
priv->grantee_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_UID);
const char *object_type =
tuple_field_cstr_xc(tuple, BOX_PRIV_FIELD_OBJECT_TYPE);
- priv->object_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_OBJECT_ID);
priv->object_type = schema_object_type(object_type);
+ priv_def_try_fill(priv, tuple);
if (priv->object_type == SC_UNKNOWN) {
tnt_raise(ClientError, ER_UNKNOWN_SCHEMA_OBJECT,
  object_type);
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index b9b8c9004..294dcf4d3 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1809,9 +1809,9 @@ local function object_resolve(object_type, object_name)
        return 0
    end
    if object_type == 'space' then
-        if object_name == nil or object_name == 0 then
-            return 0
-        end
+ if object_name == nil then
+     return '*'
+ end

Bad indentation - we use spaces in Lua code, not tabs.

        local space = box.space[object_name]
        if  space == nil then
            box.error(box.error.NO_SUCH_SPACE, object_name)
@@ -1819,9 +1819,9 @@ local function object_resolve(object_type, object_name)
        return space.id
    end
    if object_type == 'function' then
-        if object_name == nil or object_name == 0 then
-            return 0
-        end
+ if object_name == nil then
+     return '*'
+ end

Ditto

        local _vfunc = box.space[box.schema.VFUNC_ID]
        local func
        if type(object_name) == 'string' then
@@ -1836,8 +1836,8 @@ local function object_resolve(object_type, object_name)
        end
    end
    if object_type == 'sequence' then
-        if object_name == nil or object_name == 0 then
-            return 0
+        if object_name == nil then
+            return '*'
        end
        local seq = sequence_resolve(object_name)
        if seq == nil then
@@ -1846,6 +1846,9 @@ local function object_resolve(object_type, object_name)
        return seq
    end
    if object_type == 'role' then
+ if object_name == nil then
+     return '*'
+ end

Ditto.

        local _vuser = box.space[box.schema.VUSER_ID]
        local role
        if type(object_name) == 'string' then
@@ -1867,6 +1870,9 @@ local function object_name(object_type, object_id)
    if object_type == 'universe' then
        return ""
    end
+    if object_id == '*' then
+        return '*'
+    end
    local space
    if object_type == 'space' then
        space = box.space._vspace
@@ -2079,9 +2085,8 @@ local function grant(uid, name, privilege, object_type,
        object_name = privilege
        privilege = 'execute'
    end
-    local privilege_hex = privilege_check(privilege, object_type)
-
    local oid = object_resolve(object_type, object_name)
+    local privilege_hex = privilege_check(privilege, object_type)

I don't understand why you need this.

    options = options or {}
    if options.grantor == nil then
        options.grantor = session.euid()
@@ -2106,10 +2111,10 @@ local function grant(uid, name, privilege, object_type,
        _priv:replace{options.grantor, uid, object_type, oid, privilege_hex}
    elseif not options.if_not_exists then
            if object_type == 'role' then
-                box.error(box.error.ROLE_GRANTED, name, object_name)
+                box.error(box.error.ROLE_GRANTED, name, object_name or '*')
            else
                box.error(box.error.PRIV_GRANTED, name, privilege,
-                          object_type, object_name)
+                          object_type, object_name or '*')

Instead you could set object_name to '*' in the beginning of
grant/revoke functions. This would make your patch shorter,
because you wouldn't have to do it here, nor in object_resolve.

            end
    end
end
@@ -2122,9 +2127,9 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
        object_name = privilege
        privilege = 'execute'
    end
-    local privilege_hex = privilege_check(privilege, object_type)
    options = options or {}
    local oid = object_resolve(object_type, object_name)
+    local privilege_hex = privilege_check(privilege, object_type)

I don't understand why you need this.

    local _priv = box.space[box.schema.PRIV_ID]
    local _vpriv = box.space[box.schema.VPRIV_ID]
    local tuple = _vpriv:get{uid, object_type, oid}
@@ -2134,10 +2139,10 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
            return
        end
        if object_type == 'role' then
-            box.error(box.error.ROLE_NOT_GRANTED, name, object_name)
+            box.error(box.error.ROLE_NOT_GRANTED, name, object_name or '*')
        else
            box.error(box.error.PRIV_NOT_GRANTED, name, privilege,
-                      object_type, object_name)
+                      object_type, object_name or '*')
        end
    end
    local old_privilege = tuple[5]
@@ -2153,13 +2158,14 @@ local function revoke(uid, name, privilege, object_type, object_name, options)
            return
        end
        box.error(box.error.PRIV_NOT_GRANTED, name, privilege,
-                  object_type, object_name)
+                  object_type, object_name or '*')

This hunk as well as the one above wouldn't be needed if you set
object_name to '*' in the beginning of the function (also, see my
comment to grant function).

    end
    if privilege_hex ~= 0 then
        _priv:replace{grantor, uid, object_type, oid, privilege_hex}
    else
        _priv:delete{uid, object_type, oid}
    end
+

Extraneous change. Please remove.

end

local function drop(uid, opts)
@@ -2194,7 +2200,8 @@ local function drop(uid, opts)
    for k, tuple in pairs(privs) do
-- we need an additional box.session.su() here, because of
-- unnecessary check for privilege PRIV_REVOKE in priv_def_check()
-        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], tuple[4])
+        local oid = tuple[4] ~= '*' and tuple[4] or nil
+        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], oid)

Why? The 'revoke' function should be able to interpret '*' correctly.

    end
    box.space[box.schema.USER_ID]:delete{uid}
end
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 0293f6ef8..4acf20152 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -964,6 +964,42 @@ local function upgrade_to_1_10_0()
    create_vsequence_space()
end

+--------------------------------------------------------------------------------
+--- Tarantool 1.10.1
+--------------------------------------------------------------------------------
+local function upgrade_space_priv_to_1_10_1()
+    local _priv = box.space._priv
+    local _vpriv = box.space._vpriv
+    local f = _priv:format()
+
+    f[4].type = 'scalar'
+    _priv:format(f)
+    f = _vpriv:format()
+    f[4].type = 'scalar'
+    _vpriv:format(f)
+    _priv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
+    _vpriv.index.primary:alter{parts={2, 'unsigned', 3, 'string', 4, 'scalar'}}
+    _priv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
+    _vpriv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
+end
+
+local function upgrade_privs_to_1_10_1()

No need to split this code in two functions.

+    local _priv = box.space._priv
+
+    for _, priv in _priv:pairs() do

AFAICS interpretation of '0' as 'all objects of a kind' was added after
1.10.1 i.e. there was no Tarantool release that would use this feature.
So I guess you don't need to upgrade existing privileges, neither do you
need to preserve any code that handles '0' object id. Please consult
with Georgy and Kostja regarding this.

+        if priv[4] == 0 then
+            if priv[3] ~= 'universe' and priv[3] ~= 'user' and priv[3] ~= 'role' then

Please rewrite it as

 if priv[4] == 0 and priv[3] ~= 'universe' and ... then

+                _priv:delete{priv[2], priv[3], priv[4]}
+ _priv:insert{priv[1], priv[2], priv[3], '*', priv[5]}

Bad indentation - we use spaces in Lua code, not tabs.

+            end
+        end
+    end
+end
+
+local function upgrade_to_1_10_1()
+    upgrade_space_priv_to_1_10_1()
+    upgrade_privs_to_1_10_1()
+end

local function get_version()
    local version = box.space._schema:get{'version'}
@@ -991,6 +1027,7 @@ local function upgrade(options)
        {version = mkversion(1, 7, 6), func = upgrade_to_1_7_6, auto = false},
        {version = mkversion(1, 7, 7), func = upgrade_to_1_7_7, auto = true},
        {version = mkversion(1, 10, 0), func = upgrade_to_1_10_0, auto = true},
+        {version = mkversion(1, 10, 1), func = upgrade_to_1_10_1, auto = true},
    }

    for _, handler in ipairs(handlers) do
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 433f52c08..02f55aaad 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -536,8 +536,26 @@ schema_find_name(enum schema_object_type type, uint32_t object_id)
switch (type) {
case SC_UNIVERSE:
return "";
+ case SC_ENTITY_SPACE:
+ return "SPACE";
+ case SC_ENTITY_FUNCTION:
+ return "FUNCTION";
+ case SC_ENTITY_SEQUENCE:
+ return "SEQUENCE";
+ case SC_ENTITY_ROLE:
+ return "ROLE";
+ case SC_ENTITY_USER:
+ return "USER";

Shouldn't it be "*»?

case SC_SPACE:
{
+ /*
+  * Even though we have a separate type
+  * for grants on entire entity, we have to
+  * leave the check for an empty object_id in place,
+  * because without it recover from an old version WAL 
+  * fails even before the upgrade script is being run.
+  * Same applies to SC_FUNCTION and SC_SEQUENCE below.
+  */
if (object_id == 0)
return "SPACE";
struct space *space = space_by_id(object_id);
diff --git a/src/box/schema.h b/src/box/schema.h
index 0822262d0..f1735ff34 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -250,16 +250,19 @@ static inline
struct access *
entity_access_get(enum schema_object_type type)
{
-       switch (type) {
-       case SC_SPACE:
-               return entity_access.space;
-       case SC_FUNCTION:
-               return entity_access.function;
-       case SC_SEQUENCE:
-               return entity_access.sequence;
-       default:
-               return NULL;
-       }
+ switch (type) {
+ case SC_SPACE:
+ case SC_ENTITY_SPACE:
+ return entity_access.space;
+ case SC_FUNCTION:
+ case SC_ENTITY_FUNCTION:
+ return entity_access.function;
+ case SC_SEQUENCE:
+ case SC_ENTITY_SEQUENCE:
+ return entity_access.sequence;
+ default:
+ return NULL;
+ }
}

#endif /* INCLUDES_TARANTOOL_BOX_SCHEMA_H */
diff --git a/src/box/schema_def.c b/src/box/schema_def.c
index 97c074ab2..51d418e42 100644
--- a/src/box/schema_def.c
+++ b/src/box/schema_def.c
@@ -31,16 +31,39 @@
#include "schema_def.h"

static const char *object_type_strs[] = {
- /* [SC_UKNNOWN]         = */ "unknown",
- /* [SC_UNIVERSE]        = */ "universe",
- /* [SC_SPACE]           = */ "space",
- /* [SC_FUNCTION]        = */ "function",
- /* [SC_USER]            = */ "user",
- /* [SC_ROLE]            = */ "role",
- /* [SC_SEQUENCE]        = */ "sequence",
- /* [SC_COLLATION]       = */ "collation",
+ /* [SC_UKNNOWN]     = */ "unknown",
+ /* [SC_UNIVERSE]     = */ "universe",
+ /* [SC_SPACE]     = */ "space",
+ /* [SC_ENTITY_SPACE]     = */ "all spaces",
+ /* [SC_FUNCTION]     = */ "function",
+ /* [SC_ENTITY_FUNCTION]     = */ "all functions",
+ /* [SC_USER]     = */ "user",
+ /* [SC_ENTITY_USER]     = */ "all users",
+ /* [SC_ROLE]     = */ "role",
+ /* [SC_ENTITY_ROLE]     = */ "all roles",
+ /* [SC_SEQUENCE]     = */ "sequence",
+ /* [SC_ENTITY_SEQUENCE]     = */ "all sequences",
+ /* [SC_COLLATION]     = */ "collation",
+ /* [SC_ENTITY_COLLATION]    = */ "all collations",

Why? I don't think that you need to report "all spaces" anywhere.