On Mon, 9 Oct 2017 at 20:45 Sergei Golubchik <[email protected]> wrote:
> 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 [email protected] >
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
