Hi, Anel! On May 08, Anel Husakovic wrote:

revision-id: 4312f382b6f (mariadb-10.1.43-138-g4312f382b6f) parent(s): 8c4b5261210 author: Anel Husakovic <anel@mariadb.org> committer: Anel Husakovic <anel@mariadb.org> 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@mariadb.org