From 6e88fe702a20081485645d45dbc523c9ac112e33 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Mon, 13 Jun 2022 15:19:31 -0500 Subject: [PATCH 01/32] init push or ct-660 work --- .../macros/adapters/apply_grants.sql | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 core/dbt/include/global_project/macros/adapters/apply_grants.sql diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql new file mode 100644 index 00000000000..156eec3fbee --- /dev/null +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -0,0 +1,33 @@ +{% macro get_show_grant_sql(relation) %} +{{ return(adapter.dispatch('get_show_grant_sql', 'dbt')(relation)) }} +{% endmacro %} + +{% macro default__get_show_grant_sql(relation) %} +{{ return('show grants on table'(relation.schema)) }} +{% 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) %} +{{ return(relation) }} +{% 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) %} +return +{% endmacro %} + + +{% macro apply_grants(revoke, relation, grant_config) %} +{{ return(adapter.dispatch('apply_grant', 'dbt')(revoke, relation, grant_config)) }} +{% endmacro %} + +{% macro default__apply_grants(revoke, relation, grant_config) %} +{{ get_show_grant_sql() }} +{% return %} +{% endmacro %} From 6d4b9389b108f6ac9e3c006af6d4bd716eccd1be Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Tue, 14 Jun 2022 10:49:48 -0500 Subject: [PATCH 02/32] changes to default versions of get_show_grant_sql and get_grant_sql --- .../global_project/macros/adapters/apply_grants.sql | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 156eec3fbee..1835ced5f83 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -3,7 +3,7 @@ {% endmacro %} {% macro default__get_show_grant_sql(relation) %} -{{ return('show grants on table'(relation.schema)) }} +show grants on {{ relation.type }} {{ relation }} {% endmacro %} {% macro get_grant_sql(relation, grant_config) %} @@ -11,7 +11,7 @@ {% endmacro %} {% macro default__get_grant_sql(relation, grant_config) %} -{{ return(relation) }} +grant {{ privalage }} on {{ relation.type }} {{ relation }} to {{ recipients }} {% endmacro %} {% macro get_revoke_sql(relation, grant_config) %} @@ -19,15 +19,11 @@ {% endmacro %} {% macro default__get_revoke_sql(relation, grant_config) %} -return {% endmacro %} - -{% macro apply_grants(revoke, relation, grant_config) %} -{{ return(adapter.dispatch('apply_grant', 'dbt')(revoke, relation, grant_config)) }} +{% macro apply_grants(relation, grant_config, should_revoke) %} +{{ return(adapter.dispatch('apply_grant', 'dbt')(relation, grant_config, should_revoke)) }} {% endmacro %} {% macro default__apply_grants(revoke, relation, grant_config) %} -{{ get_show_grant_sql() }} -{% return %} {% endmacro %} From ea35fd14546f501c2a0089efa89ae54a8dcc41a8 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Tue, 14 Jun 2022 16:10:22 -0500 Subject: [PATCH 03/32] completing init default versions of all macros being called for look over and collaboration --- .../macros/adapters/apply_grants.sql | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 1835ced5f83..b418f14186c 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -1,5 +1,5 @@ {% macro get_show_grant_sql(relation) %} -{{ return(adapter.dispatch('get_show_grant_sql', 'dbt')(relation)) }} +{{ return(adapter.dispatch("get_show_grant_sql", "dbt")(relation)) }} {% endmacro %} {% macro default__get_show_grant_sql(relation) %} @@ -11,19 +11,27 @@ show grants on {{ relation.type }} {{ relation }} {% endmacro %} {% macro default__get_grant_sql(relation, grant_config) %} -grant {{ privalage }} on {{ relation.type }} {{ relation }} to {{ recipients }} +grant "select" on {{ relation.type }} {{ relation }} to {{ grant_config["select"].join(", ") }} {% endmacro %} {% macro get_revoke_sql(relation, grant_config) %} -{{ return(adapter.dispatch('get_revoke_sql', 'dbt')(relation, grant_config)) }} +{{ return(adapter.dispatch("get_revoke_sql", "dbt")(relation, grant_config)) }} {% endmacro %} {% macro default__get_revoke_sql(relation, grant_config) %} +revoke "select" on {{ relation }} from {{ grant_config["select"].join(", ") }} {% endmacro %} {% macro apply_grants(relation, grant_config, should_revoke) %} -{{ return(adapter.dispatch('apply_grant', 'dbt')(relation, grant_config, should_revoke)) }} +{{ return(adapter.dispatch("apply_grants", "dbt")(relation, grant_config, should_revoke)) }} {% endmacro %} -{% macro default__apply_grants(revoke, relation, grant_config) %} +{% macro default__apply_grants(relation, grant_config, should_revoke) %} +{% if "select" not in grant_config %} +get_show_grant_sql(relation) + {% if should_revoke %} + get_revoke_sql(relation, grant_config) + {% endif%} +get_grant_sql(relation, grant_config) +{% endif%} {% endmacro %} From 7e2b7078ecb2288a1f57f7e7c0a7daeae868bbba Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Tue, 14 Jun 2022 16:20:19 -0500 Subject: [PATCH 04/32] minor update to should_revoke --- .../macros/adapters/apply_grants.sql | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index b418f14186c..f8817f16926 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -26,12 +26,14 @@ revoke "select" on {{ relation }} from {{ grant_config["select"].join(", ") }} {{ return(adapter.dispatch("apply_grants", "dbt")(relation, grant_config, should_revoke)) }} {% endmacro %} -{% macro default__apply_grants(relation, grant_config, should_revoke) %} -{% if "select" not in grant_config %} -get_show_grant_sql(relation) - {% if should_revoke %} - get_revoke_sql(relation, grant_config) +{% macro default__apply_grants(relation, grant_config, should_revoke=True) %} +{% if grant_config %} + {% if "select" not in grant_config %} + get_show_grant_sql(relation) + {% if should_revoke %} + get_revoke_sql(relation, grant_config) + {% endif%} + get_grant_sql(relation, grant_config) {% endif%} -get_grant_sql(relation, grant_config) {% endif%} {% endmacro %} From 898aa8ffed6259a88d215d481618f257f61af1ab Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Wed, 15 Jun 2022 14:59:44 -0500 Subject: [PATCH 05/32] post pairing push up (does have log statements to make sure we remove) --- .../macros/adapters/apply_grants.sql | 27 ++++++++++++------- .../materializations/models/table/table.sql | 3 ++- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index f8817f16926..eda81832284 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -11,7 +11,10 @@ show grants on {{ relation.type }} {{ relation }} {% endmacro %} {% macro default__get_grant_sql(relation, grant_config) %} -grant "select" on {{ relation.type }} {{ relation }} to {{ grant_config["select"].join(", ") }} +{% for privilege in grant_config.keys() %} + {% set recipients = grant_config[privilege] %} + grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ recipients | join(', ') }} + {% endfor %} {% endmacro %} {% macro get_revoke_sql(relation, grant_config) %} @@ -19,7 +22,8 @@ grant "select" on {{ relation.type }} {{ relation }} to {{ grant_config["select" {% endmacro %} {% macro default__get_revoke_sql(relation, grant_config) %} -revoke "select" on {{ relation }} from {{ grant_config["select"].join(", ") }} +revoke all on {{ relation }} from {{ grant_config["select"].join(", ") }} +where grantee != {{ target.user }} {% endmacro %} {% macro apply_grants(relation, grant_config, should_revoke) %} @@ -27,13 +31,18 @@ revoke "select" on {{ relation }} from {{ grant_config["select"].join(", ") }} {% endmacro %} {% macro default__apply_grants(relation, grant_config, should_revoke=True) %} +{{ log("In apply grants ===") }} {% if grant_config %} - {% if "select" not in grant_config %} - get_show_grant_sql(relation) + {{ log("got grant config ===") }} + {% set current_grants = get_show_grant_sql(relation) %} + {% call statement('grants') %} {% if should_revoke %} - get_revoke_sql(relation, grant_config) - {% endif%} - get_grant_sql(relation, grant_config) - {% endif%} -{% endif%} + {{ get_revoke_sql(relation, grant_config) }} + {{ log("should revoke ===") }} + {% endif %} + {{ log(current_grants) }} + {{ get_grant_sql(relation, grant_config) }} + {% endcall %} + {{ log("after call ===") }} +{% endif %} {% endmacro %} diff --git a/core/dbt/include/global_project/macros/materializations/models/table/table.sql b/core/dbt/include/global_project/macros/materializations/models/table/table.sql index 2009183ec41..8848397105e 100644 --- a/core/dbt/include/global_project/macros/materializations/models/table/table.sql +++ b/core/dbt/include/global_project/macros/materializations/models/table/table.sql @@ -39,7 +39,8 @@ {% do create_indexes(target_relation) %} {{ run_hooks(post_hooks, inside_transaction=True) }} - + {{ log(config.get('grants'), "what grants are we passing") }} + {% do apply_grants(target_relation, config.get('grants'))%} {% do persist_docs(target_relation, model) %} -- `COMMIT` happens here From 76050da4826482b60feb06d579134fb537e9c365 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Wed, 15 Jun 2022 15:43:36 -0500 Subject: [PATCH 06/32] minor spacing changes --- .../global_project/macros/adapters/apply_grants.sql | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index eda81832284..0f831bee697 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -11,10 +11,10 @@ show grants on {{ relation.type }} {{ relation }} {% endmacro %} {% macro default__get_grant_sql(relation, grant_config) %} -{% for privilege in grant_config.keys() %} - {% set recipients = grant_config[privilege] %} - grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ recipients | join(', ') }} - {% endfor %} + {% for privilege in grant_config.keys() %} + {% set recipients = grant_config[privilege] %} + grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ recipients | join(', ') }} + {% endfor %} {% endmacro %} {% macro get_revoke_sql(relation, grant_config) %} From 5fb07c1a107dcd6dd60e5dcc757fa8750ff22fc8 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Thu, 16 Jun 2022 16:21:18 -0500 Subject: [PATCH 07/32] minor changes, and removal of logs so people can have clean grab of code --- .../global_project/macros/adapters/apply_grants.sql | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 0f831bee697..c5fd2a3ee4f 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -31,18 +31,13 @@ where grantee != {{ target.user }} {% endmacro %} {% macro default__apply_grants(relation, grant_config, should_revoke=True) %} -{{ log("In apply grants ===") }} {% if grant_config %} - {{ log("got grant config ===") }} - {% set current_grants = get_show_grant_sql(relation) %} {% call statement('grants') %} {% if should_revoke %} + {% set current_grants = run_query(get_show_grant_sql(relation)) %} {{ get_revoke_sql(relation, grant_config) }} - {{ log("should revoke ===") }} {% endif %} - {{ log(current_grants) }} {{ get_grant_sql(relation, grant_config) }} {% endcall %} - {{ log("after call ===") }} {% endif %} {% endmacro %} From 4cf705f377304ca1734d449abd9a39fc8f3fb6af Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Fri, 17 Jun 2022 12:47:30 -0500 Subject: [PATCH 08/32] minor changes to how get_revoke_sql works --- .../global_project/macros/adapters/apply_grants.sql | 12 +++++++++--- .../macros/materializations/models/table/table.sql | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index c5fd2a3ee4f..98b1e2a51d0 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -22,8 +22,11 @@ show grants on {{ relation.type }} {{ relation }} {% endmacro %} {% macro default__get_revoke_sql(relation, grant_config) %} -revoke all on {{ relation }} from {{ grant_config["select"].join(", ") }} -where grantee != {{ target.user }} + {% for privilege in grant_config.keys() %} + {% set recipients = grant_config[privilege] %} + revoke {{ privilege }} on {{ relation.type }} {{ relation }} from {{ recipients | join(', ') }} + where {{ recipients }} != {{ target.user }} + {% endfor %} {% endmacro %} {% macro apply_grants(relation, grant_config, should_revoke) %} @@ -35,7 +38,10 @@ where grantee != {{ target.user }} {% call statement('grants') %} {% if should_revoke %} {% set current_grants = run_query(get_show_grant_sql(relation)) %} - {{ get_revoke_sql(relation, grant_config) }} + {{ log(current_grants, info = true) }} + {% set diff_grants = { k: current_grants[k] for k in set(current_grants) - set(grant_config) } %} + {{ log(diff_grants, info = true) }} + {{ get_revoke_sql(relation, diff_grants) }} {% endif %} {{ get_grant_sql(relation, grant_config) }} {% endcall %} diff --git a/core/dbt/include/global_project/macros/materializations/models/table/table.sql b/core/dbt/include/global_project/macros/materializations/models/table/table.sql index 8848397105e..5c69e4d7949 100644 --- a/core/dbt/include/global_project/macros/materializations/models/table/table.sql +++ b/core/dbt/include/global_project/macros/materializations/models/table/table.sql @@ -40,7 +40,7 @@ {{ run_hooks(post_hooks, inside_transaction=True) }} {{ log(config.get('grants'), "what grants are we passing") }} - {% do apply_grants(target_relation, config.get('grants'))%} + {% do apply_grants(target_relation, config.get('grants')) %} {% do persist_docs(target_relation, model) %} -- `COMMIT` happens here From 2fff84bb55a14e9f0c5fd877316e46630244ce96 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Fri, 17 Jun 2022 13:33:45 -0500 Subject: [PATCH 09/32] init attempt at applying apply_grants to all materialzations --- .../materializations/models/incremental/incremental.sql | 5 +++++ .../macros/materializations/models/table/table.sql | 6 ++++-- .../macros/materializations/models/view/view.sql | 5 +++++ .../global_project/macros/materializations/seeds/seed.sql | 5 +++++ .../macros/materializations/snapshots/snapshot.sql | 5 ++++- 5 files changed, 23 insertions(+), 3 deletions(-) diff --git a/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql b/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql index 1cb316b1ab0..20dc75e6c1a 100644 --- a/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql +++ b/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql @@ -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) }} @@ -59,6 +61,9 @@ {% do to_drop.append(backup_relation) %} {% endif %} + {{ log(grant_config, "what grants are we passing") }} + {% do apply_grants(target_relation, grant_config) %} + {% do persist_docs(target_relation, model) %} {% if existing_relation is none or existing_relation.is_view or should_full_refresh() %} diff --git a/core/dbt/include/global_project/macros/materializations/models/table/table.sql b/core/dbt/include/global_project/macros/materializations/models/table/table.sql index 5c69e4d7949..a5e03095f8b 100644 --- a/core/dbt/include/global_project/macros/materializations/models/table/table.sql +++ b/core/dbt/include/global_project/macros/materializations/models/table/table.sql @@ -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) }} @@ -39,8 +41,8 @@ {% do create_indexes(target_relation) %} {{ run_hooks(post_hooks, inside_transaction=True) }} - {{ log(config.get('grants'), "what grants are we passing") }} - {% do apply_grants(target_relation, config.get('grants')) %} + {{ log(grant_config, "what grants are we passing") }} + {% do apply_grants(target_relation, grant_config) %} {% do persist_docs(target_relation, model) %} -- `COMMIT` happens here diff --git a/core/dbt/include/global_project/macros/materializations/models/view/view.sql b/core/dbt/include/global_project/macros/materializations/models/view/view.sql index 776cf11dfa0..d27a89fb20c 100644 --- a/core/dbt/include/global_project/macros/materializations/models/view/view.sql +++ b/core/dbt/include/global_project/macros/materializations/models/view/view.sql @@ -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) }} @@ -47,6 +49,9 @@ {% endif %} {{ adapter.rename_relation(intermediate_relation, target_relation) }} + {{ log(grant_config, "what grants are we passing") }} + {% do apply_grants(target_relation, grant_config) %} + {% do persist_docs(target_relation, model) %} {{ run_hooks(post_hooks, inside_transaction=True) }} diff --git a/core/dbt/include/global_project/macros/materializations/seeds/seed.sql b/core/dbt/include/global_project/macros/materializations/seeds/seed.sql index 7461d33eab0..b427d2dca17 100644 --- a/core/dbt/include/global_project/macros/materializations/seeds/seed.sql +++ b/core/dbt/include/global_project/macros/materializations/seeds/seed.sql @@ -9,6 +9,9 @@ {%- set exists_as_view = (old_relation is not none and old_relation.is_view) -%} {%- set agate_table = load_agate_table() -%} + -- grab current tables grants config for comparision later on + {% set grant_config = config.get('grants') %} + {%- do store_result('agate_table', response='OK', agate_table=agate_table) -%} {{ run_hooks(pre_hooks, inside_transaction=False) }} @@ -35,6 +38,8 @@ {% endcall %} {% set target_relation = this.incorporate(type='table') %} + {{ log(grant_config, "what grants are we passing") }} + {% do apply_grants(target_relation, grant_config) %} {% do persist_docs(target_relation, model) %} {% if full_refresh_mode or not exists_as_table %} diff --git a/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql b/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql index c1da517c343..1637439bb06 100644 --- a/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql +++ b/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql @@ -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, @@ -72,7 +74,8 @@ {% call statement('main') %} {{ final_sql }} {% endcall %} - + {{ log(grant_config, "what grants are we passing") }} + {% do apply_grants(target_relation, grant_config) %} {% do persist_docs(target_relation, model) %} {% if not target_relation_exists %} From 6fccef6dac7accdd0e22d80ae19affe62388997c Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Fri, 17 Jun 2022 14:43:25 -0500 Subject: [PATCH 10/32] name change from recipents -> grantee --- .../global_project/macros/adapters/apply_grants.sql | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 98b1e2a51d0..1b04dcbab34 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -12,8 +12,8 @@ show grants on {{ relation.type }} {{ relation }} {% macro default__get_grant_sql(relation, grant_config) %} {% for privilege in grant_config.keys() %} - {% set recipients = grant_config[privilege] %} - grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ recipients | join(', ') }} + {% set grantee = grant_config[privilege] %} + grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ grantee | join(', ') }} {% endfor %} {% endmacro %} @@ -23,9 +23,9 @@ show grants on {{ relation.type }} {{ relation }} {% macro default__get_revoke_sql(relation, grant_config) %} {% for privilege in grant_config.keys() %} - {% set recipients = grant_config[privilege] %} - revoke {{ privilege }} on {{ relation.type }} {{ relation }} from {{ recipients | join(', ') }} - where {{ recipients }} != {{ target.user }} + {% set grantee = grant_config[privilege] %} + revoke {{ privilege }} on {{ relation.type }} {{ relation }} from {{ grantee | join(', ') }} + where {{ grantee }} != {{ target.user }} {% endfor %} {% endmacro %} @@ -40,6 +40,7 @@ show grants on {{ relation.type }} {{ relation }} {% set current_grants = run_query(get_show_grant_sql(relation)) %} {{ log(current_grants, info = true) }} {% set diff_grants = { k: current_grants[k] for k in set(current_grants) - set(grant_config) } %} + {{ log(diff_grants, info = true) }} {{ get_revoke_sql(relation, diff_grants) }} {% endif %} From 528dff684d5b80a672bd18aefed6e08d17ef62be Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Fri, 17 Jun 2022 16:53:02 -0500 Subject: [PATCH 11/32] minor changes --- core/dbt/include/global_project/macros/adapters/apply_grants.sql | 1 - 1 file changed, 1 deletion(-) diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 1b04dcbab34..3ab6d1b477d 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -40,7 +40,6 @@ show grants on {{ relation.type }} {{ relation }} {% set current_grants = run_query(get_show_grant_sql(relation)) %} {{ log(current_grants, info = true) }} {% set diff_grants = { k: current_grants[k] for k in set(current_grants) - set(grant_config) } %} - {{ log(diff_grants, info = true) }} {{ get_revoke_sql(relation, diff_grants) }} {% endif %} From 9079c4a9c5d3a5cdf700728304281356215301b9 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Tue, 21 Jun 2022 17:05:13 -0500 Subject: [PATCH 12/32] 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 --- core/dbt/context/base.py | 7 +++++++ .../macros/adapters/apply_grants.sql | 14 ++++++++++---- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/core/dbt/context/base.py b/core/dbt/context/base.py index c2bf912d55c..eec3065a561 100644 --- a/core/dbt/context/base.py +++ b/core/dbt/context/base.py @@ -662,3 +662,10 @@ def generate_base_context(cli_vars: Dict[str, Any]) -> Dict[str, Any]: ctx = BaseContext(cli_vars) # This is not a Mashumaro to_dict call return ctx.to_dict() + + +def diff_of_two_dicts( + grant_config: Dict[str, Any], current_grants: Dict[str, Any] +) -> Dict[str, Any]: + diff_dict = {k: current_grants[k] for k in set(current_grants) - set(grant_config)} + return diff_dict diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 3ab6d1b477d..33a7bc77f77 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -38,12 +38,18 @@ show grants on {{ relation.type }} {{ relation }} {% call statement('grants') %} {% if should_revoke %} {% set current_grants = run_query(get_show_grant_sql(relation)) %} - {{ log(current_grants, info = true) }} - {% set diff_grants = { k: current_grants[k] for k in set(current_grants) - set(grant_config) } %} - {{ log(diff_grants, info = true) }} - {{ get_revoke_sql(relation, diff_grants) }} + {% for privilege in grant_config.keys() %} + {% if privilege not in current_grants.keys() %} + {% set diff_grants = diff_of_two_dicts(grant_config, current_grants) %} + {% endif %} + {% endfor %} + {{ log(diff_grants, info=True) }} + {% set revoke_grants = get_revoke_sql(relation, diff_grants) %} {% endif %} {{ get_grant_sql(relation, grant_config) }} {% endcall %} {% endif %} {% endmacro %} + +-- {% set diff_grants = { k: current_grants[k] for k in set(current_grants) - set(grant_config) } %} +-- {%- set _ = diff_grants.update({privilege: grant_config[privilege]}) -%} From 6bdb4b5152cf64bd8cb7df0e9e5e524fe9520ad8 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Wed, 22 Jun 2022 10:34:12 -0500 Subject: [PATCH 13/32] removing logs from most materializations to better track diff of grants generation logs --- core/dbt/context/base.py | 1 + .../dbt/include/global_project/macros/adapters/apply_grants.sql | 1 + .../macros/materializations/models/incremental/incremental.sql | 1 - .../macros/materializations/models/table/table.sql | 2 +- .../global_project/macros/materializations/models/view/view.sql | 1 - .../global_project/macros/materializations/seeds/seed.sql | 1 - .../macros/materializations/snapshots/snapshot.sql | 2 +- 7 files changed, 4 insertions(+), 5 deletions(-) diff --git a/core/dbt/context/base.py b/core/dbt/context/base.py index eec3065a561..2e66362a644 100644 --- a/core/dbt/context/base.py +++ b/core/dbt/context/base.py @@ -668,4 +668,5 @@ def diff_of_two_dicts( grant_config: Dict[str, Any], current_grants: Dict[str, Any] ) -> Dict[str, Any]: diff_dict = {k: current_grants[k] for k in set(current_grants) - set(grant_config)} + print("************", diff_dict) return diff_dict diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 33a7bc77f77..014d91df192 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -38,6 +38,7 @@ show grants on {{ relation.type }} {{ relation }} {% call statement('grants') %} {% if should_revoke %} {% set current_grants = run_query(get_show_grant_sql(relation)) %} + {{ log(current_grants, info=True) }} {% for privilege in grant_config.keys() %} {% if privilege not in current_grants.keys() %} {% set diff_grants = diff_of_two_dicts(grant_config, current_grants) %} diff --git a/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql b/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql index 20dc75e6c1a..fb52b5e4bdb 100644 --- a/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql +++ b/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql @@ -61,7 +61,6 @@ {% do to_drop.append(backup_relation) %} {% endif %} - {{ log(grant_config, "what grants are we passing") }} {% do apply_grants(target_relation, grant_config) %} {% do persist_docs(target_relation, model) %} diff --git a/core/dbt/include/global_project/macros/materializations/models/table/table.sql b/core/dbt/include/global_project/macros/materializations/models/table/table.sql index a5e03095f8b..6cb0bf82a3f 100644 --- a/core/dbt/include/global_project/macros/materializations/models/table/table.sql +++ b/core/dbt/include/global_project/macros/materializations/models/table/table.sql @@ -41,7 +41,7 @@ {% do create_indexes(target_relation) %} {{ run_hooks(post_hooks, inside_transaction=True) }} - {{ log(grant_config, "what grants are we passing") }} + {% do apply_grants(target_relation, grant_config) %} {% do persist_docs(target_relation, model) %} diff --git a/core/dbt/include/global_project/macros/materializations/models/view/view.sql b/core/dbt/include/global_project/macros/materializations/models/view/view.sql index d27a89fb20c..c75873d9292 100644 --- a/core/dbt/include/global_project/macros/materializations/models/view/view.sql +++ b/core/dbt/include/global_project/macros/materializations/models/view/view.sql @@ -49,7 +49,6 @@ {% endif %} {{ adapter.rename_relation(intermediate_relation, target_relation) }} - {{ log(grant_config, "what grants are we passing") }} {% do apply_grants(target_relation, grant_config) %} {% do persist_docs(target_relation, model) %} diff --git a/core/dbt/include/global_project/macros/materializations/seeds/seed.sql b/core/dbt/include/global_project/macros/materializations/seeds/seed.sql index b427d2dca17..42701380d2e 100644 --- a/core/dbt/include/global_project/macros/materializations/seeds/seed.sql +++ b/core/dbt/include/global_project/macros/materializations/seeds/seed.sql @@ -38,7 +38,6 @@ {% endcall %} {% set target_relation = this.incorporate(type='table') %} - {{ log(grant_config, "what grants are we passing") }} {% do apply_grants(target_relation, grant_config) %} {% do persist_docs(target_relation, model) %} diff --git a/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql b/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql index 1637439bb06..9bd0aff79ae 100644 --- a/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql +++ b/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql @@ -74,7 +74,7 @@ {% call statement('main') %} {{ final_sql }} {% endcall %} - {{ log(grant_config, "what grants are we passing") }} + {% do apply_grants(target_relation, grant_config) %} {% do persist_docs(target_relation, model) %} From 597ab0548b29aa8c5afbbf4169a1b54c1868006e Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Wed, 22 Jun 2022 17:01:01 -0500 Subject: [PATCH 14/32] 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 --- .../global_project/macros/adapters/apply_grants.sql | 2 +- .../postgres/dbt/include/postgres/macros/adapters.sql | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 014d91df192..55926c3e4be 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -30,7 +30,7 @@ show grants on {{ relation.type }} {{ relation }} {% endmacro %} {% macro apply_grants(relation, grant_config, should_revoke) %} -{{ return(adapter.dispatch("apply_grants", "dbt")(relation, grant_config, should_revoke)) }} +{{ return(adapter.dispatch("apply_grants", "dbt")(relation, grant_config, should_revoke=True)) }} {% endmacro %} {% macro default__apply_grants(relation, grant_config, should_revoke=True) %} diff --git a/plugins/postgres/dbt/include/postgres/macros/adapters.sql b/plugins/postgres/dbt/include/postgres/macros/adapters.sql index b7348472854..1da3494322e 100644 --- a/plugins/postgres/dbt/include/postgres/macros/adapters.sql +++ b/plugins/postgres/dbt/include/postgres/macros/adapters.sql @@ -202,3 +202,14 @@ 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) %} + {% call statement('get_show_grant_sql', fetch_result=True) -%} + select grantee + from information_schema.role_table_grants + where grantor = current_role + and grantee != current_role + and table_schema = '{{ relation.schema }}' + and table_name = '{{ relation.identifier }}' + {% endcall -%} +{% endmacro %} From 138f4436fcb969f1967ff57a466d0e205a1f2a14 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Mon, 27 Jun 2022 17:05:52 -0500 Subject: [PATCH 15/32] 6/27 eod update looking into diff_grants variable not getting passed into get_revoke_sql --- core/dbt/context/base.py | 18 ++++++++++-------- .../macros/adapters/apply_grants.sql | 14 ++++++-------- .../models/incremental/incremental.sql | 2 +- .../materializations/models/table/table.sql | 2 +- .../materializations/models/view/view.sql | 2 +- .../macros/materializations/seeds/seed.sql | 2 +- .../materializations/snapshots/snapshot.sql | 2 +- .../dbt/include/postgres/macros/adapters.sql | 1 + 8 files changed, 22 insertions(+), 21 deletions(-) diff --git a/core/dbt/context/base.py b/core/dbt/context/base.py index 2e66362a644..e900b66140f 100644 --- a/core/dbt/context/base.py +++ b/core/dbt/context/base.py @@ -657,16 +657,18 @@ def print(msg: str) -> str: print(msg) return "" + @contextmember + @staticmethod + def diff_of_two_dicts(grant_config, current_grants): + """compares list of current grants and any changes made to grants on a model in a + current run and returns the diff as a new object + """ + diff_dict = {k: grant_config[k] for k in set(grant_config) - set(current_grants)} + print("diff_dict ==", diff_dict) + return diff_dict + def generate_base_context(cli_vars: Dict[str, Any]) -> Dict[str, Any]: ctx = BaseContext(cli_vars) # This is not a Mashumaro to_dict call return ctx.to_dict() - - -def diff_of_two_dicts( - grant_config: Dict[str, Any], current_grants: Dict[str, Any] -) -> Dict[str, Any]: - diff_dict = {k: current_grants[k] for k in set(current_grants) - set(grant_config)} - print("************", diff_dict) - return diff_dict diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 55926c3e4be..e50f3aac2ac 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -37,20 +37,18 @@ show grants on {{ relation.type }} {{ relation }} {% if grant_config %} {% call statement('grants') %} {% if should_revoke %} - {% set current_grants = run_query(get_show_grant_sql(relation)) %} - {{ log(current_grants, info=True) }} + {% set diff_grants = {} %} + {% set current_grants = get_show_grant_sql(relation) %} {% for privilege in grant_config.keys() %} - {% if privilege not in current_grants.keys() %} - {% set diff_grants = diff_of_two_dicts(grant_config, current_grants) %} + {% if privilege not in current_grants %} + {% diff_grants = diff_of_two_dicts(grant_config, current_grants) %} + {{ log('diff_grants: ' ~ diff_grants, info=True)}} {% endif %} {% endfor %} - {{ log(diff_grants, info=True) }} + {{ log('diff_grants: ' ~ diff_grants, info=True)}} {% set revoke_grants = get_revoke_sql(relation, diff_grants) %} {% endif %} {{ get_grant_sql(relation, grant_config) }} {% endcall %} {% endif %} {% endmacro %} - --- {% set diff_grants = { k: current_grants[k] for k in set(current_grants) - set(grant_config) } %} --- {%- set _ = diff_grants.update({privilege: grant_config[privilege]}) -%} diff --git a/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql b/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql index fb52b5e4bdb..2450e628244 100644 --- a/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql +++ b/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql @@ -61,7 +61,7 @@ {% do to_drop.append(backup_relation) %} {% endif %} - {% do apply_grants(target_relation, grant_config) %} + {% do apply_grants(target_relation, grant_config, should_revoke=True) %} {% do persist_docs(target_relation, model) %} diff --git a/core/dbt/include/global_project/macros/materializations/models/table/table.sql b/core/dbt/include/global_project/macros/materializations/models/table/table.sql index 6cb0bf82a3f..5f1d3697f50 100644 --- a/core/dbt/include/global_project/macros/materializations/models/table/table.sql +++ b/core/dbt/include/global_project/macros/materializations/models/table/table.sql @@ -42,7 +42,7 @@ {{ run_hooks(post_hooks, inside_transaction=True) }} - {% do apply_grants(target_relation, grant_config) %} + {% do apply_grants(target_relation, grant_config, should_revoke=True) %} {% do persist_docs(target_relation, model) %} -- `COMMIT` happens here diff --git a/core/dbt/include/global_project/macros/materializations/models/view/view.sql b/core/dbt/include/global_project/macros/materializations/models/view/view.sql index c75873d9292..766b811d6d5 100644 --- a/core/dbt/include/global_project/macros/materializations/models/view/view.sql +++ b/core/dbt/include/global_project/macros/materializations/models/view/view.sql @@ -49,7 +49,7 @@ {% endif %} {{ adapter.rename_relation(intermediate_relation, target_relation) }} - {% do apply_grants(target_relation, grant_config) %} + {% do apply_grants(target_relation, grant_config, should_revoke=True) %} {% do persist_docs(target_relation, model) %} diff --git a/core/dbt/include/global_project/macros/materializations/seeds/seed.sql b/core/dbt/include/global_project/macros/materializations/seeds/seed.sql index 42701380d2e..96564827b5c 100644 --- a/core/dbt/include/global_project/macros/materializations/seeds/seed.sql +++ b/core/dbt/include/global_project/macros/materializations/seeds/seed.sql @@ -38,7 +38,7 @@ {% endcall %} {% set target_relation = this.incorporate(type='table') %} - {% do apply_grants(target_relation, grant_config) %} + {% do apply_grants(target_relation, grant_config, should_revoke=True) %} {% do persist_docs(target_relation, model) %} {% if full_refresh_mode or not exists_as_table %} diff --git a/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql b/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql index 9bd0aff79ae..c1a38489572 100644 --- a/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql +++ b/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql @@ -75,7 +75,7 @@ {{ final_sql }} {% endcall %} - {% do apply_grants(target_relation, grant_config) %} + {% do apply_grants(target_relation, grant_config, should_revoke=True) %} {% do persist_docs(target_relation, model) %} {% if not target_relation_exists %} diff --git a/plugins/postgres/dbt/include/postgres/macros/adapters.sql b/plugins/postgres/dbt/include/postgres/macros/adapters.sql index 1da3494322e..b9e0779194a 100644 --- a/plugins/postgres/dbt/include/postgres/macros/adapters.sql +++ b/plugins/postgres/dbt/include/postgres/macros/adapters.sql @@ -207,6 +207,7 @@ {% call statement('get_show_grant_sql', fetch_result=True) -%} select grantee from information_schema.role_table_grants + {{ log(grantee, info=True) }} where grantor = current_role and grantee != current_role and table_schema = '{{ relation.schema }}' From fc7e24b42631bdff0f74ebdca6d8da750acd8714 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Tue, 28 Jun 2022 11:28:38 -0500 Subject: [PATCH 16/32] changes to loop cases --- core/dbt/context/base.py | 1 - .../macros/adapters/apply_grants.sql | 41 +++++++++---------- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/core/dbt/context/base.py b/core/dbt/context/base.py index e900b66140f..884b478ad2a 100644 --- a/core/dbt/context/base.py +++ b/core/dbt/context/base.py @@ -664,7 +664,6 @@ def diff_of_two_dicts(grant_config, current_grants): current run and returns the diff as a new object """ diff_dict = {k: grant_config[k] for k in set(grant_config) - set(current_grants)} - print("diff_dict ==", diff_dict) return diff_dict diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index e50f3aac2ac..7a27f946897 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -12,8 +12,11 @@ show grants on {{ relation.type }} {{ relation }} {% macro default__get_grant_sql(relation, grant_config) %} {% for privilege in grant_config.keys() %} - {% set grantee = grant_config[privilege] %} - grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ grantee | join(', ') }} + {{ log('privilege: ' ~ privilege) }} + {% set grantees = grant_config[privilege] %} + {% for grantee in grantees %} + grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ grantee }}; + {% endfor %} {% endfor %} {% endmacro %} @@ -23,9 +26,10 @@ show grants on {{ relation.type }} {{ relation }} {% macro default__get_revoke_sql(relation, grant_config) %} {% for privilege in grant_config.keys() %} - {% set grantee = grant_config[privilege] %} - revoke {{ privilege }} on {{ relation.type }} {{ relation }} from {{ grantee | join(', ') }} - where {{ grantee }} != {{ target.user }} + {% set grantees = grant_config[privilege] %} + {% for grantee in grantees if grantee != target.user %} + revoke {{ privilege }} on {{ relation.type }} {{ relation }} from {{ grantee }} + {% endfor %} {% endfor %} {% endmacro %} @@ -34,21 +38,14 @@ show grants on {{ relation.type }} {{ relation }} {% endmacro %} {% macro default__apply_grants(relation, grant_config, should_revoke=True) %} -{% if grant_config %} - {% call statement('grants') %} - {% if should_revoke %} - {% set diff_grants = {} %} - {% set current_grants = get_show_grant_sql(relation) %} - {% for privilege in grant_config.keys() %} - {% if privilege not in current_grants %} - {% diff_grants = diff_of_two_dicts(grant_config, current_grants) %} - {{ log('diff_grants: ' ~ diff_grants, info=True)}} - {% endif %} - {% endfor %} - {{ log('diff_grants: ' ~ diff_grants, info=True)}} - {% set revoke_grants = get_revoke_sql(relation, diff_grants) %} - {% endif %} - {{ get_grant_sql(relation, grant_config) }} - {% endcall %} -{% endif %} + {% if grant_config %} + {% call statement('grants') %} + {% if should_revoke %} + {% set current_grants = get_show_grant_sql(relation) %} + {% set diff_grants = diff_of_two_dicts(grant_config, current_grants) %} + {% set revoke_grants = get_revoke_sql(relation, diff_grants) %} + {% endif %} + {{ get_grant_sql(relation, grant_config) }} + {% endcall %} + {% endif %} {% endmacro %} From 6f53b3db76c8b99b325cc72a2f5cb19e2b4b57b6 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Tue, 28 Jun 2022 16:11:40 -0500 Subject: [PATCH 17/32] changes after pairing meeting --- core/dbt/context/base.py | 1 + .../include/global_project/macros/adapters/apply_grants.sql | 3 ++- .../global_project/macros/materializations/seeds/seed.sql | 2 +- .../macros/materializations/snapshots/snapshot.sql | 2 +- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/core/dbt/context/base.py b/core/dbt/context/base.py index 884b478ad2a..6dda37aefe6 100644 --- a/core/dbt/context/base.py +++ b/core/dbt/context/base.py @@ -663,6 +663,7 @@ def diff_of_two_dicts(grant_config, current_grants): """compares list of current grants and any changes made to grants on a model in a current run and returns the diff as a new object """ + diff_dict = {k: grant_config[k] for k in set(grant_config) - set(current_grants)} return diff_dict diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 7a27f946897..57f544678e0 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -41,7 +41,8 @@ show grants on {{ relation.type }} {{ relation }} {% if grant_config %} {% call statement('grants') %} {% if should_revoke %} - {% set current_grants = get_show_grant_sql(relation) %} + {% set current_grants = run_query(get_show_grant_sql(relation)) %} + {{ log(current_grants.print_table()) }} {% set diff_grants = diff_of_two_dicts(grant_config, current_grants) %} {% set revoke_grants = get_revoke_sql(relation, diff_grants) %} {% endif %} diff --git a/core/dbt/include/global_project/macros/materializations/seeds/seed.sql b/core/dbt/include/global_project/macros/materializations/seeds/seed.sql index 96564827b5c..8ebf91cc4e9 100644 --- a/core/dbt/include/global_project/macros/materializations/seeds/seed.sql +++ b/core/dbt/include/global_project/macros/materializations/seeds/seed.sql @@ -8,9 +8,9 @@ {%- 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 - {% set grant_config = config.get('grants') %} {%- do store_result('agate_table', response='OK', agate_table=agate_table) -%} diff --git a/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql b/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql index c1a38489572..6fb2f6ef656 100644 --- a/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql +++ b/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql @@ -6,7 +6,7 @@ {%- 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 grant_config = config.get('grants') -%} {% set target_relation_exists, target_relation = get_or_create_relation( database=model.database, From 99b14455dfebc34f8486b6f683854cab2f1f7162 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Tue, 5 Jul 2022 11:03:58 -0500 Subject: [PATCH 18/32] adding apply_grants to create_or_replace_view.sql --- core/dbt/adapters/base/impl.py | 5 +++ core/dbt/context/base.py | 20 +++++++--- .../macros/adapters/apply_grants.sql | 40 ++++++++++++------- .../models/view/create_or_replace_view.sql | 3 +- .../postgres/dbt/adapters/postgres/impl.py | 13 ++++++ .../dbt/include/postgres/macros/adapters.sql | 5 +-- 6 files changed, 62 insertions(+), 24 deletions(-) diff --git a/core/dbt/adapters/base/impl.py b/core/dbt/adapters/base/impl.py index aef00e80d4b..09fd79c1201 100644 --- a/core/dbt/adapters/base/impl.py +++ b/core/dbt/adapters/base/impl.py @@ -1063,6 +1063,11 @@ def get_compiler(self): return Compiler(self.config) + # used in apply_grants -- True is a safe default + @available + def do_i_carry_over_grants_when_an_object_is_replaced(self) -> bool: + return True + # Methods used in adapter tests def update_column_sql( self, diff --git a/core/dbt/context/base.py b/core/dbt/context/base.py index 6dda37aefe6..bb78f72d5bb 100644 --- a/core/dbt/context/base.py +++ b/core/dbt/context/base.py @@ -659,13 +659,23 @@ def print(msg: str) -> str: @contextmember @staticmethod - def diff_of_two_dicts(grant_config, current_grants): - """compares list of current grants and any changes made to grants on a model in a - current run and returns the diff as a new object + 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 """ - diff_dict = {k: grant_config[k] for k in set(grant_config) - set(current_grants)} - return diff_dict + dict_diff = {} + for k in dict_a: + if k in dict_b: + diff = list(set(dict_a[k]) - set(dict_b[k])) + 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]: diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 57f544678e0..acb83577e06 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -12,11 +12,11 @@ show grants on {{ relation.type }} {{ relation }} {% macro default__get_grant_sql(relation, grant_config) %} {% for privilege in grant_config.keys() %} - {{ log('privilege: ' ~ privilege) }} {% set grantees = grant_config[privilege] %} - {% for grantee in grantees %} - grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ grantee }}; - {% endfor %} + {% if grantees %} + {{ log(privilege, info=True)}} + grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ grantees | join(', ')}}; + {% endif %} {% endfor %} {% endmacro %} @@ -27,26 +27,36 @@ show grants on {{ relation.type }} {{ relation }} {% macro default__get_revoke_sql(relation, grant_config) %} {% for privilege in grant_config.keys() %} {% set grantees = grant_config[privilege] %} - {% for grantee in grantees if grantee != target.user %} - revoke {{ privilege }} on {{ relation.type }} {{ relation }} from {{ grantee }} - {% endfor %} + {% if grantees %} + {{ log(privilege, info=True)}} + revoke {{ privilege }} on {{ relation.type }} {{ relation }} from {{ grantees | join(', ') }}; + {% endif %} {% endfor %} {% endmacro %} {% macro apply_grants(relation, grant_config, should_revoke) %} -{{ return(adapter.dispatch("apply_grants", "dbt")(relation, grant_config, should_revoke=True)) }} +{{ 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 %} - {% call statement('grants') %} {% if should_revoke %} - {% set current_grants = run_query(get_show_grant_sql(relation)) %} - {{ log(current_grants.print_table()) }} - {% set diff_grants = diff_of_two_dicts(grant_config, current_grants) %} - {% set revoke_grants = get_revoke_sql(relation, diff_grants) %} + {% 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 %} - {{ get_grant_sql(relation, grant_config) }} - {% endcall %} {% endif %} {% endmacro %} diff --git a/core/dbt/include/global_project/macros/materializations/models/view/create_or_replace_view.sql b/core/dbt/include/global_project/macros/materializations/models/view/create_or_replace_view.sql index 967086b7424..ebd50edb633 100644 --- a/core/dbt/include/global_project/macros/materializations/models/view/create_or_replace_view.sql +++ b/core/dbt/include/global_project/macros/materializations/models/view/create_or_replace_view.sql @@ -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) }} @@ -34,6 +34,7 @@ {{ get_create_view_as_sql(target_relation, sql) }} {%- endcall %} + {% do apply_grants(target_relation, grant_config, should_revoke=True) %} {{ run_hooks(post_hooks) }} {{ return({'relations': [target_relation]}) }} diff --git a/plugins/postgres/dbt/adapters/postgres/impl.py b/plugins/postgres/dbt/adapters/postgres/impl.py index 116ec5e7727..8ff673adc34 100644 --- a/plugins/postgres/dbt/adapters/postgres/impl.py +++ b/plugins/postgres/dbt/adapters/postgres/impl.py @@ -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 @@ -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"].lower() + privilege = row["privilege"].lower() + 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. diff --git a/plugins/postgres/dbt/include/postgres/macros/adapters.sql b/plugins/postgres/dbt/include/postgres/macros/adapters.sql index b9e0779194a..19de18990f6 100644 --- a/plugins/postgres/dbt/include/postgres/macros/adapters.sql +++ b/plugins/postgres/dbt/include/postgres/macros/adapters.sql @@ -204,13 +204,12 @@ {% endmacro %} {% macro postgres__get_show_grant_sql(relation) %} - {% call statement('get_show_grant_sql', fetch_result=True) -%} - select grantee + {% ('get_show_grant_sql', fetch_result=True) -%} + select grantee, privilege_type from information_schema.role_table_grants {{ log(grantee, info=True) }} where grantor = current_role and grantee != current_role and table_schema = '{{ relation.schema }}' and table_name = '{{ relation.identifier }}' - {% endcall -%} {% endmacro %} From 7946a3b8503ba0444c404e6c8bb24dc2eed2a235 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Tue, 5 Jul 2022 16:20:58 -0500 Subject: [PATCH 19/32] models are building but testing out small issues around revoke statement never building --- .../macros/adapters/apply_grants.sql | 18 +++++++++++++----- .../materializations/models/view/view.sql | 2 +- plugins/postgres/dbt/adapters/postgres/impl.py | 4 ++-- .../dbt/include/postgres/macros/adapters.sql | 1 - 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index acb83577e06..8ccc60f4f50 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -14,8 +14,9 @@ show grants on {{ relation.type }} {{ relation }} {% for privilege in grant_config.keys() %} {% set grantees = grant_config[privilege] %} {% if grantees %} - {{ log(privilege, info=True)}} - grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ grantees | join(', ')}}; + {% for grantee in grantees %} + grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ grantee}}; + {% endfor %} {% endif %} {% endfor %} {% endmacro %} @@ -26,10 +27,17 @@ show grants on {{ relation.type }} {{ relation }} {% macro default__get_revoke_sql(relation, grant_config) %} {% for privilege in grant_config.keys() %} - {% set grantees = grant_config[privilege] %} + {% set grantees = [] %} + {% set all_grantees = grant_config[privilege] %} + {% for grantee in all_grantees %} + {% if grantee != target.user %} + {% do grantees.append(grantee) %} + {% endif %} + {% endfor%} {% if grantees %} - {{ log(privilege, info=True)}} - revoke {{ privilege }} on {{ relation.type }} {{ relation }} from {{ grantees | join(', ') }}; + {% for grantee in grantees %} + revoke {{ privilege }} on {{ relation.type }} {{ relation }} from {{ grantee }}; + {% endfor %} {% endif %} {% endfor %} {% endmacro %} diff --git a/core/dbt/include/global_project/macros/materializations/models/view/view.sql b/core/dbt/include/global_project/macros/materializations/models/view/view.sql index 766b811d6d5..7b2bd5125d7 100644 --- a/core/dbt/include/global_project/macros/materializations/models/view/view.sql +++ b/core/dbt/include/global_project/macros/materializations/models/view/view.sql @@ -49,7 +49,7 @@ {% endif %} {{ adapter.rename_relation(intermediate_relation, target_relation) }} - {% do apply_grants(target_relation, grant_config, should_revoke=True) %} + {% do apply_grants(target_relation, grant_config, should_revoke=False) %} {% do persist_docs(target_relation, model) %} diff --git a/plugins/postgres/dbt/adapters/postgres/impl.py b/plugins/postgres/dbt/adapters/postgres/impl.py index 8ff673adc34..246bfe8df09 100644 --- a/plugins/postgres/dbt/adapters/postgres/impl.py +++ b/plugins/postgres/dbt/adapters/postgres/impl.py @@ -90,8 +90,8 @@ def parse_index(self, raw_index: Any) -> Optional[PostgresIndexConfig]: def standardize_grants_dict(self, grants_table: agate.Table) -> dict: grants_dict = {} for row in grants_table: - grantee = row["grantee"].lower() - privilege = row["privilege"].lower() + grantee = row["grantee"] + privilege = row["privilege"] if privilege in grants_dict.keys(): grants_dict[privilege].append(grantee) else: diff --git a/plugins/postgres/dbt/include/postgres/macros/adapters.sql b/plugins/postgres/dbt/include/postgres/macros/adapters.sql index 19de18990f6..bf3af87074f 100644 --- a/plugins/postgres/dbt/include/postgres/macros/adapters.sql +++ b/plugins/postgres/dbt/include/postgres/macros/adapters.sql @@ -204,7 +204,6 @@ {% endmacro %} {% macro postgres__get_show_grant_sql(relation) %} - {% ('get_show_grant_sql', fetch_result=True) -%} select grantee, privilege_type from information_schema.role_table_grants {{ log(grantee, info=True) }} From 0597398a07c9d86f619506bd4c475da073845364 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Wed, 6 Jul 2022 11:51:13 -0500 Subject: [PATCH 20/32] postgrest must fixes from jeremy's feedback --- core/dbt/adapters/base/impl.py | 7 ++++ .../macros/adapters/apply_grants.sql | 36 +++++++++---------- .../postgres/dbt/adapters/postgres/impl.py | 2 +- .../dbt/include/postgres/macros/adapters.sql | 17 +++++++-- 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/core/dbt/adapters/base/impl.py b/core/dbt/adapters/base/impl.py index 09fd79c1201..628e7154299 100644 --- a/core/dbt/adapters/base/impl.py +++ b/core/dbt/adapters/base/impl.py @@ -536,6 +536,13 @@ def list_relations_without_caching(self, schema_relation: BaseRelation) -> List[ "`list_relations_without_caching` is not implemented for this " "adapter!" ) + @abc.abstractclassmethod + def standardize_grants_dict(cls, grants_table: agate.Table) -> dict: + """Return standardized grants received from database, to match format of grants_config.""" + raise NotImplementedException( + "`standardize_grants_dict` is not implemented for this adapter!" + ) + ### # Provided methods about relations ### diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 8ccc60f4f50..604b71470fd 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -11,36 +11,36 @@ show grants on {{ relation.type }} {{ relation }} {% endmacro %} {% macro default__get_grant_sql(relation, grant_config) %} - {% for privilege in grant_config.keys() %} + {%- for privilege in grant_config.keys() -%} {% set grantees = grant_config[privilege] %} {% if grantees %} - {% for grantee in grantees %} + {%- for grantee in grantees -%} grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ grantee}}; - {% endfor %} - {% endif %} - {% endfor %} -{% endmacro %} + {%- 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() %} + {%- for privilege in grant_config.keys() -%} {% set grantees = [] %} {% set all_grantees = grant_config[privilege] %} - {% for grantee in all_grantees %} - {% if grantee != target.user %} + {%- for grantee in all_grantees -%} + {%- if grantee != target.user -%} {% do grantees.append(grantee) %} - {% endif %} - {% endfor%} - {% if grantees %} - {% for grantee in grantees %} + {%- endif -%} + {%- endfor -%} + {%- if grantees -%} + {%- for grantee in grantees -%} revoke {{ privilege }} on {{ relation.type }} {{ relation }} from {{ grantee }}; - {% endfor %} - {% endif %} - {% endfor %} -{% endmacro %} + {%- endfor -%} + {%- endif -%} + {%- endfor -%} +{%- endmacro %} {% macro apply_grants(relation, grant_config, should_revoke) %} {{ return(adapter.dispatch("apply_grants", "dbt")(relation, grant_config, should_revoke)) }} @@ -49,7 +49,7 @@ show grants on {{ relation.type }} {{ relation }} {% 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_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) %} diff --git a/plugins/postgres/dbt/adapters/postgres/impl.py b/plugins/postgres/dbt/adapters/postgres/impl.py index 246bfe8df09..3fc05c085a8 100644 --- a/plugins/postgres/dbt/adapters/postgres/impl.py +++ b/plugins/postgres/dbt/adapters/postgres/impl.py @@ -91,7 +91,7 @@ def standardize_grants_dict(self, grants_table: agate.Table) -> dict: grants_dict = {} for row in grants_table: grantee = row["grantee"] - privilege = row["privilege"] + privilege = row["privilege_type"] if privilege in grants_dict.keys(): grants_dict[privilege].append(grantee) else: diff --git a/plugins/postgres/dbt/include/postgres/macros/adapters.sql b/plugins/postgres/dbt/include/postgres/macros/adapters.sql index bf3af87074f..72a1919396f 100644 --- a/plugins/postgres/dbt/include/postgres/macros/adapters.sql +++ b/plugins/postgres/dbt/include/postgres/macros/adapters.sql @@ -203,12 +203,23 @@ {% endfor %} {% endmacro %} -{% macro postgres__get_show_grant_sql(relation) %} +{%- macro postgres__get_show_grant_sql(relation) -%} select grantee, privilege_type from information_schema.role_table_grants - {{ log(grantee, info=True) }} where grantor = current_role and grantee != current_role and table_schema = '{{ relation.schema }}' and table_name = '{{ relation.identifier }}' -{% endmacro %} +{%- endmacro -%} + + +{%- macro postgres__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 table {{ relation }} to {{ grantee}}; + {% endfor -%} + {%- endif -%} + {%- endfor -%} +{%- endmacro -%} From 1d263b7c75c0ef05cc27273e38bf3cdcc7e2b886 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Wed, 6 Jul 2022 11:59:29 -0500 Subject: [PATCH 21/32] postgres minor change to standarize_grants_dict --- plugins/postgres/dbt/adapters/postgres/impl.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/postgres/dbt/adapters/postgres/impl.py b/plugins/postgres/dbt/adapters/postgres/impl.py index 3fc05c085a8..e96b6574c9a 100644 --- a/plugins/postgres/dbt/adapters/postgres/impl.py +++ b/plugins/postgres/dbt/adapters/postgres/impl.py @@ -90,8 +90,8 @@ def parse_index(self, raw_index: Any) -> Optional[PostgresIndexConfig]: def standardize_grants_dict(self, grants_table: agate.Table) -> dict: grants_dict = {} for row in grants_table: - grantee = row["grantee"] - privilege = row["privilege_type"] + grantee = row["grantee"].lower() + privilege = row["privilege_type"].lower() if privilege in grants_dict.keys(): grants_dict[privilege].append(grantee) else: From c2e9aebbfe92faf994fb81f823e56595c667ad98 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Wed, 6 Jul 2022 15:26:52 -0500 Subject: [PATCH 22/32] updating after pairing with dough and jeremey incorporating the new version of should revoke logic. --- core/dbt/adapters/base/impl.py | 7 ++- core/dbt/context/base.py | 6 ++- .../macros/adapters/apply_grants.sql | 49 ++++++++++++++----- .../models/incremental/incremental.sql | 3 +- .../materializations/models/table/table.sql | 4 +- .../materializations/models/view/view.sql | 3 +- .../macros/materializations/seeds/seed.sql | 5 +- .../materializations/snapshots/snapshot.sql | 4 +- .../postgres/dbt/adapters/postgres/impl.py | 4 +- .../dbt/include/postgres/macros/adapters.sql | 6 ++- 10 files changed, 68 insertions(+), 23 deletions(-) diff --git a/core/dbt/adapters/base/impl.py b/core/dbt/adapters/base/impl.py index 628e7154299..81a0d82a390 100644 --- a/core/dbt/adapters/base/impl.py +++ b/core/dbt/adapters/base/impl.py @@ -538,7 +538,12 @@ def list_relations_without_caching(self, schema_relation: BaseRelation) -> List[ @abc.abstractclassmethod def standardize_grants_dict(cls, grants_table: agate.Table) -> dict: - """Return standardized grants received from database, to match format of grants_config.""" + """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). + """ raise NotImplementedException( "`standardize_grants_dict` is not implemented for this adapter!" ) diff --git a/core/dbt/context/base.py b/core/dbt/context/base.py index bb78f72d5bb..ed6dcfab0eb 100644 --- a/core/dbt/context/base.py +++ b/core/dbt/context/base.py @@ -668,9 +668,13 @@ def diff_of_two_dicts(dict_a, 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: - diff = list(set(dict_a[k]) - set(dict_b[k])) + 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: diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 604b71470fd..707097f075b 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -1,19 +1,42 @@ +{% macro are_grants_copied_over_when_replaced() %} + {{ return(adapter.dispatch('are_grants_copied_over_when_replaced', 'dbt')()) }} +{% endmacro %} + +{% macro default__are_grants_copied_over_when_replaced() %} + {{ return(True) }} +{% endmacro %} + +{% macro do_we_need_to_show_and_revoke_grants(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(are_grants_copied_over_when_replaced()) }} + {% 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)) }} + {{ return(adapter.dispatch("get_show_grant_sql", "dbt")(relation)) }} {% endmacro %} {% macro default__get_show_grant_sql(relation) %} -show grants on {{ relation.type }} {{ relation }} + show grants on {{ relation.type }} {{ relation }} {% endmacro %} {% macro get_grant_sql(relation, grant_config) %} -{{ return(adapter.dispatch('get_grant_sql', 'dbt')(relation, grant_config)) }} + {{ return(adapter.dispatch('get_grant_sql', 'dbt')(relation, grant_config)) }} {% endmacro %} -{% macro default__get_grant_sql(relation, grant_config) %} +{%- macro default__get_grant_sql(relation, grant_config) -%} {%- for privilege in grant_config.keys() -%} - {% set grantees = grant_config[privilege] %} - {% if grantees %} + {%- set grantees = grant_config[privilege] -%} + {%- if grantees -%} {%- for grantee in grantees -%} grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ grantee}}; {%- endfor -%} @@ -22,28 +45,28 @@ show grants on {{ relation.type }} {{ relation }} {%- endmacro %} {% macro get_revoke_sql(relation, grant_config) %} -{{ return(adapter.dispatch("get_revoke_sql", "dbt")(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 = [] %} - {% set all_grantees = grant_config[privilege] %} + {%- set grantees = [] -%} + {%- set all_grantees = grant_config[privilege] -%} {%- for grantee in all_grantees -%} {%- if grantee != target.user -%} {% do grantees.append(grantee) %} {%- endif -%} - {%- endfor -%} + {% endfor -%} {%- if grantees -%} {%- for grantee in grantees -%} revoke {{ privilege }} on {{ relation.type }} {{ relation }} from {{ grantee }}; - {%- endfor -%} + {% endfor -%} {%- endif -%} {%- endfor -%} -{%- endmacro %} +{%- endmacro -%} {% macro apply_grants(relation, grant_config, should_revoke) %} -{{ return(adapter.dispatch("apply_grants", "dbt")(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) %} diff --git a/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql b/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql index 2450e628244..f9988209a2a 100644 --- a/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql +++ b/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql @@ -61,7 +61,8 @@ {% do to_drop.append(backup_relation) %} {% endif %} - {% do apply_grants(target_relation, grant_config, should_revoke=True) %} + {% set should_revoke = do_we_need_to_show_and_revoke_grants(existing_relation, full_refresh_mode) %} + {% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %} {% do persist_docs(target_relation, model) %} diff --git a/core/dbt/include/global_project/macros/materializations/models/table/table.sql b/core/dbt/include/global_project/macros/materializations/models/table/table.sql index 5f1d3697f50..282fa86a9f3 100644 --- a/core/dbt/include/global_project/macros/materializations/models/table/table.sql +++ b/core/dbt/include/global_project/macros/materializations/models/table/table.sql @@ -42,7 +42,9 @@ {{ run_hooks(post_hooks, inside_transaction=True) }} - {% do apply_grants(target_relation, grant_config, should_revoke=True) %} + {% set should_revoke = do_we_need_to_show_and_revoke_grants(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 diff --git a/core/dbt/include/global_project/macros/materializations/models/view/view.sql b/core/dbt/include/global_project/macros/materializations/models/view/view.sql index 7b2bd5125d7..4d4a61a0e89 100644 --- a/core/dbt/include/global_project/macros/materializations/models/view/view.sql +++ b/core/dbt/include/global_project/macros/materializations/models/view/view.sql @@ -49,7 +49,8 @@ {% endif %} {{ adapter.rename_relation(intermediate_relation, target_relation) }} - {% do apply_grants(target_relation, grant_config, should_revoke=False) %} + {% set should_revoke = do_we_need_to_show_and_revoke_grants(existing_relation, full_refresh_mode=True) %} + {% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %} {% do persist_docs(target_relation, model) %} diff --git a/core/dbt/include/global_project/macros/materializations/seeds/seed.sql b/core/dbt/include/global_project/macros/materializations/seeds/seed.sql index 8ebf91cc4e9..312808e9d1c 100644 --- a/core/dbt/include/global_project/macros/materializations/seeds/seed.sql +++ b/core/dbt/include/global_project/macros/materializations/seeds/seed.sql @@ -38,7 +38,10 @@ {% endcall %} {% set target_relation = this.incorporate(type='table') %} - {% do apply_grants(target_relation, grant_config, should_revoke=True) %} + + {% set should_revoke = do_we_need_to_show_and_revoke_grants(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 %} diff --git a/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql b/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql index 6fb2f6ef656..9a126bcbfd8 100644 --- a/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql +++ b/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql @@ -75,7 +75,9 @@ {{ final_sql }} {% endcall %} - {% do apply_grants(target_relation, grant_config, should_revoke=True) %} + {% set should_revoke = do_we_need_to_show_and_revoke_grants(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 %} diff --git a/plugins/postgres/dbt/adapters/postgres/impl.py b/plugins/postgres/dbt/adapters/postgres/impl.py index e96b6574c9a..3fc05c085a8 100644 --- a/plugins/postgres/dbt/adapters/postgres/impl.py +++ b/plugins/postgres/dbt/adapters/postgres/impl.py @@ -90,8 +90,8 @@ def parse_index(self, raw_index: Any) -> Optional[PostgresIndexConfig]: def standardize_grants_dict(self, grants_table: agate.Table) -> dict: grants_dict = {} for row in grants_table: - grantee = row["grantee"].lower() - privilege = row["privilege_type"].lower() + grantee = row["grantee"] + privilege = row["privilege_type"] if privilege in grants_dict.keys(): grants_dict[privilege].append(grantee) else: diff --git a/plugins/postgres/dbt/include/postgres/macros/adapters.sql b/plugins/postgres/dbt/include/postgres/macros/adapters.sql index 72a1919396f..153a56f83eb 100644 --- a/plugins/postgres/dbt/include/postgres/macros/adapters.sql +++ b/plugins/postgres/dbt/include/postgres/macros/adapters.sql @@ -218,8 +218,12 @@ {%- set grantees = grant_config[privilege] -%} {%- if grantees -%} {%- for grantee in grantees %} - grant {{ privilege }} on table {{ relation }} to {{ grantee}}; + grant {{ privilege }} on table {{ relation }} to {{ grantee }}; {% endfor -%} {%- endif -%} {%- endfor -%} {%- endmacro -%} + +{% macro postgres__are_grants_copied_over_when_replaced() %} + {{ return(False) }} +{% endmacro %} From 077e4ffe50c6dfa37ddb836c131bf537694c566a Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Wed, 6 Jul 2022 15:49:50 -0500 Subject: [PATCH 23/32] adding ref of diff_of_two_dicts to base keys ref --- test/unit/test_context.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/unit/test_context.py b/test/unit/test_context.py index 8b8c62e1c80..296eac527a8 100644 --- a/test/unit/test_context.py +++ b/test/unit/test_context.py @@ -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", } ) From ab6be852a1eb0b3091cd3666249de7b38feb6dba Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Wed, 6 Jul 2022 15:59:35 -0500 Subject: [PATCH 24/32] change of method type for standardize_grants_dict --- core/dbt/adapters/base/impl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/dbt/adapters/base/impl.py b/core/dbt/adapters/base/impl.py index dc2ee9f4e67..2cc2fda8f06 100644 --- a/core/dbt/adapters/base/impl.py +++ b/core/dbt/adapters/base/impl.py @@ -538,7 +538,7 @@ def list_relations_without_caching(self, schema_relation: BaseRelation) -> List[ "`list_relations_without_caching` is not implemented for this " "adapter!" ) - @abc.abstractclassmethod + @abc.abstractmethod def standardize_grants_dict(cls, 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. From da557d96b71c9f506460db8d084097c1c93a8bbe Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Wed, 6 Jul 2022 16:34:07 -0500 Subject: [PATCH 25/32] minor update trying to fix unit test --- core/dbt/adapters/base/impl.py | 11 ++++++++++- test/unit/test_context.py | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/core/dbt/adapters/base/impl.py b/core/dbt/adapters/base/impl.py index 2cc2fda8f06..bda7110177a 100644 --- a/core/dbt/adapters/base/impl.py +++ b/core/dbt/adapters/base/impl.py @@ -159,6 +159,7 @@ class BaseAdapter(metaclass=AdapterMeta): - convert_datetime_type - convert_date_type - convert_time_type + - standardize_grants_dict Macros: - get_catalog @@ -538,13 +539,21 @@ 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(cls, grants_table: agate.Table) -> dict: + 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!" diff --git a/test/unit/test_context.py b/test/unit/test_context.py index 296eac527a8..6ed16bd0e39 100644 --- a/test/unit/test_context.py +++ b/test/unit/test_context.py @@ -199,7 +199,7 @@ def assert_has_keys(required_keys: Set[str], maybe_keys: Set[str], ctx: Dict[str "modules", "flags", "print", - "diff_of_two_dicts", + "diff_of_two_dicts" } ) From f2c957f9cb572281b69d6097ae1e76e6c8a8c6ce Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Thu, 7 Jul 2022 11:12:10 -0500 Subject: [PATCH 26/32] changes based on morning feedback --- core/dbt/adapters/base/impl.py | 5 ---- .../macros/adapters/apply_grants.sql | 24 +++++++------------ .../models/incremental/incremental.sql | 2 +- .../materializations/models/table/table.sql | 2 +- .../models/view/create_or_replace_view.sql | 2 ++ .../materializations/models/view/view.sql | 2 +- .../macros/materializations/seeds/seed.sql | 2 +- .../materializations/snapshots/snapshot.sql | 2 +- .../dbt/include/postgres/macros/adapters.sql | 14 +---------- 9 files changed, 17 insertions(+), 38 deletions(-) diff --git a/core/dbt/adapters/base/impl.py b/core/dbt/adapters/base/impl.py index bda7110177a..29288ea73db 100644 --- a/core/dbt/adapters/base/impl.py +++ b/core/dbt/adapters/base/impl.py @@ -1093,11 +1093,6 @@ def get_compiler(self): return Compiler(self.config) - # used in apply_grants -- True is a safe default - @available - def do_i_carry_over_grants_when_an_object_is_replaced(self) -> bool: - return True - # Methods used in adapter tests def update_column_sql( self, diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 707097f075b..de8d8370558 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -1,19 +1,19 @@ -{% macro are_grants_copied_over_when_replaced() %} - {{ return(adapter.dispatch('are_grants_copied_over_when_replaced', 'dbt')()) }} +{% macro copy_grants() %} + {{ return(adapter.dispatch('copy_grants', 'dbt')()) }} {% endmacro %} -{% macro default__are_grants_copied_over_when_replaced() %} +{% macro default__copy_grants() %} {{ return(True) }} {% endmacro %} -{% macro do_we_need_to_show_and_revoke_grants(existing_relation, full_refresh_mode=True) %} +{% 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(are_grants_copied_over_when_replaced()) }} + {{ return(copy_grants()) }} {% else %} {#-- The table is being merged/upserted/inserted -- grants will be carried over --#} {{ return(True) }} @@ -26,7 +26,7 @@ {% endmacro %} {% macro default__get_show_grant_sql(relation) %} - show grants on {{ relation.type }} {{ relation }} + show grants on {{ relation }} {{ relation }} {% endmacro %} {% macro get_grant_sql(relation, grant_config) %} @@ -38,7 +38,7 @@ {%- set grantees = grant_config[privilege] -%} {%- if grantees -%} {%- for grantee in grantees -%} - grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ grantee}}; + grant {{ privilege }} on {{ relation }} {{ relation }} to {{ grantee}}; {%- endfor -%} {%- endif -%} {%- endfor -%} @@ -50,16 +50,10 @@ {% macro default__get_revoke_sql(relation, grant_config) %} {%- for privilege in grant_config.keys() -%} - {%- set grantees = [] -%} - {%- set all_grantees = grant_config[privilege] -%} - {%- for grantee in all_grantees -%} - {%- if grantee != target.user -%} - {% do grantees.append(grantee) %} - {%- endif -%} - {% endfor -%} + {%- set grantees = grant[privilege] -%} {%- if grantees -%} {%- for grantee in grantees -%} - revoke {{ privilege }} on {{ relation.type }} {{ relation }} from {{ grantee }}; + revoke {{ privilege }} on {{ relation }} {{ relation }} from {{ grantee }}; {% endfor -%} {%- endif -%} {%- endfor -%} diff --git a/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql b/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql index f9988209a2a..b4649429fcd 100644 --- a/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql +++ b/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql @@ -61,7 +61,7 @@ {% do to_drop.append(backup_relation) %} {% endif %} - {% set should_revoke = do_we_need_to_show_and_revoke_grants(existing_relation, full_refresh_mode) %} + {% 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) %} diff --git a/core/dbt/include/global_project/macros/materializations/models/table/table.sql b/core/dbt/include/global_project/macros/materializations/models/table/table.sql index 282fa86a9f3..abf7a7dc0c5 100644 --- a/core/dbt/include/global_project/macros/materializations/models/table/table.sql +++ b/core/dbt/include/global_project/macros/materializations/models/table/table.sql @@ -42,7 +42,7 @@ {{ run_hooks(post_hooks, inside_transaction=True) }} - {% set should_revoke = do_we_need_to_show_and_revoke_grants(existing_relation, full_refresh_mode=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) %} diff --git a/core/dbt/include/global_project/macros/materializations/models/view/create_or_replace_view.sql b/core/dbt/include/global_project/macros/materializations/models/view/create_or_replace_view.sql index ebd50edb633..5d763ff138d 100644 --- a/core/dbt/include/global_project/macros/materializations/models/view/create_or_replace_view.sql +++ b/core/dbt/include/global_project/macros/materializations/models/view/create_or_replace_view.sql @@ -34,7 +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]}) }} diff --git a/core/dbt/include/global_project/macros/materializations/models/view/view.sql b/core/dbt/include/global_project/macros/materializations/models/view/view.sql index 4d4a61a0e89..106e271b057 100644 --- a/core/dbt/include/global_project/macros/materializations/models/view/view.sql +++ b/core/dbt/include/global_project/macros/materializations/models/view/view.sql @@ -49,7 +49,7 @@ {% endif %} {{ adapter.rename_relation(intermediate_relation, target_relation) }} - {% set should_revoke = do_we_need_to_show_and_revoke_grants(existing_relation, full_refresh_mode=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) %} diff --git a/core/dbt/include/global_project/macros/materializations/seeds/seed.sql b/core/dbt/include/global_project/macros/materializations/seeds/seed.sql index 312808e9d1c..5a46a029c61 100644 --- a/core/dbt/include/global_project/macros/materializations/seeds/seed.sql +++ b/core/dbt/include/global_project/macros/materializations/seeds/seed.sql @@ -39,7 +39,7 @@ {% set target_relation = this.incorporate(type='table') %} - {% set should_revoke = do_we_need_to_show_and_revoke_grants(old_relation, full_refresh_mode) %} + {% 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) %} diff --git a/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql b/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql index 9a126bcbfd8..4cee08ddb1b 100644 --- a/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql +++ b/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql @@ -75,7 +75,7 @@ {{ final_sql }} {% endcall %} - {% set should_revoke = do_we_need_to_show_and_revoke_grants(target_relation_exists, full_refresh_mode=False) %} + {% 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) %} diff --git a/plugins/postgres/dbt/include/postgres/macros/adapters.sql b/plugins/postgres/dbt/include/postgres/macros/adapters.sql index 153a56f83eb..6af74728ba5 100644 --- a/plugins/postgres/dbt/include/postgres/macros/adapters.sql +++ b/plugins/postgres/dbt/include/postgres/macros/adapters.sql @@ -212,18 +212,6 @@ and table_name = '{{ relation.identifier }}' {%- endmacro -%} - -{%- macro postgres__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 table {{ relation }} to {{ grantee }}; - {% endfor -%} - {%- endif -%} - {%- endfor -%} -{%- endmacro -%} - -{% macro postgres__are_grants_copied_over_when_replaced() %} +{% macro postgres__coppy_grants() %} {{ return(False) }} {% endmacro %} From eb935d50206362f9eda10176aa11cdbb1741d2f9 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Thu, 7 Jul 2022 14:57:34 -0500 Subject: [PATCH 27/32] change log message in default_apply_grants macro --- .../dbt/include/global_project/macros/adapters/apply_grants.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index de8d8370558..757b635d938 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -71,7 +71,7 @@ {% 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.')}} + {{ log('On ' ~ relation ~': All grants are in place, no revocation or granting needed.')}} {% endif %} {% else %} {% set needs_revoking = {} %} From cefcd4fb9c5079d41ad39f73b26171652b1ded42 Mon Sep 17 00:00:00 2001 From: Gerda Shank Date: Thu, 7 Jul 2022 16:02:42 -0400 Subject: [PATCH 28/32] CT-808 grant adapter tests (#5447) * Create initial test for grants Co-authored-by: Jeremy Cohen --- .../Under the Hood-20220706-215001.yaml | 7 + .github/workflows/main.yml | 3 + .../structured-logging-schema-check.yml | 5 + core/dbt/adapters/base/impl.py | 4 +- .../macros/adapters/apply_grants.sql | 6 +- .../models/incremental/incremental.sql | 2 +- core/dbt/parser/manifest.py | 4 +- core/dbt/tests/util.py | 10 +- .../dbt/include/postgres/macros/adapters.sql | 2 +- test/setup_db.sh | 3 + .../dbt/tests/adapter/grants/test_models.py | 149 ++++++++++++++++++ .../functional/configs/test_grant_configs.py | 28 ++-- 12 files changed, 197 insertions(+), 26 deletions(-) create mode 100644 .changes/unreleased/Under the Hood-20220706-215001.yaml create mode 100644 tests/adapter/dbt/tests/adapter/grants/test_models.py diff --git a/.changes/unreleased/Under the Hood-20220706-215001.yaml b/.changes/unreleased/Under the Hood-20220706-215001.yaml new file mode 100644 index 00000000000..a18d87f19a8 --- /dev/null +++ b/.changes/unreleased/Under the Hood-20220706-215001.yaml @@ -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" diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index d2ede8e3d48..216055031bd 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -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 diff --git a/.github/workflows/structured-logging-schema-check.yml b/.github/workflows/structured-logging-schema-check.yml index 0eb359fbebe..67392b973c7 100644 --- a/.github/workflows/structured-logging-schema-check.yml +++ b/.github/workflows/structured-logging-schema-check.yml @@ -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 diff --git a/core/dbt/adapters/base/impl.py b/core/dbt/adapters/base/impl.py index 29288ea73db..e09447c5840 100644 --- a/core/dbt/adapters/base/impl.py +++ b/core/dbt/adapters/base/impl.py @@ -540,9 +540,9 @@ def list_relations_without_caching(self, schema_relation: BaseRelation) -> List[ ) ### - # Abstract methods about grants + # Methods about grants ### - @abc.abstractmethod + @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. diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 757b635d938..9e6593983d6 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -26,7 +26,7 @@ {% endmacro %} {% macro default__get_show_grant_sql(relation) %} - show grants on {{ relation }} {{ relation }} + show grants on {{ relation }} {% endmacro %} {% macro get_grant_sql(relation, grant_config) %} @@ -38,7 +38,7 @@ {%- set grantees = grant_config[privilege] -%} {%- if grantees -%} {%- for grantee in grantees -%} - grant {{ privilege }} on {{ relation }} {{ relation }} to {{ grantee}}; + grant {{ privilege }} on {{ relation }} to {{ grantee }}; {%- endfor -%} {%- endif -%} {%- endfor -%} @@ -53,7 +53,7 @@ {%- set grantees = grant[privilege] -%} {%- if grantees -%} {%- for grantee in grantees -%} - revoke {{ privilege }} on {{ relation }} {{ relation }} from {{ grantee }}; + revoke {{ privilege }} on {{ relation }} from {{ grantee }}; {% endfor -%} {%- endif -%} {%- endfor -%} diff --git a/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql b/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql index b4649429fcd..54956142f19 100644 --- a/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql +++ b/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql @@ -61,7 +61,7 @@ {% do to_drop.append(backup_relation) %} {% endif %} - {% set should_revoke = should_reovke(existing_relation, full_refresh_mode) %} + {% 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) %} diff --git a/core/dbt/parser/manifest.py b/core/dbt/parser/manifest.py index e6aa424d4b2..35145aa0fb2 100644 --- a/core/dbt/parser/manifest.py +++ b/core/dbt/parser/manifest.py @@ -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 diff --git a/core/dbt/tests/util.py b/core/dbt/tests/util.py index 7ecf36fca7d..311a2249f7c 100644 --- a/core/dbt/tests/util.py +++ b/core/dbt/tests/util.py @@ -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() @@ -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 diff --git a/plugins/postgres/dbt/include/postgres/macros/adapters.sql b/plugins/postgres/dbt/include/postgres/macros/adapters.sql index 6af74728ba5..e864df1df20 100644 --- a/plugins/postgres/dbt/include/postgres/macros/adapters.sql +++ b/plugins/postgres/dbt/include/postgres/macros/adapters.sql @@ -212,6 +212,6 @@ and table_name = '{{ relation.identifier }}' {%- endmacro -%} -{% macro postgres__coppy_grants() %} +{% macro postgres__are_grants_copied_over_when_replaced() %} {{ return(False) }} {% endmacro %} diff --git a/test/setup_db.sh b/test/setup_db.sh index 1b22a004eb6..de59bf0fac6 100755 --- a/test/setup_db.sh +++ b/test/setup_db.sh @@ -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;' diff --git a/tests/adapter/dbt/tests/adapter/grants/test_models.py b/tests/adapter/dbt/tests/adapter/grants/test_models.py new file mode 100644 index 00000000000..b0765bd2ab2 --- /dev/null +++ b/tests/adapter/dbt/tests/adapter/grants/test_models.py @@ -0,0 +1,149 @@ +import pytest +import os +from dbt.tests.util import ( + run_dbt_and_capture, + get_manifest, + relation_from_name, + write_file, + get_connection, +) + +from dbt.context.base import BaseContext # diff_of_two_dicts only + +TEST_USER_ENV_VARS = ["DBT_TEST_USER_1", "DBT_TEST_USER_2", "DBT_TEST_USER_3"] + +my_model_sql = """ + select 1 as fun +""" + +model_schema_yml = """ +version: 2 +models: + - name: my_model + config: + grants: + select: ["{{ env_var('DBT_TEST_USER_1') }}"] +""" + +user2_model_schema_yml = """ +version: 2 +models: + - name: my_model + config: + grants: + select: ["{{ env_var('DBT_TEST_USER_2') }}"] +""" + +table_model_schema_yml = """ +version: 2 +models: + - name: my_model + config: + materialized: table + grants: + select: ["{{ env_var('DBT_TEST_USER_1') }}"] +""" + +user2_table_model_schema_yml = """ +version: 2 +models: + - name: my_model + config: + materialized: table + grants: + select: ["{{ env_var('DBT_TEST_USER_2') }}"] +""" + + +def format_grant_log_line(relation, user_name): + return f"grant select on {relation} to {user_name};" + + +class TestModelGrants: + @pytest.fixture(scope="class") + def models(self): + return {"my_model.sql": my_model_sql, "schema.yml": model_schema_yml} + + @pytest.fixture(scope="class", autouse=True) + def get_test_users(self, project): + test_users = [] + for env_var in TEST_USER_ENV_VARS: + user_name = os.getenv(env_var) + if user_name: + test_users.append(user_name) + return test_users + + def get_grants_on_relation(self, project, relation_name): + relation = relation_from_name(project.adapter, relation_name) + adapter = project.adapter + with get_connection(adapter): + kwargs = {"relation": relation} + show_grant_sql = adapter.execute_macro("get_show_grant_sql", kwargs=kwargs) + _, grant_table = adapter.execute(show_grant_sql, fetch=True) + actual_grants = adapter.standardize_grants_dict(grant_table) + return actual_grants + + def test_model_grants(self, project, get_test_users, logs_dir): + # we want the test to fail, not silently skip + test_users = get_test_users + assert len(test_users) == 3 + + # View materialization, single select grant + (results, log_output) = run_dbt_and_capture(["--debug", "run"]) + assert len(results) == 1 + manifest = get_manifest(project.project_root) + model_id = "model.test.my_model" + model = manifest.nodes[model_id] + expected = {"select": [test_users[0]]} + assert model.config.grants == expected + assert model.config.materialized == "view" + + my_model_relation = relation_from_name(project.adapter, "my_model") + grant_log_line = format_grant_log_line(my_model_relation, test_users[0]) + assert grant_log_line in log_output + + # validate actual grants in database + actual_grants = self.get_grants_on_relation(project, "my_model") + # actual_grants: {'SELECT': ['dbt_test_user_1']} + # need a case-insensitive comparison + # so just a simple "assert expected == actual_grants" won't work + diff = BaseContext.diff_of_two_dicts(expected, actual_grants) + assert diff == {} + + # View materialization, change select grant user + write_file(user2_model_schema_yml, project.project_root, "models", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "run"]) + assert len(results) == 1 + grant_log_line = format_grant_log_line(my_model_relation, test_users[1]) + assert grant_log_line in log_output + + expected = {"select": [get_test_users[1]]} + actual_grants = self.get_grants_on_relation(project, "my_model") + diff = BaseContext.diff_of_two_dicts(expected, actual_grants) + assert diff == {} + + # Table materialization, single select grant + write_file(table_model_schema_yml, project.project_root, "models", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "run"]) + assert len(results) == 1 + grant_log_line = format_grant_log_line(my_model_relation, test_users[0]) + assert grant_log_line in log_output + manifest = get_manifest(project.project_root) + model = manifest.nodes[model_id] + model.config.materialized == "table" + actual_grants = self.get_grants_on_relation(project, "my_model") + diff = BaseContext.diff_of_two_dicts({"select": [test_users[0]]}, actual_grants) + assert diff == {} + + # Table materialization, change select grant user + write_file(user2_table_model_schema_yml, project.project_root, "models", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "run"]) + assert len(results) == 1 + grant_log_line = format_grant_log_line(my_model_relation, test_users[1]) + assert grant_log_line in log_output + manifest = get_manifest(project.project_root) + model = manifest.nodes[model_id] + model.config.materialized == "table" + actual_grants = self.get_grants_on_relation(project, "my_model") + diff = BaseContext.diff_of_two_dicts({"select": [test_users[1]]}, actual_grants) + assert diff == {} diff --git a/tests/functional/configs/test_grant_configs.py b/tests/functional/configs/test_grant_configs.py index 6d4df29ed78..64d3f48d4ca 100644 --- a/tests/functional/configs/test_grant_configs.py +++ b/tests/functional/configs/test_grant_configs.py @@ -57,11 +57,10 @@ def project_config_update(self): return dbt_project_yml def test_model_grant_config(self, project, logs_dir): - # This test uses "my_select" instead of "select", so that when - # actual granting of permissions happens, it won't break this - # test. - results = run_dbt(["run"]) - assert len(results) == 1 + # This test uses "my_select" instead of "select", so we need + # use "parse" instead of "run" because we will get compilation + # errors for the grants. + run_dbt(["parse"]) manifest = get_manifest(project.project_root) model_id = "model.test.my_model" @@ -77,7 +76,7 @@ def test_model_grant_config(self, project, logs_dir): # add model grant with clobber write_file(my_model_clobber_sql, project.project_root, "models", "my_model.sql") - results = run_dbt(["run"]) + run_dbt(["parse"]) manifest = get_manifest(project.project_root) model_config = manifest.nodes[model_id].config @@ -86,7 +85,7 @@ def test_model_grant_config(self, project, logs_dir): # change model to extend grants write_file(my_model_extend_sql, project.project_root, "models", "my_model.sql") - results = run_dbt(["run"]) + run_dbt(["parse"]) manifest = get_manifest(project.project_root) model_config = manifest.nodes[model_id].config @@ -95,8 +94,7 @@ def test_model_grant_config(self, project, logs_dir): # add schema file with extend write_file(append_schema_yml, project.project_root, "models", "schema.yml") - results = run_dbt(["run"]) - assert len(results) == 1 + run_dbt(["parse"]) manifest = get_manifest(project.project_root) model_config = manifest.nodes[model_id].config @@ -106,8 +104,7 @@ def test_model_grant_config(self, project, logs_dir): # change model file to have string instead of list write_file(my_model_extend_string_sql, project.project_root, "models", "my_model.sql") - results = run_dbt(["run"]) - assert len(results) == 1 + run_dbt(["parse"]) manifest = get_manifest(project.project_root) model_config = manifest.nodes[model_id].config @@ -117,8 +114,7 @@ def test_model_grant_config(self, project, logs_dir): # change model file to have string instead of list write_file(my_model_extend_twice_sql, project.project_root, "models", "my_model.sql") - results = run_dbt(["run"]) - assert len(results) == 1 + run_dbt(["parse"]) manifest = get_manifest(project.project_root) model_config = manifest.nodes[model_id].config @@ -135,8 +131,7 @@ def test_model_grant_config(self, project, logs_dir): "log-path": logs_dir, } write_config_file(config, project.project_root, "dbt_project.yml") - results = run_dbt(["run"]) - assert len(results) == 1 + run_dbt(["parse"]) manifest = get_manifest(project.project_root) model_config = manifest.nodes[model_id].config @@ -146,8 +141,7 @@ def test_model_grant_config(self, project, logs_dir): # Remove my_model config, leaving only schema file write_file(my_model_base_sql, project.project_root, "models", "my_model.sql") - results = run_dbt(["run"]) - assert len(results) == 1 + run_dbt(["parse"]) manifest = get_manifest(project.project_root) model_config = manifest.nodes[model_id].config From bdc0c71d4056c3c3651bb7d3ca8216a949a885e5 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Thu, 7 Jul 2022 17:08:20 -0500 Subject: [PATCH 29/32] rename grant[privilege] -> grant_config[privilege] --- .../dbt/include/global_project/macros/adapters/apply_grants.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 9e6593983d6..20571d9241f 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -50,7 +50,7 @@ {% macro default__get_revoke_sql(relation, grant_config) %} {%- for privilege in grant_config.keys() -%} - {%- set grantees = grant[privilege] -%} + {%- set grantees = grant_config[privilege] -%} {%- if grantees -%} {%- for grantee in grantees -%} revoke {{ privilege }} on {{ relation }} from {{ grantee }}; From 72bdae970093bc056cdde902db2bf89a62b8704e Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Mon, 11 Jul 2022 09:06:56 -0500 Subject: [PATCH 30/32] postgres macro rename to copy_grants --- plugins/postgres/dbt/include/postgres/macros/adapters.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/postgres/dbt/include/postgres/macros/adapters.sql b/plugins/postgres/dbt/include/postgres/macros/adapters.sql index e864df1df20..55c2d605f34 100644 --- a/plugins/postgres/dbt/include/postgres/macros/adapters.sql +++ b/plugins/postgres/dbt/include/postgres/macros/adapters.sql @@ -212,6 +212,6 @@ and table_name = '{{ relation.identifier }}' {%- endmacro -%} -{% macro postgres__are_grants_copied_over_when_replaced() %} +{% macro postgres__copy_grants() %} {{ return(False) }} {% endmacro %} From 794f4d1dcb993b25b7d8d6794ca8376e31a7decd Mon Sep 17 00:00:00 2001 From: Gerda Shank Date: Mon, 11 Jul 2022 11:06:55 -0400 Subject: [PATCH 31/32] CT-808 more grant adapter tests (#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 Co-authored-by: Emily Rockman --- core/dbt/adapters/base/impl.py | 17 +- core/dbt/context/base.py | 29 ++-- .../macros/adapters/apply_grants.sql | 154 ++++++++++++----- .../postgres/dbt/adapters/postgres/impl.py | 13 -- .../dbt/include/postgres/macros/adapters.sql | 2 +- .../dbt/tests/adapter/grants/base_grants.py | 58 +++++++ .../adapter/grants/test_incremental_grants.py | 102 ++++++++++++ .../adapter/grants/test_invalid_grants.py | 68 ++++++++ .../tests/adapter/grants/test_model_grants.py | 156 ++++++++++++++++++ .../dbt/tests/adapter/grants/test_models.py | 149 ----------------- .../tests/adapter/grants/test_seed_grants.py | 143 ++++++++++++++++ .../adapter/grants/test_snapshot_grants.py | 78 +++++++++ 12 files changed, 752 insertions(+), 217 deletions(-) create mode 100644 tests/adapter/dbt/tests/adapter/grants/base_grants.py create mode 100644 tests/adapter/dbt/tests/adapter/grants/test_incremental_grants.py create mode 100644 tests/adapter/dbt/tests/adapter/grants/test_invalid_grants.py create mode 100644 tests/adapter/dbt/tests/adapter/grants/test_model_grants.py delete mode 100644 tests/adapter/dbt/tests/adapter/grants/test_models.py create mode 100644 tests/adapter/dbt/tests/adapter/grants/test_seed_grants.py create mode 100644 tests/adapter/dbt/tests/adapter/grants/test_snapshot_grants.py diff --git a/core/dbt/adapters/base/impl.py b/core/dbt/adapters/base/impl.py index e09447c5840..a77a14717a6 100644 --- a/core/dbt/adapters/base/impl.py +++ b/core/dbt/adapters/base/impl.py @@ -547,17 +547,24 @@ 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). + 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 """ - raise NotImplementedException( - "`standardize_grants_dict` is not implemented for this adapter!" - ) + 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 diff --git a/core/dbt/context/base.py b/core/dbt/context/base.py index ed6dcfab0eb..4a71388f81d 100644 --- a/core/dbt/context/base.py +++ b/core/dbt/context/base.py @@ -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 @@ -659,22 +659,27 @@ def print(msg: str) -> str: @contextmember @staticmethod - def diff_of_two_dicts(dict_a, dict_b): + def diff_of_two_dicts( + dict_a: Dict[str, List[str]], dict_b: Dict[str, List[str]] + ) -> Dict[str, List[str]]: """ - 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 + 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_a = {k.lower(): v for k, v in dict_a.items()} - dict_b = {k.lower(): v for k, v in dict_b.items()} + 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 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 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: diff --git a/core/dbt/include/global_project/macros/adapters/apply_grants.sql b/core/dbt/include/global_project/macros/adapters/apply_grants.sql index 20571d9241f..10906e7ffa7 100644 --- a/core/dbt/include/global_project/macros/adapters/apply_grants.sql +++ b/core/dbt/include/global_project/macros/adapters/apply_grants.sql @@ -1,3 +1,17 @@ +{# ------- 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 %} @@ -6,6 +20,25 @@ {{ 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 %} @@ -21,6 +54,8 @@ {% endmacro %} +{# ------- DCL STATEMENT TEMPLATES --------- #} + {% macro get_show_grant_sql(relation) %} {{ return(adapter.dispatch("get_show_grant_sql", "dbt")(relation)) }} {% endmacro %} @@ -29,59 +64,104 @@ show grants on {{ relation }} {% endmacro %} -{% macro get_grant_sql(relation, grant_config) %} - {{ return(adapter.dispatch('get_grant_sql', 'dbt')(relation, grant_config)) }} + +{% 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_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 -%} +{%- 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 get_revoke_sql(relation, grant_config) %} - {{ return(adapter.dispatch("get_revoke_sql", "dbt")(relation, grant_config)) }} + +{% 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 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 is {} or None, this is a no-op --#} {% 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 %} + {% 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 %} - {% if needs_granting or needs_revoking %} - {% call statement('grants') %} - {{ get_revoke_sql(relation, needs_revoking) }} - {{ get_grant_sql(relation, needs_granting) }} - {% endcall %} + {% 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 %} diff --git a/plugins/postgres/dbt/adapters/postgres/impl.py b/plugins/postgres/dbt/adapters/postgres/impl.py index 3fc05c085a8..116ec5e7727 100644 --- a/plugins/postgres/dbt/adapters/postgres/impl.py +++ b/plugins/postgres/dbt/adapters/postgres/impl.py @@ -10,7 +10,6 @@ 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 @@ -86,18 +85,6 @@ 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. diff --git a/plugins/postgres/dbt/include/postgres/macros/adapters.sql b/plugins/postgres/dbt/include/postgres/macros/adapters.sql index 55c2d605f34..11e79078e1d 100644 --- a/plugins/postgres/dbt/include/postgres/macros/adapters.sql +++ b/plugins/postgres/dbt/include/postgres/macros/adapters.sql @@ -205,7 +205,7 @@ {%- macro postgres__get_show_grant_sql(relation) -%} select grantee, privilege_type - from information_schema.role_table_grants + from {{ relation.information_schema('role_table_grants') }} where grantor = current_role and grantee != current_role and table_schema = '{{ relation.schema }}' diff --git a/tests/adapter/dbt/tests/adapter/grants/base_grants.py b/tests/adapter/dbt/tests/adapter/grants/base_grants.py new file mode 100644 index 00000000000..82f5b9fe664 --- /dev/null +++ b/tests/adapter/dbt/tests/adapter/grants/base_grants.py @@ -0,0 +1,58 @@ +import pytest +import os +from dbt.tests.util import ( + relation_from_name, + get_connection, +) +from dbt.context.base import BaseContext # diff_of_two_dicts only + +TEST_USER_ENV_VARS = ["DBT_TEST_USER_1", "DBT_TEST_USER_2", "DBT_TEST_USER_3"] + + +def replace_all(text, dic): + for i, j in dic.items(): + text = text.replace(i, j) + return text + + +class BaseGrants: + def privilege_grantee_name_overrides(self): + # these privilege and grantee names are valid on most databases, but not all! + # looking at you, BigQuery + # optionally use this to map from "select" --> "other_select_name", "insert" --> ... + return { + "select": "select", + "insert": "insert", + "fake_privilege": "fake_privilege", + "invalid_user": "invalid_user", + } + + def interpolate_name_overrides(self, yaml_text): + return replace_all(yaml_text, self.privilege_grantee_name_overrides()) + + @pytest.fixture(scope="class", autouse=True) + def get_test_users(self, project): + test_users = [] + for env_var in TEST_USER_ENV_VARS: + user_name = os.getenv(env_var) + if user_name: + test_users.append(user_name) + return test_users + + def get_grants_on_relation(self, project, relation_name): + relation = relation_from_name(project.adapter, relation_name) + adapter = project.adapter + with get_connection(adapter): + kwargs = {"relation": relation} + show_grant_sql = adapter.execute_macro("get_show_grant_sql", kwargs=kwargs) + _, grant_table = adapter.execute(show_grant_sql, fetch=True) + actual_grants = adapter.standardize_grants_dict(grant_table) + return actual_grants + + def assert_expected_grants_match_actual(self, project, relation_name, expected_grants): + actual_grants = self.get_grants_on_relation(project, relation_name) + # need a case-insensitive comparison + # so just a simple "assert expected == actual_grants" won't work + diff_a = BaseContext.diff_of_two_dicts(actual_grants, expected_grants) + diff_b = BaseContext.diff_of_two_dicts(expected_grants, actual_grants) + assert diff_a == diff_b == {} diff --git a/tests/adapter/dbt/tests/adapter/grants/test_incremental_grants.py b/tests/adapter/dbt/tests/adapter/grants/test_incremental_grants.py new file mode 100644 index 00000000000..2f28eac02ab --- /dev/null +++ b/tests/adapter/dbt/tests/adapter/grants/test_incremental_grants.py @@ -0,0 +1,102 @@ +import pytest +from dbt.tests.util import ( + run_dbt, + run_dbt_and_capture, + get_manifest, + write_file, + relation_from_name, + get_connection, +) +from dbt.tests.adapter.grants.base_grants import BaseGrants + +my_incremental_model_sql = """ + select 1 as fun +""" + +incremental_model_schema_yml = """ +version: 2 +models: + - name: my_incremental_model + config: + materialized: incremental + grants: + select: ["{{ env_var('DBT_TEST_USER_1') }}"] +""" + +user2_incremental_model_schema_yml = """ +version: 2 +models: + - name: my_incremental_model + config: + materialized: incremental + grants: + select: ["{{ env_var('DBT_TEST_USER_2') }}"] +""" + + +class BaseIncrementalGrants(BaseGrants): + @pytest.fixture(scope="class") + def models(self): + updated_schema = self.interpolate_name_overrides(incremental_model_schema_yml) + return { + "my_incremental_model.sql": my_incremental_model_sql, + "schema.yml": updated_schema, + } + + def test_incremental_grants(self, project, get_test_users): + # we want the test to fail, not silently skip + test_users = get_test_users + select_privilege_name = self.privilege_grantee_name_overrides()["select"] + assert len(test_users) == 3 + + # Incremental materialization, single select grant + (results, log_output) = run_dbt_and_capture(["--debug", "run"]) + assert len(results) == 1 + manifest = get_manifest(project.project_root) + model_id = "model.test.my_incremental_model" + model = manifest.nodes[model_id] + assert model.config.materialized == "incremental" + expected = {select_privilege_name: [test_users[0]]} + self.assert_expected_grants_match_actual(project, "my_incremental_model", expected) + + # Incremental materialization, run again without changes + (results, log_output) = run_dbt_and_capture(["--debug", "run"]) + assert len(results) == 1 + assert "revoke " not in log_output + assert "grant " not in log_output # with space to disambiguate from 'show grants' + self.assert_expected_grants_match_actual(project, "my_incremental_model", expected) + + # Incremental materialization, change select grant user + updated_yaml = self.interpolate_name_overrides(user2_incremental_model_schema_yml) + write_file(updated_yaml, project.project_root, "models", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "run"]) + assert len(results) == 1 + assert "revoke " in log_output + manifest = get_manifest(project.project_root) + model = manifest.nodes[model_id] + assert model.config.materialized == "incremental" + expected = {select_privilege_name: [test_users[1]]} + self.assert_expected_grants_match_actual(project, "my_incremental_model", expected) + + # Incremental materialization, same config, now with --full-refresh + run_dbt(["--debug", "run", "--full-refresh"]) + assert len(results) == 1 + # whether grants or revokes happened will vary by adapter + self.assert_expected_grants_match_actual(project, "my_incremental_model", expected) + + # Now drop the schema (with the table in it) + adapter = project.adapter + relation = relation_from_name(adapter, "my_incremental_model") + with get_connection(adapter): + adapter.drop_schema(relation) + + # Incremental materialization, same config, rebuild now that table is missing + (results, log_output) = run_dbt_and_capture(["--debug", "run"]) + assert len(results) == 1 + assert "grant " in log_output + assert "revoke " not in log_output + self.assert_expected_grants_match_actual(project, "my_incremental_model", expected) + + +class TestIncrementalGrants(BaseIncrementalGrants): + pass diff --git a/tests/adapter/dbt/tests/adapter/grants/test_invalid_grants.py b/tests/adapter/dbt/tests/adapter/grants/test_invalid_grants.py new file mode 100644 index 00000000000..b16cedaac84 --- /dev/null +++ b/tests/adapter/dbt/tests/adapter/grants/test_invalid_grants.py @@ -0,0 +1,68 @@ +import pytest +from dbt.tests.util import ( + run_dbt_and_capture, + write_file, +) +from dbt.tests.adapter.grants.base_grants import BaseGrants + +my_invalid_model_sql = """ + select 1 as fun +""" + +invalid_user_table_model_schema_yml = """ +version: 2 +models: + - name: my_invalid_model + config: + materialized: table + grants: + select: ['invalid_user'] +""" + +invalid_privilege_table_model_schema_yml = """ +version: 2 +models: + - name: my_invalid_model + config: + materialized: table + grants: + fake_privilege: ["{{ env_var('DBT_TEST_USER_2') }}"] +""" + + +class BaseInvalidGrants(BaseGrants): + # The purpose of this test is to understand the user experience when providing + # an invalid 'grants' configuration. dbt will *not* try to intercept or interpret + # the database's own error at runtime -- it will just return those error messages. + # Hopefully they're helpful! + + @pytest.fixture(scope="class") + def models(self): + return { + "my_invalid_model.sql": my_invalid_model_sql, + } + + # Adapters will need to reimplement these methods with the specific + # language of their database + def grantee_does_not_exist_error(self): + return "does not exist" + + def privilege_does_not_exist_error(self): + return "unrecognized privilege" + + def test_invalid_grants(self, project, get_test_users, logs_dir): + # failure when grant to a user/role that doesn't exist + yaml_file = self.interpolate_name_overrides(invalid_user_table_model_schema_yml) + write_file(yaml_file, project.project_root, "models", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "run"], expect_pass=False) + assert self.grantee_does_not_exist_error() in log_output + + # failure when grant to a privilege that doesn't exist + yaml_file = self.interpolate_name_overrides(invalid_privilege_table_model_schema_yml) + write_file(yaml_file, project.project_root, "models", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "run"], expect_pass=False) + assert self.privilege_does_not_exist_error() in log_output + + +class TestInvalidGrants(BaseInvalidGrants): + pass diff --git a/tests/adapter/dbt/tests/adapter/grants/test_model_grants.py b/tests/adapter/dbt/tests/adapter/grants/test_model_grants.py new file mode 100644 index 00000000000..db2fe379f5b --- /dev/null +++ b/tests/adapter/dbt/tests/adapter/grants/test_model_grants.py @@ -0,0 +1,156 @@ +import pytest +from dbt.tests.util import ( + run_dbt_and_capture, + get_manifest, + write_file, +) +from dbt.tests.adapter.grants.base_grants import BaseGrants + +my_model_sql = """ + select 1 as fun +""" + +model_schema_yml = """ +version: 2 +models: + - name: my_model + config: + grants: + select: ["{{ env_var('DBT_TEST_USER_1') }}"] +""" + +user2_model_schema_yml = """ +version: 2 +models: + - name: my_model + config: + grants: + select: ["{{ env_var('DBT_TEST_USER_2') }}"] +""" + +table_model_schema_yml = """ +version: 2 +models: + - name: my_model + config: + materialized: table + grants: + select: ["{{ env_var('DBT_TEST_USER_1') }}"] +""" + +user2_table_model_schema_yml = """ +version: 2 +models: + - name: my_model + config: + materialized: table + grants: + select: ["{{ env_var('DBT_TEST_USER_2') }}"] +""" + +multiple_users_table_model_schema_yml = """ +version: 2 +models: + - name: my_model + config: + materialized: table + grants: + select: ["{{ env_var('DBT_TEST_USER_1') }}", "{{ env_var('DBT_TEST_USER_2') }}"] +""" + +multiple_privileges_table_model_schema_yml = """ +version: 2 +models: + - name: my_model + config: + materialized: table + grants: + select: ["{{ env_var('DBT_TEST_USER_1') }}"] + insert: ["{{ env_var('DBT_TEST_USER_2') }}"] +""" + + +class BaseModelGrants(BaseGrants): + @pytest.fixture(scope="class") + def models(self): + updated_schema = self.interpolate_name_overrides(model_schema_yml) + return { + "my_model.sql": my_model_sql, + "schema.yml": updated_schema, + } + + def test_view_table_grants(self, project, get_test_users): + # we want the test to fail, not silently skip + test_users = get_test_users + select_privilege_name = self.privilege_grantee_name_overrides()["select"] + insert_privilege_name = self.privilege_grantee_name_overrides()["insert"] + assert len(test_users) == 3 + + # View materialization, single select grant + (results, log_output) = run_dbt_and_capture(["--debug", "run"]) + assert len(results) == 1 + manifest = get_manifest(project.project_root) + model_id = "model.test.my_model" + model = manifest.nodes[model_id] + expected = {select_privilege_name: [test_users[0]]} + assert model.config.grants == expected + assert model.config.materialized == "view" + self.assert_expected_grants_match_actual(project, "my_model", expected) + + # View materialization, change select grant user + updated_yaml = self.interpolate_name_overrides(user2_model_schema_yml) + write_file(updated_yaml, project.project_root, "models", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "run"]) + assert len(results) == 1 + + expected = {select_privilege_name: [get_test_users[1]]} + self.assert_expected_grants_match_actual(project, "my_model", expected) + + # Table materialization, single select grant + updated_yaml = self.interpolate_name_overrides(table_model_schema_yml) + write_file(updated_yaml, project.project_root, "models", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "run"]) + assert len(results) == 1 + manifest = get_manifest(project.project_root) + model_id = "model.test.my_model" + model = manifest.nodes[model_id] + assert model.config.materialized == "table" + expected = {select_privilege_name: [test_users[0]]} + self.assert_expected_grants_match_actual(project, "my_model", expected) + + # Table materialization, change select grant user + updated_yaml = self.interpolate_name_overrides(user2_table_model_schema_yml) + write_file(updated_yaml, project.project_root, "models", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "run"]) + assert len(results) == 1 + manifest = get_manifest(project.project_root) + model = manifest.nodes[model_id] + assert model.config.materialized == "table" + expected = {select_privilege_name: [test_users[1]]} + self.assert_expected_grants_match_actual(project, "my_model", expected) + + # Table materialization, multiple grantees + updated_yaml = self.interpolate_name_overrides(multiple_users_table_model_schema_yml) + write_file(updated_yaml, project.project_root, "models", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "run"]) + assert len(results) == 1 + manifest = get_manifest(project.project_root) + model = manifest.nodes[model_id] + assert model.config.materialized == "table" + expected = {select_privilege_name: [test_users[0], test_users[1]]} + self.assert_expected_grants_match_actual(project, "my_model", expected) + + # Table materialization, multiple privileges + updated_yaml = self.interpolate_name_overrides(multiple_privileges_table_model_schema_yml) + write_file(updated_yaml, project.project_root, "models", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "run"]) + assert len(results) == 1 + manifest = get_manifest(project.project_root) + model = manifest.nodes[model_id] + assert model.config.materialized == "table" + expected = {select_privilege_name: [test_users[0]], insert_privilege_name: [test_users[1]]} + self.assert_expected_grants_match_actual(project, "my_model", expected) + + +class TestModelGrants(BaseModelGrants): + pass diff --git a/tests/adapter/dbt/tests/adapter/grants/test_models.py b/tests/adapter/dbt/tests/adapter/grants/test_models.py deleted file mode 100644 index b0765bd2ab2..00000000000 --- a/tests/adapter/dbt/tests/adapter/grants/test_models.py +++ /dev/null @@ -1,149 +0,0 @@ -import pytest -import os -from dbt.tests.util import ( - run_dbt_and_capture, - get_manifest, - relation_from_name, - write_file, - get_connection, -) - -from dbt.context.base import BaseContext # diff_of_two_dicts only - -TEST_USER_ENV_VARS = ["DBT_TEST_USER_1", "DBT_TEST_USER_2", "DBT_TEST_USER_3"] - -my_model_sql = """ - select 1 as fun -""" - -model_schema_yml = """ -version: 2 -models: - - name: my_model - config: - grants: - select: ["{{ env_var('DBT_TEST_USER_1') }}"] -""" - -user2_model_schema_yml = """ -version: 2 -models: - - name: my_model - config: - grants: - select: ["{{ env_var('DBT_TEST_USER_2') }}"] -""" - -table_model_schema_yml = """ -version: 2 -models: - - name: my_model - config: - materialized: table - grants: - select: ["{{ env_var('DBT_TEST_USER_1') }}"] -""" - -user2_table_model_schema_yml = """ -version: 2 -models: - - name: my_model - config: - materialized: table - grants: - select: ["{{ env_var('DBT_TEST_USER_2') }}"] -""" - - -def format_grant_log_line(relation, user_name): - return f"grant select on {relation} to {user_name};" - - -class TestModelGrants: - @pytest.fixture(scope="class") - def models(self): - return {"my_model.sql": my_model_sql, "schema.yml": model_schema_yml} - - @pytest.fixture(scope="class", autouse=True) - def get_test_users(self, project): - test_users = [] - for env_var in TEST_USER_ENV_VARS: - user_name = os.getenv(env_var) - if user_name: - test_users.append(user_name) - return test_users - - def get_grants_on_relation(self, project, relation_name): - relation = relation_from_name(project.adapter, relation_name) - adapter = project.adapter - with get_connection(adapter): - kwargs = {"relation": relation} - show_grant_sql = adapter.execute_macro("get_show_grant_sql", kwargs=kwargs) - _, grant_table = adapter.execute(show_grant_sql, fetch=True) - actual_grants = adapter.standardize_grants_dict(grant_table) - return actual_grants - - def test_model_grants(self, project, get_test_users, logs_dir): - # we want the test to fail, not silently skip - test_users = get_test_users - assert len(test_users) == 3 - - # View materialization, single select grant - (results, log_output) = run_dbt_and_capture(["--debug", "run"]) - assert len(results) == 1 - manifest = get_manifest(project.project_root) - model_id = "model.test.my_model" - model = manifest.nodes[model_id] - expected = {"select": [test_users[0]]} - assert model.config.grants == expected - assert model.config.materialized == "view" - - my_model_relation = relation_from_name(project.adapter, "my_model") - grant_log_line = format_grant_log_line(my_model_relation, test_users[0]) - assert grant_log_line in log_output - - # validate actual grants in database - actual_grants = self.get_grants_on_relation(project, "my_model") - # actual_grants: {'SELECT': ['dbt_test_user_1']} - # need a case-insensitive comparison - # so just a simple "assert expected == actual_grants" won't work - diff = BaseContext.diff_of_two_dicts(expected, actual_grants) - assert diff == {} - - # View materialization, change select grant user - write_file(user2_model_schema_yml, project.project_root, "models", "schema.yml") - (results, log_output) = run_dbt_and_capture(["--debug", "run"]) - assert len(results) == 1 - grant_log_line = format_grant_log_line(my_model_relation, test_users[1]) - assert grant_log_line in log_output - - expected = {"select": [get_test_users[1]]} - actual_grants = self.get_grants_on_relation(project, "my_model") - diff = BaseContext.diff_of_two_dicts(expected, actual_grants) - assert diff == {} - - # Table materialization, single select grant - write_file(table_model_schema_yml, project.project_root, "models", "schema.yml") - (results, log_output) = run_dbt_and_capture(["--debug", "run"]) - assert len(results) == 1 - grant_log_line = format_grant_log_line(my_model_relation, test_users[0]) - assert grant_log_line in log_output - manifest = get_manifest(project.project_root) - model = manifest.nodes[model_id] - model.config.materialized == "table" - actual_grants = self.get_grants_on_relation(project, "my_model") - diff = BaseContext.diff_of_two_dicts({"select": [test_users[0]]}, actual_grants) - assert diff == {} - - # Table materialization, change select grant user - write_file(user2_table_model_schema_yml, project.project_root, "models", "schema.yml") - (results, log_output) = run_dbt_and_capture(["--debug", "run"]) - assert len(results) == 1 - grant_log_line = format_grant_log_line(my_model_relation, test_users[1]) - assert grant_log_line in log_output - manifest = get_manifest(project.project_root) - model = manifest.nodes[model_id] - model.config.materialized == "table" - actual_grants = self.get_grants_on_relation(project, "my_model") - diff = BaseContext.diff_of_two_dicts({"select": [test_users[1]]}, actual_grants) - assert diff == {} diff --git a/tests/adapter/dbt/tests/adapter/grants/test_seed_grants.py b/tests/adapter/dbt/tests/adapter/grants/test_seed_grants.py new file mode 100644 index 00000000000..aff20c65cad --- /dev/null +++ b/tests/adapter/dbt/tests/adapter/grants/test_seed_grants.py @@ -0,0 +1,143 @@ +import pytest +from dbt.tests.util import ( + run_dbt, + run_dbt_and_capture, + get_manifest, + write_file, +) +from dbt.tests.adapter.grants.base_grants import BaseGrants + +seeds__my_seed_csv = """ +id,name,some_date +1,Easton,1981-05-20T06:46:51 +2,Lillian,1978-09-03T18:10:33 +""".lstrip() + +schema_base_yml = """ +version: 2 +seeds: + - name: my_seed + config: + grants: + select: ["{{ env_var('DBT_TEST_USER_1') }}"] +""" + +user2_schema_base_yml = """ +version: 2 +seeds: + - name: my_seed + config: + grants: + select: ["{{ env_var('DBT_TEST_USER_2') }}"] +""" + +ignore_grants_yml = """ +version: 2 +seeds: + - name: my_seed + config: + grants: {} +""" + +zero_grants_yml = """ +version: 2 +seeds: + - name: my_seed + config: + grants: + select: [] +""" + + +class BaseSeedGrants(BaseGrants): + def seeds_support_partial_refresh(self): + return True + + @pytest.fixture(scope="class") + def seeds(self): + updated_schema = self.interpolate_name_overrides(schema_base_yml) + return { + "my_seed.csv": seeds__my_seed_csv, + "schema.yml": updated_schema, + } + + def test_seed_grants(self, project, get_test_users): + test_users = get_test_users + select_privilege_name = self.privilege_grantee_name_overrides()["select"] + + # seed command + (results, log_output) = run_dbt_and_capture(["--debug", "seed"]) + assert len(results) == 1 + manifest = get_manifest(project.project_root) + seed_id = "seed.test.my_seed" + seed = manifest.nodes[seed_id] + expected = {select_privilege_name: [test_users[0]]} + assert seed.config.grants == expected + assert "grant " in log_output + self.assert_expected_grants_match_actual(project, "my_seed", expected) + + # run it again, with no config changes + (results, log_output) = run_dbt_and_capture(["--debug", "seed"]) + assert len(results) == 1 + if self.seeds_support_partial_refresh(): + # grants carried over -- nothing should have changed + assert "revoke " not in log_output + assert "grant " not in log_output + else: + # seeds are always full-refreshed on this adapter, so we need to re-grant + assert "grant " in log_output + self.assert_expected_grants_match_actual(project, "my_seed", expected) + + # change the grantee, assert it updates + updated_yaml = self.interpolate_name_overrides(user2_schema_base_yml) + write_file(updated_yaml, project.project_root, "seeds", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "seed"]) + assert len(results) == 1 + expected = {select_privilege_name: [test_users[1]]} + self.assert_expected_grants_match_actual(project, "my_seed", expected) + + # run it again, with --full-refresh, grants should be the same + run_dbt(["seed", "--full-refresh"]) + self.assert_expected_grants_match_actual(project, "my_seed", expected) + + # change config to 'grants: {}' -- should be completely ignored + updated_yaml = self.interpolate_name_overrides(ignore_grants_yml) + write_file(updated_yaml, project.project_root, "seeds", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "seed"]) + assert len(results) == 1 + assert "revoke " not in log_output + assert "grant " not in log_output + manifest = get_manifest(project.project_root) + seed_id = "seed.test.my_seed" + seed = manifest.nodes[seed_id] + expected_config = {} + expected_actual = {select_privilege_name: [test_users[1]]} + assert seed.config.grants == expected_config + if self.seeds_support_partial_refresh(): + # ACTUAL grants will NOT match expected grants + self.assert_expected_grants_match_actual(project, "my_seed", expected_actual) + else: + # there should be ZERO grants on the seed + self.assert_expected_grants_match_actual(project, "my_seed", expected_config) + + # now run with ZERO grants -- all grants should be removed + # whether explicitly (revoke) or implicitly (recreated without any grants added on) + updated_yaml = self.interpolate_name_overrides(zero_grants_yml) + write_file(updated_yaml, project.project_root, "seeds", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "seed"]) + assert len(results) == 1 + if self.seeds_support_partial_refresh(): + assert "revoke " in log_output + expected = {} + self.assert_expected_grants_match_actual(project, "my_seed", expected) + + # run it again -- dbt shouldn't try to grant or revoke anything + (results, log_output) = run_dbt_and_capture(["--debug", "seed"]) + assert len(results) == 1 + assert "revoke " not in log_output + assert "grant " not in log_output + self.assert_expected_grants_match_actual(project, "my_seed", expected) + + +class TestSeedGrants(BaseSeedGrants): + pass diff --git a/tests/adapter/dbt/tests/adapter/grants/test_snapshot_grants.py b/tests/adapter/dbt/tests/adapter/grants/test_snapshot_grants.py new file mode 100644 index 00000000000..6bf69b3bb94 --- /dev/null +++ b/tests/adapter/dbt/tests/adapter/grants/test_snapshot_grants.py @@ -0,0 +1,78 @@ +import pytest +from dbt.tests.util import ( + run_dbt, + run_dbt_and_capture, + get_manifest, + write_file, +) +from dbt.tests.adapter.grants.base_grants import BaseGrants + +my_snapshot_sql = """ +{% snapshot my_snapshot %} + {{ config( + check_cols='all', unique_key='id', strategy='check', + target_database=database, target_schema=schema + ) }} + select 1 as id, cast('blue' as {{ type_string() }}) as color +{% endsnapshot %} +""".strip() + +snapshot_schema_yml = """ +version: 2 +snapshots: + - name: my_snapshot + config: + grants: + select: ["{{ env_var('DBT_TEST_USER_1') }}"] +""" + +user2_snapshot_schema_yml = """ +version: 2 +snapshots: + - name: my_snapshot + config: + grants: + select: ["{{ env_var('DBT_TEST_USER_2') }}"] +""" + + +class BaseSnapshotGrants(BaseGrants): + @pytest.fixture(scope="class") + def snapshots(self): + return { + "my_snapshot.sql": my_snapshot_sql, + "schema.yml": self.interpolate_name_overrides(snapshot_schema_yml), + } + + def test_snapshot_grants(self, project, get_test_users): + test_users = get_test_users + select_privilege_name = self.privilege_grantee_name_overrides()["select"] + + # run the snapshot + results = run_dbt(["snapshot"]) + assert len(results) == 1 + manifest = get_manifest(project.project_root) + snapshot_id = "snapshot.test.my_snapshot" + snapshot = manifest.nodes[snapshot_id] + expected = {select_privilege_name: [test_users[0]]} + assert snapshot.config.grants == expected + self.assert_expected_grants_match_actual(project, "my_snapshot", expected) + + # run it again, nothing should have changed + (results, log_output) = run_dbt_and_capture(["--debug", "snapshot"]) + assert len(results) == 1 + assert "revoke " not in log_output + assert "grant " not in log_output + self.assert_expected_grants_match_actual(project, "my_snapshot", expected) + + # change the grantee, assert it updates + updated_yaml = self.interpolate_name_overrides(user2_snapshot_schema_yml) + write_file(updated_yaml, project.project_root, "snapshots", "schema.yml") + (results, log_output) = run_dbt_and_capture(["--debug", "snapshot"]) + assert len(results) == 1 + expected = {select_privilege_name: [test_users[1]]} + self.assert_expected_grants_match_actual(project, "my_snapshot", expected) + + +class TestSnapshotGrants(BaseSnapshotGrants): + pass From 068c59bb3235e271be7138c1db29aa20e3361018 Mon Sep 17 00:00:00 2001 From: Matthew McKnight Date: Mon, 11 Jul 2022 11:15:44 -0500 Subject: [PATCH 32/32] update to main, create changelog, whitespace fixes --- .changes/unreleased/Features-20220711-111514.yaml | 8 ++++++++ .../materializations/models/incremental/incremental.sql | 2 +- .../macros/materializations/models/table/table.sql | 2 +- .../models/view/create_or_replace_view.sql | 2 +- .../macros/materializations/models/view/view.sql | 2 +- .../global_project/macros/materializations/seeds/seed.sql | 2 +- .../macros/materializations/snapshots/snapshot.sql | 2 +- 7 files changed, 14 insertions(+), 6 deletions(-) create mode 100644 .changes/unreleased/Features-20220711-111514.yaml diff --git a/.changes/unreleased/Features-20220711-111514.yaml b/.changes/unreleased/Features-20220711-111514.yaml new file mode 100644 index 00000000000..020274a47cb --- /dev/null +++ b/.changes/unreleased/Features-20220711-111514.yaml @@ -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" diff --git a/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql b/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql index 54956142f19..52c141c27a2 100644 --- a/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql +++ b/core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql @@ -21,7 +21,7 @@ {%- 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') %} + {% set grant_config = config.get('grants') %} {{ drop_relation_if_exists(preexisting_intermediate_relation) }} {{ drop_relation_if_exists(preexisting_backup_relation) }} diff --git a/core/dbt/include/global_project/macros/materializations/models/table/table.sql b/core/dbt/include/global_project/macros/materializations/models/table/table.sql index abf7a7dc0c5..d142310addd 100644 --- a/core/dbt/include/global_project/macros/materializations/models/table/table.sql +++ b/core/dbt/include/global_project/macros/materializations/models/table/table.sql @@ -15,7 +15,7 @@ -- 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') %} + {% set grant_config = config.get('grants') %} -- drop the temp relations if they exist already in the database {{ drop_relation_if_exists(preexisting_intermediate_relation) }} diff --git a/core/dbt/include/global_project/macros/materializations/models/view/create_or_replace_view.sql b/core/dbt/include/global_project/macros/materializations/models/view/create_or_replace_view.sql index 5d763ff138d..649daf2a0b6 100644 --- a/core/dbt/include/global_project/macros/materializations/models/view/create_or_replace_view.sql +++ b/core/dbt/include/global_project/macros/materializations/models/view/create_or_replace_view.sql @@ -18,7 +18,7 @@ {%- set target_relation = api.Relation.create( identifier=identifier, schema=schema, database=database, type='view') -%} - {% set grant_config = config.get('grants') %} + {% set grant_config = config.get('grants') %} {{ run_hooks(pre_hooks) }} diff --git a/core/dbt/include/global_project/macros/materializations/models/view/view.sql b/core/dbt/include/global_project/macros/materializations/models/view/view.sql index 106e271b057..a135da12f31 100644 --- a/core/dbt/include/global_project/macros/materializations/models/view/view.sql +++ b/core/dbt/include/global_project/macros/materializations/models/view/view.sql @@ -26,7 +26,7 @@ -- 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') %} + {% set grant_config = config.get('grants') %} {{ run_hooks(pre_hooks, inside_transaction=False) }} diff --git a/core/dbt/include/global_project/macros/materializations/seeds/seed.sql b/core/dbt/include/global_project/macros/materializations/seeds/seed.sql index 5a46a029c61..d43b7f3ccf1 100644 --- a/core/dbt/include/global_project/macros/materializations/seeds/seed.sql +++ b/core/dbt/include/global_project/macros/materializations/seeds/seed.sql @@ -8,7 +8,7 @@ {%- 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 grant_config = config.get('grants') -%} {%- set agate_table = load_agate_table() -%} -- grab current tables grants config for comparision later on diff --git a/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql b/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql index 4cee08ddb1b..ccd0ef422ad 100644 --- a/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql +++ b/core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql @@ -6,7 +6,7 @@ {%- 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 grant_config = config.get('grants') -%} {% set target_relation_exists, target_relation = get_or_create_relation( database=model.database,