Hi, Oleksandr,
On Sep 04, Oleksandr Byelkin wrote:
> revision-id: 44e692d0dbc (mariadb-11.0.1-228-g44e692d0dbc)
> parent(s): 9cb75f333fa
> author: Oleksandr Byelkin
> committer: Oleksandr Byelkin
> timestamp: 2023-09-04 14:12:29 +0200
> message:
>
> MDEV-29167 new db-level SHOW CREATE ROUTINE privilege
>
> -- Create general_log if CSV is enabled.
> diff --git a/scripts/mariadb_system_tables_fix.sql b/scripts/mariadb_system_tables_fix.sql
> --- a/scripts/mariadb_system_tables_fix.sql
> +++ b/scripts/mariadb_system_tables_fix.sql
> @@ -706,6 +706,9 @@ ALTER TABLE db modify Delete_history_priv enum('N','Y') COLLATE utf8mb3_general_
>
> UPDATE user SET Delete_history_priv = Super_priv WHERE @had_user_delete_history_priv = 0;
>
> +ALTER TABLE db ADD Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N' AFTER Delete_history_priv;
> +ALTER TABLE db modify Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N';
Why two statements?
I also wonder about this pattern, but then I think I got the idea, 1 add the field if it was absolutely absent, 2. fix properties if they were incorrect. But mostly I follow the pattern of other fields adding.
> +
> ALTER TABLE user ADD plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL AFTER max_user_connections,
> ADD authentication_string TEXT NOT NULL AFTER plugin;
> ALTER TABLE user CHANGE auth_string authentication_string TEXT NOT NULL;
> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> --- a/sql/sql_acl.cc
> +++ b/sql/sql_acl.cc
> @@ -9003,7 +9008,7 @@ bool check_routine_level_acl(THD *thd, const char *db, const char *name,
> NULL, db,
> sctx->priv_role,
> name, sph, 0)))
> - no_routine_acl= !(grant_proc->privs & SHOW_PROC_ACLS);
> + no_routine_acl= !(grant_proc->privs & SHOW_PROC_NO_DEF_ACLS);
Oh, now I get it. All the time up until this line I thought that
SHOW_PROC_NO_DEF_ACLS means "privileges that *not a definer* of the
routine needs to have"
And now I see that it means to "show procedure without definition".
Please, rename to something, like, SHOW_PROC_EXISTANCE or
SHOW_PROC_NO_BODY or even SHOW_PROC_WITHOUT_DEFINITION. But
SHOW_PROC_NO_BODY was my first idea, but BODY used in other sens (package for example) so I will go with
SHOW_PROC_WITHOUT_DEFINITION
SHOW_PROC_NO_DEF is rather misleading.
> }
> mysql_rwlock_unlock(&LOCK_grant);
> return no_routine_acl;
> @@ -9237,7 +9242,7 @@ static const char *command_array[]=
> "CREATE USER", "EVENT", "TRIGGER", "CREATE TABLESPACE", "DELETE HISTORY",
> "SET USER", "FEDERATED ADMIN", "CONNECTION ADMIN", "READ_ONLY ADMIN",
> "REPLICATION SLAVE ADMIN", "REPLICATION MASTER ADMIN", "BINLOG ADMIN",
> - "BINLOG REPLAY", "SLAVE MONITOR"
> + "BINLOG REPLAY", "SLAVE MONITOR", "SHOW CREATE ROUTINE"
> };
>
> static uint command_lengths[]=
> @@ -9250,7 +9255,7 @@ static uint command_lengths[]=
> 11, 5, 7, 17, 14,
> 8, 15, 16, 15,
> 23, 24, 12,
> - 13, 13
> + 13, 13, 19
May be you'll change these two arrays into one of LEX_CSTRING's ?
in a separate commit, of course.
OK
> };
>
> diff --git a/sql/sql_show.cc b/sql/sql_show.cc
> --- a/sql/sql_show.cc
> +++ b/sql/sql_show.cc
> @@ -6676,7 +6677,8 @@ int store_schema_params(THD *thd, TABLE *table, TABLE *proc_table,
> DBUG_RETURN(0);
>
> if (!full_access)
> - full_access= !strcmp(sp_user, definer.str);
> + full_access= !check_db_routine_access(thd, SHOW_CREATE_ROUTINE_ACL,
> + db.str, name.str, sph, TRUE);
Wait, I thought we agreed that to see a routine body one has to
be either the definer or granted SHOW CREATE ROUTINE.
As far as I can see check_db_routine_access doesn't check for the
user being the definer.
yes, I have just accidently lost the condition (will add also tests for it)
> if (!full_access &&
> check_some_routine_access(thd, db.str, name.str, sph))
> DBUG_RETURN(0);
Regards,
Sergei
VP of MariaDB Server Engineering
and security@mariadb.org