BUG#26979375 : ADMIN API COMMANDS FAILING WHEN THE USER CONNECTED TO ROOT@%

This patch fixes an issue with the account validation being done on
the check_instance_configuration method that did not work correctly
unless the session account existed.  With this patch, that validation
is now done using the account that was authenticated by the server,
i.e., the account returned by the SELECT CURRENT_USER() SQL query.
Furthermore, it now also makes sure that the correct account is used
by the MP backend on the check command.

New tests were added to verify the fix.

Author: Nelson Goncalves

modules/adminapi/mod_dba.cc

@@ -19,6 +19,9 @@
 
 #include <string>
 #include <random>
+#include <mysqld_error.h>
+
+
 #ifndef WIN32
 #include <sys/un.h>
 #endif
@@ -1588,6 +1591,12 @@ shcore::Value::Map_type_ref Dba::_check_instance_configuration(const shcore::Arg
   shcore::Value::Map_type_ref validate_options;
 
   std::string cnfpath, cluster_admin, cluster_admin_password;
+  std::string admin_user, admin_user_host, validate_user, validate_host;
+  std::string current_user, current_host, account_type, error_msg_extra;
+  std::string uri_user = instance_def->get_string(
+      instance_def->has_key("user") ? "user" : "dbUser");
+  std::string uri_host = instance_def->get_string("host");
+  bool cluster_admin_user_exists = false;
   bool clear_read_only = false;
 
   if (args.size() == 2) {
@@ -1632,18 +1641,80 @@ 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);
 
+  // get the current user
+  auto result = session->execute_sql("SELECT CURRENT_USER()");
+  auto row = result->fetch_one();
+  std::string current_account = row->get_value(0).as_string();
+  split_account(current_account, &current_user, &current_host);
+
+  // if this is a configureLocalInstance operation and the clusterAdmin
+  // option was used and that user exists, validate its privileges, otherwise
+  // validate the privileges of the current user
+  if (allow_update && !cluster_admin.empty()) {
+    shcore::split_account(cluster_admin, &admin_user, &admin_user_host);
+    // Host '%' is used by default if not provided in the user account.
+    if (admin_user_host.empty())
+      admin_user_host = "%";
+    // Check if the cluster_admin user exists
+    shcore::sqlstring query = shcore::sqlstring(
+        "SELECT COUNT(*) from mysql.user WHERE "
+        "User = ? AND Host = ?", 0);
+    query << admin_user << admin_user_host;
+    query.done();
+    try {
+      auto result = session->execute_sql(query);
+      cluster_admin_user_exists =
+          result->fetch_one()->get_value(0).as_int() != 0;
+    } catch (shcore::Exception &err) {
+      if (err.code() == ER_TABLEACCESS_DENIED_ERROR) {
+        // the current_account doesn't have enough privileges to execute
+        // the query
+        std::string error_msg;
+        if (uri_host != current_host || uri_user != current_user)
+          error_msg =
+              "Session account '" + current_user + "'@'" + current_host +
+              "'(used to authenticate '" + uri_user + "'@'" + uri_host +
+              "') does not have all the required privileges to execute this "
+              "operation. For more information, see the online documentation.";
+        else
+          error_msg =
+              "Session account '" + current_user + "'@'" + current_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);
+      } else {
+        throw;
+      }
+    }
+  }
+  if (cluster_admin_user_exists) {
+    // cluster admin account exists, so we will validate its privileges
+    validate_user = admin_user;
+    validate_host = admin_user_host;
+    account_type = "Cluster Admin";
+  } else {
+    // cluster admin doesn't exist, so we validate the privileges of the
+    // current user
+    validate_user = current_user;
+    validate_host = current_host;
+    account_type = "Session";
+    if (uri_host != current_host || uri_user != current_user)
+      error_msg_extra =
+          " (used to authenticate '" + uri_user + "'@'" + uri_host + "')";
+  }
   // Validate the permissions of the user running the operation.
-  if (!validate_cluster_admin_user_privileges(session, session->get_user(),
-                                              session->get_host())) {
+  if (!validate_cluster_admin_user_privileges(session, validate_user,
+                                              validate_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.";
+        account_type + " account '" + validate_user + "'@'" + validate_host +
+        "'" + error_msg_extra +
+        " 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();
 
@@ -1658,8 +1729,7 @@ shcore::Value::Map_type_ref Dba::_check_instance_configuration(const shcore::Arg
   else if (type == GRInstanceType::InnoDBCluster && !allow_update) {
     session->close(shcore::Argument_list());
     throw shcore::Exception::runtime_error("The instance '" + uri + "' is already part of an InnoDB Cluster");
-  }
-  else {
+  } else {
     if (!cluster_admin.empty() && allow_update) {
       auto classic =
           dynamic_cast<mysqlsh::mysql::ClassicSession*>(session.get());
@@ -1669,39 +1739,16 @@ shcore::Value::Map_type_ref Dba::_check_instance_configuration(const shcore::Arg
       // and right before some execution failure of the command leaving the
       // instance in an incorrect state
       bool super_read_only = validate_super_read_only(classic, clear_read_only);
-
-      try {
-        create_cluster_admin_user(session,
-                                  cluster_admin,
-                                  cluster_admin_password);
-      } catch (shcore::Exception &err) {
-        // Catch ER_CANNOT_USER (1396) if the user already exists, and skip it
-        // if the user has the needed privileges.
-        if (err.code() == ER_CANNOT_USER) {
-          std::string admin_user, admin_user_host;
-          shcore::split_account(cluster_admin, &admin_user, &admin_user_host);
-          // Host '%' is used by default if not provided in the user account.
-          if (admin_user_host.empty())
-            admin_user_host = "%";
-          if (!validate_cluster_admin_user_privileges(session, admin_user,
-                                                      admin_user_host)) {
-            std::string error_msg =
-                "User " + cluster_admin + " already exists but it does not "
-                "have all the privileges for managing an InnoDB cluster. "
-                "Please provide a non-existing user to be created or a "
-                "different one with all the required privileges.";
-            errors->push_back(shcore::Value(error_msg));
-            log_error("%s", error_msg.c_str());
-          } else {
-            log_warning("User %s already exists.", cluster_admin.c_str());
-          }
-        } else {
+      if (!cluster_admin_user_exists) {
+        try {
+          create_cluster_admin_user(session, cluster_admin,
+                                    cluster_admin_password);
+        } catch (shcore::Exception &err) {
           std::string error_msg = "Unexpected error creating " + cluster_admin +
                                   " user: " + err.what();
           errors->push_back(shcore::Value(error_msg));
           log_error("%s", error_msg.c_str());
         }
-
         // If we disabled super_read_only we must enable it back
         // also confirm that the initial status was 1/ON
         if (clear_read_only && super_read_only) {
@@ -1710,12 +1757,12 @@ shcore::Value::Map_type_ref Dba::_check_instance_configuration(const shcore::Arg
                    session_address.c_str());
           set_global_variable(classic->connection(), "super_read_only", "ON");
         }
+      } else {
+        log_warning("User %s already exists.", cluster_admin.c_str());
       }
     }
-
-    std::string host = instance_def->get_string("host");
     int port = instance_def->get_int("port");
-    std::string endpoint = host + ":" + std::to_string(port);
+    std::string endpoint = uri_host + ":" + std::to_string(port);
 
     if (type == GRInstanceType::InnoDBCluster) {
       auto seeds = get_peer_seeds(session->connection(), endpoint);
@@ -1725,8 +1772,16 @@ shcore::Value::Map_type_ref Dba::_check_instance_configuration(const shcore::Arg
 
     std::string user;
     std::string password;
-    user = instance_def->get_string(instance_def->has_key("user") ? "user" : "dbUser");
-    password = instance_def->get_string(instance_def->has_key("password") ? "password" : "dbPassword");
+    // If a clusterAdmin account was specified, use it, otherwise user the
+    // user of the instance.
+    if (!cluster_admin.empty()) {
+      user = admin_user;
+      password = cluster_admin_password;
+    } else {
+      user = uri_user;
+      password = instance_def->get_string(
+          instance_def->has_key("password") ? "password" : "dbPassword");
+    }
 
     // Get SSL values to connect to instance
     Value::Map_type_ref instance_ssl_opts(new shcore::Value::Map_type);
@@ -1740,7 +1795,9 @@ shcore::Value::Map_type_ref Dba::_check_instance_configuration(const shcore::Arg
     // Verbose is mandatory for checkInstanceConfiguration
     shcore::Value::Array_type_ref mp_errors;
 
-    if ((_provisioning_interface->check(user, host, port, password, instance_ssl_opts, cnfpath, allow_update, mp_errors) == 0)) {
+    if ((_provisioning_interface->check(user, uri_host, port, password,
+                                        instance_ssl_opts, cnfpath,
+                                        allow_update, mp_errors) == 0)) {
       // Only return status "ok" if no previous errors were found.
       if (errors->size() == 0) {
         (*ret_val)["status"] = shcore::Value("ok");

modules/adminapi/mod_dba.h

@@ -31,8 +31,6 @@
 #include "mod_dba_provisioning_interface.h"
 #include "modules/adminapi/mod_dba_common.h"
 
-#define ER_CANNOT_USER 1396
-
 namespace mysqlsh {
 namespace dba {
 /**

modules/adminapi/mod_dba_common.cc

@@ -534,7 +534,7 @@ 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},
-  {"mysql", {"SELECT", "INSERT", "UPDATE", "DELETE"}} // need for mysql.plugin, mysql.user others
+  {"mysql", {"INSERT", "UPDATE", "DELETE"}} // need for mysql.plugin, mysql.user others
 };
 
 /** Check that the provided account has privileges to manage a cluster.
@@ -628,7 +628,7 @@ 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 *.*",
-  "GRANT SELECT, INSERT, UPDATE, DELETE ON mysql.*"
+  "GRANT INSERT, UPDATE, DELETE ON mysql.*"
 };
 
 void create_cluster_admin_user(std::shared_ptr<mysqlsh::mysql::ClassicSession> session,

unittest/scripts/js_devapi/scripts/dba_check_instance_configuration_session.js

@@ -18,7 +18,27 @@ 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:''});
+session.close()
+
+//@ Check instance configuration using a non existing user that authenticates as another user that does not have enough privileges (BUG#26979375)
+shell.connect({host: hostname, port: __mysql_sandbox_port1, user: 'test_user', password: ''});
+dba.checkInstanceConfiguration({host: hostname, port: __mysql_sandbox_port1, user: 'test_user', password:''});
+session.close()
+
+//@ Check instance configuration using a non existing user that authenticates as another user that has all privileges (BUG#26979375)
+// Grant the missing privilege again
+shell.connect({host: localhost, port: __mysql_sandbox_port1, user: 'root', password: 'root'});
+session.runSql("SET sql_log_bin = 0");
+session.runSql("GRANT SELECT on *.* TO 'test_user'@'%'");
+session.runSql("SET sql_log_bin = 1");
+session.close()
+
+shell.connect({host: hostname, port: __mysql_sandbox_port1, user: 'test_user', password: ''});
+dba.checkInstanceConfiguration({host: hostname, port: __mysql_sandbox_port1, user: 'test_user', password:''});
+session.close()
+
 // Drop user
+shell.connect({host: localhost, port: __mysql_sandbox_port1, user: 'root', password: 'root'});
 session.runSql("SET sql_log_bin = 0");
 session.runSql("DROP user 'test_user'@'%'");
 session.runSql("SET sql_log_bin = 1");

unittest/scripts/js_devapi/scripts/dba_configure_local_instance.js

@@ -11,6 +11,12 @@ 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'});
+//@ Error: session user has privileges to run the configure command but we pass it an existing clusterAdmin user that doesn't have enough privileges (BUG#26979375)
+dba.configureLocalInstance("root@localhost:"+__mysql_sandbox_port1, {mycnfPath: cnfPath1, dbPassword:'root', clusterAdmin: 'gr_user', clusterAdminPassword: "root"});
+
+//@ Session user has privileges to run the configure command and we pass it an non existing clusterAdmin user(BUG#26979375)
+dba.configureLocalInstance("root@localhost:"+__mysql_sandbox_port1, {mycnfPath: cnfPath1, dbPassword:'root', clusterAdmin: 'gr_user2', clusterAdminPassword: "root"});
+
 // restore select privilege to gr_user
 session.runSql("SET sql_log_bin = 0");
 session.runSql("GRANT SELECT on *.* TO 'gr_user'@'%'");

unittest/scripts/js_devapi/scripts/dba_interactive.js

@@ -67,6 +67,31 @@ var restored_sql_mode = row[0];
 var was_restored = restored_sql_mode == original_sql_mode;
 print("Original SQL_MODE has been restored: "+ was_restored + "\n");
 
+//@ Dba: create cluster using a non existing user that authenticates as another user (BUG#26979375)
+// Clear super read_only
+session.runSql("set GLOBAL super_read_only = 0");
+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("SET sql_log_bin = 1");
+session.close()
+
+shell.connect({host: hostname, port: __mysql_sandbox_port1, user: 'test_user', password: ''});
+c1 = dba.createCluster("devCluster", {clearReadOnly: true});
+c1
+
+//@ Dba: dissolve cluster created using a non existing user that authenticates as another user (BUG#26979375)
+c1.dissolve({force:true});
+session.close()
+shell.connect({host: localhost, port: __mysql_sandbox_port1, user: 'root', password: 'root'});
+
+// drop created test_user
+// Clear super read_only
+session.runSql("set GLOBAL super_read_only = 0");
+session.runSql("SET sql_log_bin = 0");
+session.runSql("DROP user 'test_user'@'%'");
+session.runSql("SET sql_log_bin = 1");
+
 //@<OUT> Dba: createCluster with interaction
 if (__have_ssl)
   var c1 = dba.createCluster('devCluster', {memberSslMode: 'REQUIRED', clearReadOnly: true});
@@ -184,7 +209,7 @@ session.runSql("REVOKE REPLICATION SLAVE ON *.* FROM 'dba_test'@'%'");
 session.runSql("SET SQL_LOG_BIN=1");
 session.close();
 
-//@<OUT> Dba: configureLocalInstance create existing invalid admin user
+//@ Dba: configureLocalInstance create existing invalid admin user
 // Regression for BUG#25519190 : CONFIGURELOCALINSTANCE() FAILS UNGRACEFUL IF CALLED TWICE
 dba.configureLocalInstance('mydba@localhost:' + __mysql_sandbox_port2);
 

unittest/scripts/js_devapi/scripts/dba_no_interactive.js

@@ -198,7 +198,7 @@ session.runSql("REVOKE REPLICATION SLAVE ON *.* FROM 'dba_test'@'%'");
 session.runSql("SET SQL_LOG_BIN=1");
 session.close();
 
-//@<OUT> Dba: configureLocalInstance create existing invalid admin user
+//@ Dba: configureLocalInstance create existing invalid admin user
 // Regression for BUG#25519190 : CONFIGURELOCALINSTANCE() FAILS UNGRACEFUL IF CALLED TWICE
 dba.configureLocalInstance('mydba:@localhost:' + __mysql_sandbox_port2,
     {clusterAdmin: "dba_test", clusterAdminPassword:"",

unittest/scripts/js_devapi/validation/dba_check_instance_configuration_session.js

@@ -8,4 +8,10 @@
 ||
 
 //@ 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.
+||Dba.checkInstanceConfiguration: Session account 'test_user'@'%' (used to authenticate 'test_user'@'<<<localhost>>>') does not have all the required privileges to execute this operation. For more information, see the online documentation.
+
+//@ Check instance configuration using a non existing user that authenticates as another user that does not have enough privileges (BUG#26979375)
+||Dba.checkInstanceConfiguration: Session account 'test_user'@'%' (used to authenticate 'test_user'@'<<<hostname>>>') does not have all the required privileges to execute this operation. For more information, see the online documentation.
+
+//@ Check instance configuration using a non existing user that authenticates as another user that has all privileges (BUG#26979375)
+||

unittest/scripts/js_devapi/validation/dba_configure_local_instance.js

@@ -4,7 +4,13 @@
 }
 
 //@ 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.
+||Dba.configureLocalInstance: Session account 'gr_user'@'%' (used to authenticate 'gr_user'@'<<<localhost>>>') does not have all the required privileges to execute this operation. For more information, see the online documentation.
+
+//@ Error: session user has privileges to run the configure command but we pass it an existing clusterAdmin user that doesn't have enough privileges (BUG#26979375)
+||Dba.configureLocalInstance: Cluster Admin account 'gr_user'@'%' does not have all the required privileges to execute this operation. For more information, see the online documentation.
+
+//@ Session user has privileges to run the configure command and we pass it an non existing clusterAdmin user(BUG#26979375)
+||
 
 //@ create cluster using cluster admin account (BUG#26523629)
 ||