From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Wed, 22 Aug 2018 15:48:17 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [tarantool-patches] [PATCH 4/4] Add a privilege upgrade script
 and update tests.
Message-ID: <20180822124817.itomvbpuyjxnri5a@esperanza>
References: <cover.1534751862.git.sergepetrenko@tarantool.org>
 <29cee395a9c98d53f81fe9facd470b7842a6a0a5.1534751862.git.sergepetrenko@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <29cee395a9c98d53f81fe9facd470b7842a6a0a5.1534751862.git.sergepetrenko@tarantool.org>
To: Serge Petrenko <sergepetrenko@tarantool.org>
Cc: kostja@tarantool.org, tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Mon, Aug 20, 2018 at 11:10:08AM +0300, Serge Petrenko wrote:
> This patch adds a privilege upgrade script, which runs on upgrade to
> 1.10.2 and automatically grants CREATE,ALTER,DROP on objects and entities
> to all users, who have READ and WRITE access on them.

You modify the upgrade script, but not access_check_ddl() - the latter
will still treat read,write as create,alter,drop. Now, suppose the user
will grant read,write to universe *after* upgrading to 1.10.2.
Everything's going to work fine until we disable legacy mode and start
claiming that create,alter,drop are set explicitly. And there will be no
upgrade script to fix that. That said, if you change the upgrade script
you should also remove the legacy mode from access_check_ddl.

> Also all tests are rewritten to grant only necessary privileges, not
> privileges to universe.

AFAIU the tests you're patching have nothing to do with this particular
problem. Please submit them separately.

> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index 091da2dc4..d8d03288c 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -983,8 +983,31 @@ local function upgrade_space_priv_to_1_10_2()
>      _vpriv.index.object:alter{parts={3, 'string', 4, 'scalar'}}
>  end
>  
> +local function upgrade_users_to_1_10_2()

I don't think it's worth factoring this code out to a separate function.
Let's just fold it into upgrade_priv_to_1_10_2, because after all it's
about updating privileges.

> +    local _priv = box.space[box.schema.PRIV_ID]
> +    local _user = box.space[box.schema.USER_ID]
> +

Please add a comprehensive comment explaining what you're doing here
and why. Don't forget to mention why you need to handle 'universe' in
a special way.

> +    for _, user in _user:pairs() do
> +        if user[0] ~= ADMIN and user[0] ~= SUPER then
> +            for _, priv in _priv:pairs(user[0]) do
> +                if bit.band(priv[5], box.priv.W) ~= 0 and
> +                bit.band(priv[5], box.priv.R) ~= 0 then
> +                    local new_privs = bit.bor(box.priv.A, box.priv.D)
> +                    -- for universal grants

This particular comment is useless. Please remove.

> +                    if priv[3] == 'universe' then
> +                        new_privs = bit.bor(new_privs, box.priv.C)
> +                    end
> +                    _priv:update({priv[2], priv[3], priv[4]},
> +                                 {{ "|", 5, new_privs}})
> +                end
> +            end
> +        end
> +    end
> +end
> +
>  local function upgrade_to_1_10_2()
>      upgrade_space_priv_to_1_10_2()
> +    upgrade_users_to_1_10_2()
>  end
>  
>  local function get_version()