-
Notifications
You must be signed in to change notification settings - Fork 40
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
[CT-1661] [Bug] Grants fails to correctly quote username when revoking #156
Comments
Recent & related: dbt-labs/dbt-core#6378 (comment) |
Thanks for reporting this @njrs92 ! Does it work if you add double quotes within your configuration? For example, like this for models:
- name: specific_model
config:
grants:
select: ['"first.last"'] or this for model configuration: {{
config(
grants = {'select': ['"first.last"']}
)
}} or this for project configuration: models:
+grants:
select: ['"first.last"'] |
Unfortunately not I'm granting to a group like so But it looks like dbt then runs if the table already exists
Then with the list of users returned it creates a long revoke command then the grant
Which in this case fails with user user.three as it needs quoting |
Yep, I see what you are saying @njrs92 ✔️ Agreed that the fix might be as "simple" as updating this: to be something like this instead (untested!): revoke {{ privilege }} on {{ relation }} from {% for grantee in grantees %}{{ adapter.quote(grantee) }}{{ "," if not loop.last else "" }}{% endfor %} The quoting would need to be database-specific, which is why I am guessing that I don't know a more elegant way to apply the quoting other than using a loop. In the example above, I went ahead and added the comma while looping. There might be other more readable approaches to solving this too, like: revoke {{ privilege }} on {{ relation }} from
{% for grantee in grantees %}
{{ adapter.quote(grantee) }}
{% if not loop.last %},{% endif %}
{% endfor %} |
I can't speak for other databases but that macro worked when I ran it on Redshift.
|
This issue has been marked as Stale because it has been open for 180 days with no activity. If you would like the issue to remain open, please comment on the issue or else it will be closed in 7 days. |
Although we are closing this issue as stale, it's not gone forever. Issues can be reopened if there is renewed community interest. Just add a comment to notify the maintainers. |
We're continuing to hit this issue where the revoke command fails on Redshift due to a username having a |
Thanks for the nudge @FridayPush. I'm re-opening this issue and marking it as a "good first issue" if someone wants to pick it up. See here an implementation that was reported to work. |
@dbeatty10 I've recreated this bug in redshift and solved the issue using the code you suggested. If I fork dbt-core and then open a PR from my fork as suggested in community guidelines are you happy to approve? Very new to this but like you say adapter.quote() should ensure this works across different databases. From looking at other first time contributor PRs it seems like there is some sort of form for me to fill in too? |
@barton996 so sorry to leave you hanging! Yes, that would be awesome if you open a PR 🚀
For a first time contributor PR, a bot will reply with a message that starts like this:
Once you fill it out for the first time, you'll be set from here on out. In fact, you could just click that "Contributor License Agreement" link right now if you want -- then it will already recognize you by the time you open your PR 😎 |
This issue is biting us as well in Redshift. I added an |
Hi, this problem is still pretty much alive in Redshift (dbt-core=1.8.7, dbt-redshift=1.8.1, dbt-adapters=1.9.0). I have IAM users with - in the name, and if it wasn't for this fix, we wouldn't have a way to work:
The problem happens for incremental models on REVOKE commands, as stated here. Couldn't this solution be part of the default packages? |
Is this a new bug in dbt-core?
Current Behavior
As part of the new grants feature, https://docs.getdbt.com/reference/resource-configs/grants dbt will attempt to revoke users not part of the grant before reapplying users listed in the grant.
DBT does this (at least in the Redshift adaptor ) by pulling the table permissions and explicitly looping through every user then using a revoke command like
revoke select on "database"."schema"."table" from user1, user2 ;
The user is not quoted so a user like user.name will fail with
syntax error at or near "."
LINE 2: ...user1, user.name, user2...
I am fairly certain the macro in https://github.com/dbt-labs/dbt-core/blob/main/core/dbt/include/global_project/macros/adapters/apply_grants.sql line 82 just needs some quotes
Expected Behavior
the revoke command should quote users
revoke select on "database"."schema"."table" from "user1", "user2" ;
Steps To Reproduce
Create a user with a period in the name for example in Redshift
create user "first.last";
Create a simple dbt incremental model and user the grant syntax to give permission to that table within dbt.
Run DBT twice (on the first run the revoke macro is not run as the table is new)
Relevant log output
No response
Environment
Which database adapter are you using with dbt?
redshift
Additional Context
No response
The text was updated successfully, but these errors were encountered: