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)));
I fixed the other change, but the logic you propose here is slightly different. Your version will check all APPLICABLE_ROLES, not only ENABLED_ROLES. We need to look at the subgraph of thd->priv_role only, not those of priv_user@priv_host.
I've added an extra test case for this exact behaviour now too.
Regards,
Sergei
Chief Architect MariaDB
and security@mariadb.org