Page MenuHomePhabricator

Make "bin/storage upgrade" deal with GRANT issues more gracefully
Closed, ResolvedPublic

Description

See https://discourse.phabricator-community.org/t/fixed-2021-week-8-storage-upgrade-fails-with-access-denied/4620.

If an install is missing TEMPORARY TABLES permission, phabricator:20210215.changeset.02.phid-populate.php fails with an opaque error (see also T13613):

[2021-02-26 14:46:49] EXCEPTION: (AphrontAccessDeniedQueryException) #1044: Access denied for user 'phabricator'@'our_phabricator_servver' to database 'phabricator_differential' at [<phabricator>/src/infrastructure/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:350]

This could be improved by:

  • Catching MySQL error 1044 and contextualizing it ("This may be caused by a missing GRANT permission."), since the message is opaque and does not lead users toward the solution.
  • Running a self-test for required GRANT permissions at startup in bin/storage upgrade before applying patches.

Event Timeline

epriestley triaged this task as Wishlist priority.Feb 26 2021, 6:24 PM
epriestley created this task.

I looked into this briefly, but I can't find a simple way to show all the current user's database permissions.

SHOW GRANTS is the obvious starting point, but the syntax seems very complex:

https://dev.mysql.com/doc/refman/8.0/en/show-grants.html

In particular, you can GRANT a set of permissions to a group, then grant the group to a user, and grants may be applied to a database or table pattern. Figuring out if we have a particular permission on a particular database seems quite complicated.

There are also already a bunch of tailored messages in other context:

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);

...and another for .sql patches:

} catch (AphrontAccessDeniedQueryException $ex) {
  throw new PhutilProxyException(
    pht(
      'Unable to access a required database or table. This almost '.
      'always means that the user you are connecting with ("%s") does '.
      'not have sufficient permissions granted in MySQL. You can '.
      'use `bin/storage databases` to get a list of all databases '.
      'permission is required on.',
      $this->getUser()),
    $ex);
}

I'm going to do this:

  • Slightly improve the "Access denied" error text.
  • Narrowly tailor error handling for the "TEMPORARY TABLE" query. If this becomes common, it could be moved to a migration helper (...::createTemporaryTable(...)).