Tarantool development patches archive
 help / color / mirror / Atom feed
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.

  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