diff --git a/datadog_sync/commands/shared/options.py b/datadog_sync/commands/shared/options.py index a519b6ec..1de6ea08 100644 --- a/datadog_sync/commands/shared/options.py +++ b/datadog_sync/commands/shared/options.py @@ -591,6 +591,18 @@ def click_config_file_provider(ctx: Context, opts: CustomOptionClass, value: Non help="Allow self-lockout when syncing restriction policies.", cls=CustomOptionClass, ), + option( + "--use-v1-user-api", + required=False, + envvar=constants.DD_USE_V1_USER_API, + type=bool, + default=False, + show_default=True, + help="Create users via the v1 API (/api/v1/user) instead of v2. v2 cannot set a " + "handle (it derives one from the email), which collides when users share an email; " + "v1 accepts an explicit handle so each user keeps its own. Off by default.", + cls=CustomOptionClass, + ), ] diff --git a/datadog_sync/constants.py b/datadog_sync/constants.py index d62d8485..6c8fefd5 100644 --- a/datadog_sync/constants.py +++ b/datadog_sync/constants.py @@ -29,6 +29,7 @@ DD_SHOW_PROGRESS_BAR = "DD_SHOW_PROGRESS_BAR" DD_VERIFY_SSL_CERTIFICATES = "DD_VERIFY_SSL_CERTIFICATES" DD_ALLOW_PARTIAL_PERMISSIONS_ROLES = "DD_ALLOW_PARTIAL_PERMISSIONS_ROLES" +DD_USE_V1_USER_API = "DD_USE_V1_USER_API" DD_SYNC_JSON = "DD_SYNC_JSON" DD_DATADOG_HOST_OVERRIDE = "DD_DATADOG_HOST_OVERRIDE" diff --git a/datadog_sync/model/users.py b/datadog_sync/model/users.py index 5b4e5a79..52403428 100644 --- a/datadog_sync/model/users.py +++ b/datadog_sync/model/users.py @@ -30,7 +30,11 @@ class Users(BaseResource): "attributes.status", "attributes.verified", "attributes.service_account", - "attributes.handle", + # NOTE: attributes.handle is deliberately NOT excluded here. It is the + # user mapping key (resource_mapping_key below) and the payload for the + # v1 user creation, so it must survive prep_resource. It is popped + # manually before the v2 POST/PATCH (v2 treats handle as read-only) and + # kept out of update diffs via deep_diff_config.exclude_regex_paths below. "attributes.icon", "attributes.modified_at", "attributes.mfa_enabled", @@ -40,7 +44,11 @@ class Users(BaseResource): "relationships.org", "relationships.team_roles", ], - resource_mapping_key="attributes.email", + resource_mapping_key="attributes.handle", + # Handle is read-only in v2 and is popped from the create/update payload; + # exclude it from update diffs so a source-vs-destination handle difference + # never drives a spurious PATCH of a field v2 will not accept. + deep_diff_config={"ignore_order": True, "exclude_regex_paths": [r".*\['handle'\]"]}, ) # Additional Users specific attributes pagination_config = PaginationConfig( @@ -80,12 +88,63 @@ async def create_resource(self, _id: str, resource: Dict) -> Tuple[str, Dict]: self.config.state.destination[self.resource_type][_id] = self._existing_resources_map[key] return await self.update_resource(_id, resource) + attributes = resource["attributes"] + if self.config.use_v1_user_api: + # v2 create cannot set a handle (it derives one from the email), which + # collapses distinct-handle users that share an email onto one handle. + # v1 accepts an explicit handle, so each user keeps its own. + return _id, await self._create_via_v1( + attributes.get("handle"), attributes.get("name"), attributes.get("email") + ) + destination_client = self.config.destination_client - resource["attributes"].pop("disabled", None) + # handle is read-only in v2 (derived from email) and must not be sent. + attributes.pop("disabled", None) + attributes.pop("handle", None) resp = await destination_client.post(self.resource_config.base_path, {"data": resource}) - return _id, resp["data"] + async def _create_via_v1(self, handle: Optional[str], name: Optional[str], email: Optional[str]) -> Dict: + """Create the user via the v1 API, which accepts an explicit handle. + + v2 ``POST /api/v2/users`` cannot set a handle (it is derived from the + email), so users that share an email collapse onto one handle and later + creates 409 — and the v2 "winner" is created with the wrong handle. v1 + ``POST /api/v1/user`` accepts a distinct handle. The v1 response is the + legacy user shape, so re-resolve the v2 UUID by handle and return that + record — state and downstream references (roles, team_memberships) key + on the v2 UUID. + """ + if not handle: + raise ValueError("v1 user creation requires a handle") + + destination_client = self.config.destination_client + await destination_client.post("/api/v1/user", {"handle": handle, "name": name, "email": email}) + + user = await self._get_destination_user_by_handle(handle) + if user is None: + raise ValueError("v1-created user not found by handle after create") + return user + + async def _get_destination_user_by_handle(self, handle: str) -> Optional[Dict]: + """Return the destination user whose handle matches exactly, or None. + + Transient HTTP errors are already retried by the client's + ``request_with_retry``; this adds a small, sleep-free re-query loop only + to absorb read-after-write visibility lag after a v1 create. + """ + destination_client = self.config.destination_client + for _ in range(3): + resp = await destination_client.paginated_request(destination_client.get)( + self.resource_config.base_path, + pagination_config=self.pagination_config, + params={"filter": handle}, + ) + for user in resp: + if user.get("attributes", {}).get("handle") == handle: + return user + return None + async def update_resource(self, _id: str, resource: Dict) -> Tuple[str, Dict]: destination_client = self.config.destination_client @@ -94,6 +153,7 @@ async def update_resource(self, _id: str, resource: Dict) -> Tuple[str, Dict]: await self.update_user_roles(self.config.state.destination[self.resource_type][_id]["id"], diff) resource["id"] = self.config.state.destination[self.resource_type][_id]["id"] resource.pop("relationships", None) + resource["attributes"].pop("handle", None) resp = await destination_client.patch( self.resource_config.base_path + f"/{self.config.state.destination[self.resource_type][_id]['id']}", {"data": resource}, diff --git a/datadog_sync/utils/configuration.py b/datadog_sync/utils/configuration.py index 75589760..41b39128 100644 --- a/datadog_sync/utils/configuration.py +++ b/datadog_sync/utils/configuration.py @@ -109,6 +109,7 @@ class Configuration(object): max_workers_per_type: Dict[str, int] = field(default_factory=dict) command: str = "" allow_partial_permissions_roles: List[str] = field(default_factory=list) + use_v1_user_api: bool = False resources: Dict[str, BaseResource] = field(default_factory=dict) resources_arg: List[str] = field(default_factory=list) # --id-file: id-targeted import via stdin or file payload. @@ -435,6 +436,7 @@ def build_config(cmd: Command, **kwargs: Optional[Any]) -> Configuration: max_workers_per_type = _parse_max_workers_per_type(max_workers_per_type_raw, known_resource_types) create_global_downtime = kwargs.get("create_global_downtime") validate = kwargs.get("validate") + use_v1_user_api = kwargs.get("use_v1_user_api") or False verify_ddr_status = kwargs.get("verify_ddr_status") backup_before_reset = not kwargs.get("do_not_backup") show_progress_bar = kwargs.get("show_progress_bar") @@ -691,6 +693,7 @@ def build_config(cmd: Command, **kwargs: Optional[Any]) -> Configuration: emit_json=emit_json, command=cmd.value, allow_partial_permissions_roles=allow_partial_permissions_roles, + use_v1_user_api=use_v1_user_api, id_payload=id_payload, max_concurrent_reads=max_concurrent_reads, transient_failure_threshold_pct=transient_failure_threshold_pct, diff --git a/setup.cfg b/setup.cfg index 0b06fe83..e9bc0462 100644 --- a/setup.cfg +++ b/setup.cfg @@ -56,6 +56,7 @@ tests = black==24.3.0 pytest==8.1.1 pytest-black + pytest-cov pytest-console-scripts pytest-recording==0.13.2 pytest-retry==1.7.0 diff --git a/tests/unit/test_users.py b/tests/unit/test_users.py new file mode 100644 index 00000000..8a74a1db --- /dev/null +++ b/tests/unit/test_users.py @@ -0,0 +1,334 @@ +# 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. + +"""Tests for handle-keyed users + the --use-v1-user-api flag. + +The Datadog destination enforces user uniqueness on the ``handle``, not the +``email`` — multiple users may share an email. Keying ``_existing_resources_map`` +by email collapsed distinct-handle users onto one derived handle and caused a +409 Conflict on the second create. These tests cover switching the user mapping +key to the exact-case handle (PR1) and wiring the opt-in --use-v1-user-api flag. + +Tests ``a``/``a2``/``a3``/``f`` are red against ``resource_mapping_key= +"attributes.email"`` and green after the switch to ``"attributes.handle"`` plus +the manual handle pop before the v2 POST. Test ``g`` is a green/green guard that +handle stays excluded from update diffs across the excluded_attributes -> +exclude_regex_paths migration. ``p1``/``p2``/``p3`` guard the flag wiring. +""" + +import asyncio +from unittest.mock import AsyncMock, MagicMock + +import pytest +from click.testing import CliRunner + +from datadog_sync.cli import cli +from datadog_sync.constants import Command +from datadog_sync.model.users import Users +from datadog_sync.utils.configuration import build_config +from datadog_sync.utils.resource_utils import CustomClientHTTPError, check_diff + + +def _http_error(status): + """Build a CustomClientHTTPError with the given status code.""" + response = MagicMock() + response.status = status + response.message = "Conflict" if status == 409 else "Error" + return CustomClientHTTPError(response) + + +def _make_user(handle, email, user_id, name="User"): + """Build a user dict shaped like the v2 GET/create response.""" + return { + "id": user_id, + "type": "users", + "attributes": { + "handle": handle, + "email": email, + "name": name, + "disabled": False, + }, + "relationships": {"roles": {"data": []}}, + } + + +def _base_kwargs(tmp_path): + """Minimal build_config kwargs that avoid network/validation.""" + return dict( + resources="users", + resource_per_file=True, + source_api_key="k", + source_app_key="k", + destination_api_key="k", + destination_app_key="k", + source_api_url="https://example.com", + destination_api_url="https://example.com", + storage_type="local", + source_resources_path=str(tmp_path / "source"), + destination_resources_path=str(tmp_path / "dest"), + max_workers=1, + send_metrics=False, + verify_ddr_status=False, + validate=False, + show_progress_bar=False, + allow_self_lockout=False, + force_missing_dependencies=False, + skip_failed_resource_connections=False, + ) + + +class TestHandleMappingKey: + def test_mapping_key_is_exact_case_handle(self, mock_config): + """a: mapping key is the handle, preserved exact-case (no lowercasing).""" + instance = Users(mock_config) + resource = {"attributes": {"handle": "User-A@example.com", "email": "shared@example.com"}} + assert instance.get_resource_mapping_key(resource) == "User-A@example.com" + + def test_map_keeps_shared_email_distinct_by_handle(self, mock_config): + """a2: two destination users sharing an email but with distinct handles + remain two entries in the map (email keying would collapse to one).""" + instance = Users(mock_config) + dest = [ + _make_user("user-a@example.com", "shared@example.com", "dest-a"), + _make_user("user-b@example.com", "shared@example.com", "dest-b"), + ] + instance.get_resources = AsyncMock(return_value=dest) + asyncio.run(instance.map_existing_resources()) + assert set(instance._existing_resources_map.keys()) == { + "user-a@example.com", + "user-b@example.com", + } + + def test_source_matched_by_handle_not_email(self, mock_config): + """a3: against a pre-existing destination user, a source user is matched + by handle — a same-email/different-handle source is NOT a match.""" + instance = Users(mock_config) + instance.get_resources = AsyncMock( + return_value=[_make_user("user-a@example.com", "shared@example.com", "dest-a")] + ) + asyncio.run(instance.map_existing_resources()) + + same_handle = _make_user("user-a@example.com", "shared@example.com", "src-a") + assert instance.get_resource_mapping_key(same_handle) in instance._existing_resources_map + + diff_handle_same_email = _make_user("user-b@example.com", "shared@example.com", "src-b") + assert instance.get_resource_mapping_key(diff_handle_same_email) not in instance._existing_resources_map + + +class TestV2CreatePayload: + def test_v2_post_body_excludes_handle_and_disabled(self, mock_config): + """f: the v2 create body must not carry read-only handle or disabled.""" + mock_config.use_v1_user_api = False + instance = Users(mock_config) + instance._existing_resources_map = {} + mock_config.destination_client.post = AsyncMock( + return_value={"data": {"id": "dest-x", "attributes": {}}} + ) + src = _make_user("user-a@example.com", "shared@example.com", "src-a") + + asyncio.run(instance.create_resource("src-a", src)) + + mock_config.destination_client.post.assert_called_once() + _, body = mock_config.destination_client.post.call_args.args + attrs = body["data"]["attributes"] + assert "handle" not in attrs + assert "disabled" not in attrs + + def test_v2_patch_body_excludes_handle(self, mock_config): + """f2: the v2 update (PATCH) body must not carry the read-only handle.""" + instance = Users(mock_config) + _id = "src-a" + mock_config.state.destination["users"][_id] = _make_user( + "user-a@example.com", "shared@example.com", "dest-a" + ) + # A differing name forces a diff -> the PATCH branch. + src = _make_user("user-a@example.com", "shared@example.com", "dest-a", name="New Name") + mock_config.destination_client.patch = AsyncMock( + return_value={"data": {"id": "dest-a", "attributes": {}}} + ) + + asyncio.run(instance.update_resource(_id, src)) + + mock_config.destination_client.patch.assert_called_once() + _, body = mock_config.destination_client.patch.call_args.args + assert "handle" not in body["data"]["attributes"] + + +class TestHandleDiffExclusion: + def test_handle_excluded_from_update_diff(self): + """g: two users differing only by handle produce no diff (guards that + handle stays diff-excluded after moving off excluded_attributes).""" + dest = _make_user("user-a@example.com", "shared@example.com", "same-id") + src = _make_user("user-b@example.com", "shared@example.com", "same-id") + assert not check_diff(Users.resource_config, dest, src) + + +class TestV1UserApiFlagWiring: + def test_build_config_flag_true(self, tmp_path): + """p1: --use-v1-user-api flows from kwargs into Configuration.""" + cfg = build_config(Command.IMPORT, use_v1_user_api=True, **_base_kwargs(tmp_path)) + assert cfg.use_v1_user_api is True + + def test_build_config_flag_default_false(self, tmp_path): + """p1: absent flag defaults to False (guards a typo'd kwarg key).""" + cfg = build_config(Command.IMPORT, **_base_kwargs(tmp_path)) + assert cfg.use_v1_user_api is False + + def test_sync_accepts_v1_flag(self): + """p2: the sync command recognizes the flag (exit 2 == usage error).""" + result = CliRunner(mix_stderr=False).invoke( + cli, ["sync", "--use-v1-user-api=true", "--validate=false"] + ) + assert result.exit_code != 2 + + def test_migrate_accepts_v1_flag(self): + """p3: migrate reuses @sync_options, so it recognizes the flag too.""" + result = CliRunner(mix_stderr=False).invoke( + cli, ["migrate", "--use-v1-user-api=true", "--validate=false"] + ) + assert result.exit_code != 2 + + +def _mock_paginated(config, pages): + """Make destination_client.paginated_request(get)(...) yield ``pages`` in order.""" + inner = AsyncMock(side_effect=pages) + config.destination_client.paginated_request = MagicMock(return_value=inner) + return inner + + +def _post_paths(mock_config): + return [c.args[0] for c in mock_config.destination_client.post.call_args_list] + + +class TestV2CreatePath: + def test_flag_off_uses_v2_not_v1(self, mock_config): + """b: with the flag off, create goes through the v2 endpoint and never + touches the v1 API (preserves the default upstream behavior).""" + mock_config.use_v1_user_api = False + instance = Users(mock_config) + instance._existing_resources_map = {} + mock_config.destination_client.post = AsyncMock(return_value={"data": {"id": "dest-x", "attributes": {}}}) + + asyncio.run(instance.create_resource("src-a", _make_user("user-a@example.com", "shared@example.com", "src-a"))) + + assert _post_paths(mock_config) == ["/api/v2/users"] + + +class TestV1CreatePath: + def test_flag_on_uses_v1_with_handle_not_v2(self, mock_config): + """d1: with the flag on, create posts to /api/v1/user carrying the handle, + never calls the v2 users endpoint, and returns the reconciled v2 UUID.""" + mock_config.use_v1_user_api = True + instance = Users(mock_config) + instance._existing_resources_map = {} + dest_user = _make_user("user-a@example.com", "shared@example.com", "dest-uuid-a") + mock_config.destination_client.post = AsyncMock(return_value={"data": {}}) + _mock_paginated(mock_config, [[dest_user]]) + + _id, r = asyncio.run( + instance.create_resource("src-a", _make_user("user-a@example.com", "shared@example.com", "src-a")) + ) + + v1_calls = [c for c in mock_config.destination_client.post.call_args_list if c.args[0] == "/api/v1/user"] + assert len(v1_calls) == 1 + assert v1_calls[0].args[1]["handle"] == "user-a@example.com" + assert "/api/v2/users" not in _post_paths(mock_config) + assert r["id"] == "dest-uuid-a" + + def test_shared_email_distinct_handles_both_created_via_v1(self, mock_config): + """The core fix. Two users share an email but have distinct handles: one + handle equals the shared email, the other differs. Under v2 the first + create derives its handle from the email and steals the second user's + handle, so the second 409s and can never be created. Via v1 each user is + created with its OWN handle, so both succeed.""" + mock_config.use_v1_user_api = True + instance = Users(mock_config) + instance._existing_resources_map = {} + dest_a = _make_user("abc@example.com", "jsmith@example.com", "dest-a") + dest_b = _make_user("jsmith@example.com", "jsmith@example.com", "dest-b") + mock_config.destination_client.post = AsyncMock(return_value={"data": {}}) + _mock_paginated(mock_config, [[dest_a], [dest_b]]) + + asyncio.run(instance.create_resource("src-a", _make_user("abc@example.com", "jsmith@example.com", "src-a"))) + asyncio.run(instance.create_resource("src-b", _make_user("jsmith@example.com", "jsmith@example.com", "src-b"))) + + v1_handles = [ + c.args[1]["handle"] + for c in mock_config.destination_client.post.call_args_list + if c.args[0] == "/api/v1/user" + ] + assert v1_handles == ["abc@example.com", "jsmith@example.com"] + assert "/api/v2/users" not in _post_paths(mock_config) + + def test_requires_handle(self, mock_config): + """d0: v1 creation raises when there is no handle to send.""" + mock_config.use_v1_user_api = True + instance = Users(mock_config) + with pytest.raises(ValueError): + asyncio.run(instance._create_via_v1("", "Person Name", "e@example.com")) + + def test_v1_post_failure_reraises(self, mock_config): + """d4: a v1 POST failure propagates so the apply loop counts it failed + and continues (DR-safe).""" + mock_config.use_v1_user_api = True + instance = Users(mock_config) + instance._existing_resources_map = {} + mock_config.destination_client.post = AsyncMock(side_effect=_http_error(400)) + with pytest.raises(CustomClientHTTPError): + asyncio.run( + instance.create_resource("src-a", _make_user("user-a@example.com", "shared@example.com", "src-a")) + ) + + +class TestReconcileByHandle: + def test_no_exact_match_raises(self, mock_config): + """If no exact-handle match is ever found after the v1 create, it raises.""" + mock_config.use_v1_user_api = True + instance = Users(mock_config) + instance._existing_resources_map = {} + mock_config.destination_client.post = AsyncMock(return_value={"data": {}}) + other = _make_user("user-b@example.com", "shared@example.com", "dest-b") + _mock_paginated(mock_config, [[other], [other], [other]]) + with pytest.raises(ValueError): + asyncio.run( + instance.create_resource("src-a", _make_user("user-a@example.com", "shared@example.com", "src-a")) + ) + + def test_requeries_on_empty_then_matches(self, mock_config): + """d3b: read-after-write — an empty first page then a match re-queries + (exactly two calls) rather than giving up on the first empty result.""" + instance = Users(mock_config) + match = _make_user("user-a@example.com", "shared@example.com", "dest-uuid-a") + inner = _mock_paginated(mock_config, [[], [match]]) + user = asyncio.run(instance._get_destination_user_by_handle("user-a@example.com")) + assert user["id"] == "dest-uuid-a" + assert inner.call_count == 2 + + def test_selects_exact_case_handle_among_candidates(self, mock_config): + """d5: with multiple filter candidates, only the exact-case handle wins.""" + instance = Users(mock_config) + a = _make_user("user-a@example.com", "shared@example.com", "dest-a") + b = _make_user("user-b@example.com", "shared@example.com", "dest-b") + _mock_paginated(mock_config, [[b, a]]) + user = asyncio.run(instance._get_destination_user_by_handle("user-a@example.com")) + assert user["id"] == "dest-a" + + +class TestUpdatePathRegression: + def test_existing_handle_takes_update_path_no_create(self, mock_config): + """e: an existing destination handle routes to the update path — no v1 or + v2 create, no duplicate.""" + mock_config.use_v1_user_api = True + instance = Users(mock_config) + dest_user = _make_user("user-a@example.com", "shared@example.com", "dest-a") + instance._existing_resources_map = {"user-a@example.com": dest_user} + mock_config.destination_client.post = AsyncMock() + mock_config.destination_client.patch = AsyncMock(return_value={"data": dest_user}) + + _id, r = asyncio.run( + instance.create_resource("src-a", _make_user("user-a@example.com", "shared@example.com", "src-a")) + ) + mock_config.destination_client.post.assert_not_called() + assert r["id"] == "dest-a"