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: Tue, 7 Aug 2018 19:38:42 +0300 [thread overview]
Message-ID: <20180807163842.agnqqk7gyua2vwzv@esperanza> (raw)
In-Reply-To: <20180802105558.20488-1-sergepetrenko@tarantool.org>
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.
next prev parent reply other threads:[~2018-08-07 16:38 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 [this message]
2018-08-14 13:41 ` Serge Petrenko
2018-08-17 9:19 ` Vladimir Davydov
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=20180807163842.agnqqk7gyua2vwzv@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