From 712d22b8f5df34f613d58ba609afc25c596c4eb2 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Mon, 27 Jun 2022 10:27:11 -0500 Subject: [PATCH 01/24] Add get_show_grant_sql --- dbt/include/bigquery/macros/adapters/apply_grants.sql | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 dbt/include/bigquery/macros/adapters/apply_grants.sql diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql new file mode 100644 index 000000000..4c6f5af51 --- /dev/null +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -0,0 +1,4 @@ +{% macro bigquery__get_show_grant_sql(relation) %} + select * from {{ relation.project }}.'{{ target.location }}'.INFORMATION_SCHEMA.OBJECT_PRIVILEGES + where object_schema = '{{ relation.dataset }}' and object_name = '{{ relation.identifier }}' +{% endmacro %} \ No newline at end of file From 5bb3afd7970c68dd065f77cc5ffee6e83cc6cf3e Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Mon, 27 Jun 2022 15:38:34 -0500 Subject: [PATCH 02/24] first pass at granting with sql --- dbt/include/bigquery/macros/adapters/apply_grants.sql | 6 ++++-- dbt/include/bigquery/macros/materializations/copy.sql | 6 +++++- .../bigquery/macros/materializations/incremental.sql | 7 ++++++- dbt/include/bigquery/macros/materializations/table.sql | 5 +++++ dbt/include/bigquery/macros/materializations/view.sql | 4 ++++ 5 files changed, 24 insertions(+), 4 deletions(-) diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index 4c6f5af51..83040f6ec 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -1,4 +1,6 @@ {% macro bigquery__get_show_grant_sql(relation) %} - select * from {{ relation.project }}.'{{ target.location }}'.INFORMATION_SCHEMA.OBJECT_PRIVILEGES - where object_schema = '{{ relation.dataset }}' and object_name = '{{ relation.identifier }}' + {# Note: This only works if the location is defined in the profile. It is an optional field right now. #} + +select * from {{ relation.project }}.{{ target.location }}.INFORMATION_SCHEMA.OBJECT_PRIVILEGES +where object_schema = "{{ relation.dataset }}" and object_name = "{{ relation.identifier }}" {% endmacro %} \ No newline at end of file diff --git a/dbt/include/bigquery/macros/materializations/copy.sql b/dbt/include/bigquery/macros/materializations/copy.sql index 6a86fbe44..cb766e0ab 100644 --- a/dbt/include/bigquery/macros/materializations/copy.sql +++ b/dbt/include/bigquery/macros/materializations/copy.sql @@ -1,6 +1,9 @@ {% materialization copy, adapter='bigquery' -%} {# Setup #} + -- grab current tables grants config for comparision later on + {# TODO: this is a guess I need to circle back to - does the BQ copy API also copy grants automatically? If yes it seems like we would want to option to overwrite them if needed? #} + {%- set grant_config = config.get('grants') -%} {{ run_hooks(pre_hooks) }} {% set destination = this.incorporate(type='table') %} @@ -16,7 +19,7 @@ {{ source_array.append(source(*src_table)) }} {% endfor %} - {# Call adapter's copy_table function #} + {# Call adapter copy_table function #} {%- set result_str = adapter.copy_table( source_array, destination, @@ -26,6 +29,7 @@ {# Clean up #} {{ run_hooks(post_hooks) }} + {%- do apply_grants(target_relation, grant_config) -%} {{ adapter.commit() }} {{ return({'relations': [destination]}) }} diff --git a/dbt/include/bigquery/macros/materializations/incremental.sql b/dbt/include/bigquery/macros/materializations/incremental.sql index b6d387890..cd57afcaa 100644 --- a/dbt/include/bigquery/macros/materializations/incremental.sql +++ b/dbt/include/bigquery/macros/materializations/incremental.sql @@ -143,7 +143,7 @@ {%- set tmp_relation = make_temp_relation(this) %} {#-- Validate early so we don't run SQL if the strategy is invalid --#} - {% set strategy = dbt_bigquery_validate_get_incremental_strategy(config) -%} + {%- set strategy = dbt_bigquery_validate_get_incremental_strategy(config) -%} {%- set raw_partition_by = config.get('partition_by', none) -%} {%- set partition_by = adapter.parse_partition_by(raw_partition_by) -%} @@ -152,6 +152,9 @@ {% set on_schema_change = incremental_validate_on_schema_change(config.get('on_schema_change'), default='ignore') %} + -- grab current tables grants config for comparision later on + {% set grant_config = config.get('grants') %} + {{ run_hooks(pre_hooks) }} {% if existing_relation is none %} @@ -197,6 +200,8 @@ {% set target_relation = this.incorporate(type='table') %} + {% do apply_grants(target_relation, grant_config) %} + {% do persist_docs(target_relation, model) %} {{ return({'relations': [target_relation]}) }} diff --git a/dbt/include/bigquery/macros/materializations/table.sql b/dbt/include/bigquery/macros/materializations/table.sql index a7452265b..9ef67ae98 100644 --- a/dbt/include/bigquery/macros/materializations/table.sql +++ b/dbt/include/bigquery/macros/materializations/table.sql @@ -5,6 +5,9 @@ {%- set exists_not_as_table = (old_relation is not none and not old_relation.is_table) -%} {%- set target_relation = api.Relation.create(database=database, schema=schema, identifier=identifier, type='table') -%} + -- grab current tables grants config for comparision later on + {%- set grant_config = config.get('grants') -%} + {{ run_hooks(pre_hooks) }} {# @@ -30,6 +33,8 @@ {{ run_hooks(post_hooks) }} + {%- do apply_grants(target_relation, grant_config) -%} + {% do persist_docs(target_relation, model) %} {{ return({'relations': [target_relation]}) }} diff --git a/dbt/include/bigquery/macros/materializations/view.sql b/dbt/include/bigquery/macros/materializations/view.sql index 97e3d2761..933185fb2 100644 --- a/dbt/include/bigquery/macros/materializations/view.sql +++ b/dbt/include/bigquery/macros/materializations/view.sql @@ -9,9 +9,13 @@ {% materialization view, adapter='bigquery' -%} + -- grab current tables grants config for comparision later on + {% set grant_config = config.get('grants') %} + {% set to_return = create_or_replace_view() %} {% set target_relation = this.incorporate(type='view') %} + {% do apply_grants(target_relation, grant_config) %} {% do persist_docs(target_relation, model) %} {% if config.get('grant_access_to') %} From aeb19f3199d2f92ed54c554f280b91b198b6a5ec Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Tue, 28 Jun 2022 19:19:36 -0500 Subject: [PATCH 03/24] more macro overrides --- .../bigquery/macros/adapters/apply_grants.sql | 29 ++++++++++++++++--- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index 83040f6ec..1833e6850 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -1,6 +1,27 @@ {% macro bigquery__get_show_grant_sql(relation) %} - {# Note: This only works if the location is defined in the profile. It is an optional field right now. #} - -select * from {{ relation.project }}.{{ target.location }}.INFORMATION_SCHEMA.OBJECT_PRIVILEGES +{# Note: This only works if the location is defined in the profile. It is an optional field right now. #} +{# TODO: location is hardcoded for now - need logic around this (default to us)#} +select * from {{ relation.project }}.`region-us`.INFORMATION_SCHEMA.OBJECT_PRIVILEGES where object_schema = "{{ relation.dataset }}" and object_name = "{{ relation.identifier }}" -{% endmacro %} \ No newline at end of file +{% endmacro %} + +{% macro bigquery__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.dataset }}.{{ relation.identifier }} to "{{ grantee }}"; + {% endfor %} + {% endfor %} +{% endmacro %} + +{% macro bigquery__get_revoke_sql(relation, grant_config) %} +{# TODO: will probably need to do some more tweaks around revoke and table fields once that is resolved in core #} + {% 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.dataset }}.{{ relation.identifier }} from "{{ grantee }}"; + {% endfor %} + {% endfor %} +{% endmacro %} + From 42ae831ace34a003132a128d6bd36dc4064fa0d9 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Thu, 7 Jul 2022 10:07:52 -0500 Subject: [PATCH 04/24] update macros, temp update to dev-req for CI --- dbt/adapters/bigquery/impl.py | 23 ++++++ .../bigquery/macros/adapters/apply_grants.sql | 77 ++++++++++++++----- dev-requirements.txt | 9 ++- 3 files changed, 86 insertions(+), 23 deletions(-) diff --git a/dbt/adapters/bigquery/impl.py b/dbt/adapters/bigquery/impl.py index 344ffb22e..b3e201d70 100644 --- a/dbt/adapters/bigquery/impl.py +++ b/dbt/adapters/bigquery/impl.py @@ -261,6 +261,29 @@ def list_relations_without_caching( logger.debug("list_relations_without_caching error: {}".format(str(exc))) return [] + @available + def standardize_grants_dict(self, grants_table: agate.Table) -> dict: + """Translate the result of `show grants` (or equivalent) to match the + grants which a user would configure in their project. + + If relevant -- filter down to grants made BY the current user role, + and filter OUT any grants TO the current user/role (e.g. OWNERSHIP). + + :param grants_table: An agate table containing the query result of + the SQL returned by get_show_grant_sql + :return: A standardized dictionary matching the `grants` config + :rtype: dict + """ + 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 get_relation(self, database: str, schema: str, identifier: str) -> BigQueryRelation: if self._schema_is_cached(database, schema): # if it's in the cache, use the parent's model of going through diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index 1833e6850..c39a5c875 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -1,27 +1,62 @@ {% macro bigquery__get_show_grant_sql(relation) %} -{# Note: This only works if the location is defined in the profile. It is an optional field right now. #} -{# TODO: location is hardcoded for now - need logic around this (default to us)#} -select * from {{ relation.project }}.`region-us`.INFORMATION_SCHEMA.OBJECT_PRIVILEGES -where object_schema = "{{ relation.dataset }}" and object_name = "{{ relation.identifier }}" + select * from {{ relation.project }}.`region-{{ target.location }}`.INFORMATION_SCHEMA.OBJECT_PRIVILEGES + where object_schema = "{{ relation.dataset }}" and object_name = "{{ relation.identifier }}" {% endmacro %} -{% macro bigquery__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.dataset }}.{{ relation.identifier }} to "{{ grantee }}"; - {% endfor %} - {% endfor %} -{% endmacro %} + +{%- macro bigquery__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.type }} {{ relation }} to {{ grantee}}; + {%- endfor -%} + {%- endif -%} + {%- endfor -%} +{%- endmacro %} + {% macro bigquery__get_revoke_sql(relation, grant_config) %} -{# TODO: will probably need to do some more tweaks around revoke and table fields once that is resolved in core #} - {% 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.dataset }}.{{ relation.identifier }} from "{{ grantee }}"; - {% endfor %} - {% endfor %} -{% endmacro %} + {%- 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 -%} + {%- if grantees -%} + {%- for grantee in grantees -%} + revoke `{{ privilege }}` on {{ relation.type }} {{ relation.dataset }}.{{ relation.identifier }} from "{{ grantee }}"; + {% endfor -%} + {%- endif -%} + {%- endfor -%} +{%- endmacro -%} + +{% macro bigquery__apply_grants(relation, grant_config, should_revoke=True) %} + {% if grant_config %} + {% if should_revoke %} + {#-- This check is the only reason BQ needs its own copy of apply_grants --#} + {% if not target.location %} + {{ exceptions.raise_compiler_error("In order to use the grants feature, you must specify a location ") }} + {% endif %} + {% set current_grants_table = run_query(get_show_grant_sql(relation)) %} + {% set current_grants_dict = adapter.standardize_grants_dict(current_grants_table) %} + {% set needs_granting = diff_of_two_dicts(grant_config, current_grants_dict) %} + {% set needs_revoking = diff_of_two_dicts(current_grants_dict, grant_config) %} + {% if not (needs_granting or needs_revoking) %} + {{ log('All grants are in place, no revocation or granting needed.')}} + {% endif %} + {% else %} + {% set needs_revoking = {} %} + {% set needs_granting = grant_config %} + {% endif %} + {% if needs_granting or needs_revoking %} + {% call statement('grants') %} + {{ get_revoke_sql(relation, needs_revoking) }} + {{ get_grant_sql(relation, needs_granting) }} + {% endcall %} + {% endif %} + {% endif %} +{% endmacro %} \ No newline at end of file diff --git a/dev-requirements.txt b/dev-requirements.txt index 09cf7f7c2..ffe319e95 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,7 +1,12 @@ # install latest changes in dbt-core # TODO: how to automate switching from develop to version branches? -git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-core&subdirectory=core -git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-tests-adapter&subdirectory=tests/adapter + +git+https://github.com/dbt-labs/dbt-core.git@ct-660-grant-sql#egg=dbt-core&subdirectory=core +git+https://github.com/dbt-labs/dbt-core.git@ct-660-grant-sql#egg=dbt-tests-adapter&subdirectory=tests/adapter + +# TODO: replace above with this after dbt-core/#5263 gets merged to main +# git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-core&subdirectory=core +# git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-tests-adapter&subdirectory=tests/adapter black==22.3.0 bumpversion From a661549e7cd4b1fb690d3ef3394ae7daf466033c Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Thu, 7 Jul 2022 10:40:49 -0500 Subject: [PATCH 05/24] tweak to sql and added some TODOs --- .../bigquery/macros/adapters/apply_grants.sql | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index c39a5c875..c68016984 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -1,15 +1,17 @@ {% macro bigquery__get_show_grant_sql(relation) %} - select * from {{ relation.project }}.`region-{{ target.location }}`.INFORMATION_SCHEMA.OBJECT_PRIVILEGES + select privilege_type, grantee from {{ relation.project }}.`region-{{ target.location }}`.INFORMATION_SCHEMA.OBJECT_PRIVILEGES where object_schema = "{{ relation.dataset }}" and object_name = "{{ relation.identifier }}" {% endmacro %} {%- macro bigquery__get_grant_sql(relation, grant_config) -%} +{# TODO: remove these when quotes are added to default macro #} + {%- for privilege in grant_config.keys() -%} {%- set grantees = grant_config[privilege] -%} {%- if grantees -%} {%- for grantee in grantees -%} - grant {{ privilege }} on {{ relation.type }} {{ relation }} to {{ grantee}}; + grant {{ privilege }} on {{ relation.type }} {{ relation }} to "{{ grantee}}"; {%- endfor -%} {%- endif -%} {%- endfor -%} @@ -17,6 +19,8 @@ {% macro bigquery__get_revoke_sql(relation, grant_config) %} +{# TODO: remove these when quotes are added to default macro #} + {%- for privilege in grant_config.keys() -%} {%- set grantees = [] -%} {%- set all_grantees = grant_config[privilege] -%} @@ -27,7 +31,7 @@ {% endfor -%} {%- if grantees -%} {%- for grantee in grantees -%} - revoke `{{ privilege }}` on {{ relation.type }} {{ relation.dataset }}.{{ relation.identifier }} from "{{ grantee }}"; + revoke `{{ privilege }}` on {{ relation.type }} {{ relation }} from "{{ grantee }}"; {% endfor -%} {%- endif -%} {%- endfor -%} @@ -35,9 +39,10 @@ {% macro bigquery__apply_grants(relation, grant_config, should_revoke=True) %} + {% if grant_config %} {% if should_revoke %} - {#-- This check is the only reason BQ needs its own copy of apply_grants --#} + {# This check is the only reason BQ needs its own copy of apply_grants - is there a better way? #} {% if not target.location %} {{ exceptions.raise_compiler_error("In order to use the grants feature, you must specify a location ") }} {% endif %} @@ -59,4 +64,4 @@ {% endcall %} {% endif %} {% endif %} -{% endmacro %} \ No newline at end of file +{% endmacro %} From b0b06ef1af76069a12753031c1b64cd872073abb Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Thu, 7 Jul 2022 11:37:30 -0500 Subject: [PATCH 06/24] move things around --- .../bigquery/macros/adapters/apply_grants.sql | 41 +++---------------- dev-requirements.txt | 4 +- 2 files changed, 8 insertions(+), 37 deletions(-) diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index c68016984..178c96756 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -1,17 +1,19 @@ {% macro bigquery__get_show_grant_sql(relation) %} + {% if not target.location %} + {{ exceptions.raise_compiler_error("In order to use the grants feature, you must specify a location ") }} + {% endif %} + select privilege_type, grantee from {{ relation.project }}.`region-{{ target.location }}`.INFORMATION_SCHEMA.OBJECT_PRIVILEGES where object_schema = "{{ relation.dataset }}" and object_name = "{{ relation.identifier }}" {% endmacro %} {%- macro bigquery__get_grant_sql(relation, grant_config) -%} -{# TODO: remove these when quotes are added to default macro #} - {%- for privilege in grant_config.keys() -%} {%- set grantees = grant_config[privilege] -%} {%- if grantees -%} {%- for grantee in grantees -%} - grant {{ privilege }} on {{ relation.type }} {{ relation }} to "{{ grantee}}"; + grant `{{ privilege }}` on {{ relation.type }} {{ relation }} to "{{ grantee }}"; {%- endfor -%} {%- endif -%} {%- endfor -%} @@ -19,8 +21,6 @@ {% macro bigquery__get_revoke_sql(relation, grant_config) %} -{# TODO: remove these when quotes are added to default macro #} - {%- for privilege in grant_config.keys() -%} {%- set grantees = [] -%} {%- set all_grantees = grant_config[privilege] -%} @@ -28,7 +28,7 @@ {%- 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 }}"; @@ -36,32 +36,3 @@ {%- endif -%} {%- endfor -%} {%- endmacro -%} - - -{% macro bigquery__apply_grants(relation, grant_config, should_revoke=True) %} - - {% if grant_config %} - {% if should_revoke %} - {# This check is the only reason BQ needs its own copy of apply_grants - is there a better way? #} - {% if not target.location %} - {{ exceptions.raise_compiler_error("In order to use the grants feature, you must specify a location ") }} - {% endif %} - {% set current_grants_table = run_query(get_show_grant_sql(relation)) %} - {% set current_grants_dict = adapter.standardize_grants_dict(current_grants_table) %} - {% set needs_granting = diff_of_two_dicts(grant_config, current_grants_dict) %} - {% set needs_revoking = diff_of_two_dicts(current_grants_dict, grant_config) %} - {% if not (needs_granting or needs_revoking) %} - {{ log('All grants are in place, no revocation or granting needed.')}} - {% endif %} - {% else %} - {% set needs_revoking = {} %} - {% set needs_granting = grant_config %} - {% endif %} - {% if needs_granting or needs_revoking %} - {% call statement('grants') %} - {{ get_revoke_sql(relation, needs_revoking) }} - {{ get_grant_sql(relation, needs_granting) }} - {% endcall %} - {% endif %} - {% endif %} -{% endmacro %} diff --git a/dev-requirements.txt b/dev-requirements.txt index ffe319e95..dc6aa4177 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,8 +1,8 @@ # install latest changes in dbt-core # TODO: how to automate switching from develop to version branches? -git+https://github.com/dbt-labs/dbt-core.git@ct-660-grant-sql#egg=dbt-core&subdirectory=core -git+https://github.com/dbt-labs/dbt-core.git@ct-660-grant-sql#egg=dbt-tests-adapter&subdirectory=tests/adapter +git+https://github.com/dbt-labs/dbt-core.git@ct-808-grant_adapter_tests#egg=dbt-core&subdirectory=core +git+https://github.com/dbt-labs/dbt-core.git@ct-808-grant_adapter_tests#egg=dbt-tests-adapter&subdirectory=tests/adapter # TODO: replace above with this after dbt-core/#5263 gets merged to main # git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-core&subdirectory=core From e0909ca4a84d6d8def7018beccad768a6fa3d746 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Thu, 7 Jul 2022 11:45:11 -0500 Subject: [PATCH 07/24] exclude session_user --- dbt/include/bigquery/macros/adapters/apply_grants.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index 178c96756..edeffcdcc 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -4,7 +4,7 @@ {% endif %} select privilege_type, grantee from {{ relation.project }}.`region-{{ target.location }}`.INFORMATION_SCHEMA.OBJECT_PRIVILEGES - where object_schema = "{{ relation.dataset }}" and object_name = "{{ relation.identifier }}" + where object_schema = "{{ relation.dataset }}" and object_name = "{{ relation.identifier }}" and grantee != "user:"||session_user() {% endmacro %} From 0733a5f4d0afdf37bb45e5fb88deef6f78797fcc Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Thu, 7 Jul 2022 11:57:27 -0500 Subject: [PATCH 08/24] fix mypy error, fix exclude to be more than users --- dbt/adapters/bigquery/impl.py | 2 +- dbt/include/bigquery/macros/adapters/apply_grants.sql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dbt/adapters/bigquery/impl.py b/dbt/adapters/bigquery/impl.py index b3e201d70..5468759fe 100644 --- a/dbt/adapters/bigquery/impl.py +++ b/dbt/adapters/bigquery/impl.py @@ -274,7 +274,7 @@ def standardize_grants_dict(self, grants_table: agate.Table) -> dict: :return: A standardized dictionary matching the `grants` config :rtype: dict """ - grants_dict = {} + grants_dict: Dict[str, List] = {} for row in grants_table: grantee = row["grantee"] privilege = row["privilege_type"] diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index edeffcdcc..04f497e61 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -4,7 +4,7 @@ {% endif %} select privilege_type, grantee from {{ relation.project }}.`region-{{ target.location }}`.INFORMATION_SCHEMA.OBJECT_PRIVILEGES - where object_schema = "{{ relation.dataset }}" and object_name = "{{ relation.identifier }}" and grantee != "user:"||session_user() + where object_schema = "{{ relation.dataset }}" and object_name = "{{ relation.identifier }}" and split(grantee, ':')[offset(1)] != current_user() {% endmacro %} From ce094b7d18c35987e8d6d0629cb7cf931400058e Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Thu, 7 Jul 2022 13:18:01 -0500 Subject: [PATCH 09/24] simplify revoke logic --- .../bigquery/macros/adapters/apply_grants.sql | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index 04f497e61..9f9040f1d 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -22,17 +22,8 @@ {% macro bigquery__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 -%} - {%- if grantees -%} - {%- for grantee in grantees -%} - revoke `{{ privilege }}` on {{ relation.type }} {{ relation }} from "{{ grantee }}"; - {% endfor -%} - {%- endif -%} + {%- for grantee in grant_config[privilege] -%} + revoke `{{ privilege }}` on {{ relation.type }} {{ relation }} from "{{ grantee }}"; + {% endfor -%} {%- endfor -%} {%- endmacro -%} From 83c73e0fce4ab570b03a24403a8db8f534fa5041 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Fri, 8 Jul 2022 08:23:23 -0500 Subject: [PATCH 10/24] small cleanup, point to ct-660 branch --- .../bigquery/macros/adapters/apply_grants.sql | 13 +++++-------- .../bigquery/macros/materializations/copy.sql | 3 --- .../macros/materializations/incremental.sql | 2 +- .../bigquery/macros/materializations/table.sql | 2 +- .../bigquery/macros/materializations/view.sql | 2 +- dev-requirements.txt | 4 ++-- 6 files changed, 10 insertions(+), 16 deletions(-) diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index 9f9040f1d..cfee67955 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -4,18 +4,15 @@ {% endif %} select privilege_type, grantee from {{ relation.project }}.`region-{{ target.location }}`.INFORMATION_SCHEMA.OBJECT_PRIVILEGES - where object_schema = "{{ relation.dataset }}" and object_name = "{{ relation.identifier }}" and split(grantee, ':')[offset(1)] != current_user() + where object_schema = "{{ relation.dataset }}" and object_name = "{{ relation.identifier }}" and split(grantee, ':')[offset(1)] != session_user() {% endmacro %} {%- macro bigquery__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.type }} {{ relation }} to "{{ grantee }}"; - {%- endfor -%} - {%- endif -%} + {%- for grantee in grant_config[privilege] -%} + grant `{{ privilege }}` on {{ relation.type }} {{ relation }} to "{{ grantee }}"; + {% endfor -%} {%- endfor -%} {%- endmacro %} @@ -23,7 +20,7 @@ {% macro bigquery__get_revoke_sql(relation, grant_config) %} {%- for privilege in grant_config.keys() -%} {%- for grantee in grant_config[privilege] -%} - revoke `{{ privilege }}` on {{ relation.type }} {{ relation }} from "{{ grantee }}"; + revoke `{{ privilege }}` on {{ relation.type }} {{ relation }} from "{{ grantee }}"; {% endfor -%} {%- endfor -%} {%- endmacro -%} diff --git a/dbt/include/bigquery/macros/materializations/copy.sql b/dbt/include/bigquery/macros/materializations/copy.sql index cb766e0ab..8285dc845 100644 --- a/dbt/include/bigquery/macros/materializations/copy.sql +++ b/dbt/include/bigquery/macros/materializations/copy.sql @@ -1,9 +1,6 @@ {% materialization copy, adapter='bigquery' -%} {# Setup #} - -- grab current tables grants config for comparision later on - {# TODO: this is a guess I need to circle back to - does the BQ copy API also copy grants automatically? If yes it seems like we would want to option to overwrite them if needed? #} - {%- set grant_config = config.get('grants') -%} {{ run_hooks(pre_hooks) }} {% set destination = this.incorporate(type='table') %} diff --git a/dbt/include/bigquery/macros/materializations/incremental.sql b/dbt/include/bigquery/macros/materializations/incremental.sql index cd57afcaa..d21c7da16 100644 --- a/dbt/include/bigquery/macros/materializations/incremental.sql +++ b/dbt/include/bigquery/macros/materializations/incremental.sql @@ -153,7 +153,7 @@ {% set on_schema_change = incremental_validate_on_schema_change(config.get('on_schema_change'), default='ignore') %} -- 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) }} diff --git a/dbt/include/bigquery/macros/materializations/table.sql b/dbt/include/bigquery/macros/materializations/table.sql index 9ef67ae98..f89f50f7a 100644 --- a/dbt/include/bigquery/macros/materializations/table.sql +++ b/dbt/include/bigquery/macros/materializations/table.sql @@ -6,7 +6,7 @@ {%- set target_relation = api.Relation.create(database=database, schema=schema, identifier=identifier, type='table') -%} -- 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) }} diff --git a/dbt/include/bigquery/macros/materializations/view.sql b/dbt/include/bigquery/macros/materializations/view.sql index 933185fb2..eb31d3030 100644 --- a/dbt/include/bigquery/macros/materializations/view.sql +++ b/dbt/include/bigquery/macros/materializations/view.sql @@ -10,7 +10,7 @@ {% materialization view, adapter='bigquery' -%} -- grab current tables grants config for comparision later on - {% set grant_config = config.get('grants') %} + {% set grant_config = config.get('grants') %} {% set to_return = create_or_replace_view() %} diff --git a/dev-requirements.txt b/dev-requirements.txt index dcbad2f73..90de79983 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,8 +1,8 @@ # install latest changes in dbt-core # TODO: how to automate switching from develop to version branches? -git+https://github.com/dbt-labs/dbt-core.git@ct-808-grant_adapter_tests#egg=dbt-core&subdirectory=core -git+https://github.com/dbt-labs/dbt-core.git@ct-808-grant_adapter_tests#egg=dbt-tests-adapter&subdirectory=tests/adapter +git+https://github.com/dbt-labs/dbt-core.git@ct-660-grant-sql#egg=dbt-core&subdirectory=core +git+https://github.com/dbt-labs/dbt-core.git@ct-660-grant-sql#egg=dbt-tests-adapter&subdirectory=tests/adapter # TODO: replace above with this after dbt-core/#5263 gets merged to main # git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-core&subdirectory=core From 0cdeb1e3447cc54907cb9fc1f62f6a33832728f1 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Fri, 8 Jul 2022 08:54:44 -0500 Subject: [PATCH 11/24] grant entire list in one statement --- .../bigquery/macros/adapters/apply_grants.sql | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index cfee67955..7c03691d5 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -10,17 +10,19 @@ {%- macro bigquery__get_grant_sql(relation, grant_config) -%} {%- for privilege in grant_config.keys() -%} - {%- for grantee in grant_config[privilege] -%} - grant `{{ privilege }}` on {{ relation.type }} {{ relation }} to "{{ grantee }}"; - {% endfor -%} + {%- set grantees = grant_config[privilege] -%} + {%- if grantees -%} + grant `{{ privilege }}` on {{ relation.type }} {{ relation }} to {{ '\"' + grantees|join('\", \"') + '\"' }}; + {% endif -%} {%- endfor -%} {%- endmacro %} {% macro bigquery__get_revoke_sql(relation, grant_config) %} {%- for privilege in grant_config.keys() -%} - {%- for grantee in grant_config[privilege] -%} - revoke `{{ privilege }}` on {{ relation.type }} {{ relation }} from "{{ grantee }}"; - {% endfor -%} + {%- set grantees = grant_config[privilege] -%} + {%- if grantees -%} + revoke `{{ privilege }}` on {{ relation.type }} {{ relation }} from {{ '\"' + grantees|join('\", \"') + '\"' }}; + {% endif -%} {%- endfor -%} {%- endmacro -%} From 8070891414b9228b84d8312826170664edab392c Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Fri, 8 Jul 2022 13:30:21 -0500 Subject: [PATCH 12/24] wip --- dev-requirements.txt | 4 +-- test.env.example | 4 +++ tests/conftest.py | 2 ++ tests/functional/adapter/test_grants.py | 34 +++++++++++++++++++++++++ 4 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 tests/functional/adapter/test_grants.py diff --git a/dev-requirements.txt b/dev-requirements.txt index 90de79983..cc0d5a5b4 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,8 +1,8 @@ # install latest changes in dbt-core # TODO: how to automate switching from develop to version branches? -git+https://github.com/dbt-labs/dbt-core.git@ct-660-grant-sql#egg=dbt-core&subdirectory=core -git+https://github.com/dbt-labs/dbt-core.git@ct-660-grant-sql#egg=dbt-tests-adapter&subdirectory=tests/adapter +git+https://github.com/dbt-labs/dbt-core.git@ct-808-more_grant_adapter_tests#egg=dbt-core&subdirectory=core +git+https://github.com/dbt-labs/dbt-core.git@ct-808-more_grant_adapter_tests#egg=dbt-tests-adapter&subdirectory=tests/adapter # TODO: replace above with this after dbt-core/#5263 gets merged to main # git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-core&subdirectory=core diff --git a/test.env.example b/test.env.example index 2065e4393..ade7f1142 100644 --- a/test.env.example +++ b/test.env.example @@ -1,3 +1,7 @@ BIGQUERY_TEST_ALT_DATABASE= BIGQUERY_TEST_NO_ACCESS_DATABASE= BIGQUERY_TEST_SERVICE_ACCOUNT_JSON='{}' + +DBT_TEST_USER_1="user:buildbot@dbtlabs.com" +DBT_TEST_USER_2="user:buildbot@fishtownanalytics.com" +DBT_TEST_USER_3="group:dev-core@dbtlabs.com" diff --git a/tests/conftest.py b/tests/conftest.py index 4bbdb00e0..dc3529bac 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -30,6 +30,7 @@ def oauth_target(): 'method': 'oauth', 'threads': 1, # project isn't needed if you configure a default, via 'gcloud config set project' + 'location': 'US' } @@ -43,4 +44,5 @@ def service_account_target(): 'threads': 1, 'project': project_id, 'keyfile_json': credentials, + 'location': 'US' } diff --git a/tests/functional/adapter/test_grants.py b/tests/functional/adapter/test_grants.py new file mode 100644 index 000000000..ba5678c5d --- /dev/null +++ b/tests/functional/adapter/test_grants.py @@ -0,0 +1,34 @@ +import pytest + +from dbt.tests.adapter.grants.base_grants import BaseGrants +from dbt.tests.adapter.grants.test_model_grants import BaseModelGrants +from dbt.tests.adapter.grants.test_incremental_grants import BaseIncrementalGrants +from dbt.tests.adapter.grants.test_invalid_grants import BaseInvalidGrants +from dbt.tests.adapter.grants.test_seed_grants import BaseSeedGrants +from dbt.tests.adapter.grants.test_snapshot_grants import BaseSnapshotGrants + + +class BaseGrantsBigQuery(BaseGrants): + def privilege_names(self): + # TODO: what is insert equivalent? + return {"select": "roles/dataViewer", "insert": "roles/dataViewer", "fake_privilege": "roles/invalid"} + + +class TestModelGrantsBigQuery(BaseModelGrants): + pass + + +class TestIncrementalGrantsBigQuery(BaseIncrementalGrants): + pass + + +class TestSeedGrantsBigQuery(BaseSeedGrants): + pass + + +class TestSnapshotGrantsBigQuery(BaseSnapshotGrants): + pass + + +class TestInvalidGrantsBigQuery(BaseInvalidGrants): + pass \ No newline at end of file From d8f6e2d6e3412a0c9a28bffa76fc10debb52fab3 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Fri, 8 Jul 2022 14:34:35 -0500 Subject: [PATCH 13/24] wip, broken tests --- .../bigquery/macros/adapters/apply_grants.sql | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index 7c03691d5..e4c37ed43 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -9,20 +9,33 @@ {%- macro bigquery__get_grant_sql(relation, grant_config) -%} - {%- for privilege in grant_config.keys() -%} + {%- set grant_statements = [] -%} + {%- for privilege in grant_config.keys() %} {%- set grantees = grant_config[privilege] -%} - {%- if grantees -%} - grant `{{ privilege }}` on {{ relation.type }} {{ relation }} to {{ '\"' + grantees|join('\", \"') + '\"' }}; + {%- if grantees %} + {% set grant_sql -%} + grant `{{ privilege }}` on {{ relation.type }} {{ relation }} to {{ '\"' + grantees|join('\", \"') + '\"' }} + {%- endset %} + {%- do grant_statements.append(grant_sql) -%} {% endif -%} {%- endfor -%} + {{ return(grant_statements) }} + {%- endmacro %} {% macro bigquery__get_revoke_sql(relation, grant_config) %} + + {%- set revoke_statements = [] -%} {%- for privilege in grant_config.keys() -%} {%- set grantees = grant_config[privilege] -%} - {%- if grantees -%} - revoke `{{ privilege }}` on {{ relation.type }} {{ relation }} from {{ '\"' + grantees|join('\", \"') + '\"' }}; + {%- if grantees %} + {% set revoke_sql -%} + revoke `{{ privilege }}` on {{ relation.type }} {{ relation }} from {{ '\"' + grantees|join('\", \"') + '\"' }} + {%- endset %} + {%- do revoke_statements.append(revoke_sql) -%} {% endif -%} {%- endfor -%} + {{ return(revoke_statements) }} + {%- endmacro -%} From 10cc952a2a20da208b063da46cbb1517522aaf7a Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Fri, 8 Jul 2022 14:46:23 -0500 Subject: [PATCH 14/24] Update dbt/include/bigquery/macros/materializations/incremental.sql Co-authored-by: Anders --- dbt/include/bigquery/macros/materializations/incremental.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbt/include/bigquery/macros/materializations/incremental.sql b/dbt/include/bigquery/macros/materializations/incremental.sql index d21c7da16..b16820d84 100644 --- a/dbt/include/bigquery/macros/materializations/incremental.sql +++ b/dbt/include/bigquery/macros/materializations/incremental.sql @@ -143,7 +143,7 @@ {%- set tmp_relation = make_temp_relation(this) %} {#-- Validate early so we don't run SQL if the strategy is invalid --#} - {%- set strategy = dbt_bigquery_validate_get_incremental_strategy(config) -%} + {% set strategy = dbt_bigquery_validate_get_incremental_strategy(config) -%} {%- set raw_partition_by = config.get('partition_by', none) -%} {%- set partition_by = adapter.parse_partition_by(raw_partition_by) -%} From cb42c810dc36c5b1c585e73b3f4aaa90980624fe Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Fri, 8 Jul 2022 16:04:13 -0500 Subject: [PATCH 15/24] working on getting tests working, very WIP --- dbt/adapters/bigquery/impl.py | 23 --- .../bigquery/macros/adapters/apply_grants.sql | 25 +++ tests/functional/adapter/test_grants.py | 24 ++- .../adapter/test_model_grants_local.py | 170 ++++++++++++++++++ 4 files changed, 212 insertions(+), 30 deletions(-) create mode 100644 tests/functional/adapter/test_model_grants_local.py diff --git a/dbt/adapters/bigquery/impl.py b/dbt/adapters/bigquery/impl.py index 5468759fe..344ffb22e 100644 --- a/dbt/adapters/bigquery/impl.py +++ b/dbt/adapters/bigquery/impl.py @@ -261,29 +261,6 @@ def list_relations_without_caching( logger.debug("list_relations_without_caching error: {}".format(str(exc))) return [] - @available - def standardize_grants_dict(self, grants_table: agate.Table) -> dict: - """Translate the result of `show grants` (or equivalent) to match the - grants which a user would configure in their project. - - If relevant -- filter down to grants made BY the current user role, - and filter OUT any grants TO the current user/role (e.g. OWNERSHIP). - - :param grants_table: An agate table containing the query result of - the SQL returned by get_show_grant_sql - :return: A standardized dictionary matching the `grants` config - :rtype: dict - """ - grants_dict: Dict[str, List] = {} - 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 get_relation(self, database: str, schema: str, identifier: str) -> BigQueryRelation: if self._schema_is_cached(database, schema): # if it's in the cache, use the parent's model of going through diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index e4c37ed43..76831ae94 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -39,3 +39,28 @@ {{ return(revoke_statements) }} {%- endmacro -%} + +{% macro bigquery__apply_grants(relation, grant_config, should_revoke=True) %} + {% if grant_config %} + {% if should_revoke %} + {% set current_grants_table = run_query(get_show_grant_sql(relation)) %} + {% set current_grants_dict = adapter.standardize_grants_dict(current_grants_table) %} + {% set needs_granting = diff_of_two_dicts_no_lower(grant_config, current_grants_dict) %} + {% set needs_revoking = diff_of_two_dicts_no_lower(current_grants_dict, grant_config) %} + {% if not (needs_granting or needs_revoking) %} + {{ log('On ' ~ relation ~': All grants are in place, no revocation or granting needed.')}} + {% endif %} + {% else %} + {% set needs_revoking = {} %} + {% set needs_granting = grant_config %} + {% endif %} + {% if needs_granting or needs_revoking %} + {% set revoke_statement_list = get_revoke_sql(relation, needs_revoking) %} + {% set grant_statement_list = get_grant_sql(relation, needs_granting) %} + {% set grant_and_revoke_statement_list = revoke_statement_list + grant_statement_list %} + {% if grant_and_revoke_statement_list %} + {{ call_grant_revoke_statement_list(grant_and_revoke_statement_list) }} + {% endif %} + {% endif %} + {% endif %} +{% endmacro %} diff --git a/tests/functional/adapter/test_grants.py b/tests/functional/adapter/test_grants.py index ba5678c5d..ae2b458eb 100644 --- a/tests/functional/adapter/test_grants.py +++ b/tests/functional/adapter/test_grants.py @@ -7,28 +7,38 @@ from dbt.tests.adapter.grants.test_seed_grants import BaseSeedGrants from dbt.tests.adapter.grants.test_snapshot_grants import BaseSnapshotGrants +from dbt.context.base import BaseContext # diff_of_two_dicts_no_lower only + class BaseGrantsBigQuery(BaseGrants): def privilege_names(self): # TODO: what is insert equivalent? - return {"select": "roles/dataViewer", "insert": "roles/dataViewer", "fake_privilege": "roles/invalid"} + return {"select": "roles/bigquery.dataViewer", "insert": "roles/bigquery.dataViewer", "fake_privilege": "roles/invalid"} + + 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_no_lower(actual_grants, expected_grants) + diff_b = BaseContext.diff_of_two_dicts_no_lower(expected_grants, actual_grants) + assert diff_a == diff_b == {} -class TestModelGrantsBigQuery(BaseModelGrants): +class TestModelGrantsBigQuery(BaseGrantsBigQuery, BaseModelGrants): pass -class TestIncrementalGrantsBigQuery(BaseIncrementalGrants): +class TestIncrementalGrantsBigQuery(BaseGrantsBigQuery, BaseIncrementalGrants): pass -class TestSeedGrantsBigQuery(BaseSeedGrants): +class TestSeedGrantsBigQuery(BaseGrantsBigQuery, BaseSeedGrants): pass -class TestSnapshotGrantsBigQuery(BaseSnapshotGrants): +class TestSnapshotGrantsBigQuery(BaseGrantsBigQuery, BaseSnapshotGrants): pass -class TestInvalidGrantsBigQuery(BaseInvalidGrants): - pass \ No newline at end of file +class TestInvalidGrantsBigQuery(BaseGrantsBigQuery, BaseInvalidGrants): + pass diff --git a/tests/functional/adapter/test_model_grants_local.py b/tests/functional/adapter/test_model_grants_local.py new file mode 100644 index 000000000..66634515e --- /dev/null +++ b/tests/functional/adapter/test_model_grants_local.py @@ -0,0 +1,170 @@ +import pytest +from dbt.tests.util import ( + run_dbt_and_capture, + get_manifest, + write_file, +) +from dbt.tests.adapter.grants.base_grants import BaseGrants + +from dbt.context.base import BaseContext # diff_of_two_dicts_no_lower only + + +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 BaseGrantsBigQuery(BaseGrants): + def privilege_names(self): + # TODO: what is insert equivalent? + return {"select": "roles/bigquery.dataViewer", "insert": "roles/bigquery.dataViewer", "fake_privilege": "roles/invalid"} + + 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_no_lower(actual_grants, expected_grants) + diff_b = BaseContext.diff_of_two_dicts_no_lower(expected_grants, actual_grants) + assert diff_a == diff_b == {} + +class BaseModelGrants(BaseGrantsBigQuery): + @pytest.fixture(scope="class") + def models(self): + updated_schema = self.interpolate_privilege_names(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_names()["select"] + insert_privilege_name = self.privilege_names()["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_privilege_names(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_privilege_names(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_privilege_names(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_privilege_names(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_privilege_names(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 From 118d311aa7092f8ee3285277883be8507b1ee5df Mon Sep 17 00:00:00 2001 From: Jeremy Cohen Date: Mon, 11 Jul 2022 00:21:01 +0200 Subject: [PATCH 16/24] All tests passing locally --- .github/workflows/integration.yml | 6 ++-- .../bigquery/macros/adapters/apply_grants.sql | 35 ++++--------------- .../macros/materializations/incremental.sql | 3 +- .../macros/materializations/table.sql | 3 +- .../bigquery/macros/materializations/view.sql | 2 +- test.env.example | 6 ++-- tests/functional/adapter/test_grants.py | 32 ++++++++--------- 7 files changed, 34 insertions(+), 53 deletions(-) diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 9b5d9f88c..586fbee94 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -168,9 +168,9 @@ jobs: BIGQUERY_TEST_SERVICE_ACCOUNT_JSON: ${{ secrets.BIGQUERY_TEST_SERVICE_ACCOUNT_JSON }} BIGQUERY_TEST_ALT_DATABASE: ${{ secrets.BIGQUERY_TEST_ALT_DATABASE }} BIGQUERY_TEST_NO_ACCESS_DATABASE: ${{ secrets.BIGQUERY_TEST_NO_ACCESS_DATABASE }} - DBT_TEST_USER_1: user:buildbot@dbtlabs.com - DBT_TEST_USER_2: user:buildbot@fishtownanalytics.com - DBT_TEST_USER_3: group:dev-core@dbtlabs.com + DBT_TEST_USER_1: group:buildbot@dbtlabs.com + DBT_TEST_USER_2: group:dev-core@dbtlabs.com + DBT_TEST_USER_3: user:someone@dbtlabs.com # TODO run: tox - uses: actions/upload-artifact@v2 diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index 76831ae94..0185efc92 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -1,10 +1,14 @@ -{% macro bigquery__get_show_grant_sql(relation) %} +{% macro bigquery__get_show_grant_sql(relation) %} {% if not target.location %} {{ exceptions.raise_compiler_error("In order to use the grants feature, you must specify a location ") }} {% endif %} - select privilege_type, grantee from {{ relation.project }}.`region-{{ target.location }}`.INFORMATION_SCHEMA.OBJECT_PRIVILEGES - where object_schema = "{{ relation.dataset }}" and object_name = "{{ relation.identifier }}" and split(grantee, ':')[offset(1)] != session_user() + select privilege_type, grantee + from `{{ relation.project }}`.`region-{{ target.location }}`.INFORMATION_SCHEMA.OBJECT_PRIVILEGES + where object_schema = "{{ relation.dataset }}" + and object_name = "{{ relation.identifier }}" + -- filter out current user + and split(grantee, ':')[offset(1)] != session_user() {% endmacro %} @@ -39,28 +43,3 @@ {{ return(revoke_statements) }} {%- endmacro -%} - -{% macro bigquery__apply_grants(relation, grant_config, should_revoke=True) %} - {% if grant_config %} - {% if should_revoke %} - {% set current_grants_table = run_query(get_show_grant_sql(relation)) %} - {% set current_grants_dict = adapter.standardize_grants_dict(current_grants_table) %} - {% set needs_granting = diff_of_two_dicts_no_lower(grant_config, current_grants_dict) %} - {% set needs_revoking = diff_of_two_dicts_no_lower(current_grants_dict, grant_config) %} - {% if not (needs_granting or needs_revoking) %} - {{ log('On ' ~ relation ~': All grants are in place, no revocation or granting needed.')}} - {% endif %} - {% else %} - {% set needs_revoking = {} %} - {% set needs_granting = grant_config %} - {% endif %} - {% if needs_granting or needs_revoking %} - {% set revoke_statement_list = get_revoke_sql(relation, needs_revoking) %} - {% set grant_statement_list = get_grant_sql(relation, needs_granting) %} - {% set grant_and_revoke_statement_list = revoke_statement_list + grant_statement_list %} - {% if grant_and_revoke_statement_list %} - {{ call_grant_revoke_statement_list(grant_and_revoke_statement_list) }} - {% endif %} - {% endif %} - {% endif %} -{% endmacro %} diff --git a/dbt/include/bigquery/macros/materializations/incremental.sql b/dbt/include/bigquery/macros/materializations/incremental.sql index b16820d84..8cf2ab65c 100644 --- a/dbt/include/bigquery/macros/materializations/incremental.sql +++ b/dbt/include/bigquery/macros/materializations/incremental.sql @@ -200,7 +200,8 @@ {% set target_relation = this.incorporate(type='table') %} - {% do apply_grants(target_relation, grant_config) %} + {% set should_revoke = should_revoke(existing_relation, full_refresh_mode) %} + {% do apply_grants(target_relation, grant_config, should_revoke) %} {% do persist_docs(target_relation, model) %} diff --git a/dbt/include/bigquery/macros/materializations/table.sql b/dbt/include/bigquery/macros/materializations/table.sql index f89f50f7a..9e63637c1 100644 --- a/dbt/include/bigquery/macros/materializations/table.sql +++ b/dbt/include/bigquery/macros/materializations/table.sql @@ -33,7 +33,8 @@ {{ run_hooks(post_hooks) }} - {%- do apply_grants(target_relation, grant_config) -%} + {% set should_revoke = should_revoke(old_relation, full_refresh_mode=True) %} + {% do apply_grants(target_relation, grant_config, should_revoke) %} {% do persist_docs(target_relation, model) %} diff --git a/dbt/include/bigquery/macros/materializations/view.sql b/dbt/include/bigquery/macros/materializations/view.sql index eb31d3030..e68a51421 100644 --- a/dbt/include/bigquery/macros/materializations/view.sql +++ b/dbt/include/bigquery/macros/materializations/view.sql @@ -15,7 +15,7 @@ {% set to_return = create_or_replace_view() %} {% set target_relation = this.incorporate(type='view') %} - {% do apply_grants(target_relation, grant_config) %} + {% do persist_docs(target_relation, model) %} {% if config.get('grant_access_to') %} diff --git a/test.env.example b/test.env.example index ade7f1142..b4cbfb96d 100644 --- a/test.env.example +++ b/test.env.example @@ -2,6 +2,6 @@ BIGQUERY_TEST_ALT_DATABASE= BIGQUERY_TEST_NO_ACCESS_DATABASE= BIGQUERY_TEST_SERVICE_ACCOUNT_JSON='{}' -DBT_TEST_USER_1="user:buildbot@dbtlabs.com" -DBT_TEST_USER_2="user:buildbot@fishtownanalytics.com" -DBT_TEST_USER_3="group:dev-core@dbtlabs.com" +DBT_TEST_USER_1="group:buildbot@dbtlabs.com" +DBT_TEST_USER_2="group:buildbot@fishtownanalytics.com" +DBT_TEST_USER_3="user:someone@dbtlabs.com" # TODO diff --git a/tests/functional/adapter/test_grants.py b/tests/functional/adapter/test_grants.py index ae2b458eb..7c52c5232 100644 --- a/tests/functional/adapter/test_grants.py +++ b/tests/functional/adapter/test_grants.py @@ -7,22 +7,15 @@ from dbt.tests.adapter.grants.test_seed_grants import BaseSeedGrants from dbt.tests.adapter.grants.test_snapshot_grants import BaseSnapshotGrants -from dbt.context.base import BaseContext # diff_of_two_dicts_no_lower only - class BaseGrantsBigQuery(BaseGrants): - def privilege_names(self): - # TODO: what is insert equivalent? - return {"select": "roles/bigquery.dataViewer", "insert": "roles/bigquery.dataViewer", "fake_privilege": "roles/invalid"} - - - 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_no_lower(actual_grants, expected_grants) - diff_b = BaseContext.diff_of_two_dicts_no_lower(expected_grants, actual_grants) - assert diff_a == diff_b == {} + def privilege_grantee_name_overrides(self): + return { + "select": "roles/bigquery.dataViewer", + "insert": "roles/bigquery.dataEditor", + "fake_privilege": "roles/invalid", + "invalid_user": "user:invalid", + } class TestModelGrantsBigQuery(BaseGrantsBigQuery, BaseModelGrants): pass @@ -33,7 +26,10 @@ class TestIncrementalGrantsBigQuery(BaseGrantsBigQuery, BaseIncrementalGrants): class TestSeedGrantsBigQuery(BaseGrantsBigQuery, BaseSeedGrants): - pass + # seeds in dbt-bigquery are always "full refreshed," in such a way that + # the grants do not carry over + def seeds_support_partial_refresh(self): + return False class TestSnapshotGrantsBigQuery(BaseGrantsBigQuery, BaseSnapshotGrants): @@ -41,4 +37,8 @@ class TestSnapshotGrantsBigQuery(BaseGrantsBigQuery, BaseSnapshotGrants): class TestInvalidGrantsBigQuery(BaseGrantsBigQuery, BaseInvalidGrants): - pass + def grantee_does_not_exist_error(self): + return "User invalid does not exist." + + def privilege_does_not_exist_error(self): + return "Role roles/invalid is not supported for this resource." From 29576960e9bc2a3389ea5489eb001b8437cccf22 Mon Sep 17 00:00:00 2001 From: Doug Beatty Date: Sun, 10 Jul 2022 19:44:59 -0600 Subject: [PATCH 17/24] Updated user / group names --- .github/workflows/integration.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 9b5d9f88c..2cc8fd058 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -168,9 +168,9 @@ jobs: BIGQUERY_TEST_SERVICE_ACCOUNT_JSON: ${{ secrets.BIGQUERY_TEST_SERVICE_ACCOUNT_JSON }} BIGQUERY_TEST_ALT_DATABASE: ${{ secrets.BIGQUERY_TEST_ALT_DATABASE }} BIGQUERY_TEST_NO_ACCESS_DATABASE: ${{ secrets.BIGQUERY_TEST_NO_ACCESS_DATABASE }} - DBT_TEST_USER_1: user:buildbot@dbtlabs.com - DBT_TEST_USER_2: user:buildbot@fishtownanalytics.com - DBT_TEST_USER_3: group:dev-core@dbtlabs.com + DBT_TEST_USER_1: group:buildbot@dbtlabs.com + DBT_TEST_USER_2: group:dev-core@dbtlabs.com + DBT_TEST_USER_3: user:someone@dbtlabs.com run: tox - uses: actions/upload-artifact@v2 From f61bd39bfeb783be80d5603b4ad80bb7dac9089e Mon Sep 17 00:00:00 2001 From: Jeremy Cohen Date: Mon, 11 Jul 2022 09:40:45 +0200 Subject: [PATCH 18/24] Ongoing test cleanup --- tests/functional/adapter/test_grants.py | 4 +- .../adapter/test_model_grants_local.py | 170 ------------------ 2 files changed, 2 insertions(+), 172 deletions(-) delete mode 100644 tests/functional/adapter/test_model_grants_local.py diff --git a/tests/functional/adapter/test_grants.py b/tests/functional/adapter/test_grants.py index 7c52c5232..b35e4787e 100644 --- a/tests/functional/adapter/test_grants.py +++ b/tests/functional/adapter/test_grants.py @@ -14,7 +14,7 @@ def privilege_grantee_name_overrides(self): "select": "roles/bigquery.dataViewer", "insert": "roles/bigquery.dataEditor", "fake_privilege": "roles/invalid", - "invalid_user": "user:invalid", + "invalid_user": "user:fake@dbtlabs.com", } class TestModelGrantsBigQuery(BaseGrantsBigQuery, BaseModelGrants): @@ -38,7 +38,7 @@ class TestSnapshotGrantsBigQuery(BaseGrantsBigQuery, BaseSnapshotGrants): class TestInvalidGrantsBigQuery(BaseGrantsBigQuery, BaseInvalidGrants): def grantee_does_not_exist_error(self): - return "User invalid does not exist." + return "User fake@dbtlabs.com does not exist." def privilege_does_not_exist_error(self): return "Role roles/invalid is not supported for this resource." diff --git a/tests/functional/adapter/test_model_grants_local.py b/tests/functional/adapter/test_model_grants_local.py deleted file mode 100644 index 66634515e..000000000 --- a/tests/functional/adapter/test_model_grants_local.py +++ /dev/null @@ -1,170 +0,0 @@ -import pytest -from dbt.tests.util import ( - run_dbt_and_capture, - get_manifest, - write_file, -) -from dbt.tests.adapter.grants.base_grants import BaseGrants - -from dbt.context.base import BaseContext # diff_of_two_dicts_no_lower only - - -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 BaseGrantsBigQuery(BaseGrants): - def privilege_names(self): - # TODO: what is insert equivalent? - return {"select": "roles/bigquery.dataViewer", "insert": "roles/bigquery.dataViewer", "fake_privilege": "roles/invalid"} - - 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_no_lower(actual_grants, expected_grants) - diff_b = BaseContext.diff_of_two_dicts_no_lower(expected_grants, actual_grants) - assert diff_a == diff_b == {} - -class BaseModelGrants(BaseGrantsBigQuery): - @pytest.fixture(scope="class") - def models(self): - updated_schema = self.interpolate_privilege_names(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_names()["select"] - insert_privilege_name = self.privilege_names()["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_privilege_names(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_privilege_names(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_privilege_names(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_privilege_names(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_privilege_names(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 From c4ce8d6d42f0628fc8e75271951ca7411699241e Mon Sep 17 00:00:00 2001 From: Jeremy Cohen Date: Mon, 11 Jul 2022 12:03:53 +0200 Subject: [PATCH 19/24] Account for refactor in dbt-labs/dbt-core@c763601 --- .../bigquery/macros/adapters/apply_grants.sql | 34 +++---------------- 1 file changed, 5 insertions(+), 29 deletions(-) diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index 0185efc92..8a7adba93 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -12,34 +12,10 @@ {% endmacro %} -{%- macro bigquery__get_grant_sql(relation, grant_config) -%} - {%- set grant_statements = [] -%} - {%- for privilege in grant_config.keys() %} - {%- set grantees = grant_config[privilege] -%} - {%- if grantees %} - {% set grant_sql -%} - grant `{{ privilege }}` on {{ relation.type }} {{ relation }} to {{ '\"' + grantees|join('\", \"') + '\"' }} - {%- endset %} - {%- do grant_statements.append(grant_sql) -%} - {% endif -%} - {%- endfor -%} - {{ return(grant_statements) }} - -{%- endmacro %} - - -{% macro bigquery__get_revoke_sql(relation, grant_config) %} - - {%- set revoke_statements = [] -%} - {%- for privilege in grant_config.keys() -%} - {%- set grantees = grant_config[privilege] -%} - {%- if grantees %} - {% set revoke_sql -%} - revoke `{{ privilege }}` on {{ relation.type }} {{ relation }} from {{ '\"' + grantees|join('\", \"') + '\"' }} - {%- endset %} - {%- do revoke_statements.append(revoke_sql) -%} - {% endif -%} - {%- endfor -%} - {{ return(revoke_statements) }} +{%- macro bigquery__get_grant_sql(relation, privilege, grantee) -%} + grant `{{ privilege }}` on {{ relation.type }} {{ relation }} to {{ '\"' + grantee|join('\", \"') + '\"' }} +{%- endmacro -%} +{%- macro bigquery__get_revoke_sql(relation, privilege, grantee) -%} + revoke `{{ privilege }}` on {{ relation.type }} {{ relation }} from {{ '\"' + grantee|join('\", \"') + '\"' }} {%- endmacro -%} From cee9fa1bedd3eb31cf8af2727e802205b5a49d48 Mon Sep 17 00:00:00 2001 From: Doug Beatty Date: Mon, 11 Jul 2022 10:15:55 -0600 Subject: [PATCH 20/24] Updated third value --- .github/workflows/integration.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 2cc8fd058..4f920d100 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -170,7 +170,7 @@ jobs: BIGQUERY_TEST_NO_ACCESS_DATABASE: ${{ secrets.BIGQUERY_TEST_NO_ACCESS_DATABASE }} DBT_TEST_USER_1: group:buildbot@dbtlabs.com DBT_TEST_USER_2: group:dev-core@dbtlabs.com - DBT_TEST_USER_3: user:someone@dbtlabs.com + DBT_TEST_USER_3: serviceAccount:dbt-integration-test-user@dbt-test-env.iam.gserviceaccount.com run: tox - uses: actions/upload-artifact@v2 From 27c41ed24525290c9b9d199e990fbd910846cbbd Mon Sep 17 00:00:00 2001 From: Jeremy Cohen Date: Mon, 11 Jul 2022 18:46:58 +0200 Subject: [PATCH 21/24] Alt approach for grant location (#218) * Alt approach to grabbing + factoring info_schema region * add exception for blank location * Update dbt/adapters/bigquery/relation.py Co-authored-by: Doug Beatty <44704949+dbeatty10@users.noreply.github.com> * point to existing branch * fix pre commit errors Co-authored-by: Emily Rockman Co-authored-by: Doug Beatty <44704949+dbeatty10@users.noreply.github.com> --- dbt/adapters/bigquery/impl.py | 8 ++++ dbt/adapters/bigquery/relation.py | 43 +++++++++++++++++++ .../bigquery/macros/adapters/apply_grants.sql | 7 ++- dev-requirements.txt | 4 +- tests/conftest.py | 3 -- 5 files changed, 56 insertions(+), 9 deletions(-) diff --git a/dbt/adapters/bigquery/impl.py b/dbt/adapters/bigquery/impl.py index 344ffb22e..7c66b014d 100644 --- a/dbt/adapters/bigquery/impl.py +++ b/dbt/adapters/bigquery/impl.py @@ -757,6 +757,14 @@ def grant_access_to(self, entity, entity_type, role, grant_target_dict): dataset.access_entries = access_entries client.update_dataset(dataset, ["access_entries"]) + @available.parse_none + def get_dataset_location(self, relation): + conn = self.connections.get_thread_connection() + client = conn.handle + dataset_ref = self.connections.dataset_ref(relation.project, relation.dataset) + dataset = client.get_dataset(dataset_ref) + return dataset.location + def get_rows_different_sql( # type: ignore[override] self, relation_a: BigQueryRelation, diff --git a/dbt/adapters/bigquery/relation.py b/dbt/adapters/bigquery/relation.py index 8156e360d..7224de8cf 100644 --- a/dbt/adapters/bigquery/relation.py +++ b/dbt/adapters/bigquery/relation.py @@ -1,7 +1,10 @@ from dataclasses import dataclass from typing import Optional +from itertools import chain, islice + from dbt.adapters.base.relation import BaseRelation, ComponentName, InformationSchema +from dbt.exceptions import raise_compiler_error from dbt.utils import filter_null_values from typing import TypeVar @@ -12,6 +15,7 @@ @dataclass(frozen=True, eq=False, repr=False) class BigQueryRelation(BaseRelation): quote_character: str = "`" + location: Optional[str] = None def matches( self, @@ -52,6 +56,7 @@ def information_schema(self, identifier: Optional[str] = None) -> "BigQueryInfor @dataclass(frozen=True, eq=False, repr=False) class BigQueryInformationSchema(InformationSchema): quote_character: str = "`" + location: Optional[str] = None @classmethod def get_include_policy(cls, relation, information_schema_view): @@ -63,11 +68,49 @@ def get_include_policy(cls, relation, information_schema_view): if information_schema_view == "__TABLES__": identifier = False + # In the future, let's refactor so that location/region can also be a + # ComponentName, so that we can have logic like: + # + # region = False + # if information_schema_view == "OBJECT_PRIVILEGES": + # region = True + return relation.include_policy.replace( schema=schema, identifier=identifier, ) + def get_region_identifier(self) -> str: + region_id = f"region-{self.location}" + return self.quoted(region_id) + + @classmethod + def from_relation(cls, relation, information_schema_view): + info_schema = super().from_relation(relation, information_schema_view) + if information_schema_view == "OBJECT_PRIVILEGES": + # OBJECT_PRIVILEGES require a location. If the location is blank there is nothing + # the user can do about it. + if not relation.location: + msg = ( + f'No location/region found when trying to retrieve "{information_schema_view}"' + ) + raise raise_compiler_error(msg) + info_schema = info_schema.incorporate(location=relation.location) + return info_schema + + # override this method to interpolate the region identifier, + # if a location is required for this information schema view + def _render_iterator(self): + iterator = super()._render_iterator() + if self.location: + return chain( + islice(iterator, 1), # project, + [(None, self.get_region_identifier())], # region id, + islice(iterator, 1, None), # remaining components + ) + else: + return iterator + def replace(self, **kwargs): if "information_schema_view" in kwargs: view = kwargs["information_schema_view"] diff --git a/dbt/include/bigquery/macros/adapters/apply_grants.sql b/dbt/include/bigquery/macros/adapters/apply_grants.sql index 8a7adba93..e344862ae 100644 --- a/dbt/include/bigquery/macros/adapters/apply_grants.sql +++ b/dbt/include/bigquery/macros/adapters/apply_grants.sql @@ -1,10 +1,9 @@ {% macro bigquery__get_show_grant_sql(relation) %} - {% if not target.location %} - {{ exceptions.raise_compiler_error("In order to use the grants feature, you must specify a location ") }} - {% endif %} + {% set location = adapter.get_dataset_location(relation) %} + {% set relation = relation.incorporate(location=location) %} select privilege_type, grantee - from `{{ relation.project }}`.`region-{{ target.location }}`.INFORMATION_SCHEMA.OBJECT_PRIVILEGES + from {{ relation.information_schema("OBJECT_PRIVILEGES") }} where object_schema = "{{ relation.dataset }}" and object_name = "{{ relation.identifier }}" -- filter out current user diff --git a/dev-requirements.txt b/dev-requirements.txt index cc0d5a5b4..90de79983 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,8 +1,8 @@ # install latest changes in dbt-core # TODO: how to automate switching from develop to version branches? -git+https://github.com/dbt-labs/dbt-core.git@ct-808-more_grant_adapter_tests#egg=dbt-core&subdirectory=core -git+https://github.com/dbt-labs/dbt-core.git@ct-808-more_grant_adapter_tests#egg=dbt-tests-adapter&subdirectory=tests/adapter +git+https://github.com/dbt-labs/dbt-core.git@ct-660-grant-sql#egg=dbt-core&subdirectory=core +git+https://github.com/dbt-labs/dbt-core.git@ct-660-grant-sql#egg=dbt-tests-adapter&subdirectory=tests/adapter # TODO: replace above with this after dbt-core/#5263 gets merged to main # git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-core&subdirectory=core diff --git a/tests/conftest.py b/tests/conftest.py index dc3529bac..69a29b39c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -29,8 +29,6 @@ def oauth_target(): 'type': 'bigquery', 'method': 'oauth', 'threads': 1, - # project isn't needed if you configure a default, via 'gcloud config set project' - 'location': 'US' } @@ -44,5 +42,4 @@ def service_account_target(): 'threads': 1, 'project': project_id, 'keyfile_json': credentials, - 'location': 'US' } From a6ef9207500a2656dc56dcd842bedd3aa4274681 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Mon, 11 Jul 2022 12:01:29 -0500 Subject: [PATCH 22/24] reset to point dbt-core to main instead of branch --- dev-requirements.txt | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/dev-requirements.txt b/dev-requirements.txt index 90de79983..2775ca784 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,12 +1,7 @@ # install latest changes in dbt-core # TODO: how to automate switching from develop to version branches? - -git+https://github.com/dbt-labs/dbt-core.git@ct-660-grant-sql#egg=dbt-core&subdirectory=core -git+https://github.com/dbt-labs/dbt-core.git@ct-660-grant-sql#egg=dbt-tests-adapter&subdirectory=tests/adapter - -# TODO: replace above with this after dbt-core/#5263 gets merged to main -# git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-core&subdirectory=core -# git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-tests-adapter&subdirectory=tests/adapter +git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-core&subdirectory=core +git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-tests-adapter&subdirectory=tests/adapter black==22.6.0 bumpversion From f47322f89487dc260f17bad6d6ee015ffa4fe9b6 Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Mon, 11 Jul 2022 13:17:00 -0500 Subject: [PATCH 23/24] fix examples in test.env.example --- test.env.example | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test.env.example b/test.env.example index b4cbfb96d..f2e59e6d0 100644 --- a/test.env.example +++ b/test.env.example @@ -3,5 +3,5 @@ BIGQUERY_TEST_NO_ACCESS_DATABASE= BIGQUERY_TEST_SERVICE_ACCOUNT_JSON='{}' DBT_TEST_USER_1="group:buildbot@dbtlabs.com" -DBT_TEST_USER_2="group:buildbot@fishtownanalytics.com" -DBT_TEST_USER_3="user:someone@dbtlabs.com" # TODO +DBT_TEST_USER_2="group:dev-core@dbtlabs.com" +DBT_TEST_USER_3="serviceAccount:dbt-integration-test-user@dbt-test-env.iam.gserviceaccount.com" From d94b62315e6c1faea2d4545daaf239432bc41aae Mon Sep 17 00:00:00 2001 From: Emily Rockman Date: Mon, 11 Jul 2022 13:28:11 -0500 Subject: [PATCH 24/24] add changelog --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2dccb571b..e5cab993e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,7 @@ -## dbt-bigquery 1.2.0rc1 (Release TBD) +## dbt-bigquery 1.2.0rc1 (June 11, 2022) + +### Features +- Add grants to materializations ([#198](https://github.com/dbt-labs/dbt-bigquery/issues/198), [#212](https://github.com/dbt-labs/dbt-bigquery/pull/212)) ### Under the hood - Modify `BigQueryColumn.numeric_type` to always exclude precision + scale, since the functionality of ["parametrized data types on BigQuery](https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#parameterized_data_types) is highly constrained ([#214](https://github.com/dbt-labs/dbt-bigquery/pull/214))