Add revocations on schemas, refactor database and table/view granting
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 touser
androle
notusers
androles
. 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)
- Introduce
-
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 withfull_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.