VDB494 Slim GraphQL API #57
Conversation
m0ar
force-pushed
the
VDB494-slim-graphql-api
branch
from
a8ead3c
to
a7cbf9e
Compare
| -- Selectively grant execute to the graphql user to limit the API | ||
| REVOKE EXECUTE ON ALL FUNCTIONS IN SCHEMA maker FROM public; | ||
| ALTER DEFAULT PRIVILEGES IN SCHEMA maker REVOKE EXECUTE ON FUNCTIONS FROM public; | ||
|
|
There was a problem hiding this comment.
I'm not super familiar with sql privileges, why do we need to REVOKE EXECUTE ON ALL FUNCTIONS IN SCHEMA on line 3 and REVOKE EXECUTE ON FUNCTIONS FROM public; on line 4 as well?
There was a problem hiding this comment.
@elizabethengelman First removes all existing access, second does the same for future stuff :)
| BEGIN | ||
| IF NOT EXISTS (SELECT FROM pg_catalog.pg_user WHERE usename = 'graphql') | ||
| THEN | ||
| CREATE USER graphql WITH PASSWORD 'graphql'; |
There was a problem hiding this comment.
Even though this is a private repo, I wonder if we should be mindful of keeping this password more secure? Or, if it doesn't matter, maybe we don't need to include a password?
There was a problem hiding this comment.
Yeah, no idea how to approach that tbh. But yes, creating the user and credentials in a migration is smooth because otherwise it's more manual setup needed to get the migrations to pass :/
Wouldn't the same risks apply without a password? It's still access to this user if someone has access to the DB host directly. Do you have any suggestions on how we could handle this?
@rmulhol ?
There was a problem hiding this comment.
It seems like the same risks would indeed apply without a password. I'll be interested to see what Grizz comes up with for managing the deploy in a way that's simple/secure. My guess is that we'll eventually want to handle setting up db user access (for the prod env at least) via scripts/env variables that are kept private
| block_hash text, | ||
| ilk_id integer | ||
| CREATE TYPE maker.relevant_block AS ( | ||
| block_number BIGINT, |
There was a problem hiding this comment.
There are a couple instances of block_number instead of block_height in this query still - but I'm totally cool with taking care of those changes in https://makerdao.atlassian.net/browse/VDB-540 instead of this PR.
There was a problem hiding this comment.
This particular case is an actual block_number imo haha. It's also an internal datatype that's not presented in the API, it's a helper basically.
There was a problem hiding this comment.
True, it probably is less of a concern since it's an internal datatype. I am not sure I totally understand the difference between block_number and block_height - would you be able to expand upon that some? :)
There was a problem hiding this comment.
@elizabethengelman It's really no big deal, but I think "state at block_height" is a bit more self explanatory when used as a query argument. In my mental model, the state of an urn/ilk is as of a certain block height, while a block has a certain number. Does it make sense? It's no big deal either, I'd be happy to revert if you don't vibe with it :)
There was a problem hiding this comment.
Gotcha, that makes sense. I don't have a strong opinion either way, so I'm happy to leave it as is - was mostly just curious! :)
| Dink string | ||
| Dart string | ||
| } | ||
|
|
||
| func GetExpectedTimestamp(epoch int) string { |
| ); | ||
|
|
||
| -- Function returning state for all urns as of given block | ||
| CREATE OR REPLACE FUNCTION maker.get_all_urn_states_at_block(block_height BIGINT) | ||
| CREATE OR REPLACE FUNCTION maker.all_urns(block_height BIGINT) |
There was a problem hiding this comment.
When I try to start postgraphile locally with this updated schema, I'm seeing an error about a naming collision between the built-in urn's table allUrns query, and this function:
Error: A naming conflict has occurred - two entities have tried to define the same key 'allUrns'.
The first entity was:
Adding 'all*' relations to root Query
The second entity was:
Adding query field for function "maker"."all_urns"(...args...). You can rename this field with:
COMMENT ON FUNCTION "maker"."all_urns"(...arg types go here...) IS E'@name newNameHere';
I wonder if we should rename the urns table with a smart comment, since that won't be exposed via the API anyway. Something like urnTable or something? 馃
There was a problem hiding this comment.
@elizabethengelman Hrm, maybe that issue just disappeared when postgraphile is using the graphql user (since that doesn't have access to the urns table) 馃
Good catch! I'll sort it out :)
m0ar
force-pushed
the
VDB494-slim-graphql-api
branch
from
45adc29
to
b6ac2d0
Compare
|
@elizabethengelman I've fixed the introspection bug, and kept the graphql user as-is for now until we figure out something else :) |
m0ar
force-pushed
the
VDB494-slim-graphql-api
branch
from
b6ac2d0
to
90a6dfe
Compare
馃憢
This PR does some tidying around the API, and is kinda depending on a friend-PR in
vulcanizedb. It'll still work, but the API isn't as clean as it should be. :)A new migration adds a user
graphqlwith permissions limited to exactly what we want to show in the API. Whenpostgraphileis started with the user credentials ofgraphql, we can control precisely what shows up!I've also done a pass for fixing query naming to conform with the spec.