Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add quotes to usernames when revoking and granting table permissions #8809

Closed

Conversation

barton996
Copy link
Contributor

@barton996 barton996 commented Oct 10, 2023

resolves dbt-labs/dbt-adapters#156
resolves dbt-labs/dbt-postgres#55

Problem

For models with grants, where the corresponding table is not new (model has been run before), dbt revokes all grants on this table by looping through all users with table permissions.

The macro currently does not surround the user names that it reads from the table permissions with quotes, meaning that revoke statements on users with usernames like first.last will fail.

This is problematic when granting to a user GROUP, as you cannot control what is returned by the table permissions query using your grant yaml config e.g. grants: select: ["GROUP developers"].

Solution

apply_grants.sql jinja template is updated so that usernames are surrounded with adapter.quote() character when granting and revoking.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • [] This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX
  • This PR includes type annotations for new and modified functions

@barton996 barton996 requested review from a team as code owners October 10, 2023 18:37
@cla-bot cla-bot bot added the cla:yes label Oct 10, 2023
@barton996 barton996 mentioned this pull request Oct 10, 2023
5 tasks
@barton996
Copy link
Contributor Author

@mikealfare @dbeatty10

mikealfare
mikealfare previously approved these changes Oct 10, 2023
@graciegoheen
Copy link
Contributor

One thing to callout here - I believe this will make usernames case-sensitive, which is technically not backwards compatible.

@graciegoheen graciegoheen added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Oct 10, 2023
@mikealfare mikealfare added ok_to_test and removed ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering labels Oct 10, 2023
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.26%. Comparing base (3f7f7de) to head (bdeec24).
Report is 207 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    dbt-labs/dbt-core#8809      +/-   ##
==========================================
- Coverage   86.43%   83.26%   -3.18%     
==========================================
  Files         176      176              
  Lines       26028    26028              
==========================================
- Hits        22497    21671     -826     
- Misses       3531     4357     +826     
Flag Coverage Δ
integration 83.26% <ø> (+0.02%) ⬆️
unit ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dbeatty10
Copy link
Contributor

One thing to callout here - I believe this will make usernames case-sensitive, which is technically not backwards compatible.

Good callout @graciegoheen. I think we should finish refining the behavior within dbt-labs/dbt-postgres#55 before merging this.

Specifically, dbt-labs/dbt-postgres#55 mentions the following:

One thing for us to consider is if we can/should add a quote_grants config for grants (or something else similar to quote_columns). If we add some kind of quoting-related config, hopefully it would fit together with #2986.

@dbeatty10 dbeatty10 changed the title Iss 6444 grant revoke username quotes Add quotes to usernames when revoking and granting table permissions Oct 10, 2023
Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@barton996 and @graciegoheen adding a blocking review here to prevent an accidental merge until we decide if we can/should make this backwards-compatible or not. See discussion here.

@dbeatty10 dbeatty10 added the community This PR is from a community member label Mar 22, 2024
@dbeatty10 dbeatty10 added Team:Adapters Issues designated for the adapter area of the code dbt-adapters Needs migration to the dbt-adapters repo bug Something isn't working labels Apr 9, 2024
@dbeatty10
Copy link
Contributor

Thanks for taking the time to open this PR @barton996! Since opening, we've decoupled dbt Adapters from dbt Core, and this code now lives in a separate repo: dbt-adapters.

A consequence of the decoupling is that PR can't be merged anymore as is, so we're closing it. For more context see #9171.

The linked issue has already been transferred. Please don't hesitate to re-open your proposed code changes within this PR in the dbt-adapters repo.

@dbeatty10 dbeatty10 closed this Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla:yes community This PR is from a community member dbt-adapters Needs migration to the dbt-adapters repo ok_to_test Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
4 participants