[tarantool-patches] Re: [PATCH v2] Introduce separate entity object types for entity privileges.

  • From: Vladimir Davydov <vdavydov.dev@xxxxxxxxx>
  • To: Serge Petrenko <sergepetrenko@xxxxxxxxxxxxx>
  • Date: Fri, 17 Aug 2018 12:19:31 +0300

On Tue, Aug 14, 2018 at 04:41:25PM +0300, Serge Petrenko wrote:

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 3007a131d..7d65cab3a 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2537,10 +2537,41 @@ priv_def_create_from_tuple(struct priv_def *priv, 
struct tuple *tuple)
 {
      priv->grantor_id = tuple_field_u32_xc(tuple, BOX_PRIV_FIELD_ID);
      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);
+
+     const char *object_id = tuple_field(tuple, BOX_PRIV_FIELD_OBJECT_ID);

This isn't object_id. It's msgpack. Please rename this variable to
emphasize that. For instance, 'object_id_field', 'object_id_raw', or
simply 'data'.

+     if (object_id == NULL) {
+         tnt_raise(ClientError, ER_NO_SUCH_FIELD,
+                   BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE);
+     }
+     /*
+      * 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) {
+             object_id = tuple_field_cstr_xc(tuple, 
BOX_PRIV_FIELD_OBJECT_ID);
+             if(*object_id == '*' && strlen(object_id) == 1) {

Nit: in a case like this, better use strcmp.

+                     priv->object_id = 0;
+                     priv->object_type = 
schema_entity_type(priv->object_type);
+             } else {
+                     tnt_raise(ClientError, ER_FIELD_TYPE,
+                               BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE,
+                               field_type_strs[FIELD_TYPE_UNSIGNED]);
+             }
+     } else if (mp_typeof(*object_id) == MP_UINT) {
+             priv->object_id = tuple_field_u32_xc(tuple,
+                                                  BOX_PRIV_FIELD_OBJECT_ID);
+     } else {
+             tnt_raise(ClientError, ER_FIELD_TYPE,
+                       BOX_PRIV_FIELD_OBJECT_ID + TUPLE_INDEX_BASE,
+                       field_type_strs[FIELD_TYPE_UNSIGNED]);
+     }
+

Somehow, I don't like this piece of code. For instance, you raise
the same error (FIELD_TYPE_UNSIGNED) in two places. Also, you use
tuple_field_*_xc after you explicitly check the type, which is
pointless, because they can't fail.

May be, something like this?

        switch (mp_typeof(*data)) {
        case MP_STR:
                if (mp_decode_strl(&data) == 0) {
                        /* Entity-wide privilege. */
                        priv->object_id = 0;
                        break;
                }
                FALLTHROUGH;
        default:
                priv->object_id = tuple_field_u32_xc(...);
        }

      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..361810c79 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1801,17 +1801,19 @@ local function privilege_name(privilege)
 end
 
 local function object_resolve(object_type, object_name)
+    if object_name ~= nil and type(object_name) ~= 'string'
+            and type(object_name) ~= 'number' then
+        box.error(box.error.ILLEGAL_PARAMS, "wrong object name type")
+    end
     if object_type == 'universe' then
-        if object_name ~= nil and type(object_name) ~= 'string'
-                and type(object_name) ~= 'number' then
-            box.error(box.error.ILLEGAL_PARAMS, "wrong object name type")
-        end
         return 0
     end
+    if (object_type == 'space' or object_type == 'function' or
+       object_type == 'sequence' or object_type == 'role') and
+       object_name == nil then
+        return '*'
+    end

No, I don't like this: now, if you want to add a new object type,
you have to patch two places: here and below, where the object name
is resolved. Let's leave it as before.

     if object_type == 'space' then
-        if object_name == nil or object_name == 0 then
-            return 0
-        end
         local space = box.space[object_name]
         if  space == nil then
             box.error(box.error.NO_SUCH_SPACE, object_name)
@@ -1819,9 +1821,6 @@ 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
         local _vfunc = box.space[box.schema.VFUNC_ID]
         local func
         if type(object_name) == 'string' then
@@ -1836,9 +1835,6 @@ 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
-        end
         local seq = sequence_resolve(object_name)
         if seq == nil then
             box.error(box.error.NO_SUCH_SEQUENCE, object_name)
@@ -1867,6 +1863,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
@@ -2106,10 +2105,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 '*')
             end
     end
 end
@@ -2134,10 +2133,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 +2152,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 '*')
     end
     if privilege_hex ~= 0 then
         _priv:replace{grantor, uid, object_type, oid, privilege_hex}
     else
         _priv:delete{uid, object_type, oid}
     end
+

Extra new line, please remove.

 end
 
 local function drop(uid, opts)
@@ -2192,9 +2192,10 @@ local function drop(uid, opts)
     local privs = _vpriv.index.primary:select{uid}
 
     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])
+        -- we need an additional box.session.su() here, because of
+        -- unnecessary check for privilege PRIV_REVOKE in priv_def_check()
+        local oid = tuple[4] ~= '*' and tuple[4] or nil
+        box.session.su("admin", revoke, uid, uid, tuple[5], tuple[3], oid)
     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..5dbc09bbb 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -964,6 +964,28 @@ 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_to_1_10_1()
+    upgrade_space_priv_to_1_10_1()
+end
 
 local function get_version()
     local version = box.space._schema:get{'version'}
@@ -991,6 +1013,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},

Should be 1.10.2

1.10.1 has already been released.

     }
 
     for _, handler in ipairs(handlers) do
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 433f52c08..9958e9016 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -536,10 +536,18 @@ schema_find_name(enum schema_object_type type, uint32_t 
object_id)
      switch (type) {
      case SC_UNIVERSE:
              return "";
+     case SC_ENTITY_SPACE:
+             return "";
+     case SC_ENTITY_FUNCTION:
+             return "";
+     case SC_ENTITY_SEQUENCE:
+             return "";
+     case SC_ENTITY_ROLE:
+             return "";
+     case SC_ENTITY_USER:
+             return "";

Please rewrite it as

        case SC_ENTITY_SPACE:
        case SC_ENTITY_FUNCTION:
        case SC_ENTITY_SEQUENCE:
        ...
                return "";

Also, I don't like that sometimes we use '*' and sometimes '' when
reporting errors. What about using '' for entity-wide privileges
everywhere, including the _priv space? This would save us from
possible conflicts with objects named '*' (yeah, no sane user would
call a space like that, but still). This would also allow us to
simplify object_resolve() as I suggested in the previous review
round.

      case SC_SPACE:
              {
-                     if (object_id == 0)
-                             return "SPACE";
                      struct space *space = space_by_id(object_id);
                      if (space == NULL)
                              break;
@@ -547,8 +555,6 @@ schema_find_name(enum schema_object_type type, uint32_t 
object_id)
              }
      case SC_FUNCTION:
              {
-                     if (object_id == 0)
-                             return "FUNCTION";
                      struct func *func = func_by_id(object_id);
                      if (func == NULL)
                              break;
@@ -556,8 +562,6 @@ schema_find_name(enum schema_object_type type, uint32_t 
object_id)
              }
      case SC_SEQUENCE:
              {
-                     if (object_id == 0)
-                             return "SEQUENCE";
                      struct sequence *seq = sequence_by_id(object_id);
                      if (seq == NULL)
                              break;
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..18ec6c8d2 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]        = */ "space",
+     /* [SC_FUNCTION]            = */ "function",
+     /* [SC_ENTITY_FUNCTION]     = */ "function",
+     /* [SC_USER]                = */ "user",
+     /* [SC_ENTITY_USER]         = */ "user",
+     /* [SC_ROLE]                = */ "role",
+     /* [SC_ENTITY_ROLE]         = */ "role",
+     /* [SC_SEQUENCE]            = */ "sequence",
+     /* [SC_ENTITY_SEQUENCE]     = */ "sequence",
+     /* [SC_COLLATION]           = */ "collation",
+     /* [SC_ENTITY_COLLATION]    = */ "collation",

I don't like this code duplication. Actually, I don't think that you
need to have separate names for entity-wide privileges at all: AFAICS,
schema_object_name() is never called for SC_ENTITY_*. Let's remove
SC_ENTITY_* from this array.

If you do that, you won't be able to increment object type to get entity
type. Well, OK, it's not that scary, taking into account the fact that
you always use a helper function for that (see right below). I see it
that way

enum schema_object_type {
        SC_UNKNOWN = 0,
        SC_UNIVERSE = 1,
        SC_SPACE = 2,
        SC_FUNCTION = 3,
        SC_USER = 4,
        SC_ROLE = 5,
        SC_SEQUENCE = 6,
        SC_COLLATION = 7,
        schema_object_type_MAX = 8

        /* Entity types. */
        SC_ENTITY_SPACE,
        SC_ENTITY_FUNCTION,
        ...
}

enum schema_object_type
schema_entity_type(enum schema_object_type type)
{
        switch (type) {
        case SC_SPACE:
                return SC_ENTITY_SPACE;
        case SC_FUNCTION:
                return SC_ENTITY_FUNCTION;
        ...
        }
}

 };
 
+enum schema_object_type
+schema_entity_type(enum schema_object_type type)
+{
+     switch(type) {

             ^^
         space missing

+     case SC_SPACE:
+     case SC_FUNCTION:
+     case SC_USER:
+     case SC_ROLE:
+     case SC_SEQUENCE:
+     case SC_COLLATION:
+             return type + 1;
+             break;
+     default:
+             unreachable();
+     }
+}
+
 enum schema_object_type
 schema_object_type(const char *name)
 {
diff --git a/src/box/schema_def.h b/src/box/schema_def.h
index 2edb8d37f..9b5bd6864 100644
--- a/src/box/schema_def.h
+++ b/src/box/schema_def.h
@@ -218,19 +218,35 @@ enum {
  *
  * Use 0 for unknown to use the same index consistently
  * even when there are more object types in the future.
+ *
+ * When adding new types please follow this rule:
+ * SC_ENTITY_NEW_OBJECT = SC_NEW_OBJECT + 1
+ * schema_entity_type() relies on this convention.
  */
 enum schema_object_type {
      SC_UNKNOWN = 0,
      SC_UNIVERSE = 1,
      SC_SPACE = 2,
-     SC_FUNCTION = 3,
-     SC_USER = 4,
-     SC_ROLE = 5,
-     SC_SEQUENCE = 6,
-     SC_COLLATION = 7,
-     schema_object_type_MAX = 8
+     SC_ENTITY_SPACE = 3,
+     SC_FUNCTION = 4,
+     SC_ENTITY_FUNCTION = 5,
+     SC_USER = 6,
+     SC_ENTITY_USER = 7,
+     SC_ROLE = 8,
+     SC_ENTITY_ROLE = 9,
+     SC_SEQUENCE = 10,
+     SC_ENTITY_SEQUENCE = 11,
+     SC_COLLATION = 12,
+     SC_ENTITY_COLLATION = 13,
+     schema_object_type_MAX = 13
 };

Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 08-2018