olirice / alembic_utils

An alembic/sqlalchemy extension for migrating sql views, functions, triggers, and policies
https://olirice.github.io/alembic_utils
MIT License
211 stars 43 forks source link

Unable to GRANT SELECT on all columns (have to be explicit about which columns) #57

Closed pgorsira closed 3 years ago

pgorsira commented 3 years ago

Thanks for this library! Searched but didn't see anything about this. It seems like to GRANT SELECT using PGGrantTable, we need to explicitly specify which columns to GRANT SELECT. This excludes statements like the following from being easily encoded with this class:

GRANT SELECT ON <table> to <user>;

Was just curious if this was something that had been encountered yet.

olirice commented 3 years ago

While the feature was in development there was support for a table level grant by omitting the columns argument

The problem is that column level grants and table level grants are stored in separate locations in the DB. When you grant access to a table via

GRANT SELECT ON <table> to <role>;

it also populates the column level grants in the database.

That made reflecting out the current database state (to check for differences during --autogenerate) very complex and I wasn't confident that I had all the edge cases correct.

Since the functionality of column level grants is a superset of table grants, I decided to force the explicit column list and be more confident about correctness.


I usually do this

class Account(Base):
    __tablename__ = "account"

    id = Column(Integer, primary_key=True)
    ...

COLUMN_NAMES = [x.name for x in Account.__table__.c]

grant = PGGrantTable(
    schema="public",
    table="account",
    columns=COLUMN_NAMES,
    role="anon_user",
    grant='SELECT',
    with_grant_option=False,
)

but I agree that its not a great user experience

pgorsira commented 3 years ago

That makes sense and the snippet is helpful. So if I add a new column to the table, I'm assuming it would get picked up properly in this case as there would be a clear difference in columns that would be reflected in the list comprehension.

Do you have a way of dynamically handling this for views by any chance? That's actually the particular situation I'm facing. Not sure how to iterate over all columns in that case.

olirice commented 3 years ago

Unfortunately, if you try to add a column to a table and pipe that through to a view in the same migration it will cause an exception. That happens because the new column isn't visible to the view during reflection

https://github.com/olirice/alembic_utils/issues/41

pgorsira commented 3 years ago

Oh, I guess what I'm saying is let's say the tables that the view (MyView) depends on already exist (earlier migrations, i.e., they're aren't a concern). Do you know of any way to have

grant = PGGrantTable(
    schema="public",
    table="MyView",
    columns=<list of columns in view>,
    role="anon_user",
    grant='SELECT',
    with_grant_option=False,
)
olirice commented 3 years ago

whoops, i misunderstood

if you're feeling adventurous you could dynamically populate the columns from the database in env.py before passing them to register_entities

# env.py
my_grants: List[PGGrantTable] = ...
sess: sqlalchemy.orm.Session = ...

for grant in my_grants:
    columns, = sess.execute(text("""
      select
          array_agg(column_name order by ordinal_position)
      from
          information_schema.columns
      where
          table_schema = :schema
          and table_name = :table
      order by
          ordinal_position
   """, {"schema": grant.schema, "table": grant.table})
    ).one()

    grant.columns = columns

register_entities(*my_grants)

if you do decide to do something like that, please let me know how it goes

pgorsira commented 3 years ago

Hey, just wanted to let you know that this did seem to work! Once I wired it up and autogenerated a migration it was able to recognize a GRANT that already existed in the DB and not generate a line for that GRANT in the upgrade/downgrade functions.

However after some thinking I think I'm going to go with Ansible for keeping GRANTS in sync as I think it'll be simpler / offer more clarity as to the current state of the DB. Really appreciate your help though.