From d37153f003048cb6608d9c97812e5564d5e7339a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 21 Jan 2016 12:59:19 -0800 Subject: [PATCH] Make `bin/storage upgrade` and `bin/storage adjust` emit detailed messages if the user has no access to databases Summary: Ref T10195. Distinguish between "database does not exist" and "database exists, you just don't have permission to access it". We can't easily get this information out of INFORMATION_SCHEMA but can just `SHOW TABLES IN ...` every database that looks like it's missing and then look at the error code. Test Plan: - Created a user `limited` with limited access. - Ran `bin/storage adjust`. - Got hopefully more helpful messages about access problems, instead of "Missing" errors. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10195 Differential Revision: https://secure.phabricator.com/D15079 --- .../PhabricatorConfigDatabaseSchema.php | 24 ++++++++++--- .../schema/PhabricatorConfigSchemaQuery.php | 27 ++++++++++++++ .../schema/PhabricatorConfigStorageSchema.php | 4 +++ .../PhabricatorStorageManagementAPI.php | 10 ++++++ .../PhabricatorStorageManagementWorkflow.php | 35 ++++++++++++++++++- 5 files changed, 94 insertions(+), 6 deletions(-) diff --git a/src/applications/config/schema/PhabricatorConfigDatabaseSchema.php b/src/applications/config/schema/PhabricatorConfigDatabaseSchema.php index bf0af3f480..216a778e56 100644 --- a/src/applications/config/schema/PhabricatorConfigDatabaseSchema.php +++ b/src/applications/config/schema/PhabricatorConfigDatabaseSchema.php @@ -6,6 +6,7 @@ final class PhabricatorConfigDatabaseSchema private $characterSet; private $collation; private $tables = array(); + private $accessDenied; public function addTable(PhabricatorConfigTableSchema $table) { $key = $table->getName(); @@ -33,12 +34,16 @@ final class PhabricatorConfigDatabaseSchema PhabricatorConfigStorageSchema $expect) { $issues = array(); - if ($this->getCharacterSet() != $expect->getCharacterSet()) { - $issues[] = self::ISSUE_CHARSET; - } + if ($this->getAccessDenied()) { + $issues[] = self::ISSUE_ACCESSDENIED; + } else { + if ($this->getCharacterSet() != $expect->getCharacterSet()) { + $issues[] = self::ISSUE_CHARSET; + } - if ($this->getCollation() != $expect->getCollation()) { - $issues[] = self::ISSUE_COLLATION; + if ($this->getCollation() != $expect->getCollation()) { + $issues[] = self::ISSUE_COLLATION; + } } return $issues; @@ -68,4 +73,13 @@ final class PhabricatorConfigDatabaseSchema return $this->characterSet; } + public function setAccessDenied($access_denied) { + $this->accessDenied = $access_denied; + return $this; + } + + public function getAccessDenied() { + return $this->accessDenied; + } + } diff --git a/src/applications/config/schema/PhabricatorConfigSchemaQuery.php b/src/applications/config/schema/PhabricatorConfigSchemaQuery.php index c1e94619ce..ca9b44cc07 100644 --- a/src/applications/config/schema/PhabricatorConfigSchemaQuery.php +++ b/src/applications/config/schema/PhabricatorConfigSchemaQuery.php @@ -47,6 +47,25 @@ final class PhabricatorConfigSchemaQuery extends Phobject { $databases); $database_info = ipull($database_info, null, 'SCHEMA_NAME'); + // Find databases which exist, but which the user does not have permission + // to see. + $invisible_databases = array(); + foreach ($databases as $database_name) { + if (isset($database_info[$database_name])) { + continue; + } + + try { + queryfx($conn, 'SHOW TABLES IN %T', $database_name); + } catch (AphrontAccessDeniedQueryException $ex) { + // This database exists, the user just doesn't have permission to + // see it. + $invisible_databases[] = $database_name; + } catch (AphrontSchemaQueryException $ex) { + // This database is legitimately missing. + } + } + $sql = array(); foreach ($tables as $table) { $sql[] = qsprintf( @@ -148,6 +167,13 @@ final class PhabricatorConfigSchemaQuery extends Phobject { $server_schema->addDatabase($database_schema); } + foreach ($invisible_databases as $database_name) { + $server_schema->addDatabase( + id(new PhabricatorConfigDatabaseSchema()) + ->setName($database_name) + ->setAccessDenied(true)); + } + return $server_schema; } @@ -194,6 +220,7 @@ final class PhabricatorConfigSchemaQuery extends Phobject { if (!$actual_database) { $actual_database = $expect_database->newEmptyClone(); } + if (!$expect_database) { $expect_database = $actual_database->newEmptyClone(); } diff --git a/src/applications/config/schema/PhabricatorConfigStorageSchema.php b/src/applications/config/schema/PhabricatorConfigStorageSchema.php index 74856053e4..706be80568 100644 --- a/src/applications/config/schema/PhabricatorConfigStorageSchema.php +++ b/src/applications/config/schema/PhabricatorConfigStorageSchema.php @@ -17,6 +17,7 @@ abstract class PhabricatorConfigStorageSchema extends Phobject { const ISSUE_SUBFAIL = 'subfail'; const ISSUE_AUTOINCREMENT = 'autoincrement'; const ISSUE_UNKNOWN = 'unknown'; + const ISSUE_ACCESSDENIED = 'accessdenied'; const STATUS_OKAY = 'okay'; const STATUS_WARN = 'warn'; @@ -130,6 +131,8 @@ abstract class PhabricatorConfigStorageSchema extends Phobject { return pht('Column has Wrong Autoincrement'); case self::ISSUE_UNKNOWN: return pht('Column Has No Specification'); + case self::ISSUE_ACCESSDENIED: + return pht('Access Denied'); default: throw new Exception(pht('Unknown schema issue "%s"!', $issue)); } @@ -175,6 +178,7 @@ abstract class PhabricatorConfigStorageSchema extends Phobject { public static function getIssueStatus($issue) { switch ($issue) { case self::ISSUE_MISSING: + case self::ISSUE_ACCESSDENIED: case self::ISSUE_SURPLUS: case self::ISSUE_NULLABLE: case self::ISSUE_SUBFAIL: diff --git a/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php b/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php index 76e8e0a057..ae83be216c 100644 --- a/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php +++ b/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php @@ -123,6 +123,16 @@ final class PhabricatorStorageManagementAPI extends Phobject { 'SELECT patch FROM %T', self::TABLE_STATUS); return ipull($applied, 'patch'); + } catch (AphrontAccessDeniedQueryException $ex) { + throw new PhutilProxyException( + pht( + 'Failed while trying to read schema status: the database "%s" '. + 'exists, but the current user ("%s") does not have permission to '. + 'access it. GRANT the current user more permissions, or use a '. + 'different user.', + $this->getDatabaseName('meta_data'), + $this->getUser()), + $ex); } catch (AphrontQueryException $ex) { return null; } diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php index cbb88ce818..cc917215ee 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php +++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php @@ -422,6 +422,11 @@ abstract class PhabricatorStorageManagementWorkflow continue; } + if ($actual_database->getAccessDenied()) { + // If we can't access the database, we can't access the tables either. + continue; + } + $issues = array(); if ($database->hasIssue($issue_charset)) { $issues[] = $issue_charset; @@ -634,6 +639,8 @@ abstract class PhabricatorStorageManagementWorkflow $any_surplus = false; $all_surplus = true; + $any_access = false; + $all_access = true; foreach ($errors as $error) { $pieces = array_select_keys( $error, @@ -643,12 +650,20 @@ abstract class PhabricatorStorageManagementWorkflow $name = PhabricatorConfigStorageSchema::getIssueName($error['issue']); - if ($error['issue'] === PhabricatorConfigStorageSchema::ISSUE_SURPLUS) { + $issue = $error['issue']; + + if ($issue === PhabricatorConfigStorageSchema::ISSUE_SURPLUS) { $any_surplus = true; } else { $all_surplus = false; } + if ($issue === PhabricatorConfigStorageSchema::ISSUE_ACCESSDENIED) { + $any_access = true; + } else { + $all_access = false; + } + $table->addRow( array( 'target' => $target, @@ -668,11 +683,24 @@ abstract class PhabricatorStorageManagementWorkflow 'does not expect). For information on resolving these '. 'issues, see the "Surplus Schemata" section in the "Managing Storage '. 'Adjustments" article in the documentation.'); + } else if ($all_access) { + $message[] = pht( + 'The user you are connecting to MySQL with does not have the correct '. + 'permissions, and can not access some databases or tables that it '. + 'needs to be able to access. GRANT the user additional permissions.'); } else { $message[] = pht( 'The schemata have errors (detailed above) which the adjustment '. 'workflow can not fix.'); + if ($any_access) { + $message[] = pht( + 'Some of these errors are caused by access control problems. '. + 'The user you are connecting with does not have permission to see '. + 'all of the database or tables that Phabricator uses. You need to '. + 'GRANT the user more permission, or use a different user.'); + } + if ($any_surplus) { $message[] = pht( 'Some of these errors are caused by surplus schemata (extra '. @@ -698,6 +726,11 @@ abstract class PhabricatorStorageManagementWorkflow "** %s **\n\n%s\n", pht('SURPLUS SCHEMATA'), phutil_console_wrap($message)); + } else if ($all_access) { + $console->writeOut( + "** %s **\n\n%s\n", + pht('ACCESS DENIED'), + phutil_console_wrap($message)); } else { $console->writeOut( "** %s **\n\n%s\n",