search

Squarespace / pgbedrock

Manage a Postgres cluster's roles, role memberships, schema ownership, and privileges
https://pgbedrock.readthedocs.io/en/latest/
Other
311 stars 35 forks source link

`pgbedrock` errors on usernames with hyphens if that user also has a personal schema #43

Open Gastove opened 5 years ago

Gastove commented 5 years ago

Given a user:

captain-planet:
    has_personal_schema: true

The following error is returned by Postgres:


18-Jul-2018 19:57:19 | -- Skipping privilege configuration for user "captain-planet"": syntax error at or near "-"
-- | --
18-Jul-2018 19:57:19 | LINE 3:     ALTER DEFAULT PRIVILEGES IN SCHEMA captain-planet GRANT...

pgbedrock should be fully quoting captain-planet, which it does many places, but not, alas, enough places.

zcmarine commented 5 years ago

Hey Ross, greetings from the other side. :)

Yeah, this is a known bug and is recorded in issue #7. As a result, I'm closing this issue as a duplicate.

I think this should be doable to solve. It requires changing how an ObjectName instance's qualified_name is rendered (which is just an if-else), but the reason I didn't address it when implementing ObjectNames in the first place was a wrinkle in loading the YAML, as noted in this comment.

If you'd like to take a crack at it, please do! Otherwise it'll probably stay an open issue for a while. I'm happy to sync though if there's interest.

Gastove commented 5 years ago

Hey @zmarine! I hope things on the other side are well.

Unless I'm missing something, I think this issue is a more limited sub-issue of #7; I believe this issue should be adequately addressed by #44. While I'm also game to take on #7, it seems like it lays the groundwork for fully-qualified objects (e.g. "foo-schema"."foo-table") as well as improving this handling. Do you disagree?

Gastove commented 5 years ago

Perhaps more generally: it seems there's some confusion here in how we expect quotes to be applied around objects that need them. In many places, quotes are provided in the query template strings themselves; #44 addresses this by adding them in even more places. #7 seems like it requires re-factoring quote handling to make it the responsibility of an object to know how it should be rendered (though, most of the time, a "quote all the things" approach is likely to be both safe and harmless).

zcmarine commented 5 years ago

Hey Ross, that makes some sense to me. My one concern is whether you could then end up in a state where pgbedrock configure operates on a spec that can't be generated by pgbedrock generate. I'm not sure about that, but I suppose you could add a test that adds a role and adds a personal schema, and see if pgbedrock generate works.

I'll be heading to Ethiopia today and likely won't be online much, so from my perspective if you can verify that both the generate and configure components work with dashes then feel free to merge. I'm not sure if anyone else is an admin in this repo yet, but if not they probably should be.

Gastove commented 5 years ago

Unless I'm misreading something, we have a test for this already, which has been passing this whole time: oh boy

Gastove commented 5 years ago

So: we have a test that makes this claim:

def test_configure_schema_role_has_dash(tmpdir, capsys, db_config, cursor, base_spec):
    """
    We add a new user ('role-with-dash') through pgbedrock and make sure that that user can create
    a personal schema
    """

However: the test case is too small. It isn't until a certain threshold of complexity is hit that pgbedrock sets default role grants; it isn't until those grants are set that any of this becomes a problem.

I've added a test for this failure case; I'm going to push it to the branch with an open PR so the failing case can be tracked. My existing solution is definitely not enough.