Improve schema upgrade workflow for unprivileged users

Summary:
In a basically reasonable configuration where you connect
with a non-privileged user from the web workflow, upgrade_schema.php
won't have enough privileges. Allow the user to override the normal
auth with -u and -p.

Test Plan:
Tried to do a schema upgrade with an underprivileged user,
got a useful error message instead of garbage.

Reviewed By: Girish
Reviewers: Girish, davidrecordon, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, Girish
Differential Revision: 191

resources/sql/patches/033.privtest.sql

@@ -0,0 +1,2 @@
+# This is a no-op, just testing the upgrade_schema.php script.
+SELECT 1;

scripts/sql/upgrade_schema.php

@@ -25,14 +25,14 @@ phutil_require_module('phutil', 'console');
 
 define('SCHEMA_VERSION_TABLE_NAME', 'schema_version');
 
-if (isset($argv[1]) && !is_numeric($argv[1])) {
-  print
-    "USAGE: ./update_schema.php [first_patch_version]\n\n".
-    "run './update_schema.php 12' to apply all patches starting from ".
-    "version 12.\n".
-    "run './update_schema.php' to apply all patches that are new since\n".
-    "the last time this script was run\n\n";
-  exit(0);
+$options = getopt('v:u:p:') + array(
+  'v' => null,
+  'u' => null,
+  'p' => null,
+);
+
+if ($options['v'] && !is_numeric($options['v'])) {
+  usage();
 }
 
 echo phutil_console_wrap(
@@ -45,43 +45,44 @@ if (!phutil_console_confirm('Are you ready to continue?')) {
 }
 
 // Use always the version from the commandline if it is defined
-$next_version = isset($argv[1]) ? (int)$argv[1] : null;
+$next_version = isset($options['v']) ? (int)$options['v'] : null;
 
-// Dummy class needed for creating our database
-class DummyUser extends PhabricatorLiskDAO {
-  public function getApplicationName() {
-    return 'user';
-  }
+if ($options['u']) {
+  $conn_user = $options['u'];
+  $conn_pass = $options['p'];
+} else {
+  $conn_user = PhabricatorEnv::getEnvConfig('mysql.user');
+  $conn_pass = PhabricatorEnv::getEnvConfig('mysql.pass');
 }
+$conn_host = PhabricatorEnv::getEnvConfig('mysql.host');
 
-// Class needed for setting up the actual SQL connection
-class PhabricatorSchemaVersion extends PhabricatorLiskDAO {
-  public function getApplicationName() {
-    return 'meta_data';
-  }
-}
+$conn = new AphrontMySQLDatabaseConnection(
+  array(
+    'user'      => $conn_user,
+    'pass'      => $conn_pass,
+    'host'      => $conn_host,
+    'database'  => null,
+  ));
 
-// Connect to 'phabricator_user' db first to create our db
-$conn = id(new DummyUser())->establishConnection('w');
-$create_sql = <<<END
-CREATE DATABASE IF NOT EXISTS `phabricator_meta_data`;
+try {
+
+  $create_sql = <<<END
+  CREATE DATABASE IF NOT EXISTS `phabricator_meta_data`;
 END;
-queryfx($conn, $create_sql);
+  queryfx($conn, $create_sql);
 
-// 'phabricator_meta_data' database exists, let's connect to it now
-$conn = id(new PhabricatorSchemaVersion())->establishConnection('w');
-$create_sql = <<<END
-CREATE TABLE IF NOT EXISTS phabricator_meta_data.`schema_version` (
+  $create_sql = <<<END
+  CREATE TABLE IF NOT EXISTS phabricator_meta_data.`schema_version` (
     `version` INTEGER not null
-);
+  );
 END;
-queryfx($conn, $create_sql);
+  queryfx($conn, $create_sql);
 
-// Get the version only if commandline argument wasn't given
-if ($next_version === null) {
+  // Get the version only if commandline argument wasn't given
+  if ($next_version === null) {
     $version = queryfx_one(
       $conn,
-    'SELECT * FROM %T',
+      'SELECT * FROM phabricator_meta_data.%T',
       SCHEMA_VERSION_TABLE_NAME);
 
     if (!$version) {
@@ -92,16 +93,16 @@ if ($next_version === null) {
     }
 
     $next_version = $version['version'] + 1;
-}
+  }
 
-// Find the patch files
-$patches_dir = $root.'/resources/sql/patches/';
-$finder = id(new FileFinder($patches_dir))
+  // Find the patch files
+  $patches_dir = $root.'/resources/sql/patches/';
+  $finder = id(new FileFinder($patches_dir))
     ->withSuffix('sql');
-$results = $finder->find();
+  $results = $finder->find();
 
-$patches = array();
-foreach ($results as $r) {
+  $patches = array();
+  foreach ($results as $r) {
     $matches = array();
     if (preg_match('/(\d+)\..*\.sql$/', $r, $matches)) {
       $patches[] = array('version' => (int)$matches[1],
@@ -111,14 +112,14 @@ foreach ($results as $r) {
         "*** WARNING : File {$r} does not follow the normal naming ".
         "convention. ***\n";
     }
-}
+  }
 
-// Files are in some 'random' order returned by the operating system
-// We need to apply them in proper order
-$patches = isort($patches, 'version');
+  // Files are in some 'random' order returned by the operating system
+  // We need to apply them in proper order
+  $patches = isort($patches, 'version');
 
-$patch_applied = false;
-foreach ($patches as $patch) {
+  $patch_applied = false;
+  foreach ($patches as $patch) {
     if ($patch['version'] < $next_version) {
       continue;
     }
@@ -127,13 +128,12 @@ foreach ($patches as $patch) {
 
     $path = Filesystem::resolvePath($patches_dir.$patch['file']);
 
-  $user = PhabricatorEnv::getEnvConfig('mysql.user');
-  $pass = PhabricatorEnv::getEnvConfig('mysql.pass');
-  $host = PhabricatorEnv::getEnvConfig('mysql.host');
-
     list($stdout, $stderr) = execx(
       "mysql --user=%s --password=%s --host=%s < %s",
-    $user, $pass, $host, $path);
+      $conn_user,
+      $conn_pass,
+      $conn_host,
+      $path);
 
     if ($stderr) {
       print $stderr;
@@ -142,16 +142,44 @@ foreach ($patches as $patch) {
 
     // Patch was successful, update the db with the latest applied patch version
     // 'DELETE' and 'INSERT' instead of update, because the table might be empty
-  queryfx($conn, 'DELETE FROM %T', SCHEMA_VERSION_TABLE_NAME);
     queryfx(
       $conn,
-    'INSERT INTO %T values (%d)',
+      'DELETE FROM phabricator_meta_data.%T',
+      SCHEMA_VERSION_TABLE_NAME);
+    queryfx(
+      $conn,
+      'INSERT INTO phabricator_meta_data.%T VALUES (%d)',
       SCHEMA_VERSION_TABLE_NAME,
       $patch['version']);
 
     $patch_applied = true;
+  }
+
+  if (!$patch_applied) {
+    print "Your database is already up-to-date.\n";
+  }
+
+} catch (AphrontQueryAccessDeniedException $ex) {
+  echo
+    "ACCESS DENIED\n".
+    "The user '{$conn_user}' does not have sufficient MySQL privileges to\n".
+    "execute the schema upgrade. Use the -u and -p flags to run as a user\n".
+    "with more privileges (e.g., root).".
+    "\n\n".
+    "EXCEPTION:\n".
+    $ex->getMessage().
+    "\n\n";
+  exit(1);
 }
 
-if (!$patch_applied) {
-  print "Your database is already up-to-date.\n";
+function usage() {
+  echo
+    "usage: upgrade_schema.php [-v version] [-u user -p pass]".
+    "\n\n".
+    "Run 'upgrade_schema.php -v 12' to apply all patches starting from ".
+    "version 12.\n".
+    "Run 'upgrade_schema.php -u root -p hunter2' to override the configured ".
+    "default user.\n";
+  exit(1);
 }
+

src/__phutil_library_map__.php

@@ -45,6 +45,7 @@ phutil_register_library_map(array(
     'AphrontPageView' => 'view/page/base',
     'AphrontPagerView' => 'view/control/pager',
     'AphrontPanelView' => 'view/layout/panel',
+    'AphrontQueryAccessDeniedException' => 'storage/exception/accessdenied',
     'AphrontQueryConnectionException' => 'storage/exception/connection',
     'AphrontQueryConnectionLostException' => 'storage/exception/connectionlost',
     'AphrontQueryCountException' => 'storage/exception/count',
@@ -509,6 +510,7 @@ phutil_register_library_map(array(
     'AphrontPageView' => 'AphrontView',
     'AphrontPagerView' => 'AphrontView',
     'AphrontPanelView' => 'AphrontView',
+    'AphrontQueryAccessDeniedException' => 'AphrontQueryRecoverableException',
     'AphrontQueryConnectionException' => 'AphrontQueryException',
     'AphrontQueryConnectionLostException' => 'AphrontQueryRecoverableException',
     'AphrontQueryCountException' => 'AphrontQueryException',

src/docs/configuration_guide.diviner

@@ -119,4 +119,5 @@ change by providing overrides in ##myconfig.conf.php##.
 = Upgrading Schema =
 
 After you have configured Phabricator, you need to upgrade the database
-schema, see @{article:Upgrading Schema}
+schema, see @{article:Upgrading Schema}. You'll also need to do this after you
+update the code in the future.

src/docs/upgrade_schema.diviner

@@ -14,19 +14,25 @@ configured your Phabricator environment. If you haven't, see
 If you are doing this for the first time to a freshly installed MySQL database,
 run the following command:
 
-  PHABRICATOR_ENV=<your_config> php path/to/phabricator/scripts/sql/upgrade_schema.php 0
+  PHABRICATOR_ENV=<your_config> path/to/phabricator/scripts/sql/upgrade_schema.php -v 0
 
 This will install all the patches starting from 0. Running this script will
 store the information of the latest installed patch in the Phabricator database.
 Next time you want to upgrade your schema, just run:
 
-  PHABRICATOR_ENV=<your_config> php path/to/phabricator/scripts/sql/upgrade_schema.php
+  PHABRICATOR_ENV=<your_config> path/to/phabricator/scripts/sql/upgrade_schema.php
 
 This will install all the patches that are new since the last time you ran
 this script.
 
+If your configuration uses an unprivileged user to connect to the database, you
+may have to override the default user so the schema changes can be applied with
+root or some other admin user:
+
+  PHABRICATOR_ENV=<your_config> path/to/phabricator/scripts/sql/upgrade_schema.php -u <user> -p <pass>
+
 If you need to upgrade the schema starting from a specific patch, just run:
 
-  PHABRICATOR_ENV=<your_config> php path/to/phabricator/scripts/sql/upgrade_schema.php <patch_number>
+  PHABRICATOR_ENV=<your_config> path/to/phabricator/scripts/sql/upgrade_schema.php -v <patch_number>
 
 However, this isn't usually needed.

src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php

@@ -144,10 +144,12 @@ class AphrontMySQLDatabaseConnection extends AphrontDatabaseConnection {
         "{$error}.");
     }
 
+    if ($database !== null) {
       $ret = @mysql_select_db($database, $conn);
       if (!$ret) {
         $this->throwQueryException($conn);
       }
+    }
 
     $end = microtime(true);
 
@@ -249,6 +251,11 @@ class AphrontMySQLDatabaseConnection extends AphrontDatabaseConnection {
         // portable to parse the key out of the error and attach it to the
         // exception.
         throw new AphrontQueryDuplicateKeyException("{$errno}: {$error}");
+      case 1044: // Access denied to database
+      case 1045: // Access denied (auth)
+      case 1142: // Access denied to table
+      case 1143: // Access denied to column
+        throw new AphrontQueryAccessDeniedException("#{$errno}: {$error}");
       default:
         // TODO: 1064 is syntax error, and quite terrible in production.
         throw new AphrontQueryException("#{$errno}: {$error}");

src/storage/connection/mysql/__init__.php

@@ -8,6 +8,7 @@
 
 phutil_require_module('phabricator', 'aphront/console/plugin/services/api');
 phutil_require_module('phabricator', 'storage/connection/base');
+phutil_require_module('phabricator', 'storage/exception/accessdenied');
 phutil_require_module('phabricator', 'storage/exception/base');
 phutil_require_module('phabricator', 'storage/exception/connection');
 phutil_require_module('phabricator', 'storage/exception/connectionlost');

src/storage/exception/accessdenied/AphrontQueryAccessDeniedException.php

@@ -0,0 +1,23 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * @group storage
+ */
+class AphrontQueryAccessDeniedException
+  extends AphrontQueryRecoverableException { }

src/storage/exception/accessdenied/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'storage/exception/recoverable');
+
+
+phutil_require_source('AphrontQueryAccessDeniedException.php');