Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Privilege Grants to Roles and Groups in Materialization #626

Open
wants to merge 51 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
a1ec09c
Support Privilege Grants to Roles and Groups in Materialization
soksamnanglim Oct 4, 2023
a0ca37d
Support Privilege Grants to Roles and Groups in Materialization
soksamnanglim Oct 4, 2023
ca046c9
rectified code with pre-commit
soksamnanglim Oct 4, 2023
2bb40b8
Update CI workflow to include new env vars
soksamnanglim Oct 4, 2023
f464d44
Add fixture to create grants/roles for test purposes
colin-rogers-dbt Oct 9, 2023
7b89a16
Merge branch 'main' into extend_grant_privilege
colin-rogers-dbt Oct 11, 2023
72a2410
Fixed `ImportError` related to misnaming BaseModelGrantsRedshift
soksamnanglim Oct 12, 2023
0f6442e
Remove debugging comment
soksamnanglim Oct 12, 2023
bfc08f6
Change away from inheritance of BaseGrants
soksamnanglim Oct 12, 2023
53cc135
Merge branch 'main' into extend_grant_privilege
colin-rogers-dbt Oct 13, 2023
b546431
clean up duplicate tests
colin-rogers-dbt Oct 13, 2023
87c491d
Merge branch 'dbt-labs:main' into extend_grant_privilege
soksamnanglim Oct 24, 2023
574193c
Merge branch 'main' into extend_grant_privilege
soksamnanglim Oct 24, 2023
0aa8fb1
Split model grants tests into view and table tests
soksamnanglim Oct 25, 2023
c0235ba
Renamed model view and model table test suites
soksamnanglim Oct 25, 2023
bdc6177
added groups and roles to env-setup
soksamnanglim Oct 25, 2023
ead3a2e
fix pre-commit EOF space
soksamnanglim Oct 25, 2023
0d51ab7
Merge branch 'main' into extend_grant_privilege
soksamnanglim Oct 26, 2023
f691812
adding CI_flags override to makefile
soksamnanglim Oct 27, 2023
de7da53
Merge branch 'dbt-labs:main' into extendGrantPrivilege_test
soksamnanglim Oct 27, 2023
3cee6e2
set env vars in conftest.py
colin-rogers-dbt Oct 31, 2023
2d65c09
merge main
colin-rogers-dbt Oct 31, 2023
8ec3461
Merge remote-tracking branch 'soksamnanglim/extendGrantPrivilege_test…
colin-rogers-dbt Oct 31, 2023
016e62c
Merge branch 'main' into extend_grant_privilege
soksamnanglim Oct 31, 2023
fad2ef8
delete breakpoint and add comment to CONTRIBUTING.md
soksamnanglim Oct 31, 2023
343565f
Merge branch 'extendGrantPrivilege' into HEAD
colin-rogers-dbt Oct 31, 2023
aeadcac
fix get_show_grant_sql macro
soksamnanglim Nov 1, 2023
1da43db
Merge branch 'dbt-labs:main' into extend_grant_privilege
soksamnanglim Nov 1, 2023
cee0637
Refactor assert_expected_grants_match_actual for debugging
soksamnanglim Nov 1, 2023
e3bb550
Merge branch 'main' into extend_grant_privilege
colin-rogers-dbt Nov 3, 2023
9fd05c1
change rolename alias to grantee in get_show_grant_sql
colin-rogers-dbt Nov 3, 2023
767749b
Merge branch 'main' into extend_grant_privilege
colin-rogers-dbt Nov 8, 2023
a738720
Merge branch 'main' into extend_grant_privilege
colin-rogers-dbt Nov 9, 2023
082c590
Merge branch 'main' into extend_grant_privilege
soksamnanglim Nov 30, 2023
66782c4
Merge branch 'main' into extend_grant_privilege
soksamnanglim Dec 5, 2023
96fc734
Merge branch 'main' into extend_grant_privilege
colin-rogers-dbt May 6, 2024
4a28c99
Merge branch 'main' into extend_grant_privilege
dbeatty10 Jul 9, 2024
1f2f9e0
Update package due to decoupling of dbt-adapters from dbt-core
dbeatty10 Jul 9, 2024
53956b4
Update body of changelog entry
dbeatty10 Jul 10, 2024
5901cf2
Update whitespace
dbeatty10 Jul 10, 2024
f37d91d
Try printing the environment variables configured in the CI environment
dbeatty10 Jul 10, 2024
220c810
Examine roles in the Redshift database
dbeatty10 Jul 10, 2024
c56c61d
Remove debugging logic
dbeatty10 Jul 10, 2024
00231be
Debug output for setting up groups and roles for CI
dbeatty10 Jul 10, 2024
30043a7
Force creation of groups and roles
dbeatty10 Jul 11, 2024
12d339d
Fix typo
dbeatty10 Jul 11, 2024
3aeff0d
Debugging database roles
dbeatty10 Jul 11, 2024
c8e06f5
Re-run CI tests
dbeatty10 Jul 11, 2024
eb07cfb
Remove debugging
dbeatty10 Jul 11, 2024
655cb53
Add groups and roles for various release workflows in GitHub Actions
dbeatty10 Jul 11, 2024
5247bab
Database groups and roles for testing locally
dbeatty10 Jul 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Features-20231004-103938.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Features
body: 'Add support for specifying groups and roles in grant statements'
time: 2023-10-04T10:39:38.680813-07:00
custom:
Author: soksamnanglim
Issue: "415"
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ To confirm you have correct `dbt-core` and adapter versions installed please run

`dbt-redshift` contains [unit](https://github.com/dbt-labs/dbt-redshift/tree/main/tests/unit) and [functional](https://github.com/dbt-labs/dbt-redshift/tree/main/tests/functional) tests. Functional tests require testing against an actual Redshift warehouse. We have CI set up to test against a Redshift warehouse during PR checks.

In order to run functional tests locally, you will need a `test.env` file in the root of the repository that contains credentials for your Redshift warehouse.
In order to run functional tests locally, you will need a `test.env` file in the root of the repository that contains credentials for your Redshift warehouse. You'll need all the objects provided in `test.env.example` in `test.env` for all the tests to pass.

Note: This `test.env` file is git-ignored, but please be extra careful to never check in credentials or other sensitive information when developing. To create your `test.env` file, copy the provided example file, then supply your relevant credentials.

Expand Down
14 changes: 14 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,5 +1,19 @@
.DEFAULT_GOAL:=help

CI_FLAGS =\
DBT_TEST_USER_1=$(if $(DBT_TEST_USER_1),$(DBT_TEST_USER_1),dbt_test_user_1)\
DBT_TEST_USER_2=$(if $(DBT_TEST_USER_2),$(DBT_TEST_USER_2),dbt_test_user_2)\
DBT_TEST_USER_3=$(if $(DBT_TEST_USER_3),$(DBT_TEST_USER_3),dbt_test_user_3)\
DBT_TEST_GROUP_1=$(if $(DBT_TEST_GROUP_1),$(DBT_TEST_GROUP_1),dbt_test_group_1)\
DBT_TEST_GROUP_2=$(if $(DBT_TEST_GROUP_2),$(DBT_TEST_GROUP_2),dbt_test_group_2)\
DBT_TEST_GROUP_3=$(if $(DBT_TEST_GROUP_3),$(DBT_TEST_GROUP_3),dbt_test_group_3)\
DBT_TEST_ROLE_1=$(if $(DBT_TEST_ROLE_1),$(DBT_TEST_ROLE_1),dbt_test_role_1)\
DBT_TEST_ROLE_2=$(if $(DBT_TEST_ROLE_2),$(DBT_TEST_ROLE_2),dbt_test_role_2)\
DBT_TEST_ROLE_3=$(if $(DBT_TEST_ROLE_3),$(DBT_TEST_ROLE_3),dbt_test_role_3)\
RUSTFLAGS=$(if $(RUSTFLAGS),$(RUSTFLAGS),"-D warnings")\
LOG_DIR=$(if $(LOG_DIR),$(LOG_DIR),./logs)\
DBT_LOG_FORMAT=$(if $(DBT_LOG_FORMAT),$(DBT_LOG_FORMAT),json)

.PHONY: dev
dev: ## Installs adapter in develop mode along with development dependencies
@\
Expand Down
90 changes: 90 additions & 0 deletions dbt/adapters/redshift/impl.py
Original file line number Diff line number Diff line change
Expand Up @@ -187,3 +187,93 @@ def generate_python_submission_response(self, submission_result: Any) -> Adapter
def debug_query(self):
"""Override for DebugTask method"""
self.execute("select 1 as id")

# grant-related methods
@available
def standardize_grants_dict(self, grants_table):
Copy link
Contributor

Choose a reason for hiding this comment

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

"""
Override for standardize_grants_dict
"""
grants_dict = {} # Dict[str, Dict[str, List[str]]]
for row in grants_table:
grantee_type = row["grantee_type"]
grantee = row["grantee"]
privilege = row["privilege_type"]
if privilege not in grants_dict:
grants_dict[privilege] = {}

if grantee_type not in grants_dict[privilege]:
grants_dict[privilege][grantee_type] = []

grants_dict[privilege][grantee_type].append(grantee)

return grants_dict

@available
def diff_of_two_nested_dicts(self, dict_a, dict_b):
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of diff_of_two_dicts.

"""
Given two dictionaries of type Dict[str, Dict[str, List[str]]]:
dict_a = {'key_x': {'key_a': 'VALUE_1'}, 'KEY_Y': {'key_b': value_2'}}
dict_b = {'key_x': {'key_a': 'VALUE_1'}, 'KEY_Y': {'key_b': value_2'}}
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 = {}

for k, v_a in dict_a.items():
if k.casefold() in dict_b:
v_b = dict_b[k.casefold()]

for sub_key, values_a in v_a.items():
if sub_key in v_b:
values_b = v_b[sub_key]
diff_values = [v for v in values_a if v.casefold() not in values_b]
if diff_values:
if k in dict_diff:
dict_diff[k][sub_key] = diff_values
else:
dict_diff[k] = {sub_key: diff_values}
else:
if k in dict_diff:
if values_a:
dict_diff[k][sub_key] = values_a
else:
if values_a:
dict_diff[k] = {sub_key: values_a}
else:
dict_diff[k] = v_a

return dict_diff

@available
def process_grant_dicts(self, unknown_dict):
"""
Given a dictionary where the type can either be of type:
- Dict[str, List[str]]
- Dict[str, List[Dict[str, List[str]]
Return a processed dictionary of the type Dict[str, Dict[str, List[str]]
"""
first_value = next(iter(unknown_dict.values()))
if first_value:
is_dict = isinstance(first_value[0], dict)
else:
is_dict = False

temp = {}
if not is_dict:
for privilege, grantees in unknown_dict.items():
if grantees:
temp[privilege] = {"user": grantees}
else:
for privilege, grantee_map in unknown_dict.items():
grantees_map_temp = {}
for grantee_type, grantees in grantee_map[0].items():
if grantees:
grantees_map_temp[grantee_type] = grantees
if grantees_map_temp:
temp[privilege] = grantees_map_temp

return temp
108 changes: 107 additions & 1 deletion dbt/include/redshift/macros/adapters/apply_grants.sql
Original file line number Diff line number Diff line change
@@ -1,5 +1,42 @@
{% macro redshift__get_show_grant_sql(relation) %}
{# ------- DCL STATEMENT TEMPLATES ------- #}

{%- macro redshift__get_grant_sql(relation, privilege, grantee_dict) -%}
{#-- generates a multiple-grantees grant privilege statement --#}
Copy link
Contributor

Choose a reason for hiding this comment

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

grant {{privilege}} on {{relation}} to
{%- for grantee_type, grantees in grantee_dict.items() -%}
{%- if grantee_type == 'user' and grantees -%}
{{ " " + (grantees | join(', ')) }}
{%- elif grantee_type == 'group' and grantees -%}
{{ " " + ("group " + grantees | join(', group ')) }}
{%- elif grantee_type == 'role' and grantees -%}
{{ " " + ("role " + grantees | join(', role ')) }}
{%- endif -%}
{%- if not loop.last -%}
,
{%- endif -%}
{%- endfor -%}
{%- endmacro -%}

{%- macro redshift__get_revoke_sql(relation, privilege, revokee_dict) -%}
{#-- generates a multiple-grantees revoke privilege statement --#}
Copy link
Contributor

Choose a reason for hiding this comment

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

revoke {{privilege}} on {{relation}} from
{%- for revokee_type, revokees in revokee_dict.items() -%}
{%- if revokee_type == 'user' and revokees -%}
{{ " " + (revokees | join(', ')) }}
{%- elif revokee_type == 'group' and revokees -%}
{{ " " + ("group " + revokees | join(', group ')) }}
{%- elif revokee_type == 'role' and revokees -%}
{{ " " + ("role " + revokees | join(', role ')) }}
{%- endif -%}
{%- if not loop.last -%}
,
{%- endif -%}
{%- endfor -%}
{%- endmacro -%}


{% macro redshift__get_show_grant_sql(relation) %}
{#-- shows the privilege grants on a table for users, groups, and roles --#}
with privileges as (

-- valid options per https://docs.aws.amazon.com/redshift/latest/dg/r_HAS_TABLE_PRIVILEGE.html
Expand All @@ -16,6 +53,7 @@ with privileges as (
)

select
'user' as grantee_type,
u.usename as grantee,
p.privilege_type
from pg_user u
Expand All @@ -24,4 +62,72 @@ where has_table_privilege(u.usename, '{{ relation }}', privilege_type)
and u.usename != current_user
and not u.usesuper

union all
-- check that group has table privilege
select
'group' as grantee_type,
g.groname as grantee,
p.privilege_type
from pg_group g
cross join privileges p
where exists(
select *
from information_schema.table_privileges tp
where tp.grantee=g.groname
and tp.table_schema=replace(split_part('{{ relation }}', '.', 2), '"', '')
and tp.table_name=replace(split_part('{{ relation }}', '.', 3), '"', '')
and LOWER(tp.privilege_type)=p.privilege_type
Comment on lines +77 to +78
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use Relation class attributes (like here and here) rather than splitting on dot separators.

Suggested change
and tp.table_schema=replace(split_part('{{ relation }}', '.', 2), '"', '')
and tp.table_name=replace(split_part('{{ relation }}', '.', 3), '"', '')
and tp.table_schema = '{{ relation.schema }}'
and tp.table_name = '{{ relation.identifier }}'

)

union all
-- check that role has table privilege
select
'role' as grantee_type,
r.role_name as grantee,
p.privilege_type
from svv_roles r
cross join privileges p
Copy link
Contributor

Choose a reason for hiding this comment

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

From pairing with @dbeatty10 - in order to query svv_table_info, this requires:

  • that dbt's user has role <somerole>
  • grant access system table to role <somerole>;

References:

where exists(
select *
from svv_relation_privileges rp
where rp.identity_name=r.role_name
and rp.namespace_name=replace(split_part('{{ relation }}', '.', 2), '"', '')
and rp.relation_name=replace(split_part('{{ relation }}', '.', 3), '"', '')
and LOWER(rp.privilege_type)=p.privilege_type
Comment on lines +94 to +95
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use Relation class attributes (like here and here) rather than splitting on dot separators.

Suggested change
and rp.namespace_name=replace(split_part('{{ relation }}', '.', 2), '"', '')
and rp.relation_name=replace(split_part('{{ relation }}', '.', 3), '"', '')
and rp.namespace_name = '{{ relation.schema }}'
and rp.relation_name = '{{ relation.identifier }}'

)

{% endmacro %}

{% macro redshift__apply_grants(relation, grant_config, should_revoke=True) %}
{#-- Override for apply grants --#}
Copy link
Contributor

Choose a reason for hiding this comment

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

Overriding default__apply_grants.

{#-- If grant_config is {} or None, this is a no-op --#}
{% if grant_config %}
{#-- change grant_config to Dict[str, Dict[str, List[str]] format --#}
{% set grant_config = adapter.process_grant_dicts(grant_config) %}

{% if should_revoke %}
{#-- We think that there is a chance that grants are carried over. --#}
{#-- Show the current grants for users, roles, and groups and calculate the 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 = adapter.diff_of_two_nested_dicts(grant_config, current_grants_dict) %}
{% set needs_revoking = adapter.diff_of_two_nested_dicts(current_grants_dict, grant_config) %}
{% if not (needs_granting or needs_revoking) %}
{{ log('On ' ~ relation ~': All grants are in place, no revocation or granting needed.')}}
{% endif %}
{% else %}
{#-- We don't think there's any chance of previous grants having carried over. --#}
{#-- Jump straight to granting what the user has configured. --#}
{% set needs_revoking = {} %}
{% set needs_granting = grant_config %}
{% endif %}
{% if needs_granting or needs_revoking %}
{% set revoke_statement_list = get_dcl_statement_list(relation, needs_revoking, get_revoke_sql) %}
{% set grant_statement_list = get_dcl_statement_list(relation, needs_granting, get_grant_sql) %}
{% set dcl_statement_list = revoke_statement_list + grant_statement_list %}
{% if dcl_statement_list %}
{{ call_dcl_statements(dcl_statement_list) }}
{% endif %}
{% endif %}
{% endif %}
{% endmacro %}
6 changes: 6 additions & 0 deletions scripts/env-setup.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,9 @@ echo "INTEGRATION_TESTS_SECRETS_PREFIX=REDSHIFT_TEST" >> $GITHUB_ENV
echo "DBT_TEST_USER_1=dbt_test_user_1" >> $GITHUB_ENV
echo "DBT_TEST_USER_2=dbt_test_user_2" >> $GITHUB_ENV
echo "DBT_TEST_USER_3=dbt_test_user_3" >> $GITHUB_ENV
echo "DBT_TEST_GROUP_1=dbt_test_group_1" >> $GITHUB_ENV
echo "DBT_TEST_GROUP_2=dbt_test_group_2" >> $GITHUB_ENV
echo "DBT_TEST_GROUP_3=dbt_test_group_3" >> $GITHUB_ENV
echo "DBT_TEST_ROLE_1=dbt_test_role_1" >> $GITHUB_ENV
echo "DBT_TEST_ROLE_2=dbt_test_role_2" >> $GITHUB_ENV
echo "DBT_TEST_ROLE_3=dbt_test_role_3" >> $GITHUB_ENV
10 changes: 10 additions & 0 deletions test.env.example
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,13 @@ REDSHIFT_TEST_IAM_ROLE_PROFILE=
DBT_TEST_USER_1=dbt_test_user_1
DBT_TEST_USER_2=dbt_test_user_2
DBT_TEST_USER_3=dbt_test_user_3

# Database groups for testing
DBT_TEST_GROUP_1=dbt_test_group_1
DBT_TEST_GROUP_2=dbt_test_group_2
DBT_TEST_GROUP_3=dbt_test_group_3

# Database roles for testing
DBT_TEST_ROLE_1=dbt_test_role_1
DBT_TEST_ROLE_2=dbt_test_role_2
DBT_TEST_ROLE_3=dbt_test_role_3
43 changes: 43 additions & 0 deletions tests/functional/adapter/conftest.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,47 @@
import pytest
import os
dbeatty10 marked this conversation as resolved.
Show resolved Hide resolved

from dbt_common.exceptions import DbtDatabaseError

# This is a hack to prevent the fixture from running more than once
GRANTS_AND_ROLES_SETUP = False

GROUPS = {
"DBT_TEST_GROUP_1": "dbt_test_group_1",
"DBT_TEST_GROUP_2": "dbt_test_group_2",
"DBT_TEST_GROUP_3": "dbt_test_group_3",
}
ROLES = {
"DBT_TEST_ROLE_1": "dbt_test_role_1",
"DBT_TEST_ROLE_2": "dbt_test_role_2",
"DBT_TEST_ROLE_3": "dbt_test_role_3",
}


@pytest.fixture(scope="class", autouse=True)
def setup_grants_and_roles(project):
global GRANTS_AND_ROLES_SETUP
for env_name, env_var in GROUPS.items():
os.environ[env_name] = env_var
for env_name, env_var in ROLES.items():
os.environ[env_name] = env_var
if not GRANTS_AND_ROLES_SETUP:
with project.adapter.connection_named("__test"):
for group in GROUPS.values():
try:
project.adapter.execute(f"CREATE GROUP {group}")
except DbtDatabaseError:
# This is expected if the group already exists
pass

for role in ROLES.values():
try:
project.adapter.execute(f"CREATE ROLE {role}")
except DbtDatabaseError:
# This is expected if the group already exists
pass

GRANTS_AND_ROLES_SETUP = True


@pytest.fixture
Expand Down
Loading
Loading