Hi, Sujatha! On Nov 12, sujatha wrote:
diff --git a/mysql-test/main/grant.test b/mysql-test/main/grant.test index 38baf6738255..63be18ecb292 100644 --- a/mysql-test/main/grant.test +++ b/mysql-test/main/grant.test @@ -1833,6 +1833,7 @@ GRANT REPLICATION SLAVE ON *.* TO mysqltest_u1@localhost; GRANT SHOW DATABASES ON *.* TO mysqltest_u1@localhost; GRANT SHUTDOWN ON *.* TO mysqltest_u1@localhost; GRANT USAGE ON *.* TO mysqltest_u1@localhost; +GRANT REPLICA MONITOR ON *.* TO mysqltest_u1@localhost; But the parser should, of course, accept both, as everywhere.
Yes, both REPLICA MONITOR/ SLAVE MONITOR will work.
Right, I know. I meant that it's fine (and even good) to use GRANT REPLICA MONITOR in test files, no matter how it's shown in SHOW GRANTS.
diff --git a/mysql-test/suite/rpl/t/rpl_replica_monitor_priv.test b/mysql-test/suite/rpl/t/rpl_replica_monitor_priv.test new file mode 100644 index 000000000000..f7b2af2e0c51 --- /dev/null +++ b/mysql-test/suite/rpl/t/rpl_replica_monitor_priv.test @@ -0,0 +1,90 @@ +# ==== Purpose ==== +# +# REPLICA MONITOR privilege is required to execute following commands. +# SHOW SLAVE STATUS +# SHOW RELAYLOG EVENTS +# +# ==== Implementation ==== +# +# Step1: GRANT ALL privileges for a new user 'user1' and then REVOKE REPLICA +# MONITOR and SUPER privileges. +# Step2: Execute SHOW SLAVE STAUTS/SHOW RELAYLOG commands and expect +# ER_SPECIFIC_ACCESS_DENIED_ERROR. This also verifies that REPLICATION +# SLAVE ADMIN privilege is not required for these two commands. +# Step3: GRANT REPLICA MONITOR privilege and observe that both commands are +# allowd to execute. +# Step4: GRANT SUPER privilege and observe that only SHOW SLAVE STATUS command +# is allowed. why do you need a full master-slave replication setup for that?
It can be added in main suite as well. Since these are replication specific privileges, added them as part of 'rpl' suite.
I think it's better to have it in the main suite. rpl suite is for testing replication features that need a working replication topology set up. For example binlog specific tests that do not need working replication go into the binlog suite, not in rpl.
+--error ER_SPECIFIC_ACCESS_DENIED_ERROR +SHOW SLAVE STATUS; +--echo "Verify that having REPLICATION SLAVE ADMIN doesn't allow SHOW RELAYLOG EVENTS" +--echo "Expected error: Access denied; you need (at least one of) the REPLICA MONITOR privilege(s) for this operation" +--disable_ps_protocol why?
I observed following failure during buildbot testing
http://buildbot.askmonty.org/buildbot/builders/kvm-fulltest/builds/26635/ste...
rpl.rpl_replica_monitor_priv 'mix' w1 [ fail ] Test ended at 2020-11-10 11:22:28 CURRENT_TEST: rpl.rpl_replica_monitor_priv mysqltest: At line 46: query 'SHOW RELAYLOG EVENTS' failed with wrong errno 1295: 'This command is not supported in the prepared statement protocol yet', instead of 1227... To address this issue I disabled the command.
This is a bug. It is assumed that mysqltest will only use ps protocol for statements that support it. It has a big regex to detect that. Alas, it only has "|[[:space:]]*SHOW[[:space:]]|" which matches all SHOW commands, and it'd be rather annoying and fragile to list all SHOW variants excluding just RELAYLOG (and BINLOG) EVENTS. On the other hand, it seems really easy to make them to support ps protocol. Perhaps it only needs something like (untested): =============================== diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -2369,6 +2369,14 @@ static bool check_prepared_statement(Prepared_statement > DBUG_RETURN(FALSE); } break; + case SQLCOM_SHOW_BINLOG_EVENTS: + case SQLCOM_SHOW_RELAYLOG_EVENTS: + { + List<Item> field_list; + Log_event::init_show_field_list(thd, &field_list); + if ((res= send_stmt_metadata(thd, stmt, &field_list)) == 2) + DBUG_RETURN(FALSE); + } #endif /* EMBEDDED_LIBRARY */ case SQLCOM_SHOW_CREATE_PROC: if ((res= mysql_test_show_create_routine(stmt, &sp_handler_procedure)) == > =============================== Could you try that, please? If it'll work, better to commit it separately, though.
diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index ef96e8f24843..8a3551c14f53 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -1057,6 +1057,9 @@ class User_table_tabular: public User_table if (access & REPL_SLAVE_ACL) access|= REPL_MASTER_ADMIN_ACL;
+ if (access & FILE_ACL) + access|= REPLICA_MONITOR_ACL; Why is that?
Please find the reason mentioned as part of the following comment.
Yes, I've seen it already, but I still don't understand what FILE_ACL has to do with REPLICA_MONITOR_ACL.
So, indeed, old code was correct, that if() should be inside SUPER_ACL, one should only get REPL_MASTER_ADMIN_ACL if one had both SUPER_ACL and REPL_SLAVE_ACL.
I will try to rewrite this part such that
SUPER_ACL / REPL_CLIENT_ACL - SLAVE_MONITOR_ACL
REPL_SLAVE_ACL - SLAVE_MONITOR_ACL
Exactly. Thanks! Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org