Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MDEV-5228 Fix incorrect message on a failed attempt to revoke grants from a role #3114

Open
wants to merge 1 commit into
base: 10.5
Choose a base branch
from

Conversation

lottaquestions
Copy link
Contributor

@lottaquestions lottaquestions commented Mar 7, 2024

  • The Jira issue number for this PR is: MDEV-5228

Description

Ensure that when there is a failed attempt to revoke a grant for a role, the error message indicates that the failure was
on a role instead of a user.

Release Notes

Ensure that when there is a failed attempt to revoke a grant for a role, the error message indicates that the failure was on a role instead of a user.

How can this PR be tested?

Existing MTR tests that reference grant error messages were run and failing tests were modified accordingly. To this end, the following test suites were run:

  • funcs_1
  • roles
  • rpl

Additionally manual testing was performed as in the manner described in https://jira.mariadb.org/browse/MDEV-5228. See results for before and after below.

Before

MariaDB [mysql]> create role role1;
 
MariaDB [mysql]> grant select on mysql.user to role1;
 
MariaDB [mysql]> create role role2;
 
MariaDB [mysql]> grant role1 to role2;
 
MariaDB [mysql]> revoke all, grant option from role2;
ERROR 1147 (42000): There is no such grant defined for user 'role2' on host '' on table 'user' <--- Wrong error msg. role2 is a role not user
Error (Code 1147): There is no such grant defined for user 'role2' on host '' on table 'user'

After

MariaDB [(none)]> create role foo;

MariaDB [(none)]> revoke all on bar.* from foo;
ERROR 1141 (42000): There is no such grant defined for role 'foo'   <------- Correct error message
MariaDB [(none)]> create role role1;

MariaDB [(none)]> grant select on mysql.user to role1;

MariaDB [(none)]> create role role2;

MariaDB [(none)]> grant role1 to role2;

MariaDB [(none)]> revoke all, grant option from role2;
ERROR 1148 (42000): There is no such grant defined for role 'role2' on table 'user'   <------ Correct error message

If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.

Basing the PR against the correct MariaDB version

  • This is a new feature and the PR is based against the latest MariaDB development branch.
  • This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

Copyright

All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.

@CLAassistant
Copy link

CLAassistant commented Mar 7, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Member

@grooverdan grooverdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note its failing to compile on embedded. Some use of #ifdef NO_EMBEDDED_ACCESS_CHECKS I think is required.

/home/buildbot/amd64-fedora-38-last-N-failed/build/sql/sql_acl.cc:88:13: error: ‘void my_error_wrapper_for_proc_grant(bool, const char*, const char*, const char*)’ defined but not used [-Werror=unused-function]
   88 | static void my_error_wrapper_for_proc_grant(bool is_role, const char* user_or_role, const char* host, const char* routine)
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/buildbot/amd64-fedora-38-last-N-failed/build/sql/sql_acl.cc:80:13: error: ‘void my_error_wrapper_for_table_grant(bool, const char*, const char*, const char*)’ defined but not used [-Werror=unused-function]
   80 | static void my_error_wrapper_for_table_grant(bool is_role, const char* user_or_role, const char* host, const char* table)
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/buildbot/amd64-fedora-38-last-N-failed/build/sql/sql_acl.cc:72:13: error: ‘void my_error_wrapper_for_grant(bool, const char*, const char*)’ defined but not used [-Werror=unused-function]
   72 | static void my_error_wrapper_for_grant(bool is_role, const char* user_or_role, const char* host)
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [libmysqld/CMakeFiles/sql_embedded.dir/build.make:1056: libmysqld/CMakeFiles/sql_embedded.dir/__/sql/sql_acl.cc.o] Error 1

Test result look good.

Sorry for the slow review.

As a bug fix I think 10.5 is a better target. @cvicentiu happy with that?

sql/share/errmsg-utf8.txt Show resolved Hide resolved
@lottaquestions lottaquestions force-pushed the fix-incorrect-message-on-a-failed-attempt-to-revoke-grants-from-a-role branch from f24c9b4 to ca5ff4d Compare March 26, 2024 15:23
…from a role

Ensure that when there is a failed attempt to revoke a grant
for a role, the error message indicates that the failure was
on a role instead of a user.

All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.
@lottaquestions lottaquestions force-pushed the fix-incorrect-message-on-a-failed-attempt-to-revoke-grants-from-a-role branch from ca5ff4d to 695320d Compare March 26, 2024 18:58
@lottaquestions lottaquestions changed the base branch from 11.5 to 10.5 March 26, 2024 18:59
@lottaquestions
Copy link
Contributor Author

lottaquestions commented Mar 26, 2024

Note its failing to compile on embedded. Some use of #ifdef NO_EMBEDDED_ACCESS_CHECKS I think is required.

/home/buildbot/amd64-fedora-38-last-N-failed/build/sql/sql_acl.cc:88:13: error: ‘void my_error_wrapper_for_proc_grant(bool, const char*, const char*, const char*)’ defined but not used [-Werror=unused-function]
   88 | static void my_error_wrapper_for_proc_grant(bool is_role, const char* user_or_role, const char* host, const char* routine)
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/buildbot/amd64-fedora-38-last-N-failed/build/sql/sql_acl.cc:80:13: error: ‘void my_error_wrapper_for_table_grant(bool, const char*, const char*, const char*)’ defined but not used [-Werror=unused-function]
   80 | static void my_error_wrapper_for_table_grant(bool is_role, const char* user_or_role, const char* host, const char* table)
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/buildbot/amd64-fedora-38-last-N-failed/build/sql/sql_acl.cc:72:13: error: ‘void my_error_wrapper_for_grant(bool, const char*, const char*)’ defined but not used [-Werror=unused-function]
   72 | static void my_error_wrapper_for_grant(bool is_role, const char* user_or_role, const char* host)
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [libmysqld/CMakeFiles/sql_embedded.dir/build.make:1056: libmysqld/CMakeFiles/sql_embedded.dir/__/sql/sql_acl.cc.o] Error 1

Test result look good.

Sorry for the slow review.

As a bug fix I think 10.5 is a better target. @cvicentiu happy with that?

Changed the target to 10.5 and put the new code inside of the NO_EMBEDDED_ACCESS_CHECKS macro to resolve the compilation error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants