From cac90bc28d3761f04f064749815747beee478ed8 Mon Sep 17 00:00:00 2001 From: Ben Knight Date: Mon, 3 Aug 2026 18:47:38 +0000 Subject: [PATCH] feat: add `denies` config so object-level DENY survives a rebuild MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A SQL Server object-level DENY is stored against object_id, so dbt discards it on every drop-and-recreate (every run for a view). The DENY is the only way to carve an exception out of a schema-level GRANT, so losing it silently leaves a fail-open posture: the broad grant persists, its exceptions evaporate, and no run reports it. Add a model-level `denies` config, shaped like `grants` ({privilege: [principals]}), that re-applies object-level DENYs after each build, diffed against sys.database_permissions — mirroring how `masks` re-applies Dynamic Data Masking. Emits DENY for configured-not-present and REVOKE for present-not-configured; a converged, persisted relation issues no DDL. - Pure resolve/diff logic in sqlserver_deny.py (unit-tested), wired via @available resolve_denies / deny_changes and a `denies` config registered with MergeBehavior.Update. - apply_denies macros; call sites in table/incremental/snapshot and, new vs masks, view — a view is a valid securable and is recreated every run, where a DENY is lost most often. - Absent principal is warned-and-skipped; an unsupported privilege is warned-and-skipped too (never fails the run); grant∩deny overlap warns. - No-op on non-SQL-Server adapters. Existing grants/masks unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 1 + README.md | 17 ++ dbt/adapters/sqlserver/sqlserver_adapter.py | 48 +++ dbt/adapters/sqlserver/sqlserver_configs.py | 8 + dbt/adapters/sqlserver/sqlserver_deny.py | 141 +++++++++ .../macros/adapters/apply_denies.sql | 116 +++++++ .../models/incremental/incremental.sql | 5 + .../materializations/models/table/table.sql | 6 + .../materializations/models/view/view.sql | 7 + .../materializations/snapshots/snapshot.sql | 4 + tests/functional/adapter/mssql/test_denies.py | 289 ++++++++++++++++++ tests/unit/adapters/mssql/test_deny.py | 140 +++++++++ 12 files changed, 782 insertions(+) create mode 100644 dbt/adapters/sqlserver/sqlserver_deny.py create mode 100644 dbt/include/sqlserver/macros/adapters/apply_denies.sql create mode 100644 tests/functional/adapter/mssql/test_denies.py create mode 100644 tests/unit/adapters/mssql/test_deny.py diff --git a/CHANGELOG.md b/CHANGELOG.md index e658860b..50a87e12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ - **Behavior change:** `dbt_sqlserver_use_dbt_transactions` now defaults to `True`: dbt-managed transaction hooks (begin/commit) emit real `BEGIN TRANSACTION` / `COMMIT TRANSACTION` T-SQL instead of no-ops, so a failed model rolls back its own statements instead of leaving a partial result behind on autocommit. The `False` (legacy autocommit) behavior is deprecated and will be removed in a future release. - **Behavior change:** `dbt_sqlserver_use_native_string_types` now defaults to `True`: `STRING` maps to `VARCHAR(MAX)`, `NCHAR` to `NCHAR(1)`, and `NVARCHAR` to `NVARCHAR(4000)`, instead of the legacy `VARCHAR(8000)`/`CHAR(1)` mappings. The `False` (legacy) behavior is deprecated and will be removed in a future release. - Add an experimental `adbc` backend (`backend: adbc`), an alternative to `pyodbc`/`mssql-python` built on [ADBC](https://arrow.apache.org/adbc/) that talks to SQL Server via `go-mssqldb` instead of an ODBC/DB-API bridge. Install with `dbt-sqlserver[adbc]` plus the separate `dbc` CLI-installed driver binary; supports SQL Server (user/password) authentication only for now. See [docs/adbc_backend.md](docs/adbc_backend.md). [#771](https://github.com/dbt-msft/dbt-sqlserver/issues/771) +- Add a model-level `denies` config that re-applies object-level `DENY` permissions after each build, diffed against `sys.database_permissions`, so an object DENY survives dbt's drop-and-recreate. A SQL Server object DENY is the only way to carve an exception out of a schema-level GRANT, but it is stored against `object_id` and was silently discarded on every rebuild (every run for a view), leaving a fail-open posture. Shaped like `grants` (`{privilege: [principals]}`); covers `table`, `view`, `incremental` and `snapshot`; emits `DENY`/`REVOKE` only for what changed; warns-and-skips an absent principal; and is a no-op on other adapters. Mirrors the existing `masks` re-application. See the README for details. #### Bugfixes diff --git a/README.md b/README.md index faf037b8..d40fc632 100644 --- a/README.md +++ b/README.md @@ -304,6 +304,23 @@ Behaviour: **Version notes.** All masking DDL the adapter emits (`ADD MASKED`, `MASKED WITH`, `DROP MASKED`) and the functions `default()`, `email()`, `random(a,b)` and `partial(...)` work on 2016+. The `datetime()` partial-date function and granular column/schema/table-scoped `UNMASK` are SQL Server 2022+ only; the adapter never emits them, but mask-function strings are passed through verbatim, so using a 2022-only function on an older server will be rejected by SQL Server. +### Object-level DENY (`denies`) + +In SQL Server, an object-level `DENY` is the only way to carve an exception out of a schema-level `GRANT` — grant a principal `SELECT` on a whole schema, then `DENY SELECT` on the individual PII-bearing models. But a `DENY` is stored against the object's `object_id`, so dbt destroys it every time it drops and recreates the relation (which is *every* run for a view). The schema grant survives; its exceptions silently evaporate, leaving a **fail-open** posture that no run reports. + +The `denies` config re-applies object-level DENYs after each build, diffed against `sys.database_permissions`, the same way `masks` re-applies Dynamic Data Masking. Its shape mirrors `grants` — a `{privilege: [principals]}` map — and it is settable in the in-file `{{ config() }}`, the model's `.yml` `config:` block, or a directory-wide default in `dbt_project.yml` (merging key-wise across those levels, like `masks`): + +```sql +{{ config(denies={'select': ['Restricted_Read_Only']}) }} +``` + +- **Survives rebuilds:** the DENY is present after `dbt run` and *still present after the next run*, for `table`, `view`, `incremental` (append and full-refresh) and `snapshot`. Unlike masks, this **includes views** — a view is a valid securable and is recreated on every run, which is where a DENY is lost most often. +- **Reconciled:** the adapter diffs the config against the live DENY state and emits only what changed — `DENY` for configured-not-present, `REVOKE` for present-not-configured (a `REVOKE` removes a `DENY`). Removing a principal from `denies` revokes its DENY on the next run; a converged, persisted relation issues no DDL. +- **Scope:** object-level DENY for the table privileges `select`, `insert`, `update`, `delete`, `references`. An unsupported privilege is warned and skipped (the warning surfaces the likely typo without taking down the build). Column-, database- and server-scoped permissions are out of scope (the latter survive a rebuild already). +- **Absent principal:** a `DENY` to a non-existent database principal is warned-and-skipped (the run still succeeds), so one config runs unchanged across dev, CI and prod where the principal set differs. +- **Ordering vs `grants`:** grants are applied first, then denies, so the final state is unambiguous. A `(privilege, principal)` pair appearing in both `grants` and `denies` is warned as a likely mistake (a DENY overrides a GRANT in SQL Server). +- **Other adapters:** `denies` is a no-op on non-SQL-Server adapters, not an error. This is SQL-Server-specific by design — `DENY` is absent from the SQL standard and does not port. + ## Contributing [![Unit tests](https://github.com/dbt-msft/dbt-sqlserver/actions/workflows/unit-tests.yml/badge.svg)](https://github.com/dbt-msft/dbt-sqlserver/actions/workflows/unit-tests.yml) diff --git a/dbt/adapters/sqlserver/sqlserver_adapter.py b/dbt/adapters/sqlserver/sqlserver_adapter.py index a5a63d9d..f470343b 100644 --- a/dbt/adapters/sqlserver/sqlserver_adapter.py +++ b/dbt/adapters/sqlserver/sqlserver_adapter.py @@ -30,6 +30,8 @@ from dbt.adapters.sqlserver.sqlserver_column import SQLServerColumn, SQLServerColumnNative from dbt.adapters.sqlserver.sqlserver_configs import SQLServerConfigs from dbt.adapters.sqlserver.sqlserver_connections import SQLServerConnectionManager +from dbt.adapters.sqlserver.sqlserver_deny import deny_changes as _deny_changes +from dbt.adapters.sqlserver.sqlserver_deny import resolve_denies as _resolve_denies from dbt.adapters.sqlserver.sqlserver_mask import ColumnMask from dbt.adapters.sqlserver.sqlserver_mask import mask_changes as _mask_changes from dbt.adapters.sqlserver.sqlserver_mask import resolve_masks as _resolve_masks @@ -663,6 +665,52 @@ def mask_changes( existing_columns=(list(existing_columns) if existing_columns is not None else None), ) + @available + def resolve_denies(self, model: Any, model_denies: Optional[dict] = None) -> Dict[str, list]: + """Normalise the model-level `denies` config into a `{privilege: + [principals]}` map for `apply_denies`. + + `model` is the Jinja `model` dict (`node.to_dict()`); its `grants` config + (under `model['config']`) is read only to warn when a principal is both + granted and denied the same privilege. `model_denies` is + `config.get('denies')`, already surface-merged by dbt. + + Unsupported privileges (anything other than the object-level table + privileges) are warned and skipped rather than failing the run — the + warning surfaces the likely typo without taking down the build. + """ + model = model or {} + grant_config = (model.get("config") or {}).get("grants") + model_name = model.get("name") or model.get("alias") or "" + resolved, warnings, unsupported = _resolve_denies(model_denies, grant_config, model_name) + for warning in warnings: + logger.warning(warning) + if unsupported: + from dbt.adapters.sqlserver.sqlserver_deny import SUPPORTED_PRIVILEGES + + logger.warning( + f"On model '{model_name}', the `denies` config lists unsupported " + f"privilege(s): {', '.join(sorted(unsupported))}; skipping them. " + f"Object-level DENY is supported only for the table privileges: " + f"{', '.join(SUPPORTED_PRIVILEGES)}." + ) + return resolved + + @available + def deny_changes(self, existing_denies: Any, deny_config: Optional[dict]) -> dict: + """Diff a resolved deny map against current `sys.database_permissions`. + + `existing_denies` is the agate table from `get_show_deny_sql` (columns + `grantee`, `privilege_type`). Returns plain lists for jinja: `denies` and + `revokes`, each a list of `[privilege, principal]` pairs. The macro emits + `DENY` for the former and `REVOKE` for the latter.""" + rows = [] + if existing_denies is not None: + column_names = existing_denies.column_names + for row in existing_denies.rows: + rows.append(dict(zip(column_names, row))) + return _deny_changes(rows, deny_config or {}) + COLUMNS_EQUAL_SQL = """ with diff_count as ( diff --git a/dbt/adapters/sqlserver/sqlserver_configs.py b/dbt/adapters/sqlserver/sqlserver_configs.py index ceb8fce8..b749deac 100644 --- a/dbt/adapters/sqlserver/sqlserver_configs.py +++ b/dbt/adapters/sqlserver/sqlserver_configs.py @@ -25,3 +25,11 @@ class SQLServerConfigs(AdapterConfig): masks: Optional[Dict[str, Any]] = field( default_factory=dict, metadata=MergeBehavior.Update.meta() ) + # privilege -> [principals] map for the model-level `denies` surface, shaped + # like `grants`. Re-applied after each build because an object-level DENY is + # stored against object_id and discarded on drop-and-recreate. Same key-wise + # MergeBehavior.Update as `masks`, so a directory-level default and a + # per-model tweak combine instead of one clobbering the whole dict. + denies: Optional[Dict[str, Any]] = field( + default_factory=dict, metadata=MergeBehavior.Update.meta() + ) diff --git a/dbt/adapters/sqlserver/sqlserver_deny.py b/dbt/adapters/sqlserver/sqlserver_deny.py new file mode 100644 index 00000000..febf8813 --- /dev/null +++ b/dbt/adapters/sqlserver/sqlserver_deny.py @@ -0,0 +1,141 @@ +"""Pure resolution + diff logic for object-level ``DENY`` permissions. + +Kept free of any database or dbt-context dependency so it can be unit tested +in isolation, mirroring ``sqlserver_mask.py``. The adapter's ``@available`` +wrappers extract plain data from the model / ``sys.database_permissions`` and +delegate here; the Jinja ``sqlserver__apply_denies`` macro turns the diff into +DDL. + +A SQL Server object-level ``DENY`` is stored against ``object_id``, so dbt +destroys it every time it drops and recreates the relation. The ``denies:`` +config re-applies it after materialization, diffed against the live state, the +same way ``masks:`` re-applies Dynamic Data Masking. + +One config surface feeds a ``{privilege: [principals]}`` map: + +* the model-level ``denies`` dict, shaped exactly like ``grants``. + +Privilege and principal identifiers are compared case-insensitively, matching +SQL Server's default collation. The user's original spelling is the one emitted +in DDL. +""" + +from typing import Dict, List, Optional, Sequence, Tuple + +# Object-level, table-scoped privileges that a DENY can carry. Column-level +# DENY (minor_id > 0) and database/server-scoped permissions are out of scope: +# the schema-grant-with-object-exceptions pattern only needs these. +SUPPORTED_PRIVILEGES = ("select", "insert", "update", "delete", "references") + + +def _normalize(name: str) -> str: + return name.strip().lower() + + +def _dedupe_ci(principals: Sequence[str]) -> List[str]: + """Drop case-insensitive duplicates, preserving first-seen spelling/order.""" + seen: Dict[str, str] = {} + for principal in principals or []: + key = _normalize(principal) + if key and key not in seen: + seen[key] = principal + return list(seen.values()) + + +def _normalize_grant_map(grant_config: Optional[Dict[str, Sequence[str]]]) -> Dict[str, set]: + """``{privilege: [grantee]}`` -> ``{privilege_lower: {grantee_lower}}``.""" + result: Dict[str, set] = {} + for privilege, grantees in (grant_config or {}).items(): + result.setdefault(_normalize(privilege), set()).update( + _normalize(g) for g in (grantees or []) + ) + return result + + +def resolve_denies( + deny_config: Optional[Dict[str, Sequence[str]]], + grant_config: Optional[Dict[str, Sequence[str]]], + model_name: str, +) -> Tuple[Dict[str, List[str]], List[str], List[str]]: + """Normalise the ``denies`` config into a clean ``{privilege: [principals]}``. + + ``deny_config`` is already surface-merged by dbt across ``dbt_project.yml`` / + ``.yml`` / in-file ``config()``. This: + + * lower-cases privilege names and case-insensitively de-duplicates the + principal list under each (preserving the first spelling seen); + * collects any privilege that is **not** an object-level table privilege into + ``unsupported`` so the caller can fail loudly — a silently-skipped DENY is + the fail-open regression this feature exists to prevent; + * warns when a ``(privilege, principal)`` pair appears in **both** ``denies`` + and ``grants``, which would emit contradictory DDL and is almost certainly + a mistake. + + Returns ``(resolved, warnings, unsupported)``. + """ + warnings: List[str] = [] + unsupported: List[str] = [] + resolved: Dict[str, List[str]] = {} + + for privilege, principals in (deny_config or {}).items(): + norm_priv = _normalize(privilege) + if norm_priv not in SUPPORTED_PRIVILEGES: + unsupported.append(privilege) + continue + deduped = _dedupe_ci(principals) + if not deduped: + continue + # Merge case-variant privilege keys ("select" and "SELECT") into one. + existing = resolved.get(norm_priv, []) + resolved[norm_priv] = _dedupe_ci(existing + deduped) + + grant_map = _normalize_grant_map(grant_config) + for privilege, principals in resolved.items(): + granted = grant_map.get(privilege, set()) + for principal in principals: + if _normalize(principal) in granted: + warnings.append( + f"On model '{model_name}', principal '{principal}' is both granted " + f"and denied '{privilege}'. A DENY overrides a GRANT in SQL Server; " + f"remove the principal from one of `grants` or `denies` to avoid " + f"contradictory permissions." + ) + + return resolved, warnings, unsupported + + +def deny_changes( + existing_denies: Sequence[Dict[str, str]], + desired: Dict[str, Sequence[str]], +) -> Dict[str, List[Tuple[str, str]]]: + """Diff the desired deny map against current ``sys.database_permissions``. + + ``existing_denies`` is a sequence of ``{"grantee", "privilege_type"}`` rows + (state ``DENY``, class 1, minor_id 0). ``desired`` is the resolved + ``{privilege: [principals]}`` map. + + Returns lists of ``(privilege, principal)`` pairs keyed: + + * ``denies`` – configured but not yet present (emit ``DENY``), keeping the + config's spelling; + * ``revokes`` – present but no longer configured (emit ``REVOKE``, which + removes a DENY as well as a GRANT), keeping the database's spelling. + + Both dimensions are compared case-insensitively on ``(privilege, principal)``. + A converged relation returns two empty lists. + """ + existing_pairs: Dict[Tuple[str, str], Tuple[str, str]] = {} + for row in existing_denies: + privilege = row["privilege_type"] + principal = row["grantee"] + existing_pairs[(_normalize(privilege), _normalize(principal))] = (privilege, principal) + + desired_pairs: Dict[Tuple[str, str], Tuple[str, str]] = {} + for privilege, principals in (desired or {}).items(): + for principal in principals: + desired_pairs[(_normalize(privilege), _normalize(principal))] = (privilege, principal) + + denies = [pair for key, pair in desired_pairs.items() if key not in existing_pairs] + revokes = [pair for key, pair in existing_pairs.items() if key not in desired_pairs] + + return {"denies": denies, "revokes": revokes} diff --git a/dbt/include/sqlserver/macros/adapters/apply_denies.sql b/dbt/include/sqlserver/macros/adapters/apply_denies.sql new file mode 100644 index 00000000..7843eea2 --- /dev/null +++ b/dbt/include/sqlserver/macros/adapters/apply_denies.sql @@ -0,0 +1,116 @@ +{#- + Object-level DENY permissions, modelled on apply_grants / apply_masks. + + Post-materialization step: (re)apply the object-level DENYs a model declares + so they survive dbt's drop-and-recreate on every build. A DENY is stored in + sys.database_permissions against the relation's object_id, so a rebuild + (which mints a new object_id) silently discards it — the schema-GRANT it was + carved out of survives, leaving a fail-open posture. This reads the live DENY + state, diffs it against the resolved config, and emits only the changes: + DENY for configured-not-present, REVOKE for present-not-configured (REVOKE + removes a DENY as well as a GRANT). + + A no-op when nothing is configured and on adapters other than SQL Server. + Unlike apply_masks there is NO relation.type guard — a view is a valid + securable and, being recreated on every run, is where a DENY is lost most + often. + + Config surface (normalised + validated in adapter.resolve_denies): + * model-level `denies` dict, shaped exactly like `grants`. +-#} + +{% macro apply_denies(relation, deny_config, should_revoke=True) %} + {{ return(adapter.dispatch('apply_denies', 'dbt')(relation, deny_config, should_revoke)) }} +{% endmacro %} + +{#- Non-SQL-Server adapters are unaffected. -#} +{% macro default__apply_denies(relation, deny_config, should_revoke=True) %}{% endmacro %} + + +{% macro get_show_deny_sql(relation) %} + {{ return(adapter.dispatch('get_show_deny_sql', 'dbt')(relation)) }} +{% endmacro %} + +{% macro default__get_show_deny_sql(relation) %} + {{ return('') }} +{% endmacro %} + +{#- Live object-level (minor_id = 0), DENY-state permissions on the relation. -#} +{% macro sqlserver__get_show_deny_sql(relation) %} + select + pr.name as grantee, + dp.permission_name as privilege_type + from sys.database_permissions dp {{ information_schema_hints() }} + join sys.database_principals pr {{ information_schema_hints() }} + on pr.principal_id = dp.grantee_principal_id + where dp.class = 1 + and dp.major_id = OBJECT_ID('{{ escape_single_quotes(relation.include(database=False)) }}') + and dp.minor_id = 0 + and dp.state_desc = 'DENY' +{% endmacro %} + + +{#- Lower-cased names of every database principal, for the existence guard. -#} +{% macro sqlserver__get_existing_principals() %} + {% call statement('get_existing_principals', fetch_result=True) %} + select name from sys.database_principals {{ information_schema_hints() }} + {% endcall %} + {% set result = [] %} + {% for row in load_result('get_existing_principals').table.rows %} + {% do result.append(row[0] | trim | lower) %} + {% endfor %} + {{ return(result) }} +{% endmacro %} + + +{% macro sqlserver__apply_denies(relation, deny_config, should_revoke=True) %} + {#-- If deny_config is {} or None, this is a no-op (mirrors apply_grants). --#} + {% if not deny_config %} + {{ return(none) }} + {% endif %} + + {#-- Reconcile against live state. On a fresh build there is nothing to + revoke, so skip the round-trip and treat every configured deny as an + add — exactly apply_grants' should_revoke shortcut. --#} + {% if should_revoke %} + {% set existing_denies = run_query(get_show_deny_sql(relation)) %} + {% else %} + {% set existing_denies = none %} + {% endif %} + + {% set changes = adapter.deny_changes(existing_denies, deny_config) %} + + {#-- A DENY to a non-existent principal errors, and the principal set differs + across dev / CI / prod, so warn-and-skip rather than fail — one config + then runs unchanged everywhere. Revoked denies reference principals that + necessarily still exist. --#} + {% set existing_principals = sqlserver__get_existing_principals() %} + + {#-- Revoke first, then deny (order is immaterial — a (privilege, principal) + pair is only ever on one side of the diff). --#} + {% set statements = [] %} + {% for privilege, principal in changes['revokes'] %} + {% do statements.append( + "revoke " ~ privilege ~ " on " ~ relation ~ " from [" + ~ (principal | replace(']', ']]')) ~ "]") %} + {% endfor %} + {% for privilege, principal in changes['denies'] %} + {% if (principal | trim | lower) in existing_principals %} + {% do statements.append( + "deny " ~ privilege ~ " on " ~ relation ~ " to [" + ~ (principal | replace(']', ']]')) ~ "]") %} + {% else %} + {% do exceptions.warn("apply_denies on " ~ relation ~ ": database principal '" + ~ principal ~ "' does not exist; skipping DENY " ~ privilege ~ ". " + ~ "Create the principal or remove it from `denies`.") %} + {% endif %} + {% endfor %} + + {% if statements %} + {% do run_query(statements | join(";\n")) %} + {% do log("Applied " ~ statements | length ~ " deny change(s) on " + ~ relation, info=true) %} + {% else %} + {% do log("On " ~ relation ~ ": all denies are in place, no changes needed.") %} + {% endif %} +{% endmacro %} diff --git a/dbt/include/sqlserver/macros/materializations/models/incremental/incremental.sql b/dbt/include/sqlserver/macros/materializations/models/incremental/incremental.sql index d3c10736..174f42f5 100644 --- a/dbt/include/sqlserver/macros/materializations/models/incremental/incremental.sql +++ b/dbt/include/sqlserver/macros/materializations/models/incremental/incremental.sql @@ -165,6 +165,11 @@ {% set should_revoke = should_revoke(existing_relation, full_refresh_mode) %} {% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %} + {#-- Re-apply object-level DENYs after grants (covers the append and + full-refresh paths alike). --#} + {% set deny_config = adapter.resolve_denies(model, config.get('denies')) %} + {% do apply_denies(target_relation, deny_config, should_revoke=should_revoke) %} + {% do persist_docs(target_relation, model) %} {% set mask_config = adapter.resolve_masks(model, config.get('masks')) %} diff --git a/dbt/include/sqlserver/macros/materializations/models/table/table.sql b/dbt/include/sqlserver/macros/materializations/models/table/table.sql index 5a58bba7..fc29a5c0 100644 --- a/dbt/include/sqlserver/macros/materializations/models/table/table.sql +++ b/dbt/include/sqlserver/macros/materializations/models/table/table.sql @@ -116,6 +116,12 @@ {% set should_revoke = should_revoke(existing_relation, full_refresh_mode=True) %} {% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %} + {#-- Re-apply object-level DENYs after grants, so the final permission state is + unambiguous. Runs on the common tail, so it covers the rename, prebuilt and + DML-refresh build paths alike. --#} + {% set deny_config = adapter.resolve_denies(model, config.get('denies')) %} + {% do apply_denies(target_relation, deny_config, should_revoke=should_revoke) %} + {% do persist_docs(target_relation, model) %} -- `COMMIT` happens here diff --git a/dbt/include/sqlserver/macros/materializations/models/view/view.sql b/dbt/include/sqlserver/macros/materializations/models/view/view.sql index 3eea0c2d..ce9fe854 100644 --- a/dbt/include/sqlserver/macros/materializations/models/view/view.sql +++ b/dbt/include/sqlserver/macros/materializations/models/view/view.sql @@ -94,6 +94,13 @@ {% do apply_grants(target_relation, preserved_grants, should_revoke=False) %} {% endif %} + {#-- Re-apply object-level DENYs after grants. A view is a valid securable and + is recreated on every run, so this is where an object-level DENY is lost + most often — unlike apply_masks, which is absent here (DDM attaches to + base-table columns only). --#} + {% set deny_config = adapter.resolve_denies(model, config.get('denies')) %} + {% do apply_denies(target_relation, deny_config, should_revoke=should_revoke) %} + {% do persist_docs(target_relation, model) %} {{ run_hooks(post_hooks, inside_transaction=True) }} diff --git a/dbt/include/sqlserver/macros/materializations/snapshots/snapshot.sql b/dbt/include/sqlserver/macros/materializations/snapshots/snapshot.sql index 9aa6095d..13e01f2f 100644 --- a/dbt/include/sqlserver/macros/materializations/snapshots/snapshot.sql +++ b/dbt/include/sqlserver/macros/materializations/snapshots/snapshot.sql @@ -107,6 +107,10 @@ {% set should_revoke = should_revoke(target_relation_exists, full_refresh_mode=False) %} {% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %} + {#-- Re-apply object-level DENYs after grants. --#} + {% set deny_config = adapter.resolve_denies(model, config.get('denies')) %} + {% do apply_denies(target_relation, deny_config, should_revoke=should_revoke) %} + {% do persist_docs(target_relation, model) %} {% set mask_config = adapter.resolve_masks(model, config.get('masks')) %} diff --git a/tests/functional/adapter/mssql/test_denies.py b/tests/functional/adapter/mssql/test_denies.py new file mode 100644 index 00000000..10c80665 --- /dev/null +++ b/tests/functional/adapter/mssql/test_denies.py @@ -0,0 +1,289 @@ +"""Functional tests for object-level ``DENY`` permissions (the ``denies`` config). + +A SQL Server object-level ``DENY`` is stored against ``object_id``, so dbt +discards it on every drop-and-recreate — the regression this config closes. These +tests exercise that a declared DENY is present in ``sys.database_permissions`` +after a build and, crucially, **still present after a second build**, across the +table / view / incremental / snapshot materializations; that removing a principal +revokes it; that a converged model emits no DDL; that an absent principal warns +and skips without failing; and that the DENY is actually enforced against a +principal that holds a schema-level GRANT. +""" + +import pytest + +from dbt.tests.util import get_connection, run_dbt, run_dbt_and_capture + +# A login-less database user used as the deny target across the suite. +DENY_PRINCIPAL = "dbt_deny_reader" + + +# --------------------------------------------------------------------------- +# helpers +# --------------------------------------------------------------------------- + + +def denied_permissions(project, object_name, schema=None): + """Return {(PRIVILEGE, principal)} of object-level DENYs on the relation.""" + schema = schema or project.test_schema + sql = f""" + select dp.permission_name, pr.name + from sys.database_permissions dp + join sys.database_principals pr + on pr.principal_id = dp.grantee_principal_id + where dp.class = 1 + and dp.major_id = OBJECT_ID('"{schema}"."{object_name}"') + and dp.minor_id = 0 + and dp.state_desc = 'DENY' + """ + with get_connection(project.adapter): + _, table = project.adapter.execute(sql, fetch=True) + return {(row[0].upper(), row[1]) for row in table.rows} + + +def exec_sql(project, sql): + with get_connection(project.adapter): + project.adapter.execute(sql) + + +@pytest.fixture(scope="class", autouse=True) +def deny_principal(project): + """Create the login-less user the models deny, and drop it afterwards.""" + exec_sql( + project, + f"if database_principal_id('{DENY_PRINCIPAL}') is null " + f"create user {DENY_PRINCIPAL} without login;", + ) + yield DENY_PRINCIPAL + exec_sql( + project, + f"if database_principal_id('{DENY_PRINCIPAL}') is not null drop user {DENY_PRINCIPAL};", + ) + + +# --------------------------------------------------------------------------- +# table materialization +# --------------------------------------------------------------------------- + +table_deny_sql = """ +{{ config(materialized="table", denies={"select": ["dbt_deny_reader"]}) }} +select 1 as id, cast('secret' as varchar(50)) as ssn +""" + + +class TestTableDenies: + @pytest.fixture(scope="class") + def models(self): + return {"denied_table.sql": table_deny_sql} + + def test_deny_applied_and_survives_rebuild(self, project): + run_dbt(["run"]) + assert ("SELECT", DENY_PRINCIPAL) in denied_permissions(project, "denied_table") + + # the regression: a full refresh drops & recreates the table (new + # object_id), so the DENY must be re-applied or it silently vanishes + run_dbt(["run", "--full-refresh"]) + assert ("SELECT", DENY_PRINCIPAL) in denied_permissions(project, "denied_table") + + def test_deny_is_enforced_against_a_schema_grant(self, project): + """The pattern this exists for: a broad schema GRANT, carved out by an + object DENY. The denied principal must not be able to read the table.""" + run_dbt(["run"]) + exec_sql(project, f"grant select on schema::{project.test_schema} to {DENY_PRINCIPAL};") + + blocked = False + try: + with get_connection(project.adapter): + project.adapter.execute( + f"execute as user = '{DENY_PRINCIPAL}';" + f"select id from {project.test_schema}.denied_table;" + f"revert;" + ) + except Exception: + blocked = True + finally: + # make sure the impersonation context is not left open on the conn + try: + exec_sql(project, "revert;") + except Exception: + pass + assert blocked, "the denied principal was able to SELECT despite the DENY" + + +# --------------------------------------------------------------------------- +# view materialization — the case that matters most: a view is recreated on +# every run, so an object-level DENY is lost most often here. +# --------------------------------------------------------------------------- + +view_deny_sql = """ +{{ config(materialized="view", denies={"select": ["dbt_deny_reader"]}) }} +select 1 as id, cast('secret' as varchar(50)) as ssn +""" + + +class TestViewDenies: + @pytest.fixture(scope="class") + def models(self): + return {"denied_view.sql": view_deny_sql} + + def test_deny_survives_ordinary_view_rebuild(self, project): + run_dbt(["run"]) + assert ("SELECT", DENY_PRINCIPAL) in denied_permissions(project, "denied_view") + + # an ordinary view->view rebuild (no --full-refresh) still recreates the + # object; the DENY must survive it + run_dbt(["run"]) + assert ("SELECT", DENY_PRINCIPAL) in denied_permissions(project, "denied_view") + + +# --------------------------------------------------------------------------- +# incremental materialization — append and full-refresh paths +# --------------------------------------------------------------------------- + +incremental_deny_sql = """ +{{ config(materialized="incremental", denies={"select": ["dbt_deny_reader"]}) }} +select 1 as id +{% if is_incremental() %}where 1 = 0{% endif %} +""" + + +class TestIncrementalDenies: + @pytest.fixture(scope="class") + def models(self): + return {"denied_incremental.sql": incremental_deny_sql} + + def test_deny_survives_append_and_full_refresh(self, project): + run_dbt(["run"]) + assert ("SELECT", DENY_PRINCIPAL) in denied_permissions(project, "denied_incremental") + + # append path (existing table kept in place). The object_id is stable, so + # the DENY is already present and the run converges: no DDL, no-change + # message (logged at debug, as apply_masks does). + _, out = run_dbt_and_capture(["--debug", "run"]) + assert ("SELECT", DENY_PRINCIPAL) in denied_permissions(project, "denied_incremental") + assert "all denies are in place, no changes needed" in out + assert "deny change(s) on" not in out + + # full-refresh path (drop & recreate — DENY must be re-applied) + run_dbt(["run", "--full-refresh"]) + assert ("SELECT", DENY_PRINCIPAL) in denied_permissions(project, "denied_incremental") + + +# --------------------------------------------------------------------------- +# snapshot materialization +# --------------------------------------------------------------------------- + +snapshot_source_sql = """ +{{ config(materialized="table") }} +select 1 as id, cast('Smith' as varchar(50)) as surname +""" + +denied_snapshot_sql = """ +{% snapshot denied_snapshot %} +{{ config( + unique_key='id', + strategy='check', + check_cols=['surname'], + denies={'select': ['dbt_deny_reader']} +) }} +select * from {{ ref('snap_source') }} +{% endsnapshot %} +""" + + +class TestSnapshotDenies: + @pytest.fixture(scope="class") + def models(self): + return {"snap_source.sql": snapshot_source_sql} + + @pytest.fixture(scope="class") + def snapshots(self): + return {"denied_snapshot.sql": denied_snapshot_sql} + + def test_deny_applied_and_survives_second_snapshot(self, project): + run_dbt(["run"]) + run_dbt(["snapshot"]) + assert ("SELECT", DENY_PRINCIPAL) in denied_permissions(project, "denied_snapshot") + + run_dbt(["snapshot"]) + assert ("SELECT", DENY_PRINCIPAL) in denied_permissions(project, "denied_snapshot") + + +# --------------------------------------------------------------------------- +# revocation: a DENY present but no longer configured is revoked +# --------------------------------------------------------------------------- + + +class TestDenyRevocation: + @pytest.fixture(scope="class") + def models(self): + return {"denied_table.sql": table_deny_sql} + + def test_unconfigured_deny_is_revoked(self, project): + run_dbt(["run"]) + # a DENY the config does not declare (INSERT), added out of band + exec_sql( + project, + f"deny insert on {project.test_schema}.denied_table to {DENY_PRINCIPAL};", + ) + assert ("INSERT", DENY_PRINCIPAL) in denied_permissions(project, "denied_table") + + # next run reconciles: the configured SELECT stays, the stray INSERT goes + run_dbt(["run"]) + denies = denied_permissions(project, "denied_table") + assert ("SELECT", DENY_PRINCIPAL) in denies + assert ("INSERT", DENY_PRINCIPAL) not in denies + + +# --------------------------------------------------------------------------- +# absent principal: warn and skip, run still succeeds +# --------------------------------------------------------------------------- + +absent_principal_deny_sql = """ +{{ config(materialized="table", denies={"select": ["nonexistent_principal_xyz"]}) }} +select 1 as id +""" + + +class TestAbsentPrincipal: + @pytest.fixture(scope="class") + def models(self): + return {"absent_deny.sql": absent_principal_deny_sql} + + def test_absent_principal_warns_and_run_succeeds(self, project): + results, out = run_dbt_and_capture(["run"]) + assert len(results) == 1 + assert results[0].status == "success" + assert "nonexistent_principal_xyz" in out + assert "does not exist" in out + # nothing was denied, but the build did not fail + assert denied_permissions(project, "absent_deny") == set() + + +# --------------------------------------------------------------------------- +# unsupported privilege: warn and skip rather than taking down the run +# --------------------------------------------------------------------------- + +unsupported_privilege_sql = """ +{{ config( + materialized="table", + denies={"execute": ["dbt_deny_reader"], "select": ["dbt_deny_reader"]} +) }} +select 1 as id +""" + + +class TestUnsupportedPrivilege: + @pytest.fixture(scope="class") + def models(self): + return {"bad_priv.sql": unsupported_privilege_sql} + + def test_unsupported_privilege_warns_and_run_succeeds(self, project): + results, out = run_dbt_and_capture(["run"]) + assert len(results) == 1 + assert results[0].status == "success" + assert "execute" in out.lower() + # the unsupported privilege is skipped; the supported one is still applied + denies = denied_permissions(project, "bad_priv") + assert ("SELECT", DENY_PRINCIPAL) in denies + assert ("EXECUTE", DENY_PRINCIPAL) not in denies diff --git a/tests/unit/adapters/mssql/test_deny.py b/tests/unit/adapters/mssql/test_deny.py new file mode 100644 index 00000000..c6d00430 --- /dev/null +++ b/tests/unit/adapters/mssql/test_deny.py @@ -0,0 +1,140 @@ +"""Unit tests for the pure deny resolution + diff logic. + +These exercise the normalise / dedupe / grant-conflict rules and the +desired-vs-current diff without needing a database connection, mirroring +tests/unit/adapters/mssql/test_mask.py. +""" + +from dbt.adapters.sqlserver.sqlserver_deny import ( + SUPPORTED_PRIVILEGES, + deny_changes, + resolve_denies, +) + +# --------------------------------------------------------------------------- +# resolve_denies: normalise the config into one {privilege: [principals]} map +# --------------------------------------------------------------------------- + + +def test_resolve_basic(): + resolved, warnings, unsupported = resolve_denies( + {"select": ["Restricted_Read_Only"]}, + grant_config=None, + model_name="stg_patient", + ) + assert resolved == {"select": ["Restricted_Read_Only"]} + assert warnings == [] + assert unsupported == [] + + +def test_resolve_empty_config(): + resolved, warnings, unsupported = resolve_denies(None, None, "m") + assert resolved == {} + assert warnings == [] + assert unsupported == [] + + +def test_resolve_lowercases_privilege(): + resolved, _, _ = resolve_denies({"SELECT": ["A"]}, None, "m") + assert resolved == {"select": ["A"]} + + +def test_resolve_dedupes_principals_case_insensitively(): + resolved, _, _ = resolve_denies({"select": ["Reader", "reader", "READER"]}, None, "m") + # first spelling preserved, duplicates dropped + assert resolved == {"select": ["Reader"]} + + +def test_resolve_merges_case_variant_privilege_keys(): + resolved, _, _ = resolve_denies({"select": ["A"], "SELECT": ["B"]}, None, "m") + assert resolved == {"select": ["A", "B"]} + + +def test_resolve_drops_empty_principal_list(): + resolved, _, _ = resolve_denies({"select": []}, None, "m") + assert resolved == {} + + +def test_resolve_unsupported_privilege_collected_not_skipped_silently(): + resolved, _, unsupported = resolve_denies( + {"execute": ["A"], "control": ["B"], "select": ["C"]}, None, "m" + ) + assert resolved == {"select": ["C"]} + assert sorted(unsupported) == ["control", "execute"] + + +def test_resolve_all_supported_privileges_accepted(): + cfg = {priv: ["A"] for priv in SUPPORTED_PRIVILEGES} + resolved, _, unsupported = resolve_denies(cfg, None, "m") + assert set(resolved) == set(SUPPORTED_PRIVILEGES) + assert unsupported == [] + + +def test_resolve_warns_on_grant_deny_conflict(): + _, warnings, _ = resolve_denies( + {"select": ["Reader"]}, + grant_config={"select": ["reader"]}, # case-insensitive match + model_name="core_patients", + ) + assert len(warnings) == 1 + w = warnings[0] + assert "core_patients" in w and "Reader" in w and "select" in w + + +def test_resolve_no_conflict_when_different_privilege(): + _, warnings, _ = resolve_denies( + {"select": ["Reader"]}, + grant_config={"insert": ["Reader"]}, + model_name="m", + ) + assert warnings == [] + + +# --------------------------------------------------------------------------- +# deny_changes: diff desired against live sys.database_permissions state +# --------------------------------------------------------------------------- + + +def rows(*pairs): + return [{"privilege_type": p, "grantee": g} for p, g in pairs] + + +def test_changes_first_build_all_denies(): + ch = deny_changes([], {"select": ["Reader"], "insert": ["Writer"]}) + assert sorted(ch["denies"]) == [("insert", "Writer"), ("select", "Reader")] + assert ch["revokes"] == [] + + +def test_changes_converged_is_noop(): + existing = rows(("SELECT", "Reader")) + ch = deny_changes(existing, {"select": ["Reader"]}) + assert ch["denies"] == [] + assert ch["revokes"] == [] + + +def test_changes_converged_case_insensitive(): + existing = rows(("SELECT", "READER")) + ch = deny_changes(existing, {"SELECT": ["reader"]}) + assert ch["denies"] == [] + assert ch["revokes"] == [] + + +def test_changes_revoke_when_removed_from_config(): + existing = rows(("SELECT", "Reader"), ("INSERT", "OldUser")) + ch = deny_changes(existing, {"select": ["Reader"]}) + assert ch["denies"] == [] + # revoke keeps the DB's spelling + assert ch["revokes"] == [("INSERT", "OldUser")] + + +def test_changes_add_and_revoke_together(): + existing = rows(("SELECT", "Reader")) + ch = deny_changes(existing, {"update": ["NewUser"]}) + assert ch["denies"] == [("update", "NewUser")] + assert ch["revokes"] == [("SELECT", "Reader")] + + +def test_changes_deny_keeps_config_spelling(): + # nothing existing; the emitted DENY uses the principal exactly as configured + ch = deny_changes([], {"select": ["DOMAIN\\Report_Readers"]}) + assert ch["denies"] == [("select", "DOMAIN\\Report_Readers")]