Skip to content

Commit

Permalink
Add Grant SQL Macros (dbt-labs#5369)
Browse files Browse the repository at this point in the history
* init push or ct-660 work

* changes to default versions of get_show_grant_sql and get_grant_sql

* completing init default versions of all macros being called for look over and collaboration

* minor update to should_revoke

* post pairing push up (does have log statements to make sure we remove)

* minor spacing changes

* minor changes, and removal of logs so people can have clean grab of code

* minor changes to how get_revoke_sql works

* init attempt at applying apply_grants to all materialzations

* name change from recipents -> grantee

* minor changes

* working on making a context to handle the diff gathering between grant_config and curreent_grants to see what needs to be revoked, I know if we assign a role, and a model becomes dependent on it we can't drop the role now still not seeing the diff appear in log

* removing logs from most materializations to better track diff of grants generation logs

* starting to build out postgres get_show_grant_sql getting empty query errors hopefully will clear up as we add the other postgres versions of macros and isn't a psycopg2 issue as indicated by searching

* 6/27 eod update looking into diff_grants variable not getting passed into get_revoke_sql

* changes to loop cases

* changes after pairing meeting

* adding apply_grants to create_or_replace_view.sql

* models are building but testing out small issues around revoke statement never building

* postgrest must fixes from jeremy's feedback

* postgres minor change to standarize_grants_dict

* updating after pairing with dough and jeremey incorporating the new version of should revoke logic.

* adding  ref of diff_of_two_dicts to base keys ref

* change of method type for standardize_grants_dict

* minor update trying to fix unit test

* changes based on morning feedback

* change log message in default_apply_grants macro

* CT-808 grant adapter tests (dbt-labs#5447)

* Create initial test for grants

Co-authored-by: Jeremy Cohen <[email protected]>

* rename grant[privilege] -> grant_config[privilege]

* postgres macro rename to copy_grants

* CT-808 more grant adapter tests (dbt-labs#5452)

* Add tests for invalid user and invalid privilege

* Add more grant tests

* Macro touch ups

* Many more tests

* Allow easily replacing privilege names

* Keep adding tests

* Refactor macros to return lists, fix test

* Code checks

* Keep tweaking tests

* Revert cool grantees join bc Snowflake isnt happy

* Use Postgres/BQ as standard for standardize_grants_dict

* Code checks

* add missing replace

* small replace tweak,  add additional dict diffs

* All tests passing on BQ

* Add type cast to test_snapshot_grants

* Refactor for DRYer apply_grants macros

Co-authored-by: Jeremy Cohen <[email protected]>
Co-authored-by: Emily Rockman <[email protected]>

* update to main, create changelog, whitespace fixes

Co-authored-by: Gerda Shank <[email protected]>
Co-authored-by: Jeremy Cohen <[email protected]>
Co-authored-by: Emily Rockman <[email protected]>
  • Loading branch information
4 people authored and Axel Goblet committed Sep 16, 2022
1 parent aa64bc2 commit 380a8e9
Show file tree
Hide file tree
Showing 25 changed files with 924 additions and 21 deletions.
8 changes: 8 additions & 0 deletions .changes/unreleased/Features-20220711-111514.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
kind: Features
body: Allow users to define grants as a reasonable default in the dbt_project.yml
or within each model sql or yml file combined.
time: 2022-07-11T11:15:14.695386-05:00
custom:
Author: McKnight-42
Issue: "5263"
PR: "5369"
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
28 changes: 28 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,33 @@ 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.
Ideally, the SQL to show grants should also be filtering:
filter OUT any grants TO the current user/role (e.g. OWNERSHIP).
If that's not possible in SQL, it can be done in this method instead.
: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
"""
grants_dict: Dict[str, List[str]] = {}
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

###
# Provided methods about relations
###
Expand Down
31 changes: 30 additions & 1 deletion core/dbt/context/base.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import json
import os
from typing import Any, Dict, NoReturn, Optional, Mapping, Iterable, Set
from typing import Any, Dict, NoReturn, Optional, Mapping, Iterable, Set, List

from dbt import flags
from dbt import tracking
Expand Down Expand Up @@ -657,6 +657,35 @@ def print(msg: str) -> str:
print(msg)
return ""

@contextmember
@staticmethod
def diff_of_two_dicts(
dict_a: Dict[str, List[str]], dict_b: Dict[str, List[str]]
) -> Dict[str, List[str]]:
"""
Given two dictionaries of type Dict[str, List[str]]:
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,
performing a case-insensitive comparison between the strings in each.
All keys returned will be in the original case of dict_a.
returns {'key_x': ['VALUE_2'], 'KEY_Y': ['value_3']}
"""

dict_diff = {}
dict_b_lowered = {k.casefold(): [x.casefold() for x in v] for k, v in dict_b.items()}
for k in dict_a:
if k.casefold() in dict_b_lowered.keys():
diff = []
for v in dict_a[k]:
if v.casefold() not in dict_b_lowered[k.casefold()]:
diff.append(v)
if diff:
dict_diff.update({k: diff})
else:
dict_diff.update({k: dict_a[k]})
return dict_diff


def generate_base_context(cli_vars: Dict[str, Any]) -> Dict[str, Any]:
ctx = BaseContext(cli_vars)
Expand Down
167 changes: 167 additions & 0 deletions core/dbt/include/global_project/macros/adapters/apply_grants.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
{# ------- BOOLEAN MACROS --------- #}

{#
-- COPY GRANTS
-- When a relational object (view or table) is replaced in this database,
-- do previous grants carry over to the new object? This may depend on:
-- whether we use alter-rename-swap versus CREATE OR REPLACE
-- user-supplied configuration (e.g. copy_grants on Snowflake)
-- By default, play it safe, assume TRUE: that grants ARE copied over.
-- This means dbt will first "show" current grants and then calculate diffs.
-- It may require an additional query than is strictly necessary,
-- but better safe than sorry.
#}

{% macro copy_grants() %}
{{ return(adapter.dispatch('copy_grants', 'dbt')()) }}
{% endmacro %}

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


{#
-- SUPPORT MULTIPLE GRANTEES PER DCL STATEMENT
-- Does this database support 'grant {privilege} to {grantee_1}, {grantee_2}, ...'
-- Or must these be separate statements:
-- `grant {privilege} to {grantee_1}`;
-- `grant {privilege} to {grantee_2}`;
-- By default, pick the former, because it's what we prefer when available.
#}

{% macro support_multiple_grantees_per_dcl_statement() %}
{{ return(adapter.dispatch('support_multiple_grantees_per_dcl_statement', 'dbt')()) }}
{% endmacro %}

{%- macro default__support_multiple_grantees_per_dcl_statement() -%}
{{ 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 %}

{# ------- DCL STATEMENT TEMPLATES --------- #}

{% 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, privilege, grantees) %}
{{ return(adapter.dispatch('get_grant_sql', 'dbt')(relation, privilege, grantees)) }}
{% endmacro %}

{%- macro default__get_grant_sql(relation, privilege, grantees) -%}
grant {{ privilege }} on {{ relation }} to {{ grantees | join(', ') }}
{%- endmacro -%}


{% macro get_revoke_sql(relation, privilege, grantees) %}
{{ return(adapter.dispatch('get_revoke_sql', 'dbt')(relation, privilege, grantees)) }}
{% endmacro %}

{%- macro default__get_revoke_sql(relation, privilege, grantees) -%}
revoke {{ privilege }} on {{ relation }} from {{ grantees | join(', ') }}
{%- endmacro -%}


{# ------- RUNTIME APPLICATION --------- #}

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

{%- macro default__get_dcl_statement_list(relation, grant_config, get_dcl_macro) -%}
{#
-- Unpack grant_config into specific privileges and the set of users who need them granted/revoked.
-- Depending on whether this database supports multiple grantees per statement, pass in the list of
-- all grantees per privilege, or (if not) template one statement per privilege-grantee pair.
-- `get_dcl_macro` will be either `get_grant_sql` or `get_revoke_sql`
#}
{%- set dcl_statements = [] -%}
{%- for privilege, grantees in grant_config.items() %}
{%- if support_multiple_grantees_per_dcl_statement() and grantees -%}
{%- set dcl = get_dcl_macro(relation, privilege, grantees) -%}
{%- do dcl_statements.append(dcl) -%}
{%- else -%}
{%- for grantee in grantees -%}
{% set dcl = get_dcl_macro(relation, privilege, [grantee]) %}
{%- do dcl_statements.append(dcl) -%}
{% endfor -%}
{%- endif -%}
{%- endfor -%}
{{ return(dcl_statements) }}
{%- endmacro %}


{% macro call_dcl_statements(dcl_statement_list) %}
{{ return(adapter.dispatch("call_dcl_statements", "dbt")(dcl_statement_list)) }}
{% endmacro %}

{% macro default__call_dcl_statements(dcl_statement_list) %}
{#
-- By default, supply all grant + revoke statements in a single semicolon-separated block,
-- so that they're all processed together.

-- Some databases do not support this. Those adapters will need to override this macro
-- to run each statement individually.
#}
{% call statement('grants') %}
{% for dcl_statement in dcl_statement_list %}
{{ dcl_statement }};
{% endfor %}
{% endcall %}
{% 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 is {} or None, this is a no-op --#}
{% if grant_config %}
{% if should_revoke %}
{#-- We think previous grants may have carried over --#}
{#-- Show current grants and calculate diffs --#}
{% 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 %}
{#-- We don't think there's any chance of previous grants having carried over. --#}
{#-- Jump straight to granting what the user has configured. --#}
{% set needs_revoking = {} %}
{% set needs_granting = grant_config %}
{% endif %}
{% if needs_granting or needs_revoking %}
{% set revoke_statement_list = get_dcl_statement_list(relation, needs_revoking, get_revoke_sql) %}
{% set grant_statement_list = get_dcl_statement_list(relation, needs_granting, get_grant_sql) %}
{% set dcl_statement_list = revoke_statement_list + grant_statement_list %}
{% if dcl_statement_list %}
{{ call_dcl_statements(dcl_statement_list) }}
{% endif %}
{% 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
{% 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_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') %}

-- 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
Loading

0 comments on commit 380a8e9

Please sign in to comment.