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

Add Grant SQL Macros #5369

Merged
merged 37 commits into from
Jul 11, 2022
Merged
Show file tree
Hide file tree
Changes from 33 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
7 changes: 7 additions & 0 deletions .changes/unreleased/Under the Hood-20220706-215001.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: Under the Hood
body: Add tests for SQL grants
time: 2022-07-06T21:50:01.498562-04:00
custom:
Author: gshank
Issue: "5437"
PR: "5447"
3 changes: 3 additions & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ jobs:
TOXENV: integration
PYTEST_ADDOPTS: "-v --color=yes -n4 --csv integration_results.csv"
DBT_INVOCATION_ENV: github-actions
DBT_TEST_USER_1: dbt_test_user_1
DBT_TEST_USER_2: dbt_test_user_2
DBT_TEST_USER_3: dbt_test_user_3

steps:
- name: Check out the repository
Expand Down
5 changes: 5 additions & 0 deletions .github/workflows/structured-logging-schema-check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ jobs:
LOG_DIR: "/home/runner/work/dbt-core/dbt-core/logs"
# tells integration tests to output into json format
DBT_LOG_FORMAT: "json"
# Additional test users
DBT_TEST_USER_1: dbt_test_user_1
DBT_TEST_USER_2: dbt_test_user_2
DBT_TEST_USER_3: dbt_test_user_3

steps:
- name: checkout dev
uses: actions/checkout@v2
Expand Down
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!"
)

###
# Methods about grants
###
@available
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
"""
jtcohen6 marked this conversation as resolved.
Show resolved Hide resolved

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 }}
{% 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 }} to {{ grantee }};
{%- endfor -%}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved
{%- 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_config[privilege] -%}
{%- if grantees -%}
{%- for grantee in grantees -%}
revoke {{ privilege }} on {{ 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('On ' ~ relation ~': 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') %}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved
{{ 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_revoke(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') %}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved

-- 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') %}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved

{{ 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) %}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved

{{ 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') %}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved

{{ 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') -%}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved
{%- 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') -%}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved

{% 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
4 changes: 3 additions & 1 deletion core/dbt/parser/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,9 @@ def read_manifest_for_partial_parse(self) -> Optional[Manifest]:
if not flags.PARTIAL_PARSE:
fire_event(PartialParsingNotEnabled())
return None
path = os.path.join(self.root_project.target_path, PARTIAL_PARSE_FILE_NAME)
path = os.path.join(
self.root_project.project_root, self.root_project.target_path, PARTIAL_PARSE_FILE_NAME
)

reparse_reason = None

Expand Down
10 changes: 9 additions & 1 deletion core/dbt/tests/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,10 @@ def run_dbt(args: List[str] = None, expect_pass=True):
return res


# Use this if you need to capture the command logs in a test
# Use this if you need to capture the command logs in a test.
# If you want the logs that are normally written to a file, you must
# start with the "--debug" flag. The structured schema log CI test
# will turn the logs into json, so you have to be prepared for that.
def run_dbt_and_capture(args: List[str] = None, expect_pass=True):
try:
stringbuf = capture_stdout_logs()
Expand All @@ -83,6 +86,11 @@ def run_dbt_and_capture(args: List[str] = None, expect_pass=True):
finally:
stop_capture_stdout_logs()

# Json logs will have lots of escape characters which will
# make checks for strings in the logs fail, so remove those.
if '{"code":' in stdout:
stdout = stdout.replace("\\", "")

return res, stdout


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:
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved
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 %}

McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved
{%- 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
jtcohen6 marked this conversation as resolved.
Show resolved Hide resolved
and table_schema = '{{ relation.schema }}'
and table_name = '{{ relation.identifier }}'
{%- endmacro -%}

{% macro postgres__are_grants_copied_over_when_replaced() %}
{{ return(False) }}
{% endmacro %}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 3 additions & 0 deletions test/setup_db.sh
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ psql -c "GRANT CREATE, CONNECT ON DATABASE dbt TO root WITH GRANT OPTION;"
psql -c "CREATE ROLE noaccess WITH PASSWORD 'password' NOSUPERUSER;"
psql -c "ALTER ROLE noaccess WITH LOGIN;"
psql -c "GRANT CONNECT ON DATABASE dbt TO noaccess;"
psql -c "CREATE ROLE dbt_test_user_1;"
psql -c "CREATE ROLE dbt_test_user_2;"
psql -c "CREATE ROLE dbt_test_user_3;"

psql -c 'CREATE DATABASE "dbtMixedCase";'
psql -c 'GRANT CREATE, CONNECT ON DATABASE "dbtMixedCase" TO root WITH GRANT OPTION;'
Expand Down
Loading