graphile / migrate

Opinionated SQL-powered productive roll-forward migration tool for PostgreSQL.
MIT License
752 stars 58 forks source link

DATABASE_OWNER placeholder setting is overwritten with value from connection string #75

Closed Zehelein closed 4 years ago

Zehelein commented 4 years ago

The migration code is using the following code to first set the input placeholders - but overwrites the DB name and ownder name afterwards with values that come from the connection string:

  const placeholders = {
    ...parsedSettings.placeholders,
    ":DATABASE_NAME": database,
    ":DATABASE_OWNER": parsedSettings.databaseOwner,
  };

In general this is a very good approach to make sure that the DB name and owner are correct and not by error point to some other place or might not be set at all. But Azure PostgreSQL requires the login connection user name to have a namespace attached. The connection string for the user postgraphile_root and DBMS instance test would look like that: postgres://postgraphile_root%40test:secure_password@test.postgres.database.azure.com:5432/postgraphile

Where "@test" (escaped as "%40test") is the login namespace. That namespace is only used for authentication - but internally inside of your DBMS instance the user/role name is just postgraphile_root. If I set this postgraphile_root correctly in my placeholders the above code will overwrite it again as postgraphile_root@test - and then obviously fails the migration.

benjie commented 4 years ago

Well... This is certainly... Unexpected. It seems the username that you specify in the connection string for Azure may be used for routing, but does not reflect the actual username inside of the database. And since @ is a valid character in a postgresql role name:

[foo] # create user "foo@bar";
CREATE ROLE
[foo] # set role "foo@bar";
SET
[foo] # select user;
┌─────────┐
│  user   │
├─────────┤
│ foo@bar │
└─────────┘
(1 row)
[foo] # 

we can't simply trim @.* from the username part.

I'm not keen to add fudgy "if it's azure then trim it" logic, because that won't work if you connect to an IP address directly.

I'm not keen to connect to the database to determine the role, that seems unnecessarily expensive.

I'm also not keen to let users accidentally override :DATABASE_OWNER, as this is likely to cause serious issues.

It looks like if you use the library API rather than the CLI you may be able to override the databaseOwner here:

await migrate({
  connectionString: "postgres://postgraphile_root%40test:secure_password@test.postgres.database.azure.com:5432/postgraphile",
  databaseOwner: "postgraphile_root",
})

I'm not sure off-hand if this is liable to cause issues, in some places we may need to build a new role-specific connection string from a role and connectionString, and that clearly won't work on Azure.

Zehelein commented 4 years ago

yes - the "@..." part is likely used for routing to the correct DB instance. The part in front of it is the exact role name as it is in the DB. I can totally understand your concerns that you listed.

For my heroku setup for example I am using -U ${DATABASE_SUPERUSER}${DATABASE_LOGIN_NAMESPACE} \ - so maybe there could be a "namespace" configuration - and if that is set this is removed from the root username? Or have some "I know what I am doing with my root user" flag or something like that? Because otherwise I fear exactly what you mentioned - that at some point in the future that might break when internally something new is added.

Hmm.. I a quick search in Migration via github - and "DATABASE_OWNER" anyways just seems to be a convention. Maybe I just name mine something like "DB_OWNER" and it would already be fine?

benjie commented 4 years ago

Yeah, that’s probably your best bet for now 👍

Zehelein commented 4 years ago

I will close this ticket as there is a good workaround for me. And I agree with your comments that for all other reasons the approach you have is good.