diff --git a/datadog_sync/model/restriction_policies.py b/datadog_sync/model/restriction_policies.py index f3a7f4c5..10333ab2 100644 --- a/datadog_sync/model/restriction_policies.py +++ b/datadog_sync/model/restriction_policies.py @@ -35,6 +35,13 @@ class RestrictionPolicies(BaseResource): # Additional RestrictionPolicies specific attributes current_user_path: str = "/api/v2/current_user" org_principal: Optional[str] = None + # UUID of the syncing service account, captured in pre_apply_hook from + # /api/v2/current_user. Used by the self-demote pre-filter to skip + # policies that would remove this user's existing editor binding — + # the Datadog API rejects such requests with 400 "users cannot + # decrease their own level of access", producing noisy ERROR events + # in prod telemetry with no actionable signal for operators. + current_user_uuid: Optional[str] = None async def get_resources(self, client: CustomClient) -> List[Dict]: policies = [] @@ -93,6 +100,18 @@ async def import_resource(self, _id: Optional[str] = None, resource: Optional[Di return import_id, resource["data"] async def pre_resource_action_hook(self, _id, resource: Dict) -> None: + # Pre-filter two deterministic failure modes BEFORE the API call so + # they don't surface as ERROR-level events in operator/customer + # telemetry (NATHAN-50). Both checks are best-effort — they bail + # out silently when the state needed to decide is unavailable. + self._skip_if_target_dashboard_is_read_only(_id, resource) + self._skip_if_would_self_demote(_id, resource) + + # Existing behavior: rewrite the source-org "org:" principal + # to the destination-org "org:" so bindings reference the + # local org. Runs AFTER the skip filters so the skip checks see + # the as-imported source-shaped resource and aren't disturbed by + # the in-place principal rewrite. if self.org_principal: for binding in resource["attributes"]["bindings"]: for i, key in enumerate(binding["principals"]): @@ -100,12 +119,149 @@ async def pre_resource_action_hook(self, _id, resource: Dict) -> None: binding["principals"][i] = self.org_principal break + def _skip_if_target_dashboard_is_read_only(self, _id: str, resource: Dict) -> None: + """Skip dashboard-targeted policies when the destination dashboard + is read-only (built-in / template / shared). + + The Datadog restriction-policy API rejects attachment to read-only + dashboards with 403 "This dashboard is read-only". The check uses + the ``is_read_only`` field from the destination dashboard state, + which is populated by the dashboards resource's create/update + response (the API returns the full body including this field even + though it is in ``excluded_attributes`` — that list only affects + diffing, not state storage; see resource_utils.prep_resource). + """ + policy_id = resource.get("id") or _id + if not isinstance(policy_id, str) or not policy_id.startswith("dashboard:"): + # Only dashboards expose ``is_read_only``; other target types + # pass through this branch. + return + + # Resolve the destination dashboard via the source dashboard id + # (the second half of the policy id), keyed in the destination + # state by source id. ``connect_id`` does the same lookup later + # for principal rewriting. + try: + _, src_dashboard_id = policy_id.split(":", 1) + except ValueError: + return + + dashboards_state = self.config.state.destination.get("dashboards", {}) + dashboard = dashboards_state.get(src_dashboard_id) + if not dashboard: + # Dashboard not in destination state — the existing + # connect_id pass will surface a missing-connections error + # if applicable. Don't short-circuit that path here. + return + + if not dashboard.get("is_read_only"): + return + + self.config.logger.info( + f"[restriction_policies - {policy_id}] skipping: " + f"target dashboard {src_dashboard_id} is read-only on destination" + ) + raise SkipResource( + policy_id, + self.resource_type, + f"Target dashboard {src_dashboard_id} is read-only on destination " "(skipped to avoid API 403).", + ) + + def _skip_if_would_self_demote(self, _id: str, resource: Dict) -> None: + """Skip policies that would remove the syncing service account's own + ``editor`` binding. + + The Datadog API rejects such requests with 400 "users cannot + decrease their own level of access (from editor to viewer)". When + the operator has explicitly set ``--allow-self-lockout``, the + request goes through with ``?allow_self_lockout=true`` and the + API permits it — we must NOT pre-filter in that case. + + Scope: this check inspects only **direct** ``user:`` bindings. + We fire the skip only when there is at least one ``user:`` + editor principal AND the syncing SA's UUID is not among them — + that is the unambiguous "removed from editor" case the API + rejects. When the editor bindings are entirely ``role:`` / + ``team:`` / ``org:`` (no direct user bindings at all), we cannot + infer self-demote from the payload alone and let the request + proceed — the API will accept it if the SA retains effective + access via membership. + """ + if not self.current_user_uuid: + # pre_apply_hook didn't capture the uuid — no reliable way to + # decide. Let the request go through and surface the API error + # as it does today. + return + if self.config.allow_self_lockout: + # Operator opted in to bypass the safety check. + return + + sa_user_principal = f"user:{self.current_user_uuid}" + bindings = resource.get("attributes", {}).get("bindings") or [] + if not bindings: + # Empty bindings clears all restrictions — the syncing user + # keeps whatever org-default access they have. Not a + # self-demote case. + return + + # Three signals from the bindings: + # - sa_is_in_editor: SA appears under an editor binding (no self-demote). + # - sa_is_in_viewer: SA appears under a viewer/non-editor binding + # (explicit downgrade — self-demote). + # - has_user_editor_principal: at least one user: appears under an + # editor binding (lets us decide on absence: if user editors exist + # but SA isn't one, SA is being removed → self-demote). + sa_is_in_editor = False + sa_is_in_viewer = False + has_user_editor_principal = False + for binding in bindings: + relation = binding.get("relation") + for principal in binding.get("principals") or []: + if not isinstance(principal, str): + continue + if relation == "editor": + if principal.startswith("user:"): + has_user_editor_principal = True + if principal == sa_user_principal: + sa_is_in_editor = True + elif principal == sa_user_principal: + # SA listed under a non-editor relation (typically + # viewer) — explicit downgrade. + sa_is_in_viewer = True + + if sa_is_in_editor: + # SA is explicitly retained as editor — no self-demote. + return + if not sa_is_in_viewer and not has_user_editor_principal: + # No SA-specific signal and editor bindings are only + # role:/team:/org: principals. SA's effective access may come + # from membership we can't analyze. Let the API decide. + return + + policy_id = resource.get("id") or _id + self.config.logger.info( + f"[restriction_policies - {policy_id}] skipping: " + f"would self-demote syncing user {self.current_user_uuid}" + ) + raise SkipResource( + policy_id, + self.resource_type, + f"Policy would self-demote the syncing user {self.current_user_uuid} " + "(skipped to avoid API 400). Pass --allow-self-lockout to override.", + ) + async def pre_apply_hook(self) -> None: destination_client = self.config.destination_client try: resp = await destination_client.get(self.current_user_path) org_id = resp["data"]["relationships"]["org"]["data"]["id"] self.org_principal = f"org:{org_id}" + # Capture the syncing user's UUID for the self-demote pre-filter. + # ``data.id`` is the user UUID per /api/v2/current_user schema; + # ``data.attributes.uuid`` carries the same value. Fall back to + # the attributes copy if the top-level id is absent for any + # reason (it's been stable in the API for years, but be defensive). + self.current_user_uuid = resp["data"].get("id") or resp["data"].get("attributes", {}).get("uuid") except Exception as e: self.config.logger.error(f"Failed to get org details: {e}") raise diff --git a/tests/integration/resources/test_restriction_policies_skip_paths.py b/tests/integration/resources/test_restriction_policies_skip_paths.py new file mode 100644 index 00000000..cc5c8801 --- /dev/null +++ b/tests/integration/resources/test_restriction_policies_skip_paths.py @@ -0,0 +1,404 @@ +# Unless explicitly stated otherwise all files in this repository are licensed +# under the 3-clause BSD style license (see LICENSE). +# This product includes software developed at Datadog (https://www.datadoghq.com/). +# Copyright 2019 Datadog, Inc. + +""" +Integration-level tests for the restriction_policies skip filter (NATHAN-50). + +The existing VCR-driven ``TestRestrictionPoliciesResources`` cassettes only +contain dashboards with ``is_read_only: false`` and policies whose editor +bindings are limited to ``org:`` / ``team:`` principals — both skip filters +introduced in this PR are therefore unexercised end-to-end by the standard +integration suite. + +These tests close that coverage gap by replaying the apply orchestration +sequence used in production (``pre_resource_action_hook`` → ``connect_resources`` +→ ``create_resource``) against a real :class:`RestrictionPolicies` instance +and real-shaped in-memory ``state.destination`` maps (the surface the +production filter actually reads). Configuration is built from +``MagicMock`` because only ``allow_self_lockout``, ``logger``, +``destination_client`` and ``state`` are exercised; everything else on +``Configuration`` is irrelevant to this skip-filter contract. The only +boundary we mock is the destination HTTP client, which lets us assert +the precise property the production code is supposed to guarantee: + + When the filter raises ``SkipResource``, **no POST is ever issued** to + ``/api/v2/restriction_policy/`` for that resource. + +This shape is preferred over hand-authored VCR cassettes for two reasons: + +1. The existing cassettes are 12K–24K lines each (full dashboard / user / + team / role import dumps); hand-editing them to inject a single + ``is_read_only: true`` toggle is error-prone and easy to drift from the + real API shape. +2. VCR's default exact-match contract on ``method+scheme+host+port+path+query+body`` + means a misshapen cassette would fail with an opaque "no matching + interaction" error rather than the load-bearing assertion we actually + want: "the production filter prevented the POST". + +A future regression in ``RestrictionPolicies.pre_resource_action_hook`` +(e.g. someone refactoring ``BaseResource._apply_resource_cb`` to skip +the hook, or unwiring the call from within the model) would be caught +loudly by ``post_recorder.posts`` being non-empty when these tests assert +it is empty — exactly the regression class the unit tests for the filter +alone cannot detect, because they call the filter directly rather than +through the apply path. +""" + +from __future__ import annotations + +import logging +from copy import deepcopy +from typing import Any, Dict, List, Tuple +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from datadog_sync.model.restriction_policies import RestrictionPolicies +from datadog_sync.utils.resource_utils import SkipResource + +# UUID format reused across tests — chosen so the principal string +# ``user:`` is visually distinct from the ``user:other-...`` strings +# in the test bindings and easy to grep for in failure output. +SA_UUID = "17d29c8a-6285-11f0-be9b-76de006a05ea" +ORG_UUID = "00000000-0000-beef-0000-000000000000" + + +class _PostRecorder: + """Captures every destination-client write call so tests can assert the + skip filter prevented the POST. + + Wraps an :class:`AsyncMock` per HTTP verb. ``posts`` exposes the list of + paths POSTed in the order they happened; assertions compare against this + list rather than mock call-args directly so failure output names the + offending path. + """ + + def __init__(self) -> None: + self.posts: List[Tuple[str, Dict[str, Any]]] = [] + self.puts: List[Tuple[str, Dict[str, Any]]] = [] + self.deletes: List[str] = [] + + async def post(self, path: str, payload: Dict[str, Any], params: Any = None) -> Dict[str, Any]: + self.posts.append((path, payload)) + # Echo back a minimal response shaped like the real + # /api/v2/restriction_policy POST envelope so the apply path + # can store it in destination state without KeyError. + return {"data": payload.get("data", {})} + + async def put(self, path: str, payload: Dict[str, Any], params: Any = None) -> Dict[str, Any]: + self.puts.append((path, payload)) + return {"data": payload.get("data", {})} + + async def delete(self, path: str) -> None: + self.deletes.append(path) + + +def _build_resource( + *, + allow_self_lockout: bool = False, + current_user_uuid: str = SA_UUID, + org_principal: str = f"org:{ORG_UUID}", +) -> Tuple[RestrictionPolicies, _PostRecorder]: + """Build a real RestrictionPolicies wired to in-memory state + a mock client. + + Returns ``(resource, recorder)``. The recorder is attached to + ``resource.config.destination_client`` so writes from + ``create_resource``/``update_resource``/``delete_resource`` land in it. + + ``current_user_uuid`` and ``org_principal`` mirror the values that the + real ``pre_apply_hook`` would set from a ``/api/v2/current_user`` GET — + we set them directly here because ``pre_apply_hook`` has unit-test + coverage (TestRestrictionPoliciesOrgPrincipal) and is not the + behavior under test in these integration paths. + """ + recorder = _PostRecorder() + destination_client = MagicMock() + destination_client.post = AsyncMock(side_effect=recorder.post) + destination_client.put = AsyncMock(side_effect=recorder.put) + destination_client.delete = AsyncMock(side_effect=recorder.delete) + destination_client.get = AsyncMock() + + # The production filter only reads from ``config.state.destination`` and + # ``config.allow_self_lockout`` / ``config.logger``. A MagicMock-based + # config is sufficient — we plug in real-shaped dicts for ``state.*`` + # so connect_resources can rewrite dashboard ids without exploding. + mock_config = MagicMock() + mock_config.allow_self_lockout = allow_self_lockout + mock_config.destination_client = destination_client + mock_config.source_client = MagicMock() + mock_config.skip_failed_resource_connections = True + mock_config.logger = logging.getLogger("restriction_policies_skip_paths_test") + + # State.destination must support both ``.get("dashboards", {})`` (used by + # the skip filter) and dict subscription with autovivification (used by + # connect_resources when missing keys), so back it with a plain dict + # populated per-test with the resource types connect_resources reads. + mock_config.state = MagicMock() + mock_config.state.source = {"restriction_policies": {}} + mock_config.state.destination = { + "restriction_policies": {}, + "dashboards": {}, + "service_level_objectives": {}, + "notebooks": {}, + "users": {}, + "roles": {}, + "teams": {}, + } + + resource = RestrictionPolicies(mock_config) + resource.current_user_uuid = current_user_uuid + resource.org_principal = org_principal + return resource, recorder + + +async def _apply_one(resource: RestrictionPolicies, _id: str, body: Dict[str, Any]) -> str: + """Replay the apply-orchestrator sequence from + ``ResourcesHandler._apply_resource_cb`` for a single resource. + + Returns ``"created"`` on success and ``"skipped"`` when the filter raises + ``SkipResource``. Any other exception propagates so test failures show + real cause rather than a swallowed error. + + Why not call ``_apply_resource_cb`` directly: that path is bound to a + full ``ResourcesHandler`` with workers, counters, sorter, and a Click + runtime — wiring all of that just to observe "did POST happen?" pulls + in dependencies that are themselves test surface. The body of + ``_apply_resource_cb`` is the production wire we care about, and the + sequence below is the verbatim happy-path branch (see + datadog_sync/utils/resources_handler.py L292-L332). A regression that + reordered or removed the ``_pre_resource_action_hook`` call would + break this test the same way it would break ``_apply_resource_cb``. + + Scope limitations vs. the full ``_apply_resource_cb``: + - Only the **create** branch is exercised; the update branch (taken + when ``_id`` is already in destination state) isn't covered here. + Both branches sit AFTER the same ``_pre_resource_action_hook`` call, + so the skip-filter contract under test is identical between them — + a regression in the filter wiring would break both paths equally. + - ``prep_resource`` (which strips ``excluded_attributes``) is skipped. + Safe today because ``RestrictionPolicies.resource_config.excluded_attributes`` + is ``[]``; if that list ever grows, this harness will diverge from + production for fields the filter inspects. + - The per-class ``async_lock`` is not acquired. ``RestrictionPolicies`` + runs with ``concurrent=True`` (its ``ResourceConfig`` doesn't set + ``concurrent=False``), so the lock is never acquired in production + either — equivalent. + """ + # _apply_resource_cb deepcopies the source resource before mutating — + # we mirror that so the bindings/principals rewriting in + # pre_resource_action_hook doesn't leak back into the caller's dict. + working = deepcopy(body) + try: + await resource._pre_resource_action_hook(_id, working) + resource.connect_resources(_id, working) + await resource._create_resource(_id, working) + return "created" + except SkipResource: + return "skipped" + + +def _policy(policy_id: str, bindings: List[Dict[str, Any]]) -> Dict[str, Any]: + """Build a minimal restriction-policy resource body matching the shape + that ``import_resource`` produces (``data.attributes.bindings``). + """ + return { + "id": policy_id, + "type": "restriction_policy", + "attributes": {"bindings": bindings}, + } + + +@pytest.mark.asyncio +class TestRestrictionPoliciesSkipPaths: + """End-to-end coverage for the two skip filters introduced in NATHAN-50. + + Each test wires real ``RestrictionPolicies`` against a mock destination + client and asserts the production ``_pre_resource_action_hook`` → POST + contract that the existing VCR fixtures cannot exercise (their cassettes + have only writable dashboards and org/team editor bindings). + """ + + async def test_read_only_dashboard_target_is_skipped(self): + """Read-only target dashboard → skip; POST never reaches the API. + + Companion writable-dashboard policy in the same run still POSTs, + proving the filter is per-resource rather than a global short-circuit. + """ + resource, recorder = _build_resource() + # Destination state populated as if the dashboards resource had + # already been imported. Source ids are the keys; ``is_read_only`` + # comes from the dashboards API GET response (the dashboards model + # preserves it in state — see datadog_sync/model/dashboards.py + # excluded_attributes affects diffing only, not state storage). + resource.config.state.destination["dashboards"]["src-dash-readonly"] = { + "id": "dst-dash-readonly-uuid", + "is_read_only": True, + } + resource.config.state.destination["dashboards"]["src-dash-writable"] = { + "id": "dst-dash-writable-uuid", + "is_read_only": False, + } + + readonly_policy = _policy( + "dashboard:src-dash-readonly", + [{"relation": "editor", "principals": [f"user:{SA_UUID}", "user:teammate"]}], + ) + writable_policy = _policy( + "dashboard:src-dash-writable", + [{"relation": "editor", "principals": [f"user:{SA_UUID}", "user:teammate"]}], + ) + + # Both go through the same apply sequence; only the read-only one + # should short-circuit before POST. + readonly_outcome = await _apply_one(resource, "dashboard:src-dash-readonly", readonly_policy) + writable_outcome = await _apply_one(resource, "dashboard:src-dash-writable", writable_policy) + + assert readonly_outcome == "skipped" + assert writable_outcome == "created" + + # Core contract: no POST ever hit the read-only path. + skipped_path_fragment = "src-dash-readonly" + offending = [p for p, _ in recorder.posts if skipped_path_fragment in p] + assert offending == [], ( + f"Expected zero POSTs for read-only dashboard policy; got {offending!r}. " + f"This indicates pre_resource_action_hook did not raise SkipResource " + f"as the filter requires." + ) + # Sanity: the writable companion DID POST, so the orchestrator path + # itself isn't broken. POST path uses the *destination* dashboard + # uuid because connect_resources rewrites it before create_resource. + assert any( + "dst-dash-writable-uuid" in p for p, _ in recorder.posts + ), f"Expected the writable-dashboard policy to POST; got paths {[p for p, _ in recorder.posts]!r}" + + async def test_policy_that_would_self_demote_is_skipped(self): + """Editor bindings with no SA UUID → skip; companion policy still syncs.""" + resource, recorder = _build_resource() + resource.config.state.destination["dashboards"]["src-dash-a"] = { + "id": "dst-dash-a-uuid", + "is_read_only": False, + } + resource.config.state.destination["dashboards"]["src-dash-b"] = { + "id": "dst-dash-b-uuid", + "is_read_only": False, + } + + # Policy A: SA UUID absent from editor → self-demote; must skip. + self_demote_policy = _policy( + "dashboard:src-dash-a", + [{"relation": "editor", "principals": ["user:someone-else"]}], + ) + # Policy B: SA UUID included in editor → safe; must sync. + retains_editor_policy = _policy( + "dashboard:src-dash-b", + [{"relation": "editor", "principals": [f"user:{SA_UUID}", "user:someone-else"]}], + ) + + a_outcome = await _apply_one(resource, "dashboard:src-dash-a", self_demote_policy) + b_outcome = await _apply_one(resource, "dashboard:src-dash-b", retains_editor_policy) + + assert a_outcome == "skipped" + assert b_outcome == "created" + + # No POST for the self-demote policy. + offending = [p for p, _ in recorder.posts if "src-dash-a" in p or "dst-dash-a-uuid" in p] + assert offending == [], ( + f"Expected zero POSTs for self-demote policy; got {offending!r}. " + f"_skip_if_would_self_demote did not raise SkipResource." + ) + # The companion policy DID POST against the destination dashboard uuid. + assert any( + "dst-dash-b-uuid" in p for p, _ in recorder.posts + ), f"Expected the editor-retaining policy to POST; got {[p for p, _ in recorder.posts]!r}" + + async def test_self_demote_skip_bypassed_by_allow_self_lockout(self): + """``--allow-self-lockout`` re-enables the POST for self-demote cases. + + Same fixture as the previous test; the only difference is + ``allow_self_lockout=True``. Both policies must POST — the operator + has explicitly opted in to the API-rejection escape hatch. + """ + resource, recorder = _build_resource(allow_self_lockout=True) + resource.config.state.destination["dashboards"]["src-dash-a"] = { + "id": "dst-dash-a-uuid", + "is_read_only": False, + } + resource.config.state.destination["dashboards"]["src-dash-b"] = { + "id": "dst-dash-b-uuid", + "is_read_only": False, + } + + self_demote_policy = _policy( + "dashboard:src-dash-a", + [{"relation": "editor", "principals": ["user:someone-else"]}], + ) + retains_editor_policy = _policy( + "dashboard:src-dash-b", + [{"relation": "editor", "principals": [f"user:{SA_UUID}", "user:someone-else"]}], + ) + + a_outcome = await _apply_one(resource, "dashboard:src-dash-a", self_demote_policy) + b_outcome = await _apply_one(resource, "dashboard:src-dash-b", retains_editor_policy) + + # Bypass: both should POST. + assert a_outcome == "created" + assert b_outcome == "created" + posted_paths = [p for p, _ in recorder.posts] + assert any("dst-dash-a-uuid" in p for p in posted_paths), ( + f"--allow-self-lockout did not bypass the self-demote skip; " f"posted paths {posted_paths!r}" + ) + assert any( + "dst-dash-b-uuid" in p for p in posted_paths + ), f"Editor-retaining companion did not POST; paths {posted_paths!r}" + # Read-only filter is orthogonal to --allow-self-lockout; the + # destinations here are writable so the read-only path remains a + # no-op for both policies — exactly two POSTs expected. + assert ( + len(posted_paths) == 2 + ), f"Expected exactly 2 POSTs with --allow-self-lockout; got {len(posted_paths)}: {posted_paths!r}" + + async def test_skip_filter_is_actually_invoked_by_apply_path(self): + """Regression guard: a no-op ``pre_resource_action_hook`` would let + a read-only policy through and POST would be observed. + + This test patches the production hook to a no-op, replays the same + fixture as ``test_read_only_dashboard_target_is_skipped``, and + asserts the POST DOES happen — proving the original test's pass + depends on the hook firing, not on coincidence (e.g. a bug in + ``_apply_one`` that swallowed all POSTs). + + If a future refactor makes ``_apply_one`` no longer route through + the hook, the original test would silently pass and this guard + would silently fail — together they pin both directions of the + contract. + """ + resource, recorder = _build_resource() + resource.config.state.destination["dashboards"]["src-dash-readonly"] = { + "id": "dst-dash-readonly-uuid", + "is_read_only": True, + } + # Stub the hook so the filter never fires; the fixture is otherwise + # identical to the assertion-positive test above. + + async def _noop(_id, resource_): + return None + + resource.pre_resource_action_hook = _noop + + policy = _policy( + "dashboard:src-dash-readonly", + [{"relation": "editor", "principals": [f"user:{SA_UUID}", "user:teammate"]}], + ) + outcome = await _apply_one(resource, "dashboard:src-dash-readonly", policy) + + assert outcome == "created", ( + "With the hook stubbed out, the apply path must reach create_resource. " + "If this assertion fails, _apply_one has its own short-circuit that " + "would falsely satisfy the skip-path tests above." + ) + assert any("dst-dash-readonly-uuid" in p for p, _ in recorder.posts), ( + "POST never happened even with the hook stubbed — _apply_one or the " + f"mock client wiring is broken. Captured POSTs: {[p for p, _ in recorder.posts]!r}" + ) diff --git a/tests/unit/test_restriction_policies.py b/tests/unit/test_restriction_policies.py index 803e9644..b2d46a25 100644 --- a/tests/unit/test_restriction_policies.py +++ b/tests/unit/test_restriction_policies.py @@ -7,30 +7,52 @@ Unit tests for restriction_policies resource handling. These tests verify that the org: principal remapping in pre_apply_hook and -pre_resource_action_hook handles both success and failure paths correctly. +pre_resource_action_hook handles both success and failure paths correctly, +and that the deterministic skip filter (read-only dashboard targets, and +policies that would self-demote the syncing user) raises SkipResource before +the API call is issued. """ import asyncio +import logging +from collections import defaultdict + import pytest from unittest.mock import AsyncMock, MagicMock from datadog_sync.model.restriction_policies import RestrictionPolicies +from datadog_sync.utils.resource_utils import SkipResource + + +def _make_resource(allow_self_lockout: bool = False): + """Build a RestrictionPolicies instance with a lightweight mock config. + + Mirrors the unit-test pattern from tests/unit/conftest.py::mock_config — + state.source / state.destination are defaultdicts of dicts so resource + types can be indexed without explicit pre-population. + """ + mock_config = MagicMock() + mock_config.state = MagicMock() + mock_config.state.source = defaultdict(dict) + mock_config.state.destination = defaultdict(dict) + mock_config.allow_self_lockout = allow_self_lockout + return RestrictionPolicies(mock_config) class TestRestrictionPoliciesOrgPrincipal: """Test suite for org: principal remapping in restriction_policies.""" - def _make_resource(self): - mock_config = MagicMock() - mock_config.state = MagicMock() - return RestrictionPolicies(mock_config) - def test_pre_apply_hook_sets_org_principal_on_success(self): """Successful GET /api/v2/current_user sets org_principal to 'org:{org_uuid}'.""" - resource = self._make_resource() + resource = _make_resource() mock_client = AsyncMock() mock_client.get = AsyncMock( - return_value={"data": {"relationships": {"org": {"data": {"id": "00000000-0000-beef-0000-000000000000"}}}}} + return_value={ + "data": { + "id": "17d29c8a-6285-11f0-be9b-76de006a05ea", + "relationships": {"org": {"data": {"id": "00000000-0000-beef-0000-000000000000"}}}, + } + } ) resource.config.destination_client = mock_client @@ -38,9 +60,27 @@ def test_pre_apply_hook_sets_org_principal_on_success(self): assert resource.org_principal == "org:00000000-0000-beef-0000-000000000000" + def test_pre_apply_hook_captures_current_user_uuid(self): + """pre_apply_hook also captures the syncing user's UUID for self-demote checks.""" + resource = _make_resource() + mock_client = AsyncMock() + mock_client.get = AsyncMock( + return_value={ + "data": { + "id": "17d29c8a-6285-11f0-be9b-76de006a05ea", + "relationships": {"org": {"data": {"id": "00000000-0000-beef-0000-000000000000"}}}, + } + } + ) + resource.config.destination_client = mock_client + + asyncio.run(resource.pre_apply_hook()) + + assert resource.current_user_uuid == "17d29c8a-6285-11f0-be9b-76de006a05ea" + def test_pre_apply_hook_leaves_org_principal_none_on_failure(self): """Failed GET /api/v2/current_user leaves org_principal as None and raises.""" - resource = self._make_resource() + resource = _make_resource() mock_client = AsyncMock() mock_client.get = AsyncMock(side_effect=Exception("403 Forbidden")) resource.config.destination_client = mock_client @@ -49,29 +89,354 @@ def test_pre_apply_hook_leaves_org_principal_none_on_failure(self): asyncio.run(resource.pre_apply_hook()) assert resource.org_principal is None + assert resource.current_user_uuid is None def test_pre_resource_action_hook_replaces_org_when_principal_set(self): """When org_principal is set, org: entries in bindings are replaced.""" - resource = self._make_resource() + resource = _make_resource() resource.org_principal = "org:dest-pub-id" r = { - "attributes": {"bindings": [{"principals": ["org:source-pub-id", "user:some-user"], "relation": "editor"}]} + "id": "dashboard:src-dash", + "attributes": {"bindings": [{"principals": ["org:source-pub-id", "user:some-user"], "relation": "editor"}]}, } - asyncio.run(resource.pre_resource_action_hook("some-id", r)) + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", r)) assert r["attributes"]["bindings"][0]["principals"][0] == "org:dest-pub-id" assert r["attributes"]["bindings"][0]["principals"][1] == "user:some-user" def test_pre_resource_action_hook_skips_when_org_principal_none(self): """When org_principal is None, org: principals are left unchanged.""" - resource = self._make_resource() + resource = _make_resource() assert resource.org_principal is None r = { - "attributes": {"bindings": [{"principals": ["org:source-pub-id", "user:some-user"], "relation": "editor"}]} + "id": "dashboard:src-dash", + "attributes": {"bindings": [{"principals": ["org:source-pub-id", "user:some-user"], "relation": "editor"}]}, } - asyncio.run(resource.pre_resource_action_hook("some-id", r)) + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", r)) assert r["attributes"]["bindings"][0]["principals"][0] == "org:source-pub-id" assert r["attributes"]["bindings"][0]["principals"][1] == "user:some-user" + + +class TestRestrictionPoliciesSkipFilter: + """Test suite for the deterministic skip filter in pre_resource_action_hook. + + Two failure modes are skipped at the sync-cli level rather than forwarded + to the API as guaranteed-failing requests: + + 1. Target dashboard is read-only on the destination (template / built-in / + shared) — the API rejects restriction-policy attachment with 403. + 2. The policy would remove the syncing service account's own ``editor`` + binding, demoting itself to ``viewer`` or no access — the API rejects + with 400 "users cannot decrease their own level of access". + """ + + # ------------------------------------------------------------------ + # Read-only dashboard skip + # ------------------------------------------------------------------ + def test_skips_when_target_dashboard_is_read_only(self): + resource = _make_resource() + # Destination dashboard state keyed by source dashboard id. + # ``is_read_only`` is the actual Datadog dashboards API field + # (see datadog_sync/model/dashboards.py — it appears in excluded_attributes). + resource.config.state.destination["dashboards"]["src-dash"] = { + "id": "dst-dash-uuid", + "is_read_only": True, + } + policy = { + "id": "dashboard:src-dash", + "attributes": {"bindings": [{"relation": "editor", "principals": ["user:some-user"]}]}, + } + + with pytest.raises(SkipResource) as exc_info: + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", policy)) + + assert "read-only" in str(exc_info.value).lower() + assert "dashboard:src-dash" in str(exc_info.value) + + def test_does_not_skip_when_target_dashboard_is_writable(self): + resource = _make_resource() + resource.config.state.destination["dashboards"]["src-dash"] = { + "id": "dst-dash-uuid", + "is_read_only": False, + } + policy = { + "id": "dashboard:src-dash", + "attributes": {"bindings": [{"relation": "editor", "principals": ["user:some-user"]}]}, + } + + # Should not raise — writable dashboards pass through. + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", policy)) + + def test_does_not_skip_when_target_dashboard_missing_read_only_attr(self): + """Absent ``is_read_only`` (missing key) is treated as not-read-only — pass through. + + Treating missing-as-writable is intentional: the API is the source of + truth for read-only state, and a missing field would result in the API + returning a 403 (which is exactly the noisy event we want to skip) only + when the dashboard IS read-only. If the field is missing for any reason + we let the request go through rather than mask a writable dashboard. + """ + resource = _make_resource() + resource.config.state.destination["dashboards"]["src-dash"] = {"id": "dst-dash-uuid"} + policy = { + "id": "dashboard:src-dash", + "attributes": {"bindings": [{"relation": "editor", "principals": ["user:some-user"]}]}, + } + + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", policy)) + + def test_does_not_skip_non_dashboard_targets(self): + """SLO and notebook policy targets are not subject to the read-only check. + + Only dashboards have the ``is_read_only`` flag; the prod 403 noise comes + from built-in/template dashboards specifically. Other target types + pass through this branch unconditionally. + """ + resource = _make_resource() + policy = { + "id": "slo:src-slo", + "attributes": {"bindings": [{"relation": "editor", "principals": ["user:some-user"]}]}, + } + + # Should not raise — non-dashboard targets are not gated. + asyncio.run(resource.pre_resource_action_hook("slo:src-slo", policy)) + + def test_does_not_skip_when_target_dashboard_not_in_destination_state(self): + """Dashboard absent from destination state — the connect_id path will + report missing-connections and the resource will be skipped via the + ResourceConnectionError path instead. The read-only branch must not + trip on missing entries. + """ + resource = _make_resource() + # Empty destination dashboards mapping — dashboard not (yet) created. + policy = { + "id": "dashboard:src-dash", + "attributes": {"bindings": [{"relation": "editor", "principals": ["user:some-user"]}]}, + } + + # Should not raise SkipResource — must fall through cleanly so the + # downstream missing-connections cascade handles it. + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", policy)) + + # ------------------------------------------------------------------ + # Self-demote skip + # ------------------------------------------------------------------ + def test_skips_when_policy_would_self_demote_syncing_user(self): + """Syncing user has editor on destination but the new policy gives them + only viewer (different relation) — the API rejects with 400; pre-filter.""" + resource = _make_resource() + sa_uuid = "17d29c8a-6285-11f0-be9b-76de006a05ea" + resource.current_user_uuid = sa_uuid + policy = { + "id": "dashboard:src-dash", + "attributes": { + "bindings": [ + # Syncing user moved to viewer — would self-demote. + {"relation": "viewer", "principals": [f"user:{sa_uuid}"]}, + ] + }, + } + + with pytest.raises(SkipResource) as exc_info: + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", policy)) + + assert "self-demote" in str(exc_info.value).lower() + assert sa_uuid in str(exc_info.value) + + def test_skips_when_policy_omits_syncing_user_from_editor(self): + """Syncing user not listed in any binding at all — they would lose + their editor binding (effectively self-demote to viewer). API rejects.""" + resource = _make_resource() + sa_uuid = "17d29c8a-6285-11f0-be9b-76de006a05ea" + resource.current_user_uuid = sa_uuid + policy = { + "id": "dashboard:src-dash", + "attributes": { + "bindings": [ + {"relation": "editor", "principals": ["user:other-user"]}, + ] + }, + } + + with pytest.raises(SkipResource) as exc_info: + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", policy)) + + assert "self-demote" in str(exc_info.value).lower() + + def test_does_not_skip_when_syncing_user_keeps_editor(self): + """Syncing user listed under the editor binding — no self-demote.""" + resource = _make_resource() + sa_uuid = "17d29c8a-6285-11f0-be9b-76de006a05ea" + resource.current_user_uuid = sa_uuid + policy = { + "id": "dashboard:src-dash", + "attributes": { + "bindings": [ + {"relation": "editor", "principals": [f"user:{sa_uuid}", "user:other"]}, + ] + }, + } + + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", policy)) + + def test_does_not_skip_self_demote_when_allow_self_lockout_is_true(self): + """When operator explicitly opts in via --allow-self-lockout, the + sync-cli sends the request with ?allow_self_lockout=true; the API + accepts it. We must not pre-filter in that case — the operator is + intentionally taking the action.""" + resource = _make_resource(allow_self_lockout=True) + sa_uuid = "17d29c8a-6285-11f0-be9b-76de006a05ea" + resource.current_user_uuid = sa_uuid + policy = { + "id": "dashboard:src-dash", + "attributes": { + "bindings": [ + {"relation": "viewer", "principals": [f"user:{sa_uuid}"]}, + ] + }, + } + + # Should NOT raise — allow_self_lockout means the operator opted in. + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", policy)) + + def test_does_not_skip_self_demote_when_current_user_uuid_unknown(self): + """If pre_apply_hook didn't capture the uuid (e.g. legacy in-process + state with org_principal pre-set), don't second-guess — let the API + return the existing error rather than mistakenly skip valid policies.""" + resource = _make_resource() + resource.current_user_uuid = None + policy = { + "id": "dashboard:src-dash", + "attributes": { + "bindings": [ + {"relation": "viewer", "principals": ["user:someone-else"]}, + ] + }, + } + + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", policy)) + + def test_does_not_skip_when_policy_has_no_bindings(self): + """Empty bindings clears all restrictions — the syncing user retains + whatever org-default access they have. Not a self-demote case for the + purposes of this filter.""" + resource = _make_resource() + resource.current_user_uuid = "17d29c8a-6285-11f0-be9b-76de006a05ea" + policy = { + "id": "dashboard:src-dash", + "attributes": {"bindings": []}, + } + + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", policy)) + + def test_does_not_skip_when_only_org_team_role_editor_bindings_present(self): + """Editor bindings are entirely org:/team:/role: principals — no + direct user: editor binding exists, so we cannot infer self-demote + from the payload alone. The SA's effective access may come from a + membership; let the API decide. Matches the integration-fixture + case where policies grant editor to org/team principals only.""" + resource = _make_resource() + sa_uuid = "17d29c8a-6285-11f0-be9b-76de006a05ea" + resource.current_user_uuid = sa_uuid + policy = { + "id": "dashboard:src-dash", + "attributes": { + "bindings": [ + { + "relation": "editor", + "principals": [ + "org:30187db5-8582-11ef-969b-8248c7cda362", + "team:d19a4fc2-aeda-4b9e-856a-b9e48c0e19fa", + "role:f0cc21b6-6f38-49f0-8641-e25fb3b98476", + ], + }, + ] + }, + } + + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", policy)) + + # ------------------------------------------------------------------ + # Integration: hook prevents the API call + # ------------------------------------------------------------------ + def test_passes_through_when_target_is_writable_and_no_self_demote(self): + """Normal case: writable dashboard, syncing user retains editor. + Hook returns normally; downstream code calls the API.""" + resource = _make_resource() + sa_uuid = "17d29c8a-6285-11f0-be9b-76de006a05ea" + resource.current_user_uuid = sa_uuid + resource.config.state.destination["dashboards"]["src-dash"] = { + "id": "dst-dash-uuid", + "is_read_only": False, + } + policy = { + "id": "dashboard:src-dash", + "attributes": { + "bindings": [ + {"relation": "editor", "principals": [f"user:{sa_uuid}", "user:other"]}, + ] + }, + } + + # Returns None (no skip) — proceed to API call. + result = asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", policy)) + assert result is None + + # ------------------------------------------------------------------ + # Logging + # ------------------------------------------------------------------ + def test_logs_include_target_id_and_reason_for_read_only_skip(self, caplog): + """Log line for the read-only skip must include the policy id and a + machine-parsable reason marker so operators can dashboards/alert on it.""" + resource = _make_resource() + # Route the config logger to caplog's standard-library logger so the + # message text is captured regardless of how the MagicMock-wrapped + # logger forwards calls. + real_logger = logging.getLogger("datadog_sync.test.restriction_policies.readonly") + resource.config.logger = real_logger + resource.config.state.destination["dashboards"]["src-dash"] = { + "id": "dst-dash-uuid", + "is_read_only": True, + } + policy = { + "id": "dashboard:src-dash", + "attributes": {"bindings": [{"relation": "editor", "principals": ["user:some-user"]}]}, + } + + with caplog.at_level(logging.INFO, logger=real_logger.name): + with pytest.raises(SkipResource): + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", policy)) + + joined = "\n".join(rec.getMessage() for rec in caplog.records) + assert "dashboard:src-dash" in joined + assert "read-only" in joined.lower() + assert "restriction_policies" in joined or "restriction policies" in joined.lower() + + def test_logs_include_target_id_and_reason_for_self_demote_skip(self, caplog): + """Log line for the self-demote skip must include the policy id and + the syncing user uuid so operators can correlate with the API error.""" + resource = _make_resource() + sa_uuid = "17d29c8a-6285-11f0-be9b-76de006a05ea" + resource.current_user_uuid = sa_uuid + real_logger = logging.getLogger("datadog_sync.test.restriction_policies.selfdemote") + resource.config.logger = real_logger + policy = { + "id": "dashboard:src-dash", + "attributes": { + "bindings": [ + {"relation": "viewer", "principals": [f"user:{sa_uuid}"]}, + ] + }, + } + + with caplog.at_level(logging.INFO, logger=real_logger.name): + with pytest.raises(SkipResource): + asyncio.run(resource.pre_resource_action_hook("dashboard:src-dash", policy)) + + joined = "\n".join(rec.getMessage() for rec in caplog.records) + assert "dashboard:src-dash" in joined + assert sa_uuid in joined + assert "self-demote" in joined.lower()