maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Anel!

On May 08, Anel Husakovic wrote:
> revision-id: 4312f382b6f (mariadb-10.1.43-138-g4312f382b6f)
> parent(s): 8c4b5261210
> author: Anel Husakovic <anel@xxxxxxxxxxx>
> committer: Anel Husakovic <anel@xxxxxxxxxxx>
> timestamp: 2020-05-07 16:49:33 +0200
> message:
> 
> MDEV-22312: Bad error message for SET DEFAULT ROLE when user account is not granted the role
> 
> diff --git a/mysql-test/suite/roles/set_role-recursive.test b/mysql-test/suite/roles/set_role-recursive.test
> index 23d623e1966..dcf3dd5b6fc 100644
> --- a/mysql-test/suite/roles/set_role-recursive.test
> +++ b/mysql-test/suite/roles/set_role-recursive.test
> @@ -44,7 +44,7 @@ select * from mysql.roles_mapping;
>  
>  --sorted_result
>  show grants;
> ---error ER_INVALID_ROLE
> +--error ER_SET_DEFAULT_ROLE_FOR_USER
>  set role test_role2;
>  select current_user(), current_role();
>  --sorted_result
> diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
> index c5e83062f0f..83ecd4735e8 100644
> --- a/sql/share/errmsg-utf8.txt
> +++ b/sql/share/errmsg-utf8.txt
> @@ -7144,3 +7144,5 @@ ER_WARN_AGGFUNC_DEPENDENCE
>          ukr "Агрегатна функція '%-.192s)' з SELECTу #%d належить до SELECTу #%d"
>  WARN_INNODB_PARTITION_OPTION_IGNORED
>          eng "<%-.64s> option ignored for InnoDB partition"
> +ER_SET_DEFAULT_ROLE_FOR_USER
> +        eng "User '%s' has not been granted a role '%s'."

Sorry, this is an absolute no-no. We cannot add new error messages to
the old GA versions. Only to the very last RC/GA or to any alpha/beta.

When you add an error message, all error messages added after it in
newer versions will shift, change their numbers. And error numbers in
RC/GA versions cannot change.

Instead of a new error message and

   my_error(ER_SET_DEFAULT_ROLE_FOR_USER, MYF(0), c_usr.ptr(), rolename);

you have to write

   my_printf_error(ER_INVALID_ROLE, "User %`s has not been granted a role %`s",
                   MYF(0), c_usr.c_ptr(), rolename);

note other changes:
* %`s instead of '%s' for correct identifier quoting
* no dot at the end
* c_ptr() because printf expects a zero-terminated C string, and ptr()
  doesn't guarantee that.

> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> index b6a6f806e50..37141dd2390 100644
> --- a/sql/sql_acl.cc
> +++ b/sql/sql_acl.cc
> @@ -2041,6 +2041,9 @@ static int check_user_can_set_role(const char *user, const char *host,
>    ACL_USER_BASE *acl_user_base;
>    ACL_USER *UNINIT_VAR(acl_user);
>    bool is_granted= FALSE;
> +  bool user_can_see_role= FALSE;
> +  char buf[1024];
> +  String c_usr(buf, sizeof(buf), &my_charset_bin);

use StringBuffer<1024> c_str;

>    int result= 0;
>  
>    /* clear role privileges */
> @@ -2083,14 +2086,39 @@ static int check_user_can_set_role(const char *user, const char *host,
>        is_granted= TRUE;
>        break;
>      }
> +
> +    for (uint i=0; i < acl_user->role_grants.elements; i++)
> +    {
> +      ACL_ROLE *r= *(dynamic_element(&acl_user->role_grants, i, ACL_ROLE**));
> +      if (!strcmp(safe_str(r->user.str), safe_str(rolename)))
> +      {
> +        size_t len= acl_user->user.length + acl_user->hostname_length + 2;
> +        c_usr.alloc(len);
> +        const char c= '@';
> +        strmov(strmov(strmov(const_cast<char*>(c_usr.ptr()),
> +                      safe_str(user)), &c), safe_str(host));
> +        user_can_see_role= TRUE;
> +      }
> +    }

No, you repeat this for() block for evert role grant of every role's grantee.
And you only use it when there was an error, an uncommon case.
Do not do the work that you aren't going to use.

And the check doesn't seem correct. You want to use the same rule as for
applicable_roles, you need to traverse the graph. Perhaps you can simply
use check_role_is_granted() though. And also you need to check whether
the current user has read access on mysql.user table - in that case it
can see all roles, granted or not.

>    }
>  
>    /* According to SQL standard, the same error message must be presented */

this comment now belongs inside the if(), above ER_INVALID_ROLE

>    if (!is_granted)
>    {
> -    my_error(ER_INVALID_ROLE, MYF(0), rolename);
> -    result= 1;
> -    goto end;
> +    /* Handling of the bad error message - MDEV-22312 */

don't put MDEV numbers into the code, please.

> +    /* If the current user can see the role generate different type of error. */
> +    if(user_can_see_role)
> +    {
> +      my_error(ER_SET_DEFAULT_ROLE_FOR_USER, MYF(0), c_usr.ptr(), rolename);
> +      result= 1;
> +      goto end;
> +    }
> +    else
> +    {
> +      my_error(ER_INVALID_ROLE, MYF(0), rolename);
> +      result= 1;
> +      goto end;
> +    }
>    }
>  
>    if (access)

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