Skip to content
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
6e88fe7
init push or ct-660 work
McKnight-42 Jun 13, 2022
6d4b938
changes to default versions of get_show_grant_sql and get_grant_sql
McKnight-42 Jun 14, 2022
ea35fd1
completing init default versions of all macros being called for look …
McKnight-42 Jun 14, 2022
7e2b707
minor update to should_revoke
McKnight-42 Jun 14, 2022
898aa8f
post pairing push up (does have log statements to make sure we remove)
McKnight-42 Jun 15, 2022
76050da
minor spacing changes
McKnight-42 Jun 15, 2022
7a3e7c6
Merge branch 'main' of github.com:dbt-labs/dbt into ct-660-grant-sql
McKnight-42 Jun 16, 2022
5fb07c1
minor changes, and removal of logs so people can have clean grab of code
McKnight-42 Jun 16, 2022
4cf705f
minor changes to how get_revoke_sql works
McKnight-42 Jun 17, 2022
2fff84b
init attempt at applying apply_grants to all materialzations
McKnight-42 Jun 17, 2022
6fccef6
name change from recipents -> grantee
McKnight-42 Jun 17, 2022
528dff6
minor changes
McKnight-42 Jun 17, 2022
95f7ae4
Merge branch 'main' of github.com:dbt-labs/dbt into ct-660-grant-sql
McKnight-42 Jun 21, 2022
9079c4a
working on making a context to handle the diff gathering between gran…
McKnight-42 Jun 21, 2022
6bdb4b5
removing logs from most materializations to better track diff of gran…
McKnight-42 Jun 22, 2022
597ab05
starting to build out postgres get_show_grant_sql getting empty query…
McKnight-42 Jun 22, 2022
138f443
6/27 eod update looking into diff_grants variable not getting passed …
McKnight-42 Jun 27, 2022
fc7e24b
changes to loop cases
McKnight-42 Jun 28, 2022
6f53b3d
changes after pairing meeting
McKnight-42 Jun 28, 2022
b3e37cb
Merge branch 'main' of github.com:dbt-labs/dbt into ct-660-grant-sql
McKnight-42 Jun 29, 2022
99b1445
adding apply_grants to create_or_replace_view.sql
McKnight-42 Jul 5, 2022
7946a3b
models are building but testing out small issues around revoke statem…
McKnight-42 Jul 5, 2022
0597398
postgrest must fixes from jeremy's feedback
McKnight-42 Jul 6, 2022
1d263b7
postgres minor change to standarize_grants_dict
McKnight-42 Jul 6, 2022
c2e9aeb
updating after pairing with dough and jeremey incorporating the new v…
McKnight-42 Jul 6, 2022
0843f66
Merge branch 'main' of github.com:dbt-labs/dbt into ct-660-grant-sql
McKnight-42 Jul 6, 2022
077e4ff
adding ref of diff_of_two_dicts to base keys ref
McKnight-42 Jul 6, 2022
ab6be85
change of method type for standardize_grants_dict
McKnight-42 Jul 6, 2022
da557d9
minor update trying to fix unit test
McKnight-42 Jul 6, 2022
f2c957f
changes based on morning feedback
McKnight-42 Jul 7, 2022
eb935d5
change log message in default_apply_grants macro
McKnight-42 Jul 7, 2022
cefcd4f
CT-808 grant adapter tests (#5447)
gshank Jul 7, 2022
bdc0c71
rename grant[privilege] -> grant_config[privilege]
McKnight-42 Jul 7, 2022
67f5beb
Merge branch 'main' of github.com:dbt-labs/dbt into ct-660-grant-sql
McKnight-42 Jul 11, 2022
72bdae9
postgres macro rename to copy_grants
McKnight-42 Jul 11, 2022
794f4d1
CT-808 more grant adapter tests (#5452)
gshank Jul 11, 2022
068c59b
update to main, create changelog, whitespace fixes
McKnight-42 Jul 11, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions core/dbt/adapters/base/impl.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ class BaseAdapter(metaclass=AdapterMeta):
- convert_datetime_type
- convert_date_type
- convert_time_type
- standardize_grants_dict

Macros:
- get_catalog
Expand Down Expand Up @@ -538,6 +539,26 @@ def list_relations_without_caching(self, schema_relation: BaseRelation) -> List[
"`list_relations_without_caching` is not implemented for this " "adapter!"
)

###
# Abstract methods about grants
###
@abc.abstractmethod
def standardize_grants_dict(self, grants_table: agate.Table) -> dict:
"""Translate the result of `show grants` (or equivalent) to match the
grants which a user would configure in their project.

If relevant -- filter down to grants made BY the current user role,
and filter OUT any grants TO the current user/role (e.g. OWNERSHIP).

:param grants_table: An agate table containing the query result of
the SQL returned by get_show_grant_sql
:return: A standardized dictionary matching the `grants` config
:rtype: dict
"""
raise NotImplementedException(
"`standardize_grants_dict` is not implemented for this adapter!"
)

###
# Provided methods about relations
###
Expand Down
24 changes: 24 additions & 0 deletions core/dbt/context/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,30 @@ def print(msg: str) -> str:
print(msg)
return ""

@contextmember
@staticmethod
def diff_of_two_dicts(dict_a, dict_b):
"""
Given two dictionaries:
dict_a: {'key_x': ['value_1', 'value_2'], 'key_y': ['value_3']}
dict_b: {'key_x': ['value_1'], 'key_z': ['value_4']}
Return the same dictionary representation of dict_a MINUS dict_b
"""

dict_diff = {}
dict_a = {k.lower(): v for k, v in dict_a.items()}
dict_b = {k.lower(): v for k, v in dict_b.items()}
for k in dict_a:
if k in dict_b:
a_lowered = map(lambda x: x.lower(), dict_a[k])
b_lowered = map(lambda x: x.lower(), dict_b[k])
diff = list(set(a_lowered) - set(b_lowered))
if diff:
dict_diff.update({k: diff})
else:
dict_diff.update({k: dict_a[k]})
Copy link
Contributor

Choose a reason for hiding this comment

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

have we tested the performance on this when the grants dict config is huge (and very different from current grants)? might not suck once, but might if they have this same complicated diff on hundreds of models?

Copy link
Contributor

Choose a reason for hiding this comment

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

This code can + should absolutely be refactored for more Pythonic comprehension + better performance / lower memory footprint. I'm not the right person to do that :)

hundreds of models?

FWIW this code will be executed in parallel --threads, assuming the user is making use of them

return dict_diff


def generate_base_context(cli_vars: Dict[str, Any]) -> Dict[str, Any]:
ctx = BaseContext(cli_vars)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
{% macro copy_grants() %}
{{ return(adapter.dispatch('copy_grants', 'dbt')()) }}
{% endmacro %}

{% macro default__copy_grants() %}
{{ return(True) }}
{% endmacro %}

{% macro should_revoke(existing_relation, full_refresh_mode=True) %}

{% if not existing_relation %}
{#-- The table doesn't already exist, so no grants to copy over --#}
{{ return(False) }}
{% elif full_refresh_mode %}
{#-- The object is being REPLACED -- whether grants are copied over depends on the value of user config --#}
{{ return(copy_grants()) }}
{% else %}
{#-- The table is being merged/upserted/inserted -- grants will be carried over --#}
{{ return(True) }}
{% endif %}

{% endmacro %}

{% macro get_show_grant_sql(relation) %}
{{ return(adapter.dispatch("get_show_grant_sql", "dbt")(relation)) }}
{% endmacro %}

{% macro default__get_show_grant_sql(relation) %}
show grants on {{ relation }} {{ relation }}
{% endmacro %}

{% macro get_grant_sql(relation, grant_config) %}
{{ return(adapter.dispatch('get_grant_sql', 'dbt')(relation, grant_config)) }}
{% endmacro %}

{%- macro default__get_grant_sql(relation, grant_config) -%}
{%- for privilege in grant_config.keys() -%}
{%- set grantees = grant_config[privilege] -%}
{%- if grantees -%}
{%- for grantee in grantees -%}
grant {{ privilege }} on {{ relation }} {{ relation }} to {{ grantee}};
{%- endfor -%}
{%- endif -%}
{%- endfor -%}
{%- endmacro %}

{% macro get_revoke_sql(relation, grant_config) %}
{{ return(adapter.dispatch("get_revoke_sql", "dbt")(relation, grant_config)) }}
{% endmacro %}

{% macro default__get_revoke_sql(relation, grant_config) %}
{%- for privilege in grant_config.keys() -%}
{%- set grantees = grant[privilege] -%}
{%- if grantees -%}
{%- for grantee in grantees -%}
revoke {{ privilege }} on {{ relation }} {{ relation }} from {{ grantee }};
{% endfor -%}
{%- endif -%}
{%- endfor -%}
{%- endmacro -%}

{% macro apply_grants(relation, grant_config, should_revoke) %}
{{ return(adapter.dispatch("apply_grants", "dbt")(relation, grant_config, should_revoke)) }}
{% endmacro %}

{% macro default__apply_grants(relation, grant_config, should_revoke=True) %}
{% if grant_config %}
{% if should_revoke %}
{% set current_grants_table = run_query(get_show_grant_sql(relation)) %}
{% set current_grants_dict = adapter.standardize_grants_dict(current_grants_table) %}
{% set needs_granting = diff_of_two_dicts(grant_config, current_grants_dict) %}
{% set needs_revoking = diff_of_two_dicts(current_grants_dict, grant_config) %}
{% if not (needs_granting or needs_revoking) %}
{{ log('All grants are in place, no revocation or granting needed.')}}
{% endif %}
{% else %}
{% set needs_revoking = {} %}
{% set needs_granting = grant_config %}
{% endif %}
{% if needs_granting or needs_revoking %}
{% call statement('grants') %}
{{ get_revoke_sql(relation, needs_revoking) }}
{{ get_grant_sql(relation, needs_granting) }}
{% endcall %}
{% endif %}
{% endif %}
{% endmacro %}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
-- BEGIN, in a separate transaction
{%- set preexisting_intermediate_relation = load_cached_relation(intermediate_relation)-%}
{%- set preexisting_backup_relation = load_cached_relation(backup_relation) -%}
-- grab current tables grants config for comparision later on
Copy link
Contributor

Choose a reason for hiding this comment

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

Not something needed to do before merging, but we added the same change pretty much for all materialization methods. Should we consider refactoring the code so we can only do it in one place?

{% set grant_config = config.get('grants') %}
{{ drop_relation_if_exists(preexisting_intermediate_relation) }}
{{ drop_relation_if_exists(preexisting_backup_relation) }}

Expand Down Expand Up @@ -59,6 +61,9 @@
{% do to_drop.append(backup_relation) %}
{% endif %}

{% set should_revoke = should_reovke(existing_relation, full_refresh_mode) %}
{% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %}

{% do persist_docs(target_relation, model) %}

{% if existing_relation is none or existing_relation.is_view or should_full_refresh() %}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
{%- set backup_relation = make_backup_relation(target_relation, backup_relation_type) -%}
-- as above, the backup_relation should not already exist
{%- set preexisting_backup_relation = load_cached_relation(backup_relation) -%}
-- grab current tables grants config for comparision later on
{% set grant_config = config.get('grants') %}

-- drop the temp relations if they exist already in the database
{{ drop_relation_if_exists(preexisting_intermediate_relation) }}
Expand All @@ -40,6 +42,9 @@

{{ run_hooks(post_hooks, inside_transaction=True) }}

{% set should_revoke = should_revoke(existing_relation, full_refresh_mode=True) %}
{% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %}

{% do persist_docs(target_relation, model) %}

-- `COMMIT` happens here
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@
{%- set identifier = model['alias'] -%}

{%- set old_relation = adapter.get_relation(database=database, schema=schema, identifier=identifier) -%}

{%- set exists_as_view = (old_relation is not none and old_relation.is_view) -%}

{%- set target_relation = api.Relation.create(
identifier=identifier, schema=schema, database=database,
type='view') -%}
{% set grant_config = config.get('grants') %}

{{ run_hooks(pre_hooks) }}

Expand All @@ -34,6 +34,9 @@
{{ get_create_view_as_sql(target_relation, sql) }}
{%- endcall %}

{% set should_revoke = should_revoke(exists_as_view, full_refresh_mode=True) %}
{% do apply_grants(target_relation, grant_config, should_revoke=True) %}

{{ run_hooks(post_hooks) }}

{{ return({'relations': [target_relation]}) }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
{%- set backup_relation = make_backup_relation(target_relation, backup_relation_type) -%}
-- as above, the backup_relation should not already exist
{%- set preexisting_backup_relation = load_cached_relation(backup_relation) -%}
-- grab current tables grants config for comparision later on
{% set grant_config = config.get('grants') %}

{{ run_hooks(pre_hooks, inside_transaction=False) }}

Expand All @@ -47,6 +49,9 @@
{% endif %}
{{ adapter.rename_relation(intermediate_relation, target_relation) }}

{% set should_revoke = should_revoke(existing_relation, full_refresh_mode=True) %}
{% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %}

{% do persist_docs(target_relation, model) %}

{{ run_hooks(post_hooks, inside_transaction=True) }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
{%- set exists_as_table = (old_relation is not none and old_relation.is_table) -%}
{%- set exists_as_view = (old_relation is not none and old_relation.is_view) -%}

{%- set grant_config = config.get('grants') -%}
{%- set agate_table = load_agate_table() -%}
-- grab current tables grants config for comparision later on

{%- do store_result('agate_table', response='OK', agate_table=agate_table) -%}

{{ run_hooks(pre_hooks, inside_transaction=False) }}
Expand All @@ -35,6 +38,10 @@
{% endcall %}

{% set target_relation = this.incorporate(type='table') %}

{% set should_revoke = should_revoke(old_relation, full_refresh_mode) %}
{% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %}

{% do persist_docs(target_relation, model) %}

{% if full_refresh_mode or not exists_as_table %}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

{%- set strategy_name = config.get('strategy') -%}
{%- set unique_key = config.get('unique_key') %}
-- grab current tables grants config for comparision later on
{%- set grant_config = config.get('grants') -%}

{% set target_relation_exists, target_relation = get_or_create_relation(
database=model.database,
Expand Down Expand Up @@ -73,6 +75,9 @@
{{ final_sql }}
{% endcall %}

{% set should_revoke = should_revoke(target_relation_exists, full_refresh_mode=False) %}
{% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %}

{% do persist_docs(target_relation, model) %}

{% if not target_relation_exists %}
Expand Down
13 changes: 13 additions & 0 deletions plugins/postgres/dbt/adapters/postgres/impl.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from dbt.dataclass_schema import dbtClassMixin, ValidationError
import dbt.exceptions
import dbt.utils
import agate


# note that this isn't an adapter macro, so just a single underscore
Expand Down Expand Up @@ -85,6 +86,18 @@ def verify_database(self, database):
def parse_index(self, raw_index: Any) -> Optional[PostgresIndexConfig]:
return PostgresIndexConfig.parse(raw_index)

@available
def standardize_grants_dict(self, grants_table: agate.Table) -> dict:
grants_dict = {}
for row in grants_table:
grantee = row["grantee"]
privilege = row["privilege_type"]
if privilege in grants_dict.keys():
grants_dict[privilege].append(grantee)
else:
grants_dict.update({privilege: [grantee]})
return grants_dict

def _link_cached_database_relations(self, schemas: Set[str]):
"""
:param schemas: The set of schemas that should have links added.
Expand Down
13 changes: 13 additions & 0 deletions plugins/postgres/dbt/include/postgres/macros/adapters.sql
Original file line number Diff line number Diff line change
Expand Up @@ -202,3 +202,16 @@
comment on column {{ relation }}.{{ adapter.quote(column_name) if column_dict[column_name]['quote'] else column_name }} is {{ escaped_comment }};
{% endfor %}
{% endmacro %}

{%- macro postgres__get_show_grant_sql(relation) -%}
select grantee, privilege_type
from information_schema.role_table_grants
where grantor = current_role
Copy link
Contributor

@jtcohen6 jtcohen6 Jul 7, 2022

Choose a reason for hiding this comment

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

Not a TODO, just an open question: Should we / do we need to limit ourselves to just the grants provided by the current_role?

To copy-paste what I just said in dbt-snowflake:

The original motivation for this was: If someone else has granted a privilege on this model, and then dbt tries to revoke it, the revocation may fail. But if dbt's user/role owns the table, it should be able to revoke any access on it... right?

By removing this logic, we'd be making a strong claim, that ALL the grants applied on this object, MUST be applied by dbt, in the grants config. I think that might be a strong opinion we want to hold!

No action needed right now. Just wanted to leave the comment for future reference.

and grantee != current_role
and table_schema = '{{ relation.schema }}'
and table_name = '{{ relation.identifier }}'
{%- endmacro -%}

{% macro postgres__coppy_grants() %}
{{ return(False) }}
{% endmacro %}
1 change: 1 addition & 0 deletions test/unit/test_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ def assert_has_keys(required_keys: Set[str], maybe_keys: Set[str], ctx: Dict[str
"modules",
"flags",
"print",
"diff_of_two_dicts"
}
)

Expand Down