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

Issue 6444 grant username quotes #8787

Closed

Conversation

barton996
Copy link
Contributor

@barton996 barton996 commented Oct 6, 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 a review from a team as a code owner October 6, 2023 12:45
@cla-bot
Copy link

cla-bot bot commented Oct 6, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Fraser Barton.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2023

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@barton996
Copy link
Contributor Author

@dbeatty10

@barton996 barton996 requested a review from a team as a code owner October 6, 2023 13:15
@barton996 barton996 requested a review from aranke October 6, 2023 13:15
@cla-bot
Copy link

cla-bot bot commented Oct 6, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Fraser Barton.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot
Copy link

cla-bot bot commented Oct 6, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Fraser Barton.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

1 similar comment
@cla-bot
Copy link

cla-bot bot commented Oct 6, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Fraser Barton.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (eac13e3) 86.50% compared to head (5196d88) 65.08%.
Report is 13 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    dbt-labs/dbt-core#8787       +/-   ##
===========================================
- Coverage   86.50%   65.08%   -21.43%     
===========================================
  Files         176      176               
  Lines       25825    25843       +18     
===========================================
- Hits        22341    16821     -5520     
- Misses       3484     9022     +5538     
Flag Coverage Δ
integration ?
unit 65.08% <ø> (-0.03%) ⬇️

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

see 118 files with indirect coverage changes

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

@dbeatty10
Copy link
Contributor

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Oct 6, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Fraser Barton.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot
Copy link

cla-bot bot commented Oct 6, 2023

The cla-bot has been summoned, and re-checked this pull request!

@barton996
Copy link
Contributor Author

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Fraser Barton. This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@dbeatty10 I filled in this cla with this link, followed the steps asked by the cla-bot and since done a commit. What have I missed?

@dbeatty10
Copy link
Contributor

@barton996 it looks like these first two commits are the ones that the cla-bot is barking about:

image

I'm guessing the fix is a force push as described here:
https://stackoverflow.com/questions/41349644/commit-with-wrong-username-to-github

Wanna give that a shot and let me know how it goes?

@barton996 barton996 force-pushed the ISS-6444-grant-username-quotes branch from e657a60 to 021e7f3 Compare October 6, 2023 15:34
@cla-bot
Copy link

cla-bot bot commented Oct 6, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Fraser Barton.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@barton996 barton996 force-pushed the ISS-6444-grant-username-quotes branch from 021e7f3 to 5196d88 Compare October 6, 2023 15:36
@cla-bot
Copy link

cla-bot bot commented Oct 6, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Fraser Barton.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@graciegoheen
Copy link
Contributor

Looks like we're still getting an error here - do you want to just start over with a new branch & new PR? Looks like the changes were pretty small, so it may be faster to just copy & paste the code changes in this situation @barton996

@barton996
Copy link
Contributor Author

Looks like we're still getting an error here - do you want to just start over with a new branch & new PR? Looks like the changes were pretty small, so it may be faster to just copy & paste the code changes in this situation @barton996

Apologies I meant to do that on Monday. I'll do it now

@mikealfare
Copy link
Contributor

After looking at some ways to "fix" the two commits that @dbeatty10 pointed out, that seems like unnecessary effort. @graciegoheen's method will work. You can also squash merge your changes into another branch on your fork via a PR, which would become a new commit that's definitely signed (because you're doing it in GH). Then just update the branch on this PR to that new branch.

@barton996
Copy link
Contributor Author

Replaced by #8809

@barton996 barton996 closed this Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants