Skip to content

Add revocations on schemas, refactor database and table/view granting

Taylor A Murphy requested to merge (removed):revocations-on-schemas into master

Merge Request Checklist

  • Link to an issue #1480 (closed)
  • Include the proposed fix or feature
  • Include and update tests for the modified code
  • Include a documentation change
  • Add a CHANGELOG.md entry in the Unreleased section

Once your merge request is complete, please assign it a Meltano maintainer for review cycle. Once the review cycle finished, the reviewer shall approve the merge request so it can be merged by any Meltano maintainer.

Key features of this MR:

  • Add additional case to snowflaky where entities that were already quoted in Snowflake were returned double quoted

    • "3268-my_db" was coming back as ""3268-my_db"". This fixes that and there's a test!
  • Update generate_grant_roles to properly grant to user and role not users and roles. There's a test for this now too!

  • Refactor generate_grant_privileges_to_role for schemas and tables/views to follow format introduced for databases

  • Refactor generate_database_grants to better handle corner cases on shifting permissions (i.e. moving grant from write to read)

    • Added a test for this function
  • Refactor generate_schema_grants to follow pattern introduced for DBs and add revokes to function

    • Introduce full_schema_list function to factor out some common functionality between schema and table/view grants
    • Split out common privileges between read/write to better handle revocations (this pattern was added to DBs and included in tables/views)
  • Refactor generate_table_and_view_grants to follow pattern introduced for DBs. Does NOT add revokes to the function for this iteration.

  • Four new tests!!

    • snowflaky for the new edge case discovered above
    • generate_grant_roles since it makes no calls to Snowflake and has a simple permutation surface
    • generate_warehouse_grants since no snowflake calls
    • generate_database_grants since no snowflake calls

Longer details and recommendations for review:

This MR started with the goal of primarily focusing on adding schema revokes but it morphed a bit from there. In the process of refactoring schemas to match the new pattern that was introduced for databases I decided to just do the same for tables since I was there and it was fairly "easy". While I was adding revokes to the schema function I came across some tweaks that needed to happen for database revocations. This also led me to refactoring out some bits of the code across schema/table/view grants.

From a functionality standpoint, the only thing that should change are:

  • database grants are more accurate with revocations
  • schema grants now include the more accurate revocations

The functionality for table/views grants shouldn't change at all, this was just a pure refactor in prep for another iteration on the revocation piece.

Here is my recommendation for reviewing since this MR is much larger than I initially wanted:

  • Review snowflaky (and its test!)
  • Review generate_database_grants that behavior is improved for more cases (and its test!)
  • Review generate_grant_privileges_to_role for schemas
  • Review generate_schema_grants for just grants first
  • Review full_schema_list that behavior is retained from pulling it out of schema
  • Review revocations for schema
  • Review generate_grant_privileges_to_role for tables/views
  • Review generate_table_and_view_grants for full refactor with full_schema_list refactor as well
  • Review warehouse grants and its test

Note that I left some verbose comments in some areas. Some of these comments would be better suited to be actual tests. One of them also indicates why I'm doing a for loop when it's not strictly necessary.

The next iteration will add revocations for table/views.

Edited by Melty Bot

Merge request reports