From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 7 Aug 2018 19:38:42 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH v2] Introduce separate entity object types for entity privileges. Message-ID: <20180807163842.agnqqk7gyua2vwzv@esperanza> References: <20180802105558.20488-1-sergepetrenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180802105558.20488-1-sergepetrenko@tarantool.org> To: Serge Petrenko Cc: tarantool-patches@freelists.org, kostja@tarantool.org List-ID: 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.