maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr,

> commit 5154e224a97
> Author: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> Date:   Mon Dec 13 16:15:21 2021 +0100
> 
> diff --git a/scripts/mysql_system_tables.sql b/scripts/mysql_system_tables.sql
> index fd2f1c95dda..a1721913b2c 100644
> --- a/scripts/mysql_system_tables.sql
> +++ b/scripts/mysql_system_tables.sql
> @@ -47,6 +47,10 @@ INSERT IGNORE INTO global_priv SELECT * FROM tmp_user_sys WHERE 0 <> @need_sys_u
>  DROP TABLE tmp_user_sys;
>  
>  
> +-- This special "role" needed for GRAND ... TO PUBLIC
> +INSERT IGNORE INTO global_priv (Host,User,Priv) VALUES ('', 'PUBLIC', concat('{"access":0,"version_id":',regexp_replace(regexp_replace(@@version, '\\b\\d\\b', '0\\0'), '\\D', ''),',"is_role":true}'));

1. why here and not in mysql_system_tables_data.sql ?
2. why this version magic? mysql_system_tables_data.sql doesn't do it for other accounts
3. why do you do it at all? I thought the server can create this role as needed

> +
> +
>  CREATE DEFINER='mariadb.sys'@'localhost' SQL SECURITY DEFINER VIEW IF NOT EXISTS user AS SELECT
>    Host,
>    User,
> @@ -95,7 +99,9 @@ CREATE DEFINER='mariadb.sys'@'localhost' SQL SECURITY DEFINER VIEW IF NOT EXISTS
>    ELT(IFNULL(JSON_VALUE(Priv, '$.is_role'), 0) + 1, 'N', 'Y') AS is_role,
>    IFNULL(JSON_VALUE(Priv, '$.default_role'), '') AS default_role,
>    CAST(IFNULL(JSON_VALUE(Priv, '$.max_statement_time'), 0.0) AS DECIMAL(12,6)) AS max_statement_time
> -  FROM global_priv;
> +  FROM global_priv
> +-- Do not show special role for GRANT TO PUBLIC
> +  WHERE not (Host = "" and User = "PUBLIC");

Why? I don't think it's wrong to show a new PUBLIC role.

>  
>  -- Remember for later if user table already existed
>  set @had_user_table= @@warning_count != 0;
> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> index 601cae2e945..aa9bd91ec52 100644
> --- a/sql/sql_acl.cc
> +++ b/sql/sql_acl.cc
> @@ -316,6 +318,13 @@ static bool show_table_and_column_privileges(THD *, const char *, const char *,
>  static int show_routine_grants(THD *, const char *, const char *,
>                                 const Sp_handler *sph, char *, int);
>  
> +static ACL_ROLE *acl_public= NULL;
> +
> +inline privilege_t public_access()
> +{
> +  return (acl_public ? acl_public->access : NO_ACL);
> +}

Why not to have acl_public always not null?
e.g. define it as

  static ACL_ROLE acl_public;

so it's not even a pointer. And don't store it in the dynamic array.

Or make it like

  static ACL_ROLE acl_public_dummy, *acl_public= &acl_public_dummy;

then you can keep he real role in the dynamic array, but still
acl_public will never be null.

but in the standard PUBLIC is not a role at all, so may be it'd be
simpler not to store it in the dynamic array

> +
>  class Grant_tables;
>  class User_table;
>  class Proxies_priv_table;
> @@ -3148,7 +3183,7 @@ bool acl_getroot(Security_context *sctx, const char *user, const char *host,
>    }
>    else // Role, not User
>    {
> -    ACL_ROLE *acl_role= find_acl_role(user);
> +    ACL_ROLE *acl_role= find_acl_role(user, false);

are there tests for that?

>      if (acl_role)
>      {
>        res= 0;
> @@ -3161,6 +3196,26 @@ bool acl_getroot(Security_context *sctx, const char *user, const char *host,
>      }
>    }
>  
> +  /*
> +    PUBLIC magic:
> +
> +    Note: for usual user privileges of 3 component merged together:
> +      1) user privileges
> +      2) set role privileges
> +      3) public privileges
> +    But for this routine (used in Security_context::change_security_context)
> +    only 2 component merged:
> +      1) user OR role privileges we are switching to

because if we're switching to a user, there can be no set role?
I feel this comment is more confusing, then helping, really,
I'd suggest to remove it completely

> +      2) public privileges
> +  */
> +  if (acl_public)
> +  {
> +    if (ACL_DB *acl_db= acl_db_find(db, public_name.str, "", "", FALSE))
> +      sctx->db_access|= acl_db->access;
> +
> +    sctx->master_access|= acl_public->access;
> +  }
> +
>    mysql_mutex_unlock(&acl_cache->lock);
>    DBUG_RETURN(res);
>  }
> @@ -3320,20 +3375,22 @@ int acl_setrole(THD *thd, const char *rolename, privilege_t access)
>    /* merge the privileges */
>    Security_context *sctx= thd->security_ctx;
>    sctx->master_access= access;
> -  if (thd->db.str)
> -    sctx->db_access= acl_get(sctx->host, sctx->ip, sctx->user, thd->db.str, FALSE);
> -
>    if (!strcasecmp(rolename, "NONE"))
>    {
>      thd->security_ctx->priv_role[0]= 0;
>    }
>    else
>    {
> -    if (thd->db.str)
> -      sctx->db_access|= acl_get("", "", rolename, thd->db.str, FALSE);
>      /* mark the current role */
>      strmake_buf(thd->security_ctx->priv_role, rolename);
>    }
> +  if (thd->db.str)
> +    sctx->db_access= acl_get_all3(sctx, thd->db.str, FALSE);
> +
> +  // PUBLIC magic
> +  if (acl_public)
> +    sctx->master_access|= acl_public->access;

why not to do it in check_user_can_set_role()?
Like

-   *access = acl_user->access | role->access;
+   *access = acl_user->access | role->access | acl_public->access;
  
> +
>    return 0;
>  }
>  
> @@ -3347,9 +3404,13 @@ static uchar* check_get_key(ACL_USER *buff, size_t *length,
>  
>  static void acl_update_role(const char *rolename, const privilege_t privileges)
>  {
> -  ACL_ROLE *role= find_acl_role(rolename);
> +  ACL_ROLE *role= find_acl_role(rolename, true);
>    if (role)
> +  {
>      role->initial_role_access= role->access= privileges;
> +    if (strcasecmp(rolename, public_name.str) == 0)
> +      acl_public= role;

why? I mean, in what case can acl_public != role here?

> +  }
>  }
>  
>  
> @@ -3638,6 +3703,23 @@ privilege_t acl_get(const char *host, const char *ip,
>    DBUG_RETURN(db_access & host_access);
>  }
>  
> +/*
> +  Check if there is access for the host/user, role, public on the database
> +*/
> +
> +privilege_t acl_get_all3(Security_context *sctx, const char *db,
> +                         bool db_is_patern)
> +{
> +  privilege_t access= acl_get(sctx->host, sctx->ip,
> +                              sctx->priv_user, db, db_is_patern);
> +  if (sctx->priv_role[0])
> +    access|= acl_get("", "", sctx->priv_role, db, db_is_patern);
> +  if (acl_public)
> +    access|= acl_get("", "", public_name.str, db, db_is_patern);

how can this be optimized, considering that in most cases and for most db
names there will be no grants to role and public?

A couple of ideas: maintain counters total_number_of_db_grants_to_roles
ad total_number_of_db_grants_to_public. This is rather crude.

keep grants to users, to roles, and to public in separate caches.
or in different dynamic arrays (won't help much in a default setup)

keep propery per ACL_DB or shared between all ACL_DB's of one DB?
that'd be rather tricky to maintain

> +  return access;
> +}
> +
> +
>  /*
>    Check if there are any possible matching entries for this host
>  
> @@ -3759,7 +3841,7 @@ static bool add_role_user_mapping(const char *uname, const char *hname,
>                                    const char *rname)
>  {
>    ACL_USER_BASE *grantee= find_acl_user_base(uname, hname);
> -  ACL_ROLE *role= find_acl_role(rname);
> +  ACL_ROLE *role= find_acl_role(rname, true);

why?

>  
>    if (grantee == NULL || role == NULL)
>      return 1;
> @@ -4247,7 +4329,7 @@ bool is_acl_user(const char *host, const char *user)
>    if (*host) // User
>      res= find_user_exact(host, user) != NULL;
>    else // Role
> -    res= find_acl_role(user) != NULL;
> +    res= find_acl_role(user, false) != NULL;

this disallows DEFINER PUBLIC, right? I couldn't find a test for that.

>  
>    mysql_mutex_unlock(&acl_cache->lock);
>    return res;
> @@ -7537,11 +7620,11 @@ bool mysql_grant_role(THD *thd, List <LEX_USER> &list, bool revoke)
>        if (user->host.str)
>          hostname= user->host;
>        else
> -      if ((role_as_user= find_acl_role(user->user.str)))
> +      if ((role_as_user= find_acl_role(user->user.str, false)))

Why is it false here? It looks like it disallows GRANT role TO PUBLIC

>          hostname= empty_clex_str;
>        else
>        {
> -        if (is_invalid_role_name(username.str))
> +        if (check_role_name(username.str, false) == ROLE_NAME_INVALID)
>          {
>            append_user(thd, &wrong_users, &username, &empty_clex_str);
>            result= 1;
> @@ -8280,19 +8355,14 @@ bool check_grant(THD *thd, privilege_t want_access, TABLE_LIST *tables,
>      if (any_combination_will_do)
>        continue;
>  
> -    t_ref->grant.grant_table_user= grant_table; // Remember for column test
> -    t_ref->grant.grant_table_role= grant_table_role;
> -    t_ref->grant.version= grant_version;
> -    t_ref->grant.privilege|= grant_table ? grant_table->privs : NO_ACL;
> -    t_ref->grant.privilege|= grant_table_role ? grant_table_role->privs : NO_ACL;
> +    t_ref->grant.privilege|= t_ref->grant.aggregate_privs();
>      t_ref->grant.want_privilege= ((want_access & COL_ACLS) & ~t_ref->grant.privilege);
>  
>      if (!(~t_ref->grant.privilege & want_access))
>        continue;
>  
> -    if ((want_access&= ~((grant_table ? grant_table->cols : NO_ACL) |
> -                        (grant_table_role ? grant_table_role->cols : NO_ACL) |
> -                        t_ref->grant.privilege)))
> +    if ((want_access&= ~(t_ref->grant.aggregate_cols() |
> +                         t_ref->grant.privilege)))
>      {
>        goto err;                                 // impossible
>      }

1. why is this if() here, if it's impossible?
2. I'm a bit worried that you set t_ref->grant.version early when it's
   still possible to goto err. Old code was setting the version very late
   after all checks have succeeded.

> @@ -8335,6 +8405,49 @@ static void check_grant_column_int(GRANT_TABLE *grant_table, const char *name,
>    }
>  }
>  
> +inline privilege_t GRANT_INFO::aggregate_privs()
> +{
> +  return (grant_table_user ? grant_table_user->privs : NO_ACL) |
> +         (grant_table_role ?  grant_table_role->privs : NO_ACL) |
> +         (grant_public ?  grant_public->privs : NO_ACL);
> +}
> +
> +inline privilege_t GRANT_INFO::aggregate_cols()
> +{
> +  return (grant_table_user ? grant_table_user->cols : NO_ACL) |
> +         (grant_table_role ?  grant_table_role->cols : NO_ACL) |
> +         (grant_public ?  grant_public->cols : NO_ACL);
> +}
> +
> +void GRANT_INFO::refresh(const Security_context *sctx,
> +                         const char *db, const char *table)
> +{
> +  if (version != grant_version)
> +    read(sctx, db, table);
> +}
> +
> +void GRANT_INFO::read(const Security_context *sctx,
> +                         const char *db, const char *table)
> +{
> +#ifdef EMBEDDED_LIBRARY
> +  grant_table_user= grant_table_role= grant_public= NULL;
> +#else
> +  grant_table_user=
> +    table_hash_search(sctx->host, sctx->ip, db,
> +                      sctx->priv_user,
> +                      table, FALSE);         /* purecov: inspected */

you don't need to preserve purecov comments when moving the code around

> +  grant_table_role=
> +    sctx->priv_role[0] ? table_hash_search("", NULL, db,
> +                                           sctx->priv_role,
> +                                           table, TRUE) : NULL;
> +  grant_public=
> +    acl_public ? table_hash_search("", NULL, db,
> +                                   public_name.str,
> +                                   table, TRUE) : NULL;
> +#endif
> +  version= grant_version;		/* purecov: inspected */
> +}
> +
>  /*
>    Check column rights in given security context
>  
> @@ -9533,6 +9626,13 @@ static bool show_global_privileges(THD *thd, ACL_USER_BASE *acl_entry,
>      want_access= ((ACL_ROLE *)acl_entry)->initial_role_access;
>    else
>      want_access= acl_entry->access;
> +
> +  // suppress "GRANT USAGE ON *.* TO `PUBLIC`"

why? I agree that it's pointless, but disabling just it
is kind of difficult to explain.

> +  if (!(want_access & ~GRANT_ACL) &&
> +      acl_entry->user.length == public_name.length &&
> +      strcasecmp(acl_entry->user.str, public_name.str) == 0)
> +    return FALSE;
> +
>    if (test_all_bits(want_access, (GLOBAL_ACLS & ~ GRANT_ACL)))
>      global.append(STRING_WITH_LEN("ALL PRIVILEGES"));
>    else if (!(want_access & ~GRANT_ACL))
> @@ -11015,10 +11116,15 @@ bool mysql_drop_user(THD *thd, List <LEX_USER> &list, bool handle_as_role)
>    {
>      int rc;
>      user_name= get_current_user(thd, tmp_user_name, false);
> -    if (!user_name)
> +    if (!user_name || (handle_as_role &&
> +                       (strcasecmp(user_name->user.str,
> +                                   public_name.str) == 0)))

better add this condition to the following if(), not to this one.
Like:

-    if (handle_as_role != user_name->is_role())
+    if (handle_as_role != user_name->is_role() ||
+        (handle_as_role && check_role_name(...) == ROLE_NAME_INVALID))

>      {
>        thd->clear_error();
> -      append_str(&wrong_users, STRING_WITH_LEN("CURRENT_ROLE"));
> +      if (!user_name)
> +        append_str(&wrong_users, STRING_WITH_LEN("CURRENT_ROLE"));
> +      else
> +        append_str(&wrong_users, public_name.str, public_name.length);
>        result= TRUE;
>        continue;
>      }
> diff --git a/sql/sql_show.cc b/sql/sql_show.cc
> index 6290a47ee30..143e647f74c 100644
> --- a/sql/sql_show.cc
> +++ b/sql/sql_show.cc
> @@ -5304,7 +5300,7 @@ int get_all_tables(THD *thd, TABLE_LIST *tables, COND *cond)
>                         &thd->col_access, NULL, 0, 1) ||
>            (!thd->col_access && check_grant_db(thd, db_name->str))) ||
>          sctx->master_access & (DB_ACLS | SHOW_DB_ACL) ||
> -        acl_get(sctx->host, sctx->ip, sctx->priv_user, db_name->str, 0))
> +        acl_get_all3(sctx, db_name->str, 0))

Was it a bug? That one couldn't see db's if privileges were granted
via roles?

>  #endif
>      {
>        Dynamic_array<LEX_CSTRING*> table_names(PSI_INSTRUMENT_MEM);
> diff --git a/mysql-test/main/public_basic.test b/mysql-test/main/public_basic.test
> new file mode 100644
> index 00000000000..3f5993dfe97
> --- /dev/null
> +++ b/mysql-test/main/public_basic.test
> @@ -0,0 +1,93 @@
> +SHOW GRANTS FOR PUBLIC;
> +
> +--echo # it is not PUBLIC but an user
> +--echo # (this should work as it allowed for roles for example)
> +create user PUBLIC;
> +create user PUBLIC@localhost;
> +GRANT SELECT on test.* to PUBLIC@localhost;
> +drop user PUBLIC@localhost;
> +drop user PUBLIC;
> +
> +--echo # preinstalled PUBLIC
> +GRANT SELECT on test.* to PUBLIC;
> +GRANT SELECT on mysql.db to PUBLIC;
> +--replace_regex /"version_id"\:[0-9]+/"version_id":VERSION/
> +select * from mysql.global_priv where user="PUBLIC" ;
> +
> +SHOW GRANTS FOR PUBLIC;
> +
> +GRANT UPDATE on test.* to PUBLIC;
> +GRANT UPDATE on mysql.db to PUBLIC;
> +
> +SHOW GRANTS FOR PUBLIC;
> +
> +REVOKE SELECT on test.* from PUBLIC;
> +REVOKE SELECT on mysql.db from PUBLIC;
> +
> +SHOW GRANTS FOR PUBLIC;
> +
> +REVOKE UPDATE on test.* from PUBLIC;
> +REVOKE UPDATE on mysql.db from PUBLIC;
> +
> +--error ER_NONEXISTING_GRANT
> +REVOKE UPDATE on test.* from PUBLIC;
> +--error ER_NONEXISTING_TABLE_GRANT
> +REVOKE UPDATE on mysql.db from PUBLIC;
> +
> +SHOW GRANTS FOR PUBLIC;
> +
> +--echo # automaticly added PUBLIC
> +delete from mysql.global_priv where user="PUBLIC";
> +flush privileges;
> +select * from mysql.global_priv where user="PUBLIC" ;
> +GRANT SELECT on test.* to PUBLIC;
> +GRANT SELECT on mysql.db to PUBLIC;
> +--replace_regex /"version_id"\:[0-9]+/"version_id":VERSION/
> +select * from mysql.global_priv where user="PUBLIC" ;
> +
> +SHOW GRANTS FOR PUBLIC;
> +
> +GRANT UPDATE on test.* to PUBLIC;
> +GRANT UPDATE on mysql.db to PUBLIC;
> +
> +SHOW GRANTS FOR PUBLIC;
> +
> +REVOKE SELECT on test.* from PUBLIC;
> +REVOKE SELECT on mysql.db from PUBLIC;
> +
> +SHOW GRANTS FOR PUBLIC;
> +
> +REVOKE UPDATE on test.* from PUBLIC;
> +REVOKE UPDATE on mysql.db from PUBLIC;
> +
> +SHOW GRANTS FOR PUBLIC;
> +
> +--error ER_INVALID_ROLE
> +GRANT XXXXXX TO CURRENT_USER;
> +--echo # following should fail with the same error as above
> +--error ER_INVALID_ROLE
> +GRANT PUBLIC TO CURRENT_USER;
> +
> +--error ER_INVALID_ROLE
> +REVOKE XXXXXX FROM CURRENT_USER;
> +--echo # following should fail with the same error as above
> +--error ER_INVALID_ROLE
> +REVOKE PUBLIC FROM CURRENT_USER;
> +--error ER_CANNOT_USER
> +
> +drop role XXXXXX;
> +--echo # following should fail with the same error as above
> +--error ER_CANNOT_USER
> +drop role PUBLIC;
> +
> +--error ER_INVALID_ROLE
> +SET ROLE XXXXXX;
> +--echo # following should fail with the same error as above
> +--error ER_INVALID_ROLE
> +SET ROLE PUBLIC;
> +
> +--error ER_INVALID_ROLE
> +SET DEFAULT ROLE XXXXXX;
> +--echo # following should fail with the same error as above
> +--error ER_INVALID_ROLE
> +SET DEFAULT ROLE PUBLIC;

This is good, but one can update global_priv table directly
and put the default_role=PUBLIC there. Please, add a test for that.

and also a couple of tests for `public` (in backticks)

> diff --git a/mysql-test/main/public_privileges.test b/mysql-test/main/public_privileges.test
> new file mode 100644
> index 00000000000..a542376f05c
> --- /dev/null
> +++ b/mysql-test/main/public_privileges.test
> @@ -0,0 +1,293 @@
> +--echo #
> +--echo # Test DB/TABLE/COLUMN privileges in queries
> +--echo #
> +
> +SHOW GRANTS FOR PUBLIC;
> +
> +create user testuser;
> +create database testdb1;
> +use testdb1;
> +create table t1 (a int, b int);
> +insert into t1 values (1,2);
> +create database testdb2;
> +use testdb2;
> +create table t2 (a int, b int);
> +insert into t2 values (1,2);
> +create table t3 (a int, b int);
> +insert into t3 values (1,2);
> +
> +connect (testuser,localhost,testuser,,);
> +connection testuser;

"connect testuser" also switches to this connection,
the following "connection testuser" is redundant

> +--error ER_TABLEACCESS_DENIED_ERROR
> +select * from testdb1.t1;
> +--error ER_TABLEACCESS_DENIED_ERROR
> +select * from testdb2.t2;
> +--error ER_TABLEACCESS_DENIED_ERROR
> +select b from testdb2.t3;
> +--error ER_TABLEACCESS_DENIED_ERROR
> +select a from testdb2.t3;
> +
> +connection default;
> +
> +GRANT SELECT ON testdb1.* to PUBLIC;
> +GRANT SELECT ON testdb2.t2 to PUBLIC;
> +GRANT SELECT (b) ON testdb2.t3 to PUBLIC;
> +
> +disconnect testuser;
> +connect (testuser,localhost,testuser,,);
> +connection testuser;
> +select * from testdb1.t1;
> +select * from testdb2.t2;
> +select b from testdb2.t3;
> +--error ER_COLUMNACCESS_DENIED_ERROR
> +select a from testdb2.t3;
> +
> +connection default;
> +
> +--replace_column 1 # 3 # 6 # 7 #
> +SHOW PROCESSLIST;

why?

> +
> +connection default;
> +
> +use test;
> +disconnect testuser;
> +REVOKE SELECT ON testdb1.* from PUBLIC;
> +REVOKE SELECT ON testdb2.t2 from PUBLIC;
> +REVOKE SELECT (b) ON testdb2.t3 from PUBLIC;

may be `revoke all privileges, grant option` ?
looks like a good place to have it tested

> +drop user testuser;
> +drop database testdb1;
> +drop database testdb2;
> +
> +--echo #
> +--echo # test global process list privilege and EXECUTE db level
> +--echo #
> +
> +create user testuser;
> +create database testdb;
> +use testdb;
> +create procedure p1 () select 1;
> +
> +connect (testuser,localhost,testuser,,);
> +connection testuser;
> +
> +--replace_column 1 # 3 # 6 # 7 #
> +SHOW PROCESSLIST;
> +--error ER_PROCACCESS_DENIED_ERROR
> +call testdb.p1();
> +
> +connection default;
> +
> +GRANT PROCESS ON *.* to PUBLIC;
> +GRANT EXECUTE ON testdb.* to PUBLIC;
> +
> +disconnect testuser;
> +connect (testuser,localhost,testuser,,);
> +connection testuser;
> +
> +--replace_column 1 # 3 # 6 # 7 #
> +SHOW PROCESSLIST;
> +call testdb.p1();
> +
> +connection default;
> +
> +--replace_column 1 # 3 # 6 # 7 #
> +SHOW PROCESSLIST;
> +
> +connection default;
> +
> +use test;
> +disconnect testuser;
> +REVOKE PROCESS ON *.* from PUBLIC;
> +REVOKE EXECUTE ON testdb.* from PUBLIC;
> +drop user testuser;
> +drop database testdb;
> +
> +--echo #
> +--echo # test DB privilege to allow USE statement
> +--echo #
> +
> +create user testuser;
> +create database testdb;
> +
> +connect (testuser,localhost,testuser,,);
> +connection testuser;
> +
> +--error ER_DBACCESS_DENIED_ERROR
> +use testdb;
> +
> +connection default;
> +
> +GRANT LOCK TABLES ON testdb.* to PUBLIC;
> +
> +disconnect testuser;
> +connect (testuser,localhost,testuser,,);
> +connection testuser;
> +
> +use testdb;
> +
> +connection default;
> +
> +use test;
> +disconnect testuser;
> +REVOKE LOCK TABLES ON testdb.* from PUBLIC;
> +drop user testuser;
> +drop database testdb;
> +
> +
> +--echo #
> +--echo # test DB privilege to allow USE statement (as above)
> +--echo # test current db privileges
> +--echo #
> +
> +create user testuser;
> +create database testdb;
> +use testdb;
> +create table t1 (a int);
> +insert into t1 values (1);
> +GRANT LOCK TABLES ON testdb.* to PUBLIC;
> +
> +connect (testuser,localhost,testuser,,);
> +connection testuser;
> +
> +use testdb;
> +--error ER_TABLEACCESS_DENIED_ERROR
> +update t1 set a=a+1;
> +
> +connection default;
> +
> +GRANT UPDATE,SELECT ON testdb.* to PUBLIC;
> +
> +disconnect testuser;
> +connect (testuser,localhost,testuser,,);
> +connection testuser;
> +
> +use testdb;
> +update t1 set a=a+1;
> +
> +connection default;
> +select * from testdb.t1;
> +
> +use test;
> +disconnect testuser;
> +REVOKE LOCK TABLES ON testdb.* from PUBLIC;
> +REVOKE UPDATE,SELECT ON testdb.* from PUBLIC;
> +drop user testuser;
> +drop database testdb;
> +
> +
> +--echo #
> +--echo # test DB privilege to allow USE statement (as above)
> +--echo # test table/column privileges in current DB
> +--echo #
> +
> +create user testuser;
> +create database testdb;
> +use testdb;
> +create table t1 (a int);
> +insert into t1 values (1);
> +create table t2 (a int, b int);
> +insert into t2 values (1,2);
> +GRANT LOCK TABLES ON testdb.* to PUBLIC;
> +
> +connect (testuser,localhost,testuser,,);
> +connection testuser;
> +
> +use testdb;
> +--error ER_TABLEACCESS_DENIED_ERROR
> +delete from t1;
> +--error ER_TABLEACCESS_DENIED_ERROR
> +select b from t2;
> +--error ER_TABLEACCESS_DENIED_ERROR
> +select a from t2;
> +
> +connection default;
> +
> +GRANT DELETE ON testdb.t1 to PUBLIC;
> +GRANT SELECT (a) ON testdb.t2 to PUBLIC;
> +
> +disconnect testuser;
> +connect (testuser,localhost,testuser,,);
> +connection testuser;
> +
> +use testdb;
> +delete from t1;
> +select a from t2;
> +--error ER_COLUMNACCESS_DENIED_ERROR
> +select b from t2;
> +
> +connection default;
> +select * from testdb.t1;
> +
> +use test;
> +disconnect testuser;
> +REVOKE LOCK TABLES ON testdb.* from PUBLIC;
> +REVOKE DELETE ON testdb.t1 from PUBLIC;
> +REVOKE SELECT (a) ON testdb.t2 from PUBLIC;
> +drop user testuser;
> +drop database testdb;
> +
> +--echo #
> +--echo # test function privilege
> +--echo #
> +
> +create user testuser;
> +create database testdb;
> +use testdb;
> +create function f1() returns int return 2;
> +
> +connect (testuser,localhost,testuser,,);
> +connection testuser;
> +
> +--error ER_PROCACCESS_DENIED_ERROR
> +alter function testdb.f1 comment "A stupid function";
> +--error ER_PROCACCESS_DENIED_ERROR
> +select testdb.f1();
> +
> +connection default;
> +
> +GRANT ALTER ROUTINE ON testdb.* to PUBLIC;
> +
> +disconnect testuser;
> +connect (testuser,localhost,testuser,,);
> +connection testuser;
> +
> +alter function testdb.f1 comment "A stupid function";
> +--error ER_PROCACCESS_DENIED_ERROR
> +select testdb.f1();
> +
> +connection default;
> +
> +use test;
> +disconnect testuser;
> +REVOKE ALTER ROUTINE ON testdb.* from PUBLIC;
> +drop function testdb.f1;
> +drop user testuser;
> +drop database testdb;
> +
> +--echo #
> +--echo # bug with automatically added PUBLIC role
> +--echo #
> +
> +--echo # automaticly added PUBLIC
> +delete from mysql.global_priv where user="PUBLIC";
> +flush privileges;
> +GRANT SELECT on test.* to PUBLIC;
> +
> +REVOKE SELECT on test.* from PUBLIC;
> +
> +create user testuser;
> +create database testdb1;
> +use testdb1;
> +create table t1 (a int, b int);
> +insert into t1 values (1,2);
> +
> +connect (testuser,localhost,testuser,,);
> +connection testuser;
> +--error ER_TABLEACCESS_DENIED_ERROR
> +select * from testdb1.t1;
> +
> +connection default;
> + 
> +disconnect testuser;
> +drop user testuser;
> +drop database testdb1;
> diff --git a/mysql-test/suite/roles/none_public.test b/mysql-test/suite/roles/none_public.test
> index a0ec2315cfc..d0ba62d226b 100644
> --- a/mysql-test/suite/roles/none_public.test
> +++ b/mysql-test/suite/roles/none_public.test
> @@ -19,8 +19,9 @@ grant select on *.* to none;
>  grant public to role1;
>  --error ER_INVALID_ROLE
>  grant role1 to public;
^^^
this shouldn't fail

> ---error ER_INVALID_ROLE
> -grant select on *.* to public;
> +# PUBLIC is legal role
> +#--error ER_INVALID_ROLE
> +#grant select on *.* to public;

you can just remove it, commenting out isn't better

>  --error ER_INVALID_ROLE
>  grant role1 to current_role;
> @@ -40,16 +41,18 @@ revoke select on *.* from public;
>  
>  --error ER_INVALID_ROLE
>  show grants for none;
> ---error ER_INVALID_ROLE
> -show grants for public;
> +# PUBLIC is legal role
> +#--error ER_INVALID_ROLE
> +#show grants for public;
>  
>  --error ER_INVALID_ROLE
>  create definer=none view test.v1 as select 1;
> ---error ER_INVALID_ROLE
> -create definer=public view test.v1 as select 1;
> +# PUBLIC is legal role
> +#--error ER_INVALID_ROLE
> +#create definer=public view test.v1 as select 1;

eh, no, PUBLIC isn't a valid definer, as far as I understand.

>  
>  drop role role1;
>  
> -insert mysql.global_priv values ('', 'none', '{"is_role":true}'), ('', 'public', '{"is_role":true}');
> +insert mysql.global_priv values ('', 'none', '{"is_role":true}');
>  flush privileges;
> -delete from mysql.global_priv where host='';
> +delete from mysql.global_priv where host='' and user='none';
> diff --git a/mysql-test/main/sp_notembedded.result b/mysql-test/main/sp_notembedded.result
> index e03361598a6..239f8acb7f3 100644
> --- a/mysql-test/main/sp_notembedded.result
> +++ b/mysql-test/main/sp_notembedded.result
> @@ -345,7 +345,6 @@ show grants;
>  Grants for foo1@localhost
>  GRANT USAGE ON *.* TO `foo1`@`localhost` IDENTIFIED BY PASSWORD '-F3A2A51A9B0F2BE2468926B4132313728C250DBF'
>  GRANT CREATE ROUTINE ON `test`.* TO `foo1`@`localhost`
> -GRANT EXECUTE, ALTER ROUTINE ON PROCEDURE `test`.`spfoo` TO `foo1`@`localhost`

Why did this line disappear?

>  connection default;
>  disconnect foo;
>  drop procedure spfoo;

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx