BUG#26609909: MYSQL-SHELL DBA.CHECKINSTANCECONFIGURATION()

This patch fixes issues with the checkInstanceConfiguration and
configureLocalInstance commands.

No check was done to confirm if the account used to run the command
had enough privileges to actually execute it. This issue was present
in both the checkInstanceConfiguration and configurelocalinstance
commands. With this patch, the first operation done in any of the
commands (besides input validation) is checking if the provided
account has the required privileges and a runtime error is thrown in
case it doesn't.  Furthermore, a new privilege, the SELECT privilege
on all schemas was added to the list of privileges to check.

Also, the help options presented in checkInstanceConfiguration were
incorrect and showed extra options that the command did not accept.

Finally, the SELECT privilege on all schemas was also added to the
list of privileges granted to the clusterAdmin account created using
the configureLocalinstance command.

This patch requires the list of privileges required by the
clusterAdmin account to be updated on the manual
(https://dev.mysql.com/doc/refman/en/mysql-innodb-cluster-working-with-production-deployment.html),
replacing the "GRANT SELECT ON sys.* TO your_user@'%' WITH GRANT OPTION;"
" with "GRANT SELECT ON *.* TO your_user@'%' WITH GRANT OPTION;"

Author: Nelson Goncalves

modules/adminapi/mod_dba.cc

@@ -734,36 +734,33 @@ REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL13, "@li sslKey: The path to
 REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL14, "The options dictionary may contain the following options:");
 REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL15, "@li mycnfPath: The path of the MySQL configuration file for the instance.");
 REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL16, "@li password: The password to get connected to the instance.");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL17, "@li clusterAdmin: The name of the InnoDB cluster administrator user.");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL18, "@li clusterAdminPassword: The password for the InnoDB cluster "\
-                                                       "administrator account.");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL19, "The connection password may be contained on the instance "\
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL17, "The connection password may be contained on the instance "\
                                                        "definition, however, it can be overwritten "\
                                                        "if it is specified on the options.");
 
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL20, "The returned JSON object contains the following attributes:");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL21, "@li status: the final status of the command, either \"ok\" or \"error\"");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL22, "@li config_errors: a list of dictionaries containing the failed requirements");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL23, "@li errors: a list of errors of the operation");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL24, "@li restart_required: a boolean value indicating whether a "\
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL18, "The returned JSON object contains the following attributes:");
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL19, "@li status: the final status of the command, either \"ok\" or \"error\"");
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL20, "@li config_errors: a list of dictionaries containing the failed requirements");
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL21, "@li errors: a list of errors of the operation");
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL22, "@li restart_required: a boolean value indicating whether a "\
                                                        "restart is required");
 
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL25, "Each dictionary of the list of config_errors includes the "\
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL23, "Each dictionary of the list of config_errors includes the "\
                                                        "following attributes:");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL26, "@li option: The configuration option for which the requirement "\
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL24, "@li option: The configuration option for which the requirement "\
                                                        "wasn't met");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL27, "@li current: The current value of the configuration option");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL28, "@li required: The configuration option required value");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL29, "@li action: The action to be taken in order to meet the requirement");
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL25, "@li current: The current value of the configuration option");
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL26, "@li required: The configuration option required value");
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL27, "@li action: The action to be taken in order to meet the requirement");
 
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL30, "The action can be one of the following:");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL31, "@li server_update+config_update: Both the server and the "\
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL28, "The action can be one of the following:");
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL29, "@li server_update+config_update: Both the server and the "\
                                                        "configuration need to be updated");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL32, "@li config_update+restart: The configuration needs to be "\
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL30, "@li config_update+restart: The configuration needs to be "\
                                                        "updated and the server restarted");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL33, "@li config_update: The configuration needs to be updated");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL34, "@li server_update: The server needs to be updated");
-REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL35, "@li restart: The server needs to be restarted");
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL31, "@li config_update: The configuration needs to be updated");
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL32, "@li server_update: The server needs to be updated");
+REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL33, "@li restart: The server needs to be restarted");
 
 /**
 * $(DBA_CHECKINSTANCECONFIGURATION_BRIEF)
@@ -823,8 +820,6 @@ REGISTER_HELP(DBA_CHECKINSTANCECONFIGURATION_DETAIL35, "@li restart: The server
 * $(DBA_CHECKINSTANCECONFIGURATION_DETAIL31)
 * $(DBA_CHECKINSTANCECONFIGURATION_DETAIL32)
 * $(DBA_CHECKINSTANCECONFIGURATION_DETAIL33)
-* $(DBA_CHECKINSTANCECONFIGURATION_DETAIL34)
-* $(DBA_CHECKINSTANCECONFIGURATION_DETAIL35)
 */
 #if DOXYGEN_JS
 Undefined Dba::checkInstanceConfiguration(InstanceDef instance, Dictionary options) {}
@@ -1486,7 +1481,6 @@ std::shared_ptr<mysqlsh::mysql::ClassicSession> Dba::get_session(const shcore::A
 shcore::Value::Map_type_ref Dba::_check_instance_configuration(const shcore::Argument_list &args, bool allow_update) {
   shcore::Value::Map_type_ref ret_val(new shcore::Value::Map_type());
   shcore::Value::Array_type_ref errors(new shcore::Value::Array_type());
-  std::set<std::string> check_options;
 
   // Validates the connection options
   shcore::Value::Map_type_ref instance_def = get_instance_options_map(args, mysqlsh::dba::PasswordFormat::OPTIONS);
@@ -1509,16 +1503,14 @@ shcore::Value::Map_type_ref Dba::_check_instance_configuration(const shcore::Arg
     validate_options = args.map_at(1);
     shcore::Argument_map tmp_map(*validate_options);
 
+    std::set<std::string> check_options {"password", "dbPassword", "mycnfPath"};
     // The clearReadOnly option is only available in configureLocalInstance
     // i.e. with allow_update set as true
     if (allow_update) {
-      check_options = {"password", "dbPassword", "mycnfPath", "clusterAdmin",
-                       "clusterAdminPassword", "clearReadOnly"};
-    } else {
-      check_options = {"password", "dbPassword", "mycnfPath", "clusterAdmin",
-                       "clusterAdminPassword"};
+      check_options.insert("clusterAdmin");
+      check_options.insert("clusterAdminPassword");
+      check_options.insert("clearReadOnly");
     }
-
     tmp_map.ensure_keys({}, check_options, "validation options");
     validate_opt_map = tmp_map;
 
@@ -1549,6 +1541,19 @@ shcore::Value::Map_type_ref Dba::_check_instance_configuration(const shcore::Arg
   new_args.push_back(shcore::Value(instance_def));
   auto session = Dba::get_session(new_args);
 
+  // Validate the permissions of the user running the operation.
+  if (!validate_cluster_admin_user_privileges(session, session->get_user(),
+                                              session->get_host())) {
+    std::string error_msg =
+        "Account '" + session->get_user() + "'@'" +
+        session->get_host() +
+        "' does not have all the required privileges to execute this "
+        "operation. For more information, see the online documentation.";
+    log_error("%s", error_msg.c_str());
+    throw std::runtime_error(error_msg);
+  }
+
+  // Now validates the instance GR status itself
   std::string uri = session->uri();
 
   GRInstanceType type = get_gr_instance_type(session->connection());

modules/adminapi/mod_dba_common.cc

@@ -451,7 +451,7 @@ std::pair<int,int> find_cluster_admin_accounts(std::shared_ptr<mysqlsh::mysql::C
 static const std::set<std::string> k_global_privileges{
   "RELOAD", "SHUTDOWN", "PROCESS", "FILE",
   "SUPER", "REPLICATION SLAVE", "REPLICATION CLIENT",
-  "CREATE USER"
+  "CREATE USER", "SELECT"
 };
 
 // Schema privileges needed on the metadata schema
@@ -466,7 +466,6 @@ static const std::set<std::string> k_metadata_schema_privileges{
 // list of (schema, [privilege]) pairs, with the required privileges on each schema
 static const std::map<std::string, std::set<std::string>> k_schema_grants{
   {"mysql_innodb_cluster_metadata", k_metadata_schema_privileges},
-  {"performance_schema", {"SELECT"}},
   {"mysql", {"SELECT", "INSERT", "UPDATE", "DELETE"}} // need for mysql.plugin, mysql.user others
 };
 
@@ -560,8 +559,7 @@ bool validate_cluster_admin_user_privileges(std::shared_ptr<mysqlsh::mysql::Clas
 static const char *k_admin_user_grants[] = {
   "GRANT RELOAD, SHUTDOWN, PROCESS, FILE, SUPER, REPLICATION SLAVE, REPLICATION CLIENT, CREATE USER ON *.*",
   "GRANT ALL PRIVILEGES ON mysql_innodb_cluster_metadata.*",
-  "GRANT SELECT ON performance_schema.*",
-  "GRANT SELECT ON sys.*",
+  "GRANT SELECT ON *.*",
   "GRANT SELECT, INSERT, UPDATE, DELETE ON mysql.*"
 };
 

unittest/scripts/js_devapi/scripts/dba_adopt_from_gr_interactive.js

@@ -4,6 +4,14 @@ var deployed_here = reset_or_deploy_sandboxes();
 
 shell.connect({host: localhost, port: __mysql_sandbox_port1, user: 'root', password: 'root'});
 
+// Create root@<hostname> account with all privileges, required to create a
+// cluster.
+session.runSql("SET sql_log_bin = 0");
+session.runSql("CREATE USER 'root'@'" + hostname + "' IDENTIFIED BY 'root'");
+session.runSql("GRANT ALL PRIVILEGES ON *.* to 'root'@'" + hostname +
+    "'WITH GRANT OPTION");
+session.runSql("SET sql_log_bin = 1");
+
 //@ Create cluster
 if (__have_ssl)
   var cluster = dba.createCluster('testCluster', {memberSslMode: 'REQUIRED'});

unittest/scripts/js_devapi/scripts/dba_check_instance_configuration_session.js

@@ -9,6 +9,20 @@ var deployed_here = reset_or_deploy_sandbox(__mysql_sandbox_port1);
 //@ Check Instance Configuration ok with a session
 dba.checkInstanceConfiguration({host: localhost, port: __mysql_sandbox_port1, password:'root'});
 
+//@ Error: user has no privileges to run the checkInstanceConfiguration command (BUG#26609909)
+shell.connect({host: localhost, port: __mysql_sandbox_port1, user: 'root', password: 'root'});
+// Create account with all privileges but remove one of the necessary privileges
+session.runSql("SET sql_log_bin = 0");
+session.runSql("CREATE USER 'test_user'@'%'");
+session.runSql("GRANT ALL PRIVILEGES ON *.* to 'test_user'@'%' WITH GRANT OPTION");
+session.runSql("REVOKE SELECT on *.* FROM 'test_user'@'%'");
+session.runSql("SET sql_log_bin = 1");
+dba.checkInstanceConfiguration({host: localhost, port: __mysql_sandbox_port1, user: 'test_user', password:''});
+// Drop user
+session.runSql("SET sql_log_bin = 0");
+session.runSql("DROP user 'test_user'@'%'");
+session.runSql("SET sql_log_bin = 1");
+
 // Remove the sandbox
 if (deployed_here)
 	cleanup_sandbox(__mysql_sandbox_port1);

unittest/scripts/js_devapi/scripts/dba_configure_local_instance.js

@@ -1,14 +1,23 @@
 // Assumptions: smart deployment rountines available
-//@ Initialization
-var deployed_here = reset_or_deploy_sandboxes();
 
 //@<OUT> create GR admin account using configureLocalInstance
 var cnfPath1 = __sandbox_dir + __mysql_sandbox_port1 + "/my.cnf";
 dba.configureLocalInstance("root@localhost:"+__mysql_sandbox_port1, {mycnfPath: cnfPath1, dbPassword:'root', clusterAdmin: "gr_user", clusterAdminPassword: "root"});
 
+//@ Error: user has no privileges to run the configure command (BUG#26609909)
+shell.connect({host: localhost, port: __mysql_sandbox_port1, user: 'root', password: 'root'});
+// Remove select privilege to make sure an error is thrown
+session.runSql("SET sql_log_bin = 0");
+session.runSql("REVOKE SELECT on *.* FROM 'gr_user'@'%'");
+session.runSql("SET sql_log_bin = 1");
+dba.configureLocalInstance("gr_user@localhost:"+__mysql_sandbox_port1, {mycnfPath: cnfPath1, dbPassword:'root'});
+// restore select privilege to gr_user
+session.runSql("SET sql_log_bin = 0");
+session.runSql("GRANT SELECT on *.* TO 'gr_user'@'%'");
+session.runSql("SET sql_log_bin = 1");
+
 //@ create cluster using cluster admin account (BUG#26523629)
 shell.connect({host: localhost, port: __mysql_sandbox_port1, user: 'gr_user', password: 'root'});
-
 if (__have_ssl)
   var cluster = dba.createCluster('devCluster', {memberSslMode:'REQUIRED', clearReadOnly: true});
 else
@@ -65,8 +74,3 @@ wait_slave_state(cluster, 'third_sandbox', "ONLINE");
 //@<OUT> Check saved auto_inc settings are restored
 shell.connect({scheme: 'mysql', host: localhost, port: __mysql_sandbox_port3, user: 'root', password: 'root'});
 session.runSql("show global variables like 'auto_increment_%'").fetchAll();
-
-//@ Finalization
-shell.connect({scheme: 'mysql', host: localhost, port: __mysql_sandbox_port1, user: 'root', password: 'root'});
-session.runSql("DROP USER 'gr_user'@'%'");
-cleanup_sandboxes(deployed_here);

unittest/scripts/js_devapi/validation/dba_check_instance_configuration_session.js

@@ -6,3 +6,6 @@
 
 //@ Check Instance Configuration ok with a session
 ||
+
+//@ Error: user has no privileges to run the checkInstanceConfiguration command (BUG#26609909)
+||Dba.checkInstanceConfiguration: Account 'test_user'@'localhost' does not have all the required privileges to execute this operation. For more information, see the online documentation.

unittest/scripts/js_devapi/validation/dba_configure_local_instance.js

@@ -1,11 +1,11 @@
-//@ Initialization
-||
-
 //@<OUT> create GR admin account using configureLocalInstance
 {
     "status": "ok"
 }
 
+//@ Error: user has no privileges to run the configure command (BUG#26609909)
+||Dba.configureLocalInstance: Account 'gr_user'@'localhost' does not have all the required privileges to execute this operation. For more information, see the online documentation.
+
 //@ create cluster using cluster admin account (BUG#26523629)
 ||
 
@@ -146,6 +146,3 @@
         "2"
     ]
 ]
-
-//@ Finalization
-||

unittest/scripts/js_devapi/validation/dba_help.js

@@ -407,9 +407,6 @@ The options dictionary may contain the following options:
 
  - mycnfPath: The path of the MySQL configuration file for the instance.
  - password: The password to get connected to the instance.
- - clusterAdmin: The name of the InnoDB cluster administrator user.
- - clusterAdminPassword: The password for the InnoDB cluster administrator
-   account.
 
 The connection password may be contained on the instance definition, however,
 it can be overwritten if it is specified on the options.

unittest/scripts/js_devapi/validation/dba_interactive.js

@@ -122,13 +122,13 @@ Validating instance...
 |Number of accounts: 1|
 
 //@ Dba: configureLocalInstance not enough privileges 1
-||Dba.configureLocalInstance: Account 'missingprivileges'@'localhost' does not have all the privileges to create an user for managing an InnoDB cluster.
+||Dba.configureLocalInstance: Account 'missingprivileges'@'localhost' does not have all the required privileges to execute this operation. For more information, see the online documentation.
 
 //@ Dba: configureLocalInstance not enough privileges 2
-||Dba.configureLocalInstance: Account 'missingprivileges'@'localhost' does not have all the privileges to create an user for managing an InnoDB cluster.
+||Dba.configureLocalInstance: Account 'missingprivileges'@'localhost' does not have all the required privileges to execute this operation. For more information, see the online documentation.
 
 //@ Dba: configureLocalInstance not enough privileges 3
-||Dba.configureLocalInstance: Account 'missingprivileges'@'localhost' does not have all the privileges to create an user for managing an InnoDB cluster.
+||Dba.configureLocalInstance: Account 'missingprivileges'@'localhost' does not have all the required privileges to execute this operation. For more information, see the online documentation.
 
 //@ Dba: Show list of users to make sure the user missingprivileges@% was not created
 |Number of accounts: 0|

unittest/scripts/js_devapi/validation/dba_no_interactive.js

@@ -161,7 +161,7 @@
 |Number of accounts: 1|
 
 //@ Dba: configureLocalInstance not enough privileges
-||Dba.configureLocalInstance: Account 'missingprivileges'@'localhost' does not have all the privileges to create an user for managing an InnoDB cluster.
+||Dba.configureLocalInstance: Account 'missingprivileges'@'localhost' does not have all the required privileges to execute this operation. For more information, see the online documentation.
 
 //@ Dba: Show list of users to make sure the user missingprivileges@% was not created
 |Number of accounts: 0|

unittest/scripts/py_devapi/validation/dba_help.py

@@ -409,9 +409,6 @@
 
  - mycnfPath: The path of the MySQL configuration file for the instance.
  - password: The password to get connected to the instance.
- - clusterAdmin: The name of the InnoDB cluster administrator user.
- - clusterAdminPassword: The password for the InnoDB cluster administrator
-   account.
 
 The connection password may be contained on the instance definition, however,
 it can be overwritten if it is specified on the options.

unittest/scripts/py_devapi/validation/dba_interactive.py

@@ -122,13 +122,13 @@
 |Number of accounts: 1|
 
 #@ Dba: configure_local_instance not enough privileges 1
-||Dba.configure_local_instance: Account 'missingprivileges'@'localhost' does not have all the privileges to create an user for managing an InnoDB cluster.
+||Dba.configure_local_instance: Account 'missingprivileges'@'localhost' does not have all the required privileges to execute this operation. For more information, see the online documentation.
 
 #@ Dba: configure_local_instance not enough privileges 2
-||Dba.configure_local_instance: Account 'missingprivileges'@'localhost' does not have all the privileges to create an user for managing an InnoDB cluster.
+||Dba.configure_local_instance: Account 'missingprivileges'@'localhost' does not have all the required privileges to execute this operation. For more information, see the online documentation.
 
 #@ Dba: configure_local_instance not enough privileges 3
-||Dba.configure_local_instance: Account 'missingprivileges'@'localhost' does not have all the privileges to create an user for managing an InnoDB cluster.
+||Dba.configure_local_instance: Account 'missingprivileges'@'localhost' does not have all the required privileges to execute this operation. For more information, see the online documentation.
 
 #@ Dba: Show list of users to make sure the user missingprivileges@% was not created
 |Number of accounts: 0|

unittest/scripts/py_devapi/validation/dba_no_interactive.py

@@ -161,7 +161,7 @@
 |Number of accounts: 1|
 
 #@ Dba: configure_local_instance not enough privileges
-||Dba.configure_local_instance: Account 'missingprivileges'@'localhost' does not have all the privileges to create an user for managing an InnoDB cluster.
+||Dba.configure_local_instance: Account 'missingprivileges'@'localhost' does not have all the required privileges to execute this operation. For more information, see the online documentation.
 
 #@ Dba: Show list of users to make sure the user missingprivileges@% was not created
 |Number of accounts: 0|