From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Serge Petrenko <sergepetrenko@tarantool.org>
Cc: tarantool-patches@freelists.org, kostja@tarantool.org
Subject: Re: [tarantool-patches] [PATCH v2] Introduce separate entity object types for entity privileges.
Date: Fri, 17 Aug 2018 12:19:31 +0300 [thread overview]
Message-ID: <20180817091931.r45zifiikkwzuycu@esperanza> (raw)
In-Reply-To: <305E5CD8-68DD-4897-9E77-347F290091A6@tarantool.org>
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
> };
next prev parent reply other threads:[~2018-08-17 9:19 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-02 10:55 Serge Petrenko
2018-08-07 16:38 ` Vladimir Davydov
2018-08-14 13:41 ` Serge Petrenko
2018-08-17 9:19 ` Vladimir Davydov [this message]
2018-08-17 12:19 ` [tarantool-patches] " Serge Petrenko
2018-08-17 15:57 ` Vladimir Davydov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180817091931.r45zifiikkwzuycu@esperanza \
--to=vdavydov.dev@gmail.com \
--cc=kostja@tarantool.org \
--cc=sergepetrenko@tarantool.org \
--cc=tarantool-patches@freelists.org \
--subject='Re: [tarantool-patches] [PATCH v2] Introduce separate entity object types for entity privileges.' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox