Reset using connectionString instead of rootConnectionString?

dts commented 4 years ago

I am trying to "start from scratch" using graphile/migrate to manage a database. I'm making strides, but I am pretty sure there are some issues with graphile-migrate reset - it throws permissions errors when I reset the database. I'm trying to do things exactly as in starter, just writing things from scratch. here are some samples:

BEGIN;
GRANT CONNECT ON DATABASE :DATABASE_NAME TO :DATABASE_OWNER;
GRANT CONNECT ON DATABASE :DATABASE_NAME TO :DATABASE_AUTHENTICATOR;
GRANT ALL ON DATABASE :DATABASE_NAME TO :DATABASE_OWNER;

-- Some extensions require superuser privileges, so we create them before migration time.
CREATE EXTENSION IF NOT EXISTS plpgsql WITH SCHEMA pg_catalog;
CREATE EXTENSION IF NOT EXISTS "uuid-ossp" WITH SCHEMA public;
CREATE EXTENSION IF NOT EXISTS citext WITH SCHEMA public;
CREATE EXTENSION IF NOT EXISTS pgcrypto WITH SCHEMA public;
COMMIT;

  export DATABASE_URL="postgresql://${DATABASE_AUTHENTICATOR}:${DATABASE_AUTHENTICATOR_PASSWORD}@${POSTGRES_HOST}:5432/${DATABASE_NAME}"
  export SHADOW_DATABASE_URL="postgresql://${DATABASE_AUTHENTICATOR}:${DATABASE_AUTHENTICATOR_PASSWORD}@${POSTGRES_HOST}:5432/${DATABASE_NAME}_shadow"
  export ROOT_DATABASE_URL="postgresql://${DATABASE_OWNER}:${DATABASE_OWNER_PASSWORD}@${POSTGRES_HOST}/${POSTGRES_DB}"

and this is the database initialization script:

   await this.pg(`DROP DATABASE IF EXISTS "${env.DATABASE_NAME};"`);
    await this.pg(`DROP DATABASE IF EXISTS "${env.DATABASE_NAME}_shadow";`);
    await this.pg(`DROP DATABASE IF EXISTS "${env.DATABASE_NAME}_test";`);

    await this.pg(`DROP ROLE IF EXISTS "${env.DATABASE_VISITOR}";`);
    await this.pg(`DROP ROLE IF EXISTS "${env.DATABASE_AUTHENTICATOR}";`);
    await this.pg(`DROP ROLE IF EXISTS "${env.DATABASE_OWNER}";`);

    await this.pg(`CREATE ROLE "${env.DATABASE_OWNER}" WITH LOGIN PASSWORD '${env.DATABASE_OWNER_PASSWORD}' ${env.NODE_ENV == 'development' ? 'SUPERUSER' : ''};`);
    await this.pg(`CREATE ROLE "${env.DATABASE_AUTHENTICATOR}" WITH LOGIN PASSWORD '${env.DATABASE_AUTHENTICATOR_PASSWORD}' NOINHERIT;`);

    await this.pg(`CREATE ROLE "${env.DATABASE_VISITOR}";`);

    await this.pg(`GRANT "${env.DATABASE_VISITOR}" TO "${env.DATABASE_AUTHENTICATOR}";`);

Everything works correctly if I do graphile-migrate reset with connectionString being a superuser, but throws an error on "uuid-ossp" when it's a non-superuser.

Knowing this, I can easily DATABASE_URL=$ROOT_DATABASE_URL graphile-migrate reset, but it'd be good to reset using root (right?).

Tnanks!

benjie commented 4 years ago

I don’t use reset much, so haven’t noticed this, but thanks for pointing it out 👍

Not sure if it’s helpful in the mean time, but you may prefer to just drop the relevant schemas: https://github.com/graphile/starter/blob/master/%40app/db/scripts/wipe-if-demo#L19 - saves recreating the users/DB.

abeluck commented 4 years ago

Came here become I have the same issue.

The problem is not a bug in graphile-migrate, though it might be a missing feature.

Creating an extension requires the superuser role. It's that simple.

The reason it works in the starter project is because they give their DATABASE_OWNER superuser permissions (see this line).

The bug/missing feature as I see it is that the actions are not run with rootConnectionUrl.

The text in the starter implies that the actions should be run as superuser:

It's expected that this is ran with database superuser privileges as normal users often don't have sufficient permissions to install extensions.

If it is intended that the actions are to be run as superuser, then this is a bug. However if that is not the intention, I think a feature should be added so an action can be optionally executed as the superuser.

benjie commented 4 years ago

The reason I've not noticed this is because Amazon RDS lets you create allowlisted extensions without requiring superuser privileges. You can see from the starter that we create the owner in production without superuser:

https://github.com/graphile/starter/blob/a68759812cbc11def884e046c865cdaed27299bd/heroku-setup.template#L64

A workaround is to use .gmrc actions, and to have those actions be a script which can call psql with whatever connection string is suitable to take the action.

abeluck commented 4 years ago

Thanks @benjie I'm using something like this now:

in gmrc

{ "_": "command", "command": "./migrations/afterReset.sh" }

#!/bin/bash
set -eu

echo
echo "Reseting database"
# We're using 'template1' because we know it should exist. We should not actually change this database.
psql -Xv ON_ERROR_STOP=1 "${ROOT_DATABASE_URL}" <<EOF
REVOKE ALL ON DATABASE ${DATABASE_NAME} FROM PUBLIC;
GRANT CONNECT ON DATABASE ${DATABASE_NAME} TO ${DATABASE_OWNER};
GRANT CONNECT ON DATABASE ${DATABASE_NAME} TO ${DATABASE_AUTHENTICATOR};
GRANT ALL ON DATABASE ${DATABASE_NAME} TO ${DATABASE_OWNER};

CREATE EXTENSION IF NOT EXISTS plpgsql WITH SCHEMA pg_catalog;
CREATE EXTENSION IF NOT EXISTS "uuid-ossp" WITH SCHEMA public;
CREATE EXTENSION IF NOT EXISTS citext WITH SCHEMA public;
CREATE EXTENSION IF NOT EXISTS pgcrypto WITH SCHEMA public;
EOF

It's working :+1:

However now the postgraphile itself will not install the DDL watchers:

Failed to setup watch fixtures in Postgres database ️️⚠️
This is likely because the PostgreSQL user in the connection string does not have sufficient privileges; you can solve this by passing the 'owner' connection string via '--owner-connection' / 'ownerConnectionString'. If the fixtures already exist, the watch functionality may still work.
Enable DEBUG='graphile-build-pg' to see the error
  graphile-build-pg error: permission denied to create event trigger "postgraphile_watch_ddl"
  graphile-build-pg     at Connection.parseE (/home/user/waterbear/analyst-backend/node_modules/pg/lib/connection.js:614:13)
  graphile-build-pg     at Connection.parseMessage (/home/user/waterbear/analyst-backend/node_modules/pg/lib/connection.js:413:19)
  graphile-build-pg     at Socket.<anonymous> (/home/user/waterbear/analyst-backend/node_modules/pg/lib/connection.js:129:22)
  graphile-build-pg     at Socket.emit (events.js:321:20)
  graphile-build-pg     at addChunk (_stream_readable.js:297:12)
  graphile-build-pg     at readableAddChunk (_stream_readable.js:273:9)
  graphile-build-pg     at Socket.Readable.push (_stream_readable.js:214:10)
  graphile-build-pg     at TCP.onStreamRead (internal/stream_base_commons.js:186:23) +0ms

My role in ownerConnectionString doesn't have superuser perms, which I think is required to create event triggers.

Confirmed:

ERROR:  permission denied to create event trigger "postgraphile_watch_ddl"
HINT:  Must be superuser to create an event trigger.

Is it required then that the ownerConnection be a superuser for postgraphile to work properly?

benjie commented 4 years ago

Yeah, I named it wrong annoyingly :(

dts commented 4 years ago

I just wound up making the main user a superuser for development. Is there a reason not to run the reset thing using the owner connection?

abeluck commented 4 years ago

@benjie the docs actually clearly state this:

ownerConnectionString: Connection string to use to connect to the database as a privileged user (e.g. for setting up watch fixtures, logical decoding, etc).

So, whoops! Sorry for the noise here.

Getting back on track for this issue: @dts in the context of migrate if connectionString is not superuser, then you cannot perform actions that require superuser privs. You can get around this by using a script (see my post above).

I've standardized on:

postgraphile:
- pgConfig: non-superuser (DATABASE_AUTHENTICATOR in language of the starter)
- ownerConnectionString: superuser, with app database
migrate:
- connectionString: non-superuser, a user that has all privs on the database, but is not a superuser. this is the same user my node js app uses to run manual queries.
- rootConnectionString: superuser, with database template1

benjie commented 4 years ago

@abeluck Beware of using rootConnectionString with template1 - if you accidentally install extensions there rather than reconnecting to your relevant database then all database created from then onwards will contain those extensions.

benjie commented 4 years ago

I've given this a lot of thought and got as far as starting to add a "special" superuser mode by using hooks like: beforeCreateDatabase: "!./reset_roles.sql" where that file would contain something like:

DROP ROLE IF EXISTS :DATABASE_OWNER;
CREATE ROLE :DATABASE_OWNER WITH LOGIN PASSWORD ...

However it's clear that we shouldn't run this when we reset the shadow database (because that happens a lot), and further the SQL would be running against the root database (postgres or template1 or something) which is different to the SQL we'd run elsewhere which runs against the application database. There may also be other databases using this role. All in all it seems confusing and unsafe to encourage this usage.

I also want to make it so that we don't require the rootConnectionString on production because it's best that the application does not have those privileges at all. Migrations should be able to be performed by the application.

In conclusion: role setup has to be a concern outside of graphile-migrate; we give you the hooks you can use to run your own scripts that can override these decisions but I don't want to bake this in because I think it'll make it too easy for people to default to an insecure way of running. Extensions being installed afterReset, however, are fair game since they don't have the above issues and you are not expected to run reset on production.

graphile / migrate

Reset using connectionString instead of rootConnectionString? #46