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

[njrs92]

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

[jtcohen6]

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

[dbeatty10]

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]

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]

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

[njrs92]

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]

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]

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.

[FridayPush]

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]

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.

[barton996]

@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]

@dbeatty10

[dbeatty10]

@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 😎

[mike-wilson-tubi]

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.