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

[CT-1661] [Bug] Grants fails to correctly quote username when revoking #156

Open
2 tasks done
njrs92 opened this issue Dec 14, 2022 · 14 comments · May be fixed by #265
Open
2 tasks done

[CT-1661] [Bug] Grants fails to correctly quote username when revoking #156

njrs92 opened this issue Dec 14, 2022 · 14 comments · May be fixed by #265
Labels
bug Something isn't working good_first_issue Good for newcomers grants Issues related to dbt's grants functionality quoting Issues related to dbt's quoting behavior

Comments

@njrs92
Copy link

njrs92 commented Dec 14, 2022

Is this a new bug in dbt-core?

  • I believe this is a new bug in dbt-core
  • I have searched the existing issues, and I could not find an existing issue for this bug

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

- OS:Ubuntu 20.04
- Python: 3.8.10 
- dbt: 1.3.1

Which database adapter are you using with dbt?

redshift

Additional Context

No response

@njrs92 njrs92 added bug Something isn't working triage labels Dec 14, 2022
@github-actions github-actions bot changed the title [Bug] Grants fails to correctly quote username when revoking [CT-1661] [Bug] Grants fails to correctly quote username when revoking Dec 14, 2022
@jtcohen6
Copy link
Contributor

Recent & related: dbt-labs/dbt-core#6378 (comment)

@dbeatty10
Copy link
Contributor

Thanks for reporting this @njrs92 !

Does it work if you add double quotes within your configuration?

For example, like this for models YAML configuration:
models/schema.yml

models:
  - name: specific_model
    config:
      grants:
        select: ['"first.last"']

or this for model configuration:
models/specific_model.sql

{{ 
    config(
        grants = {'select': ['"first.last"']}
    )
}}

or this for project configuration:
dbt_project.yml

models:
  +grants:
    select: ['"first.last"']

@njrs92
Copy link
Author

njrs92 commented Dec 14, 2022

Thanks for reporting this @njrs92 !

Does it work if you add double quotes within your configuration?

Unfortunately not I'm granting to a group like so
+grants: select: ["GROUP developers"]

But it looks like dbt then runs if the table already exists

with privileges as (

    -- valid options per https://docs.aws.amazon.com/redshift/latest/dg/r_HAS_TABLE_PRIVILEGE.html
    select 'select' as privilege_type
    union all
    select 'insert' as privilege_type
    union all
    select 'update' as privilege_type
    union all
    select 'delete' as privilege_type
    union all
    select 'references' as privilege_type

)

select
    u.usename as grantee,
    p.privilege_type
from pg_user u
cross join privileges p
where has_table_privilege(u.usename, '"database"."schema"."table"', privilege_type)
    and u.usename != current_user
    and not u.usesuper

Then with the list of users returned it creates a long revoke command then the grant

revoke select on "database"."schema"."table" from user1, user2, user.three ;
grant select on "database"."schema"."table" to GROUP developers;

Which in this case fails with user user.three as it needs quoting

@dbeatty10
Copy link
Contributor

Yep, I see what you are saying @njrs92 ✔️

Agreed that the fix might be as "simple" as updating this:
https://github.com/dbt-labs/dbt-core/blob/e8da84fb9e177d9eee3d7722d3d6906bb283183d/core/dbt/include/global_project/macros/adapters/apply_grants.sql#L82

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 quote would be a good move here. (I say "guessing" because it's possible there are databases that use different separators for users/roles than other identifiers like table names.)

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 %}

@dbeatty10 dbeatty10 removed the triage label Dec 14, 2022
@njrs92
Copy link
Author

njrs92 commented Dec 16, 2022

I can't speak for other databases but that macro worked when I ran it on Redshift.
I chucked this into the macros folder and it worked.

{%- macro default__get_revoke_sql(relation, privilege, grantees) -%}
    revoke {{ privilege }} on {{ relation }} from
    {% for grantee in grantees %}
    {{ adapter.quote(grantee) }}
    {% if not loop.last %},{% endif %}
    {% endfor %}
{%- endmacro -%}```

@github-actions
Copy link

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.

@github-actions github-actions bot added the Stale label Jun 15, 2023
@github-actions
Copy link

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.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 22, 2023
@FridayPush
Copy link

We're continuing to hit this issue where the revoke command fails on Redshift due to a username having a . in it and not being quoted. It'd be nice to not have to override macros ourselves but have it be fixed natively.

@dbeatty10 dbeatty10 added good_first_issue Good for newcomers and removed Stale labels Jun 29, 2023
@dbeatty10
Copy link
Contributor

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 dbeatty10 reopened this Jun 29, 2023
@barton996
Copy link

barton996 commented Sep 1, 2023

@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
Copy link

@dbeatty10

@dbeatty10
Copy link
Contributor

@barton996 so sorry to leave you hanging!

Yes, that would be awesome if you open a PR 🚀

From looking at other first time contributor PRs it seems like there is some sort of form for me to fill in too?

For a first time contributor PR, a bot will reply with a message that starts like this:

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

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 😎

@dbeatty10 dbeatty10 transferred this issue from dbt-labs/dbt-core Apr 10, 2024
@dbeatty10 dbeatty10 added quoting Issues related to dbt's quoting behavior grants Issues related to dbt's grants functionality labels Apr 18, 2024
@mike-wilson-tubi
Copy link

This issue is biting us as well in Redshift. I added an IAM:user.name last night to a group and all of our DBT jobs broke that refer to this group, complaining of an unquoted user in the revoke command.

@qmg-karan qmg-karan linked a pull request Jul 19, 2024 that will close this issue
4 tasks
@hanslemm
Copy link

hanslemm commented Nov 18, 2024

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:

{%- macro default__get_revoke_sql(relation, privilege, grantees) -%}
    revoke {{ privilege }} on {{ relation }} from
    {% for grantee in grantees %}
    {{ adapter.quote(grantee) }}
    {% if not loop.last %},{% endif %}
    {% endfor %}
{%- endmacro -%}

The problem happens for incremental models on REVOKE commands, as stated here.

Couldn't this solution be part of the default packages?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good_first_issue Good for newcomers grants Issues related to dbt's grants functionality quoting Issues related to dbt's quoting behavior
Projects
None yet
7 participants