maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Vicentiu!

Looks good! A couple of comments, see below.
Ok to push after fixing, the second review is not needed.

On Oct 09, Vicentiu Ciorbaru wrote:
> revision-id: 46b93fc2a2fa198d721813d9b93bbe09d2e36eb3 (mariadb-10.0.32-50-g46b93fc)
> parent(s): dbeffabc83ed01112e09d7e782d44f044cfcb691
> author: Vicențiu Ciorbaru
> committer: Vicențiu Ciorbaru
> timestamp: 2017-10-09 13:32:40 +0300
> message:
> 
> MDEV-13676: Field "create Procedure" is NULL, even if the the user has role which is the definer. (SHOW CREATE PROCEDURE)
> 
> During show create procedure we ommited to check the current role, if it
> is the actual definer of the procedure. In addition, we should support
> indirectly granted roles to the current role. Implemented a recursive
> lookup to search the tree of grants if the rolename is present.

We both had to check the SQL standard to see what the behavior should
be. Please add here (in the commit comment) a reference to the exact
part of the standard that specifies this behavior. Like

  SQL Standard 2016, Part ... Section ... View I_S.ROUTINES selects
  ROUTINE_BODY and its WHERE clause says that the GRANTEE must be either
  PUBLIC, or CURRENT_USER or in the ENABLED_ROLES.

> diff --git a/sql/sp_head.cc b/sql/sp_head.cc
> index ea9e1c1..3dd1a65 100644
> --- a/sql/sp_head.cc
> +++ b/sql/sp_head.cc
> @@ -2588,10 +2588,18 @@ bool check_show_routine_access(THD *thd, sp_head *sp, bool *full_access)
>    *full_access= ((!check_table_access(thd, SELECT_ACL, &tables, FALSE,
>                                       1, TRUE) &&
>                    (tables.grant.privilege & SELECT_ACL) != 0) ||
> +                 /* Check if user owns the routine. */
>                   (!strcmp(sp->m_definer_user.str,
>                            thd->security_ctx->priv_user) &&
>                    !strcmp(sp->m_definer_host.str,
> -                          thd->security_ctx->priv_host)));
> +                          thd->security_ctx->priv_host)) ||
> +                 /* Check if current role or any of the sub-granted roles
> +                    own the routine. */
> +                 (sp->m_definer_host.length == 0 &&
> +                  (!strcmp(sp->m_definer_user.str,
> +                           thd->security_ctx->priv_role) ||
> +                   check_role_is_granted(thd->security_ctx->priv_role, NULL,
> +                                         sp->m_definer_user.str))));

Perhaps you could simplify the condition above a little bit,
by replacing it with

         /* Check if current role or any of the sub-granted roles
            own the routine. */
          (sp->m_definer_host.length == 0 &&
           check_role_is_granted(thd->security_ctx->priv_user,
                                 thd->security_ctx->priv_host,
                                 sp->m_definer_user.str)));

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx