From 5339b2616a3019f2fdb67dc813929b9b8f4834ae Mon Sep 17 00:00:00 2001 From: Ryan Morash Date: Mon, 29 Jun 2026 17:44:55 -0400 Subject: [PATCH 1/6] fix(unifi): free cards on deactivate + name holder on bind conflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related fixes for CODE_CREDS_NFC_HAS_BIND_USER (a card CiviCRM assigns to a member is still bound to a different UniFi user, so the force_add=false bind is rejected and the cycle alerts every run): - Deactivation now also deletes the user's NFC card(s), freeing the number so a recycled physical card can be reassigned. Status is set first, so cutting access never depends on the card cleanup succeeding. - When a bind is still rejected because the card is bound elsewhere (a member deactivated before this behavior shipped, or a manually-enrolled admin card), the per-user error now names the current holder — contact id for a sync-managed user, else the UniFi user id, plus display name and status — so the alert is actionable. Card number stays redacted (architecture §11). A token->holder map is captured for every user (sync-managed or not) during fetch_users(). The three card-delete loops and both bind sites are unified into _delete_cards_for_contact() and _bind_nfc_card(). force_add stays false. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/architecture.md | 2 + .../specs/2026-05-22-unifi-client-design.md | 4 +- src/door_sync/unifi/client.py | 187 ++++++++++++++---- tests/test_unifi_client.py | 133 ++++++++++++- 4 files changed, 285 insertions(+), 41 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 6450ab3..ab15c79 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -252,6 +252,8 @@ config.load() **Per-user isolation:** a single contact's `UnifiClientError` is recorded and the cycle continues with the remaining contacts; `apply()` then raises one summary error so the orchestrator alerts. One bad record never blocks the rest. (The batch card pre-import is a shared prerequisite and still fails fast.) **Email is best-effort:** UniFi requires globally-unique emails across users *and* admins. When a member's CiviCRM email is already registered to another account — commonly a staff member who is also a UniFi admin — the write is retried without the `user_email` field (so name/card/policy still apply) and a warning is logged. The member keeps door access; only the conflicting email isn't synced. This is *not* counted as a per-user failure, so it doesn't alert. + + **Card reuse on deactivation:** deactivating a user also deletes its NFC card(s). The status change runs first, so cutting access never depends on the card cleanup succeeding. Freeing the card releases its number for reuse — physical cards are routinely recycled to new members. door-sync still binds with `force_add=false` and never steals a card from another user. When a card a new member needs is nonetheless already bound elsewhere — e.g. a member deactivated before this behavior existed, or a manually-enrolled admin card — the bind fails as a per-user error (it does not block the rest of the cycle) whose message *names the current holder*: contact id for a sync-managed user, otherwise the UniFi user id, plus display name and status. The card number stays redacted (§11). This makes the conflict actionable instead of an opaque `CODE_CREDS_NFC_HAS_BIND_USER`. 7. **Log + persist state** — write audit entries; write last-success timestamp atomically (write to temp, fsync, rename). --- diff --git a/docs/superpowers/specs/2026-05-22-unifi-client-design.md b/docs/superpowers/specs/2026-05-22-unifi-client-design.md index 28e3079..322215f 100644 --- a/docs/superpowers/specs/2026-05-22-unifi-client-design.md +++ b/docs/superpowers/specs/2026-05-22-unifi-client-design.md @@ -149,7 +149,7 @@ Pagination follows the same shape as `CivicrmClient`: a `_MAX_PAGES = 1_000` cei - If empty: skip. - Else: assemble a CSV in memory (one row per missing card_id, no header row — see §9 for the exact format), `POST /api/v1/developer/credentials/nfc_cards/import` (multipart, field name `file`). - On success, parse the response's `data` array of `{nfc_id, token}` entries and merge into the token map. -3. **Deactivate** (`PUT /users/:id` with body `{"status": "DEACTIVATED"}` for each entry in `diff.to_deactivate`). +3. **Deactivate** — for each entry in `diff.to_deactivate`: `PUT /users/:id` with body `{"status": "DEACTIVATED"}`, then DELETE the user's cached NFC card(s) (`/users/:id/nfc_cards/delete`) so the card number is freed for reuse by a new member. Status first, card removal second: cutting access must not depend on the cleanup succeeding. 4. **Update credential** — for each `(resolved, unifi_user)` in `diff.to_update_credential`: - If `display_name` changed: `PUT /users/:id` body `{first_name, last_name}` (split via `_split_name`). - If `card_id` changed: @@ -398,5 +398,5 @@ Test changes: extend `_write_minimal_valid` in `tests/test_config.py` to include - **Retry budget of 3 / inter-call delay of 75ms.** Both are constants in `client.py`. Promotable to config fields if real-world tuning demands. - **Whether to compress the CSV upload.** Not done; CSVs are kilobyte-scale. - **First-name placeholder for single-word display names.** `"—"` (em-dash) — chosen to be visibly distinct in the UniFi UI so an operator notices and edits the CiviCRM record. Open to bikeshed. -- **Whether `force_add: true` should ever be used.** Default `false` (don't steal a card from another user). If we ever need to forcibly re-bind, that's a separate operational flow with explicit human authorization. +- **Whether `force_add: true` should ever be used.** Still `false` — door-sync never steals a card from another user. Recycled physical cards are instead freed at their *source*: deactivating a member now deletes their card (step 3), so the number is available the next time it's assigned. When a card is nonetheless still bound elsewhere (a member deactivated before this behavior shipped, or a manually-enrolled admin card), the bind fails as a per-user error that names the current holder, so an operator can free it deliberately rather than door-sync force-rebinding. A blanket `force_add: true` remains rejected because it would also clobber admin-managed bindings. - **Audit-log entries for UniFi calls.** Out of scope; `audit.py` (a separate slice) owns audit logging. The orchestrator passes the diff and the result there. diff --git a/src/door_sync/unifi/client.py b/src/door_sync/unifi/client.py index 2edcd6f..9e94cef 100644 --- a/src/door_sync/unifi/client.py +++ b/src/door_sync/unifi/client.py @@ -17,6 +17,7 @@ import ssl import time from collections.abc import Callable, Iterable +from dataclasses import dataclass from types import TracebackType from typing import Any from urllib.parse import urlsplit @@ -33,6 +34,10 @@ # Cap on per-contact detail lines in an apply()-failure summary, so a # widespread failure doesn't produce an unbounded error/alert message. _MAX_FAILURE_DETAIL = 10 +# UniFi rejects a card bind when the card's token is already bound to a +# different user. door-sync binds with force_add=false (never silently steal a +# card), so this code surfaces and, where possible, names the current holder. +_CODE_NFC_HAS_BIND_USER = "CODE_CREDS_NFC_HAS_BIND_USER" # Alias door-sync stamps on every card it imports, encoding the card number. # Neither read endpoint returns the Wiegand `nfc_id`: /users gives each card's # `token` (plus a display `id`), and the card list (/credentials/nfc_cards/ @@ -56,6 +61,22 @@ def __init__(self, message: str, *, code: str | None = None) -> None: self.code = code +@dataclass(frozen=True) +class _CardHolder: + """Who a card token is currently bound to in UniFi. + + Captured for every user during ``fetch_users()`` — sync-managed or not — + purely so a bind conflict (``CODE_CREDS_NFC_HAS_BIND_USER``) can name the + current holder in the failure and alert. Never carries the card number; only + identifies who holds it (architecture §11). + """ + + user_id: str + display_name: str + employee_number: str + status: str + + class UnifiClient: """Read+write UniFi Access local-API client. @@ -124,6 +145,10 @@ def __init__( # identifier the /users endpoint exposes) -> its card number. Populated # alongside `_nfc_token_map` so reads can resolve a user's card. self._card_id_by_token: dict[str, int] = {} + # token -> who currently holds that card in UniFi, recorded for every + # user (sync-managed or not) during fetch_users(). Used only to name the + # current holder when a bind is rejected with CODE_CREDS_NFC_HAS_BIND_USER. + self._holder_by_token: dict[str, _CardHolder] = {} self._fetched_users_done = False def _verify_tls_fingerprint(self) -> None: @@ -288,6 +313,7 @@ def fetch_users(self) -> list[UnifiUser]: f"expected list of users from /users, got {type(data).__name__}" ) for row in data: + self._record_card_holders(row) user = self._row_to_unifi_user(row) if user is not None: results.append(user) @@ -389,6 +415,36 @@ def _row_to_unifi_user(self, row: dict[str, Any]) -> UnifiUser | None: email=email, ) + def _record_card_holders(self, row: dict[str, Any]) -> None: + """Record which UniFi user holds each card token in this row. + + Runs for *every* /users row, including admin-managed users without an + `employee_number` (which `_row_to_unifi_user` drops), so a later bind + conflict can name whoever currently holds the card. Diagnostic only — it + never affects reconciliation. + + Args: + row: Raw user dict from the UniFi API. + """ + cards = row.get("nfc_cards") or [] + if not isinstance(cards, list): + return + first_name = str(row.get("first_name", "")) + last_name = str(row.get("last_name", "")) + display_name = " ".join(part for part in [first_name, last_name] if part).strip() + holder = _CardHolder( + user_id=str(row.get("id", "")), + display_name=display_name, + employee_number=str(row.get("employee_number") or ""), + status=str(row.get("status", "")), + ) + for card in cards: + if not isinstance(card, dict): + continue + token = str(card.get("token", "")) + if token: + self._holder_by_token[token] = holder + def apply(self, diff: Diff) -> None: """Apply a diff to UniFi Access. @@ -528,18 +584,8 @@ def _apply_one_credential( time.sleep(self._INTER_CALL_DELAY_SECONDS) if resolved.card_id != unifi_user.card_id: - # Delete old card(s) on the user. - for old_card in self._nfc_cards_by_contact.get(resolved.contact_id, []): - old_token = str(old_card.get("token", "")) - if not old_token: - continue - self._request( - "DELETE", - f"/api/v1/developer/users/{user_id}/nfc_cards/delete", - json={"token": old_token}, - ) - time.sleep(self._INTER_CALL_DELAY_SECONDS) - # Bind new card if specified. + # Delete old card(s) on the user, then bind the new one if specified. + self._delete_cards_for_contact(user_id, resolved.contact_id) if resolved.card_id is not None: new_token = self._ensure_nfc_token_map().get(resolved.card_id) if new_token is None: @@ -547,12 +593,69 @@ def _apply_one_credential( f"no token for card_id={_redact(resolved.card_id)} " f"after import (contact={resolved.contact_id})" ) - self._request( - "PUT", - f"/api/v1/developer/users/{user_id}/nfc_cards", - json={"token": new_token, "force_add": False}, - ) - time.sleep(self._INTER_CALL_DELAY_SECONDS) + self._bind_nfc_card(user_id, new_token, resolved.card_id) + + def _delete_cards_for_contact( + self, user_id: str, contact_id: int, *, keep_token: str | None = None + ) -> None: + """Delete a contact's cached NFC cards, freeing each for reuse. + + Args: + user_id: UniFi user identifier whose cards are removed. + contact_id: Contact whose cached card list (from fetch_users) is read. + keep_token: A token to preserve — e.g. a card about to be re-bound to + the same user; any card matching it is left in place. + """ + for old_card in self._nfc_cards_by_contact.get(contact_id, []): + old_token = str(old_card.get("token", "")) + if not old_token or old_token == keep_token: + continue + self._request( + "DELETE", + f"/api/v1/developer/users/{user_id}/nfc_cards/delete", + json={"token": old_token}, + ) + time.sleep(self._INTER_CALL_DELAY_SECONDS) + + def _bind_nfc_card(self, user_id: str, token: str, card_id: int) -> None: + """Bind an NFC card to a user, naming the current holder on conflict. + + door-sync binds with ``force_add=false`` so it never silently steals a + card from another user. When UniFi rejects the bind because the card is + already assigned (``CODE_CREDS_NFC_HAS_BIND_USER``), the current holder is + looked up from the ``fetch_users()`` snapshot and named in the raised + error, so the alert is actionable. The card number is redacted; only who + holds it is disclosed (architecture §11). + + Args: + user_id: UniFi user the card should bind to. + token: Opaque UniFi card token to bind. + card_id: Numeric card id, for a redacted reference in the error. + + Raises: + UnifiClientError: If the bind fails. On a bind-conflict the message is + enriched with the current holder; the original ``code`` is kept. + """ + try: + self._request( + "PUT", + f"/api/v1/developer/users/{user_id}/nfc_cards", + json={"token": token, "force_add": False}, + ) + except UnifiClientError as exc: + if exc.code != _CODE_NFC_HAS_BIND_USER: + raise + holder = self._holder_by_token.get(token) + who = ( + _describe_card_holder(holder) + if holder is not None + else "an unknown UniFi user (not in the fetched user list)" + ) + raise UnifiClientError( + f"{exc} (card {_redact(card_id)} is currently bound to {who})", + code=exc.code, + ) from exc + time.sleep(self._INTER_CALL_DELAY_SECONDS) def _apply_update_policy(self, diff: Diff) -> None: for resolved, _unifi_user in diff.to_update_policy: @@ -599,10 +702,18 @@ def _apply_deactivate(self, diff: Diff) -> None: f"/api/v1/developer/users/{user_id}", json={"status": "DEACTIVATED"}, ) + time.sleep(self._INTER_CALL_DELAY_SECONDS) + # Free the departed member's card(s) so the number can be + # reassigned. A deactivated user that kept its card would hold it + # indefinitely, and a recycled card could never be bound to the + # new member (CODE_CREDS_NFC_HAS_BIND_USER). Deactivation (cutting + # access) runs first so a card-delete failure never leaves an + # active record; if a delete fails here it's recorded and alerts, + # and any leftover binding is surfaced by _bind_nfc_card on reuse. + self._delete_cards_for_contact(user_id, unifi_user.contact_id) except UnifiClientError as exc: self._record_apply_failure(unifi_user.contact_id, exc) continue - time.sleep(self._INTER_CALL_DELAY_SECONDS) def _log_dry_run_actions(self, diff: Diff) -> None: # Emit would-import lines for cards not yet in the token map. @@ -841,23 +952,14 @@ def _prepare_reactivation( contact_id=resolved.contact_id, ) time.sleep(self._INTER_CALL_DELAY_SECONDS) - # Delete any old cards that differ from the new card_id. - cached_cards = self._nfc_cards_by_contact.get(resolved.contact_id, []) + # Delete any old cards that differ from the new card_id (the matching + # card, if any, is kept so the bind step doesn't have to re-add it). new_token = ( self._ensure_nfc_token_map().get(resolved.card_id) if resolved.card_id is not None else None ) - for old_card in cached_cards: - old_token = str(old_card.get("token", "")) - if not old_token or old_token == new_token: - continue - self._request( - "DELETE", - f"/api/v1/developer/users/{user_id}/nfc_cards/delete", - json={"token": old_token}, - ) - time.sleep(self._INTER_CALL_DELAY_SECONDS) + self._delete_cards_for_contact(user_id, resolved.contact_id, keep_token=new_token) def _activate_user(self, user_id: str) -> None: """Set a UniFi user's status to ACTIVE. @@ -919,12 +1021,7 @@ def _bind_card_if_set(self, user_id: str, resolved: ResolvedMember) -> None: f"no token for card_id={_redact(resolved.card_id)} " f"after import (contact={resolved.contact_id})" ) - self._request( - "PUT", - f"/api/v1/developer/users/{user_id}/nfc_cards", - json={"token": token, "force_add": False}, - ) - time.sleep(self._INTER_CALL_DELAY_SECONDS) + self._bind_nfc_card(user_id, token, resolved.card_id) def _assign_policy_if_set(self, user_id: str, resolved: ResolvedMember) -> None: """Assign the resolved access policy to a UniFi user, if set. @@ -1085,6 +1182,22 @@ def _redact(card_id: int | None) -> str: return f"****{card_id % 10000:04d}" +def _describe_card_holder(holder: _CardHolder) -> str: + """Render a card's current holder for a bind-conflict error/alert. + + A sync-managed holder (positive integer ``employee_number``) is named by its + CiviCRM contact id; anyone else (a manually-enrolled admin card, no usable + employee number) is named by UniFi user id and flagged as not sync-managed. + Never includes the card number — only who holds it (architecture §11). + """ + name = holder.display_name or "(no name)" + status = holder.status or "unknown status" + emp = holder.employee_number + if emp.isascii() and emp.isdigit() and int(emp) > 0: + return f"contact={emp} ('{name}', {status})" + return f"a non-sync UniFi user '{name}' (id={holder.user_id}, {status})" + + def _backoff_seconds(attempt: int) -> float: """Exponential backoff with ±20% jitter. attempt is 1-indexed.""" base = float(2 ** (attempt - 1)) diff --git a/tests/test_unifi_client.py b/tests/test_unifi_client.py index 8a5539d..758cc1e 100644 --- a/tests/test_unifi_client.py +++ b/tests/test_unifi_client.py @@ -1119,6 +1119,133 @@ def test_apply_deactivate_sets_status( assert body == {"status": "DEACTIVATED"} +def test_apply_deactivate_removes_nfc_card( + httpx_mock: HTTPXMock, monkeypatch: pytest.MonkeyPatch, make_client: Callable[..., UnifiClient] +) -> None: + """Deactivating a carded user also DELETEs the card so it can be reused.""" + monkeypatch.setattr("door_sync.unifi.client.time.sleep", lambda _: None) + + client = make_client() + + # Card list (token -> number) registered before fetch_users(), which resolves + # the carded user's number from its token. + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/credentials/nfc_cards/tokens?page_num=1&page_size=100", + json=_card_tokens_page([(1234, "tok-1234")]), + ) + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/users?page_num=1&page_size=100&expand[]=access_policy", + json=_users_page([_user_row(contact_id=42, user_id="uuid-42", nfc_token="tok-1234")]), + ) + fetched = client.fetch_users() + + # Deactivate status write. + httpx_mock.add_response( + method="PUT", + url="https://192.0.2.1:12445/api/v1/developer/users/uuid-42", + json={"code": "SUCCESS", "msg": "success", "data": None}, + ) + # Card removal write. + httpx_mock.add_response( + method="DELETE", + url="https://192.0.2.1:12445/api/v1/developer/users/uuid-42/nfc_cards/delete", + json={"code": "SUCCESS", "msg": "success", "data": None}, + ) + + client.apply(_diff(to_deactivate=(fetched[0],))) + + # Status set to DEACTIVATED ... + put_req = next( + r + for r in httpx_mock.get_requests() + if r.method == "PUT" and r.url.path.endswith("/users/uuid-42") + ) + assert _json.loads(put_req.content) == {"status": "DEACTIVATED"} + # ... and the card freed for reuse. + delete_req = next( + r + for r in httpx_mock.get_requests() + if r.method == "DELETE" and r.url.path.endswith("/nfc_cards/delete") + ) + assert _json.loads(delete_req.content) == {"token": "tok-1234"} + + +def test_apply_bind_conflict_names_current_holder( + httpx_mock: HTTPXMock, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, + make_client: Callable[..., UnifiClient], +) -> None: + """A card bound to another UniFi user surfaces who holds it (log + alert).""" + monkeypatch.setattr("door_sync.unifi.client.time.sleep", lambda _: None) + + client = make_client() + + # Card 1234 -> tok-1234 (registered before fetch_users()). + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/credentials/nfc_cards/tokens?page_num=1&page_size=100", + json=_card_tokens_page([(1234, "tok-1234")]), + ) + # A deactivated former member (5000) still holds tok-1234; the new member + # (7590) currently has no card and is being assigned card 1234 in CiviCRM. + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/users?page_num=1&page_size=100&expand[]=access_policy", + json=_users_page( + [ + _user_row( + contact_id=5000, + user_id="uuid-5000", + first_name="Old", + last_name="Member", + status="DEACTIVATED", + nfc_token="tok-1234", + ), + _user_row( + contact_id=7590, + user_id="uuid-7590", + first_name="Member", + last_name="7590", + ), + ] + ), + ) + fetched = client.fetch_users() + target = next(u for u in fetched if u.contact_id == 7590) + + # The bind for 7590 is rejected: card already bound to another user. + httpx_mock.add_response( + method="PUT", + url="https://192.0.2.1:12445/api/v1/developer/users/uuid-7590/nfc_cards", + json={ + "code": "CODE_CREDS_NFC_HAS_BIND_USER", + "msg": "This NFC card is already assigned to another user.", + "data": None, + }, + ) + + resolved = _resolved(contact_id=7590, card_id=1234) + with caplog.at_level(logging.ERROR, logger="door_sync.unifi.client"): + with pytest.raises(UnifiClientError) as exc_info: + client.apply(_diff(to_update_credential=((resolved, target),))) + + msg = str(exc_info.value) + # The failing member and the current holder are both named. + assert "contact=7590" in msg + assert "contact=5000" in msg + assert "Old Member" in msg + assert "DEACTIVATED" in msg + # Card is redacted; the raw number never leaks except as the ****NNNN form. + assert "****1234" in msg + assert "1234" not in msg.replace("****1234", "") + # The per-contact error log carries the holder too (the operator's signal). + logged = "\n".join(r.getMessage() for r in caplog.records) + assert "contact=5000" in logged + + def test_apply_update_credential_swaps_card( httpx_mock: HTTPXMock, monkeypatch: pytest.MonkeyPatch, make_client: Callable[..., UnifiClient] ) -> None: @@ -1876,6 +2003,7 @@ def test_apply_executes_deactivate_update_credential_update_policy_add_order( # Pre-set generic SUCCESS responses for the writes. for url, method in [ ("https://192.0.2.1:12445/api/v1/developer/users/u100", "PUT"), + ("https://192.0.2.1:12445/api/v1/developer/users/u100/nfc_cards/delete", "DELETE"), ("https://192.0.2.1:12445/api/v1/developer/users/u101/nfc_cards/delete", "DELETE"), ("https://192.0.2.1:12445/api/v1/developer/users/u101/nfc_cards", "PUT"), ("https://192.0.2.1:12445/api/v1/developer/users/u102/access_policies", "PUT"), @@ -1899,10 +2027,11 @@ def test_apply_executes_deactivate_update_credential_update_policy_add_order( if r.method in ("PUT", "POST", "DELETE") and "/credentials/nfc_cards/import" not in r.url.path ] - # Expected order: deactivate(100), update_credential(101 DELETE then PUT card), - # update_policy(102). + # Expected order: deactivate(100) sets status then frees its card, + # update_credential(101 DELETE then PUT card), update_policy(102). assert write_path_methods == [ ("PUT", "/api/v1/developer/users/u100"), + ("DELETE", "/api/v1/developer/users/u100/nfc_cards/delete"), ("DELETE", "/api/v1/developer/users/u101/nfc_cards/delete"), ("PUT", "/api/v1/developer/users/u101/nfc_cards"), ("PUT", "/api/v1/developer/users/u102/access_policies"), From 615d660cd0e5d3c00d5ee89efd9cd057ce1b1b33 Mon Sep 17 00:00:00 2001 From: Ryan Morash Date: Mon, 29 Jun 2026 17:50:43 -0400 Subject: [PATCH 2/6] fix(unifi): reclaim a card from a disabled holder on bind conflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the card-reuse story for CODE_CREDS_NFC_HAS_BIND_USER. When a bind is rejected because the card is still bound elsewhere, door-sync inspects the current holder (from the fetch_users snapshot): - Disabled holder: the card is dead state on a deactivated user (e.g. a member deactivated before cards were freed on deactivation). door-sync unbinds it from them and retries the bind, logging a redacted warning. This auto-recovers the backlog of recycled cards that the deactivate-frees-card change can't reach (already-inactive users are never re-processed by the reconciler). - Active holder (or one not in the snapshot): left untouched; the bind fails as a per-user error naming the holder, so an operator resolves it deliberately. force_add stays false — door-sync never force-steals; it only unbinds a card a disabled holder is no longer using. Card numbers stay redacted (architecture §11). The raw bind PUT is factored into _put_bind_card(). Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/architecture.md | 7 +- .../specs/2026-05-22-unifi-client-design.md | 2 +- src/door_sync/unifi/client.py | 80 +++++++++-- tests/test_unifi_client.py | 132 +++++++++++++++--- 4 files changed, 188 insertions(+), 33 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index ab15c79..d1239a2 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -253,7 +253,12 @@ config.load() **Email is best-effort:** UniFi requires globally-unique emails across users *and* admins. When a member's CiviCRM email is already registered to another account — commonly a staff member who is also a UniFi admin — the write is retried without the `user_email` field (so name/card/policy still apply) and a warning is logged. The member keeps door access; only the conflicting email isn't synced. This is *not* counted as a per-user failure, so it doesn't alert. - **Card reuse on deactivation:** deactivating a user also deletes its NFC card(s). The status change runs first, so cutting access never depends on the card cleanup succeeding. Freeing the card releases its number for reuse — physical cards are routinely recycled to new members. door-sync still binds with `force_add=false` and never steals a card from another user. When a card a new member needs is nonetheless already bound elsewhere — e.g. a member deactivated before this behavior existed, or a manually-enrolled admin card — the bind fails as a per-user error (it does not block the rest of the cycle) whose message *names the current holder*: contact id for a sync-managed user, otherwise the UniFi user id, plus display name and status. The card number stays redacted (§11). This makes the conflict actionable instead of an opaque `CODE_CREDS_NFC_HAS_BIND_USER`. + **Card reuse and bind conflicts.** A recycled physical card can be assigned to a new member while it's still bound to an old UniFi user. door-sync handles this at two points so `CODE_CREDS_NFC_HAS_BIND_USER` doesn't strand a member: + + - **On deactivation** (proactive): deactivating a user also deletes its NFC card(s). The status change runs first, so cutting access never depends on the card cleanup succeeding. This frees the number for the *next* member who's assigned it. + - **On bind conflict** (reactive): if a bind is still rejected because the card is bound elsewhere, door-sync inspects the current holder. If that holder is a **disabled** UniFi user (the card is dead state — e.g. a member deactivated before cards were freed on deactivation), it unbinds the card from them and retries the bind, logging a redacted warning; the reclaim is not a failure. If the holder is **active** (or not in the fetched snapshot), the card is left alone and the bind fails as a per-user error (it does not block the rest of the cycle) whose message *names the current holder*: contact id for a sync-managed user, otherwise the UniFi user id, plus display name and status. + + `force_add` stays `false` throughout — door-sync never force-steals; it only unbinds a card that a *disabled* holder is no longer using. The card number stays redacted everywhere (§11). 7. **Log + persist state** — write audit entries; write last-success timestamp atomically (write to temp, fsync, rename). --- diff --git a/docs/superpowers/specs/2026-05-22-unifi-client-design.md b/docs/superpowers/specs/2026-05-22-unifi-client-design.md index 322215f..c7d1eb3 100644 --- a/docs/superpowers/specs/2026-05-22-unifi-client-design.md +++ b/docs/superpowers/specs/2026-05-22-unifi-client-design.md @@ -398,5 +398,5 @@ Test changes: extend `_write_minimal_valid` in `tests/test_config.py` to include - **Retry budget of 3 / inter-call delay of 75ms.** Both are constants in `client.py`. Promotable to config fields if real-world tuning demands. - **Whether to compress the CSV upload.** Not done; CSVs are kilobyte-scale. - **First-name placeholder for single-word display names.** `"—"` (em-dash) — chosen to be visibly distinct in the UniFi UI so an operator notices and edits the CiviCRM record. Open to bikeshed. -- **Whether `force_add: true` should ever be used.** Still `false` — door-sync never steals a card from another user. Recycled physical cards are instead freed at their *source*: deactivating a member now deletes their card (step 3), so the number is available the next time it's assigned. When a card is nonetheless still bound elsewhere (a member deactivated before this behavior shipped, or a manually-enrolled admin card), the bind fails as a per-user error that names the current holder, so an operator can free it deliberately rather than door-sync force-rebinding. A blanket `force_add: true` remains rejected because it would also clobber admin-managed bindings. +- **Whether `force_add: true` should ever be used.** Still `false` — door-sync never *force*-steals a card. Recycled physical cards are freed at their source: deactivating a member deletes their card (step 3), so the number is available next time it's assigned. When a card is nonetheless still bound elsewhere on bind, door-sync inspects the holder: a **disabled** holder's card is dead state, so it unbinds the card from them (a normal `DELETE`) and retries the bind, reclaiming the number automatically. An **active** holder (or one not in the fetched snapshot) is left untouched and the bind fails as a per-user error naming the holder, so an operator can resolve it deliberately. A blanket `force_add: true` remains rejected: it would clobber *active* admin-managed bindings. - **Audit-log entries for UniFi calls.** Out of scope; `audit.py` (a separate slice) owns audit logging. The orchestrator passes the diff and the result there. diff --git a/src/door_sync/unifi/client.py b/src/door_sync/unifi/client.py index 9e94cef..8318bcc 100644 --- a/src/door_sync/unifi/client.py +++ b/src/door_sync/unifi/client.py @@ -617,35 +617,73 @@ def _delete_cards_for_contact( ) time.sleep(self._INTER_CALL_DELAY_SECONDS) + def _put_bind_card(self, user_id: str, token: str) -> None: + """Issue the raw bind request (``force_add=false``) for one card.""" + self._request( + "PUT", + f"/api/v1/developer/users/{user_id}/nfc_cards", + json={"token": token, "force_add": False}, + ) + def _bind_nfc_card(self, user_id: str, token: str, card_id: int) -> None: - """Bind an NFC card to a user, naming the current holder on conflict. + """Bind an NFC card to a user, reclaiming it from a disabled holder. + + door-sync binds with ``force_add=false`` so it never force-steals a card. + When UniFi rejects the bind because the card is already assigned + (``CODE_CREDS_NFC_HAS_BIND_USER``), the current holder is looked up from + the ``fetch_users()`` snapshot: - door-sync binds with ``force_add=false`` so it never silently steals a - card from another user. When UniFi rejects the bind because the card is - already assigned (``CODE_CREDS_NFC_HAS_BIND_USER``), the current holder is - looked up from the ``fetch_users()`` snapshot and named in the raised - error, so the alert is actionable. The card number is redacted; only who - holds it is disclosed (architecture §11). + * If the holder is a *disabled* UniFi user, the card is theirs only as + dead state — door-sync unbinds it from them and retries the bind, so a + recycled card stuck on a deactivated member is reclaimed automatically. + The reclaim is logged (a warning), not counted as a failure. + * Otherwise (an *active* user, or a holder not in the snapshot), the card + is left alone and the error is re-raised naming the holder, so the + alert is actionable. + + The card number is redacted everywhere; only who holds it is disclosed + (architecture §11). Args: user_id: UniFi user the card should bind to. token: Opaque UniFi card token to bind. - card_id: Numeric card id, for a redacted reference in the error. + card_id: Numeric card id, for a redacted reference in logs/errors. Raises: - UnifiClientError: If the bind fails. On a bind-conflict the message is - enriched with the current holder; the original ``code`` is kept. + UnifiClientError: If the bind fails and the card was not reclaimable. + On a bind-conflict the message is enriched with the current + holder; the original ``code`` is kept. """ try: - self._request( - "PUT", - f"/api/v1/developer/users/{user_id}/nfc_cards", - json={"token": token, "force_add": False}, - ) + self._put_bind_card(user_id, token) except UnifiClientError as exc: if exc.code != _CODE_NFC_HAS_BIND_USER: raise holder = self._holder_by_token.get(token) + if ( + holder is not None + and holder.user_id + and holder.user_id != user_id + and _holder_is_disabled(holder) + ): + # The card is dead state on a deactivated user; take it back. + logger.warning( + "card %s is bound to disabled %s; reclaiming it for this member", + _redact(card_id), + _describe_card_holder(holder), + ) + self._request( + "DELETE", + f"/api/v1/developer/users/{holder.user_id}/nfc_cards/delete", + json={"token": token}, + ) + time.sleep(self._INTER_CALL_DELAY_SECONDS) + self._put_bind_card(user_id, token) + # The card has moved; drop the stale holder so a later same-cycle + # lookup on this token doesn't misreport it. + self._holder_by_token.pop(token, None) + time.sleep(self._INTER_CALL_DELAY_SECONDS) + return who = ( _describe_card_holder(holder) if holder is not None @@ -1182,6 +1220,18 @@ def _redact(card_id: int | None) -> str: return f"****{card_id % 10000:04d}" +def _holder_is_disabled(holder: _CardHolder) -> bool: + """True if a card's current holder is a deactivated (non-active) UniFi user. + + Mirrors ``UnifiUser.active`` (status == "ACTIVE"): anything else — chiefly + "DEACTIVATED" — counts as disabled, so door-sync may reclaim the card. An + empty or unreadable status is treated as NOT disabled, so a card is never + pulled from a holder whose state could not be confirmed. + """ + status = holder.status.strip().upper() + return status != "" and status != "ACTIVE" + + def _describe_card_holder(holder: _CardHolder) -> str: """Render a card's current holder for a bind-conflict error/alert. diff --git a/tests/test_unifi_client.py b/tests/test_unifi_client.py index 758cc1e..8de830c 100644 --- a/tests/test_unifi_client.py +++ b/tests/test_unifi_client.py @@ -1172,38 +1172,42 @@ def test_apply_deactivate_removes_nfc_card( assert _json.loads(delete_req.content) == {"token": "tok-1234"} -def test_apply_bind_conflict_names_current_holder( +def test_apply_bind_conflict_active_holder_names_and_raises( httpx_mock: HTTPXMock, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture, make_client: Callable[..., UnifiClient], ) -> None: - """A card bound to another UniFi user surfaces who holds it (log + alert).""" + """A card held by an *active* user is left alone; the error names the holder.""" monkeypatch.setattr("door_sync.unifi.client.time.sleep", lambda _: None) client = make_client() - # Card 1234 -> tok-1234 (registered before fetch_users()). + # Card 1234 -> tok-1234. Fetched during apply (no managed carded row to + # trigger it during fetch_users); registration order is irrelevant. httpx_mock.add_response( method="GET", url="https://192.0.2.1:12445/api/v1/developer/credentials/nfc_cards/tokens?page_num=1&page_size=100", json=_card_tokens_page([(1234, "tok-1234")]), ) - # A deactivated former member (5000) still holds tok-1234; the new member - # (7590) currently has no card and is being assigned card 1234 in CiviCRM. + # An ACTIVE, manually-enrolled admin user (no employee_number → not + # sync-managed) holds tok-1234. The new member (7590) has no card and is + # being assigned card 1234. door-sync must NOT steal from an active holder. + admin_holder = { + "id": "uuid-admin", + "first_name": "Front", + "last_name": "Desk", + "employee_number": "", + "status": "ACTIVE", + "nfc_cards": [{"id": "200001", "token": "tok-1234", "type": "id_card"}], + "access_policy_ids": ["pol-1"], + } httpx_mock.add_response( method="GET", url="https://192.0.2.1:12445/api/v1/developer/users?page_num=1&page_size=100&expand[]=access_policy", json=_users_page( [ - _user_row( - contact_id=5000, - user_id="uuid-5000", - first_name="Old", - last_name="Member", - status="DEACTIVATED", - nfc_token="tok-1234", - ), + admin_holder, _user_row( contact_id=7590, user_id="uuid-7590", @@ -1232,18 +1236,114 @@ def test_apply_bind_conflict_names_current_holder( with pytest.raises(UnifiClientError) as exc_info: client.apply(_diff(to_update_credential=((resolved, target),))) + # No reclaim: the active holder's card is never deleted. + assert not [r for r in httpx_mock.get_requests() if r.method == "DELETE"] + msg = str(exc_info.value) # The failing member and the current holder are both named. assert "contact=7590" in msg - assert "contact=5000" in msg - assert "Old Member" in msg - assert "DEACTIVATED" in msg + assert "non-sync UniFi user 'Front Desk'" in msg + assert "uuid-admin" in msg + assert "ACTIVE" in msg # Card is redacted; the raw number never leaks except as the ****NNNN form. assert "****1234" in msg assert "1234" not in msg.replace("****1234", "") # The per-contact error log carries the holder too (the operator's signal). logged = "\n".join(r.getMessage() for r in caplog.records) + assert "uuid-admin" in logged + + +def test_apply_bind_conflict_reclaims_card_from_disabled_holder( + httpx_mock: HTTPXMock, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, + make_client: Callable[..., UnifiClient], +) -> None: + """A card stuck on a *disabled* user is reclaimed and bound to the new member.""" + monkeypatch.setattr("door_sync.unifi.client.time.sleep", lambda _: None) + + client = make_client() + + # Card 1234 -> tok-1234 (fetched during fetch_users() via the carded holder). + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/credentials/nfc_cards/tokens?page_num=1&page_size=100", + json=_card_tokens_page([(1234, "tok-1234")]), + ) + # A deactivated former member (5000) still holds tok-1234 — e.g. deactivated + # before cards were freed on deactivation. The new member (7590) is assigned + # card 1234 in CiviCRM. + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/users?page_num=1&page_size=100&expand[]=access_policy", + json=_users_page( + [ + _user_row( + contact_id=5000, + user_id="uuid-5000", + first_name="Old", + last_name="Member", + status="DEACTIVATED", + nfc_token="tok-1234", + ), + _user_row( + contact_id=7590, + user_id="uuid-7590", + first_name="Member", + last_name="7590", + ), + ] + ), + ) + fetched = client.fetch_users() + target = next(u for u in fetched if u.contact_id == 7590) + + # First bind for 7590 is rejected (card still on the disabled user) ... + httpx_mock.add_response( + method="PUT", + url="https://192.0.2.1:12445/api/v1/developer/users/uuid-7590/nfc_cards", + json={ + "code": "CODE_CREDS_NFC_HAS_BIND_USER", + "msg": "This NFC card is already assigned to another user.", + "data": None, + }, + ) + # ... door-sync unbinds it from the disabled holder ... + httpx_mock.add_response( + method="DELETE", + url="https://192.0.2.1:12445/api/v1/developer/users/uuid-5000/nfc_cards/delete", + json={"code": "SUCCESS", "msg": "success", "data": None}, + ) + # ... then the retried bind succeeds. + httpx_mock.add_response( + method="PUT", + url="https://192.0.2.1:12445/api/v1/developer/users/uuid-7590/nfc_cards", + json={"code": "SUCCESS", "msg": "success", "data": None}, + ) + + resolved = _resolved(contact_id=7590, card_id=1234) + with caplog.at_level(logging.WARNING, logger="door_sync.unifi.client"): + # No raise: the card is reclaimed and bound. + client.apply(_diff(to_update_credential=((resolved, target),))) + + # The card was unbound from the disabled holder ... + delete_req = next( + r for r in httpx_mock.get_requests() if r.method == "DELETE" and "uuid-5000" in r.url.path + ) + assert _json.loads(delete_req.content) == {"token": "tok-1234"} + # ... and bound to 7590 twice (the rejected attempt + the successful retry). + binds = [ + r + for r in httpx_mock.get_requests() + if r.method == "PUT" and r.url.path.endswith("/users/uuid-7590/nfc_cards") + ] + assert len(binds) == 2 + # The reclaim is logged and redacted (last-4 only). + logged = "\n".join(r.getMessage() for r in caplog.records) + assert "reclaiming" in logged assert "contact=5000" in logged + assert "****1234" in logged + assert "1234" not in logged.replace("****1234", "") def test_apply_update_credential_swaps_card( From a5ca7df1fc7e21c59ea9a52671c0798d741e67da Mon Sep 17 00:00:00 2001 From: Ryan Morash Date: Mon, 29 Jun 2026 18:07:01 -0400 Subject: [PATCH 3/6] fix(unifi): reclaim via force_add and free card before deactivating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reclaim path hit HTTP 404 "CODE_NOT_FOUND / no-man zone" in production: UniFi rejects NFC-card mutations on a *deactivated* user, so the card cannot be DELETEd off a disabled holder. Two corrections, same root cause: - Reclaim: instead of DELETE-then-rebind (impossible on a disabled holder), force-reassign the card to the new member with force_add=true, which moves it. Still gated on the holder being confirmed-disabled; active/admin holders are never displaced. - Deactivate: free the card *before* setting DEACTIVATED (while the account is still active and the delete is permitted), best-effort so a cleanup failure never blocks cutting access. Any leftover is reclaimed on next assignment. force_add stays false for every normal bind; true is used only to reclaim from a disabled holder. Tests + docs (architecture §7, design spec) updated. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/architecture.md | 8 +-- .../specs/2026-05-22-unifi-client-design.md | 4 +- src/door_sync/unifi/client.py | 57 ++++++++++++------- tests/test_unifi_client.py | 47 ++++++++------- 4 files changed, 67 insertions(+), 49 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index d1239a2..38f4b42 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -253,12 +253,12 @@ config.load() **Email is best-effort:** UniFi requires globally-unique emails across users *and* admins. When a member's CiviCRM email is already registered to another account — commonly a staff member who is also a UniFi admin — the write is retried without the `user_email` field (so name/card/policy still apply) and a warning is logged. The member keeps door access; only the conflicting email isn't synced. This is *not* counted as a per-user failure, so it doesn't alert. - **Card reuse and bind conflicts.** A recycled physical card can be assigned to a new member while it's still bound to an old UniFi user. door-sync handles this at two points so `CODE_CREDS_NFC_HAS_BIND_USER` doesn't strand a member: + **Card reuse and bind conflicts.** A recycled physical card can be assigned to a new member while it's still bound to an old UniFi user. **Key constraint:** UniFi rejects NFC-card mutations on a *deactivated* user (HTTP 404 "no-man zone") — a card cannot be unbound from a disabled account. door-sync works within that at two points so `CODE_CREDS_NFC_HAS_BIND_USER` doesn't strand a member: - - **On deactivation** (proactive): deactivating a user also deletes its NFC card(s). The status change runs first, so cutting access never depends on the card cleanup succeeding. This frees the number for the *next* member who's assigned it. - - **On bind conflict** (reactive): if a bind is still rejected because the card is bound elsewhere, door-sync inspects the current holder. If that holder is a **disabled** UniFi user (the card is dead state — e.g. a member deactivated before cards were freed on deactivation), it unbinds the card from them and retries the bind, logging a redacted warning; the reclaim is not a failure. If the holder is **active** (or not in the fetched snapshot), the card is left alone and the bind fails as a per-user error (it does not block the rest of the cycle) whose message *names the current holder*: contact id for a sync-managed user, otherwise the UniFi user id, plus display name and status. + - **On deactivation** (proactive): a departing user's NFC card(s) are deleted *before* the status is set to `DEACTIVATED`, i.e. while the account is still active and the delete is permitted. This is best-effort — a cleanup failure is logged but never blocks cutting access — so the next member assigned that number binds cleanly. + - **On bind conflict** (reactive): if a bind is still rejected because the card is bound elsewhere, door-sync inspects the current holder. If that holder is a **disabled** UniFi user (the card is dead state — e.g. a member deactivated before cards were freed on deactivation, or where the proactive cleanup didn't run), door-sync **force-reassigns** the card to the new member (`force_add: true`) and logs a redacted warning; the reclaim is not a failure. Forcing the bind is the only path here — the card can't be unbound from the disabled holder first. If the holder is **active** (or not in the fetched snapshot), the card is left alone and the bind fails as a per-user error (it does not block the rest of the cycle) whose message *names the current holder*: contact id for a sync-managed user, otherwise the UniFi user id, plus display name and status. - `force_add` stays `false` throughout — door-sync never force-steals; it only unbinds a card that a *disabled* holder is no longer using. The card number stays redacted everywhere (§11). + `force_add` is `false` for every normal bind — door-sync never displaces an *active* user. `force_add: true` is used only to reclaim a card from a confirmed-**disabled** holder. The card number stays redacted everywhere (§11). 7. **Log + persist state** — write audit entries; write last-success timestamp atomically (write to temp, fsync, rename). --- diff --git a/docs/superpowers/specs/2026-05-22-unifi-client-design.md b/docs/superpowers/specs/2026-05-22-unifi-client-design.md index c7d1eb3..8e8b8ee 100644 --- a/docs/superpowers/specs/2026-05-22-unifi-client-design.md +++ b/docs/superpowers/specs/2026-05-22-unifi-client-design.md @@ -149,7 +149,7 @@ Pagination follows the same shape as `CivicrmClient`: a `_MAX_PAGES = 1_000` cei - If empty: skip. - Else: assemble a CSV in memory (one row per missing card_id, no header row — see §9 for the exact format), `POST /api/v1/developer/credentials/nfc_cards/import` (multipart, field name `file`). - On success, parse the response's `data` array of `{nfc_id, token}` entries and merge into the token map. -3. **Deactivate** — for each entry in `diff.to_deactivate`: `PUT /users/:id` with body `{"status": "DEACTIVATED"}`, then DELETE the user's cached NFC card(s) (`/users/:id/nfc_cards/delete`) so the card number is freed for reuse by a new member. Status first, card removal second: cutting access must not depend on the cleanup succeeding. +3. **Deactivate** — for each entry in `diff.to_deactivate`: first DELETE the user's cached NFC card(s) (`/users/:id/nfc_cards/delete`) to free the number for reuse, *then* `PUT /users/:id` with body `{"status": "DEACTIVATED"}`. Card removal must precede the status change because UniFi rejects card mutations on a deactivated user (HTTP 404 "no-man zone"). The card delete is best-effort — a failure is logged but does not block the status change (access-cutting wins); any card left behind is reclaimed on its next assignment (step 4). 4. **Update credential** — for each `(resolved, unifi_user)` in `diff.to_update_credential`: - If `display_name` changed: `PUT /users/:id` body `{first_name, last_name}` (split via `_split_name`). - If `card_id` changed: @@ -398,5 +398,5 @@ Test changes: extend `_write_minimal_valid` in `tests/test_config.py` to include - **Retry budget of 3 / inter-call delay of 75ms.** Both are constants in `client.py`. Promotable to config fields if real-world tuning demands. - **Whether to compress the CSV upload.** Not done; CSVs are kilobyte-scale. - **First-name placeholder for single-word display names.** `"—"` (em-dash) — chosen to be visibly distinct in the UniFi UI so an operator notices and edits the CiviCRM record. Open to bikeshed. -- **Whether `force_add: true` should ever be used.** Still `false` — door-sync never *force*-steals a card. Recycled physical cards are freed at their source: deactivating a member deletes their card (step 3), so the number is available next time it's assigned. When a card is nonetheless still bound elsewhere on bind, door-sync inspects the holder: a **disabled** holder's card is dead state, so it unbinds the card from them (a normal `DELETE`) and retries the bind, reclaiming the number automatically. An **active** holder (or one not in the fetched snapshot) is left untouched and the bind fails as a per-user error naming the holder, so an operator can resolve it deliberately. A blanket `force_add: true` remains rejected: it would clobber *active* admin-managed bindings. +- **When `force_add: true` is used.** Never for a normal bind — door-sync doesn't displace an *active* user. It is used in exactly one case: reclaiming a card from a confirmed-**disabled** holder on a `CODE_CREDS_NFC_HAS_BIND_USER` conflict. The card can't be unbound from the disabled holder first (UniFi 404s card mutations on a deactivated user), so forcing the new bind is the only way to move it; door-sync gates this on having checked the holder's status in the fetch snapshot. An **active** holder (or one not in the snapshot) is left untouched and the bind fails as a per-user error naming the holder, so an operator resolves it deliberately. A *blanket* `force_add: true` remains rejected: it would clobber active admin-managed bindings. - **Audit-log entries for UniFi calls.** Out of scope; `audit.py` (a separate slice) owns audit logging. The orchestrator passes the diff and the result there. diff --git a/src/door_sync/unifi/client.py b/src/door_sync/unifi/client.py index 8318bcc..56a6411 100644 --- a/src/door_sync/unifi/client.py +++ b/src/door_sync/unifi/client.py @@ -617,12 +617,19 @@ def _delete_cards_for_contact( ) time.sleep(self._INTER_CALL_DELAY_SECONDS) - def _put_bind_card(self, user_id: str, token: str) -> None: - """Issue the raw bind request (``force_add=false``) for one card.""" + def _put_bind_card(self, user_id: str, token: str, *, force_add: bool = False) -> None: + """Issue the raw card-bind request for one user. + + ``force_add=False`` (default) never displaces another user. ``force_add= + True`` is used only to reclaim a card from a *disabled* holder: the card + cannot be unbound from a deactivated user (UniFi answers card mutations on + a deactivated account with HTTP 404 "no-man zone"), so the only way to + move it is to force the new bind, which reassigns it. + """ self._request( "PUT", f"/api/v1/developer/users/{user_id}/nfc_cards", - json={"token": token, "force_add": False}, + json={"token": token, "force_add": force_add}, ) def _bind_nfc_card(self, user_id: str, token: str, card_id: int) -> None: @@ -634,9 +641,12 @@ def _bind_nfc_card(self, user_id: str, token: str, card_id: int) -> None: the ``fetch_users()`` snapshot: * If the holder is a *disabled* UniFi user, the card is theirs only as - dead state — door-sync unbinds it from them and retries the bind, so a - recycled card stuck on a deactivated member is reclaimed automatically. - The reclaim is logged (a warning), not counted as a failure. + dead state — door-sync force-reassigns it to this member (``force_add= + True``), so a recycled card stuck on a deactivated member is reclaimed + automatically. It cannot be unbound from the disabled holder first + (UniFi rejects card mutations on a deactivated user), so forcing the + new bind is the only path. The reclaim is logged (a warning), not + counted as a failure. * Otherwise (an *active* user, or a holder not in the snapshot), the card is left alone and the error is re-raised naming the holder, so the alert is actionable. @@ -666,19 +676,15 @@ def _bind_nfc_card(self, user_id: str, token: str, card_id: int) -> None: and holder.user_id != user_id and _holder_is_disabled(holder) ): - # The card is dead state on a deactivated user; take it back. + # The card is dead state on a deactivated user; force-reassign it + # to this member. (It can't be unbound from the disabled holder + # first — UniFi 404s card mutations on a deactivated account.) logger.warning( "card %s is bound to disabled %s; reclaiming it for this member", _redact(card_id), _describe_card_holder(holder), ) - self._request( - "DELETE", - f"/api/v1/developer/users/{holder.user_id}/nfc_cards/delete", - json={"token": token}, - ) - time.sleep(self._INTER_CALL_DELAY_SECONDS) - self._put_bind_card(user_id, token) + self._put_bind_card(user_id, token, force_add=True) # The card has moved; drop the stale holder so a later same-cycle # lookup on this token doesn't misreport it. self._holder_by_token.pop(token, None) @@ -734,6 +740,21 @@ def _apply_deactivate(self, diff: Diff) -> None: unifi_user.contact_id, ) continue + # Free the departed member's card(s) *before* deactivating, so the + # number can be reassigned. This must happen while the account is + # still active: UniFi rejects card mutations on a deactivated user + # (HTTP 404 "no-man zone"). Best-effort — a cleanup failure must not + # block cutting access; any card left behind is reclaimed (force_add) + # when the number is next assigned (see _bind_nfc_card). + try: + self._delete_cards_for_contact(user_id, unifi_user.contact_id) + except UnifiClientError as card_exc: + logger.warning( + "contact %d: could not free card before deactivation (%s); " + "it will be reclaimed when the number is next assigned", + unifi_user.contact_id, + card_exc, + ) try: self._request( "PUT", @@ -741,14 +762,6 @@ def _apply_deactivate(self, diff: Diff) -> None: json={"status": "DEACTIVATED"}, ) time.sleep(self._INTER_CALL_DELAY_SECONDS) - # Free the departed member's card(s) so the number can be - # reassigned. A deactivated user that kept its card would hold it - # indefinitely, and a recycled card could never be bound to the - # new member (CODE_CREDS_NFC_HAS_BIND_USER). Deactivation (cutting - # access) runs first so a card-delete failure never leaves an - # active record; if a delete fails here it's recorded and alerts, - # and any leftover binding is surfaced by _bind_nfc_card on reuse. - self._delete_cards_for_contact(user_id, unifi_user.contact_id) except UnifiClientError as exc: self._record_apply_failure(unifi_user.contact_id, exc) continue diff --git a/tests/test_unifi_client.py b/tests/test_unifi_client.py index 8de830c..debc858 100644 --- a/tests/test_unifi_client.py +++ b/tests/test_unifi_client.py @@ -1122,7 +1122,11 @@ def test_apply_deactivate_sets_status( def test_apply_deactivate_removes_nfc_card( httpx_mock: HTTPXMock, monkeypatch: pytest.MonkeyPatch, make_client: Callable[..., UnifiClient] ) -> None: - """Deactivating a carded user also DELETEs the card so it can be reused.""" + """Deactivating a carded user frees the card (DELETE) before setting status. + + The card must be removed while the account is still active — UniFi rejects + card mutations on a deactivated user. + """ monkeypatch.setattr("door_sync.unifi.client.time.sleep", lambda _: None) client = make_client() @@ -1156,14 +1160,20 @@ def test_apply_deactivate_removes_nfc_card( client.apply(_diff(to_deactivate=(fetched[0],))) - # Status set to DEACTIVATED ... + writes = [ + (r.method, r.url.path) for r in httpx_mock.get_requests() if r.method in ("PUT", "DELETE") + ] + # The card is freed first (while active), then the account is deactivated. + assert writes == [ + ("DELETE", "/api/v1/developer/users/uuid-42/nfc_cards/delete"), + ("PUT", "/api/v1/developer/users/uuid-42"), + ] put_req = next( r for r in httpx_mock.get_requests() if r.method == "PUT" and r.url.path.endswith("/users/uuid-42") ) assert _json.loads(put_req.content) == {"status": "DEACTIVATED"} - # ... and the card freed for reuse. delete_req = next( r for r in httpx_mock.get_requests() @@ -1308,13 +1318,8 @@ def test_apply_bind_conflict_reclaims_card_from_disabled_holder( "data": None, }, ) - # ... door-sync unbinds it from the disabled holder ... - httpx_mock.add_response( - method="DELETE", - url="https://192.0.2.1:12445/api/v1/developer/users/uuid-5000/nfc_cards/delete", - json={"code": "SUCCESS", "msg": "success", "data": None}, - ) - # ... then the retried bind succeeds. + # ... so door-sync force-reassigns it (the card can't be DELETEd off a + # deactivated user); the forced bind succeeds. httpx_mock.add_response( method="PUT", url="https://192.0.2.1:12445/api/v1/developer/users/uuid-7590/nfc_cards", @@ -1326,18 +1331,18 @@ def test_apply_bind_conflict_reclaims_card_from_disabled_holder( # No raise: the card is reclaimed and bound. client.apply(_diff(to_update_credential=((resolved, target),))) - # The card was unbound from the disabled holder ... - delete_req = next( - r for r in httpx_mock.get_requests() if r.method == "DELETE" and "uuid-5000" in r.url.path - ) - assert _json.loads(delete_req.content) == {"token": "tok-1234"} - # ... and bound to 7590 twice (the rejected attempt + the successful retry). + # No DELETE is attempted — card mutations on a deactivated user are rejected. + assert not [r for r in httpx_mock.get_requests() if r.method == "DELETE"] + # Two binds to 7590: the rejected force_add=false, then a force_add=true retry. binds = [ - r + _json.loads(r.content) for r in httpx_mock.get_requests() if r.method == "PUT" and r.url.path.endswith("/users/uuid-7590/nfc_cards") ] - assert len(binds) == 2 + assert binds == [ + {"token": "tok-1234", "force_add": False}, + {"token": "tok-1234", "force_add": True}, + ] # The reclaim is logged and redacted (last-4 only). logged = "\n".join(r.getMessage() for r in caplog.records) assert "reclaiming" in logged @@ -2127,11 +2132,11 @@ def test_apply_executes_deactivate_update_credential_update_policy_add_order( if r.method in ("PUT", "POST", "DELETE") and "/credentials/nfc_cards/import" not in r.url.path ] - # Expected order: deactivate(100) sets status then frees its card, - # update_credential(101 DELETE then PUT card), update_policy(102). + # Expected order: deactivate(100) frees its card (while active) then sets + # status, update_credential(101 DELETE then PUT card), update_policy(102). assert write_path_methods == [ - ("PUT", "/api/v1/developer/users/u100"), ("DELETE", "/api/v1/developer/users/u100/nfc_cards/delete"), + ("PUT", "/api/v1/developer/users/u100"), ("DELETE", "/api/v1/developer/users/u101/nfc_cards/delete"), ("PUT", "/api/v1/developer/users/u101/nfc_cards"), ("PUT", "/api/v1/developer/users/u102/access_policies"), From be9c06919d09323fa754602ee04edf06a90d2cf0 Mon Sep 17 00:00:00 2001 From: Ryan Morash Date: Mon, 29 Jun 2026 18:34:57 -0400 Subject: [PATCH 4/6] =?UTF-8?q?fix(unifi):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20drop=20PII=20from=20logs,=20null-safe=20holder,=20cache=20co?= =?UTF-8?q?herence?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on PR #33 (CodeQL + Copilot): - CodeQL py/clear-text-logging-sensitive-data: _describe_card_holder logged the holder's display_name (PII), which the codebase otherwise never logs (it uses contact ids). Drop the name from _CardHolder entirely; identify holders by contact id (sync-managed) or UniFi user id, plus status. - Null-safety in _record_card_holders: use `or ""` for id/status/token. A JSON null status previously stringified to "None", which _holder_is_disabled read as *disabled* — i.e. door-sync could force-reclaim a card from a holder whose state was actually unknown. Now an unknown status is never treated as disabled. - Cache coherence: after force-reassigning a card off a disabled holder, also drop the moved token from that holder's cached card list (_forget_holder_card), so a later same-cycle op on the holder doesn't DELETE a token it no longer owns. Tests updated/added: holder identified by id not name; null-status holder is not reclaimed; reclaim prunes the holder's cached card. Docs updated. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/architecture.md | 2 +- src/door_sync/unifi/client.py | 70 ++++++++++++++++++++--------- tests/test_unifi_client.py | 84 +++++++++++++++++++++++++++++++++-- 3 files changed, 129 insertions(+), 27 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 38f4b42..8d89273 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -256,7 +256,7 @@ config.load() **Card reuse and bind conflicts.** A recycled physical card can be assigned to a new member while it's still bound to an old UniFi user. **Key constraint:** UniFi rejects NFC-card mutations on a *deactivated* user (HTTP 404 "no-man zone") — a card cannot be unbound from a disabled account. door-sync works within that at two points so `CODE_CREDS_NFC_HAS_BIND_USER` doesn't strand a member: - **On deactivation** (proactive): a departing user's NFC card(s) are deleted *before* the status is set to `DEACTIVATED`, i.e. while the account is still active and the delete is permitted. This is best-effort — a cleanup failure is logged but never blocks cutting access — so the next member assigned that number binds cleanly. - - **On bind conflict** (reactive): if a bind is still rejected because the card is bound elsewhere, door-sync inspects the current holder. If that holder is a **disabled** UniFi user (the card is dead state — e.g. a member deactivated before cards were freed on deactivation, or where the proactive cleanup didn't run), door-sync **force-reassigns** the card to the new member (`force_add: true`) and logs a redacted warning; the reclaim is not a failure. Forcing the bind is the only path here — the card can't be unbound from the disabled holder first. If the holder is **active** (or not in the fetched snapshot), the card is left alone and the bind fails as a per-user error (it does not block the rest of the cycle) whose message *names the current holder*: contact id for a sync-managed user, otherwise the UniFi user id, plus display name and status. + - **On bind conflict** (reactive): if a bind is still rejected because the card is bound elsewhere, door-sync inspects the current holder. If that holder is a **disabled** UniFi user (the card is dead state — e.g. a member deactivated before cards were freed on deactivation, or where the proactive cleanup didn't run), door-sync **force-reassigns** the card to the new member (`force_add: true`) and logs a redacted warning; the reclaim is not a failure. Forcing the bind is the only path here — the card can't be unbound from the disabled holder first. If the holder is **active** (or not in the fetched snapshot), the card is left alone and the bind fails as a per-user error (it does not block the rest of the cycle) whose message *identifies the current holder*: contact id for a sync-managed user, otherwise the UniFi user id, plus status. The member's **name is never logged** (PII stays out of logs/alerts, consistent with the rest of door-sync); the contact id resolves to the member in CiviCRM if an operator needs it. `force_add` is `false` for every normal bind — door-sync never displaces an *active* user. `force_add: true` is used only to reclaim a card from a confirmed-**disabled** holder. The card number stays redacted everywhere (§11). 7. **Log + persist state** — write audit entries; write last-success timestamp atomically (write to temp, fsync, rename). diff --git a/src/door_sync/unifi/client.py b/src/door_sync/unifi/client.py index 56a6411..80db947 100644 --- a/src/door_sync/unifi/client.py +++ b/src/door_sync/unifi/client.py @@ -66,13 +66,12 @@ class _CardHolder: """Who a card token is currently bound to in UniFi. Captured for every user during ``fetch_users()`` — sync-managed or not — - purely so a bind conflict (``CODE_CREDS_NFC_HAS_BIND_USER``) can name the - current holder in the failure and alert. Never carries the card number; only - identifies who holds it (architecture §11). + purely so a bind conflict (``CODE_CREDS_NFC_HAS_BIND_USER``) can identify the + current holder in the failure and alert. Identifies by id only — never the + member's name (PII stays out of logs) or the card number (architecture §11). """ user_id: str - display_name: str employee_number: str status: str @@ -420,8 +419,13 @@ def _record_card_holders(self, row: dict[str, Any]) -> None: Runs for *every* /users row, including admin-managed users without an `employee_number` (which `_row_to_unifi_user` drops), so a later bind - conflict can name whoever currently holds the card. Diagnostic only — it - never affects reconciliation. + conflict can identify whoever currently holds the card. Diagnostic only — + it never affects reconciliation. + + Uses ``or ""`` rather than ``.get(k, "")`` throughout: a JSON ``null`` + field would otherwise stringify to ``"None"``, which would record a bogus + token and — worse — make ``_holder_is_disabled`` read an unknown status + as disabled (and so eligible for force-reclaim). Args: row: Raw user dict from the UniFi API. @@ -429,19 +433,15 @@ def _record_card_holders(self, row: dict[str, Any]) -> None: cards = row.get("nfc_cards") or [] if not isinstance(cards, list): return - first_name = str(row.get("first_name", "")) - last_name = str(row.get("last_name", "")) - display_name = " ".join(part for part in [first_name, last_name] if part).strip() holder = _CardHolder( - user_id=str(row.get("id", "")), - display_name=display_name, + user_id=str(row.get("id") or ""), employee_number=str(row.get("employee_number") or ""), - status=str(row.get("status", "")), + status=str(row.get("status") or ""), ) for card in cards: if not isinstance(card, dict): continue - token = str(card.get("token", "")) + token = str(card.get("token") or "") if token: self._holder_by_token[token] = holder @@ -685,9 +685,11 @@ def _bind_nfc_card(self, user_id: str, token: str, card_id: int) -> None: _describe_card_holder(holder), ) self._put_bind_card(user_id, token, force_add=True) - # The card has moved; drop the stale holder so a later same-cycle - # lookup on this token doesn't misreport it. + # The card has moved off the holder; forget it on both caches so a + # later same-cycle operation on the holder (e.g. a reactivation) + # doesn't try to DELETE a token it no longer owns. self._holder_by_token.pop(token, None) + self._forget_holder_card(holder, token) time.sleep(self._INTER_CALL_DELAY_SECONDS) return who = ( @@ -701,6 +703,29 @@ def _bind_nfc_card(self, user_id: str, token: str, card_id: int) -> None: ) from exc time.sleep(self._INTER_CALL_DELAY_SECONDS) + def _forget_holder_card(self, holder: _CardHolder, token: str) -> None: + """Drop a reclaimed token from the holder's cached card list. + + After a card is force-reassigned away from a disabled holder, that + holder's cached cards (from ``fetch_users()``) are stale. Only + sync-managed holders are cached by contact id; a non-sync holder has no + entry to prune. + + Args: + holder: The disabled holder the card was reclaimed from. + token: The token that was moved to another user. + """ + emp = holder.employee_number + if not (emp.isascii() and emp.isdigit()): + return + contact_id = int(emp) + cards = self._nfc_cards_by_contact.get(contact_id) + if not cards: + return + self._nfc_cards_by_contact[contact_id] = [ + card for card in cards if str(card.get("token") or "") != token + ] + def _apply_update_policy(self, diff: Diff) -> None: for resolved, _unifi_user in diff.to_update_policy: user_id = self._unifi_user_id_by_contact.get(resolved.contact_id) @@ -1248,17 +1273,18 @@ def _holder_is_disabled(holder: _CardHolder) -> bool: def _describe_card_holder(holder: _CardHolder) -> str: """Render a card's current holder for a bind-conflict error/alert. - A sync-managed holder (positive integer ``employee_number``) is named by its - CiviCRM contact id; anyone else (a manually-enrolled admin card, no usable - employee number) is named by UniFi user id and flagged as not sync-managed. - Never includes the card number — only who holds it (architecture §11). + A sync-managed holder (positive integer ``employee_number``) is identified by + its CiviCRM contact id; anyone else (a manually-enrolled admin card, no usable + employee number) by UniFi user id, flagged as not sync-managed. Identified by + id only — never the member's name (PII stays out of logs/alerts), never the + card number (architecture §11). The contact id resolves to the member in + CiviCRM if an operator needs the name. """ - name = holder.display_name or "(no name)" status = holder.status or "unknown status" emp = holder.employee_number if emp.isascii() and emp.isdigit() and int(emp) > 0: - return f"contact={emp} ('{name}', {status})" - return f"a non-sync UniFi user '{name}' (id={holder.user_id}, {status})" + return f"contact={emp} ({status})" + return f"a non-sync UniFi user (id={holder.user_id or '?'}, {status})" def _backoff_seconds(attempt: int) -> float: diff --git a/tests/test_unifi_client.py b/tests/test_unifi_client.py index debc858..1319110 100644 --- a/tests/test_unifi_client.py +++ b/tests/test_unifi_client.py @@ -1250,17 +1250,19 @@ def test_apply_bind_conflict_active_holder_names_and_raises( assert not [r for r in httpx_mock.get_requests() if r.method == "DELETE"] msg = str(exc_info.value) - # The failing member and the current holder are both named. + # The failing member and the current holder are both identified — by id, not + # by the holder's name (member PII must stay out of logs/alerts). assert "contact=7590" in msg - assert "non-sync UniFi user 'Front Desk'" in msg - assert "uuid-admin" in msg + assert "non-sync UniFi user (id=uuid-admin" in msg assert "ACTIVE" in msg + assert "Front Desk" not in msg # Card is redacted; the raw number never leaks except as the ****NNNN form. assert "****1234" in msg assert "1234" not in msg.replace("****1234", "") # The per-contact error log carries the holder too (the operator's signal). logged = "\n".join(r.getMessage() for r in caplog.records) assert "uuid-admin" in logged + assert "Front Desk" not in logged def test_apply_bind_conflict_reclaims_card_from_disabled_holder( @@ -1343,12 +1345,86 @@ def test_apply_bind_conflict_reclaims_card_from_disabled_holder( {"token": "tok-1234", "force_add": False}, {"token": "tok-1234", "force_add": True}, ] - # The reclaim is logged and redacted (last-4 only). + # The reclaim is logged and redacted (last-4 only); no member name leaks. logged = "\n".join(r.getMessage() for r in caplog.records) assert "reclaiming" in logged assert "contact=5000" in logged + assert "Old Member" not in logged assert "****1234" in logged assert "1234" not in logged.replace("****1234", "") + # The moved card is forgotten on the holder's cached list, so a later + # same-cycle op on contact 5000 won't try to DELETE a token it no longer owns. + holder_cards = client._nfc_cards_by_contact.get(5000, []) + assert all(c.get("token") != "tok-1234" for c in holder_cards) + + +def test_apply_bind_conflict_holder_with_null_status_is_not_reclaimed( + httpx_mock: HTTPXMock, + monkeypatch: pytest.MonkeyPatch, + make_client: Callable[..., UnifiClient], +) -> None: + """A holder whose status is JSON null is treated as unknown, not disabled. + + A null status must not become the string "None" and read as disabled — that + would force-reclaim a card from a holder whose state we couldn't confirm. + """ + monkeypatch.setattr("door_sync.unifi.client.time.sleep", lambda _: None) + + client = make_client() + + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/credentials/nfc_cards/tokens?page_num=1&page_size=100", + json=_card_tokens_page([(1234, "tok-1234")]), + ) + # Holder 8000 has a JSON-null status (unknown), and holds tok-1234. + null_status_holder = { + "id": "uuid-8000", + "first_name": "Null", + "last_name": "Status", + "employee_number": "8000", + "status": None, + "nfc_cards": [{"id": "300001", "token": "tok-1234", "type": "id_card"}], + "access_policy_ids": ["pol-1"], + } + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/users?page_num=1&page_size=100&expand[]=access_policy", + json=_users_page( + [ + null_status_holder, + _user_row( + contact_id=7590, user_id="uuid-7590", first_name="Member", last_name="7590" + ), + ] + ), + ) + fetched = client.fetch_users() + target = next(u for u in fetched if u.contact_id == 7590) + + httpx_mock.add_response( + method="PUT", + url="https://192.0.2.1:12445/api/v1/developer/users/uuid-7590/nfc_cards", + json={ + "code": "CODE_CREDS_NFC_HAS_BIND_USER", + "msg": "This NFC card is already assigned to another user.", + "data": None, + }, + ) + + resolved = _resolved(contact_id=7590, card_id=1234) + with pytest.raises(UnifiClientError) as exc_info: + client.apply(_diff(to_update_credential=((resolved, target),))) + + # Not reclaimed: exactly one bind (force_add=false), no force_add retry. + binds = [ + _json.loads(r.content) + for r in httpx_mock.get_requests() + if r.method == "PUT" and r.url.path.endswith("/users/uuid-7590/nfc_cards") + ] + assert binds == [{"token": "tok-1234", "force_add": False}] + # Holder is named (unknown status), not force-stolen from. + assert "contact=8000" in str(exc_info.value) def test_apply_update_credential_swaps_card( From 453dc3baebd8982a1740220132a6f8f9f5ec5063 Mon Sep 17 00:00:00 2001 From: Ryan Morash Date: Mon, 29 Jun 2026 18:40:04 -0400 Subject: [PATCH 5/6] docs: document card-reuse/reclaim behavior and the no-PII-names convention Round out the documentation for the card-reuse work: - conventions.rst: add the "no member names in logs/alerts" convention alongside card-ID redaction (identify by contact_id; show-diff CLI may still print names as direct operator output). - unifi-client-design spec: note the bind-conflict handling in the apply-flow step; align the force_add note to "identify by id, not name". - manual-test-plan: 4e now verifies the card is freed on deactivation; new 4f covers recycled-card reassignment, reclaim from a disabled holder (the production case), and the active-holder guard. - CLAUDE.md: add the no-member-names hard rule next to card-ID redaction. Docs only; no code change. Co-Authored-By: Claude Opus 4.8 (1M context) --- CLAUDE.md | 1 + docs/architecture/conventions.rst | 8 +++++ .../specs/2026-05-22-unifi-client-design.md | 4 +-- .../specs/2026-05-27-manual-test-plan.md | 29 +++++++++++++++++-- 4 files changed, 38 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c03edab..f929f2f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -32,6 +32,7 @@ All tooling goes through `uv run` — the venv is managed by uv, not pip. - **Frozen dataclasses.** All domain models in `models.py` are `@dataclass(frozen=True)`. Never mutate; construct a new instance. - **Strict layering.** Nothing imports `orchestrator` except `scheduler` and (future) `webhook`. See dependency table in architecture.md §4. - **Card ID redaction.** Logs show last-4 only. Never log a full card ID at any level (architecture.md §11). +- **No member names in logs/alerts.** Operational and audit log streams identify members by `contact_id` (and, for unmanaged UniFi accounts, user id) — never by name. CodeQL's `py/clear-text-logging-sensitive-data` flags `display_name` as PII. The interactive `show-diff` CLI may print names (direct operator output, not a log). See architecture/conventions.rst. - **Dry-run is sacred.** Dry-run flips a flag inside `UnifiClient` that turns writes into no-ops. Pure modules behave identically in dry-run and live — do not branch on dry-run in pure code. - **Fail-secure on safety guards.** Any guard firing means zero writes that cycle. No partial application. - **Crash logging uses `_logger.error("...", exc_info=exc)`, not `_logger.exception(...)`.** `exception()` reads `sys.exc_info()`, which is `(None, None, None)` outside an active `except` clause — the traceback would be silently dropped. Python 3.5+ accepts an exception instance directly via `exc_info=`. See `orchestrator.handle_crash` for the reference impl. diff --git a/docs/architecture/conventions.rst b/docs/architecture/conventions.rst index d71b410..544594a 100644 --- a/docs/architecture/conventions.rst +++ b/docs/architecture/conventions.rst @@ -66,6 +66,14 @@ Card IDs are security-sensitive. They appear in audit logs and operational logs as last-4-digits only (e.g., ``****1234``). Full card IDs are never logged at any level. +Member names are likewise kept out of the operational and audit log streams. +Log records and alerts identify a member by CiviCRM ``contact_id`` (and, for an +unmanaged UniFi account, its user id) — never by name — so member PII does not +accumulate in the journal. A ``contact_id`` resolves back to the member in +CiviCRM when an operator needs the name. (The interactive ``show-diff`` CLI does +print names to the operator's terminal on demand; that is direct operator +output, not a persisted log.) + Error Handling -------------- diff --git a/docs/superpowers/specs/2026-05-22-unifi-client-design.md b/docs/superpowers/specs/2026-05-22-unifi-client-design.md index 8e8b8ee..a10dc40 100644 --- a/docs/superpowers/specs/2026-05-22-unifi-client-design.md +++ b/docs/superpowers/specs/2026-05-22-unifi-client-design.md @@ -154,7 +154,7 @@ Pagination follows the same shape as `CivicrmClient`: a `_MAX_PAGES = 1_000` cei - If `display_name` changed: `PUT /users/:id` body `{first_name, last_name}` (split via `_split_name`). - If `card_id` changed: - If the user has any existing card: `DELETE /users/:id/nfc_cards/delete` with body `{"token": }` (resolves old token from the cached `nfc_cards` map populated by `fetch_users()`). - - `PUT /users/:id/nfc_cards` with body `{"token": , "force_add": false}`. Token is looked up in the NFC-token map (post-import). + - `PUT /users/:id/nfc_cards` with body `{"token": , "force_add": false}`. Token is looked up in the NFC-token map (post-import). On a `CODE_CREDS_NFC_HAS_BIND_USER` conflict the card is held by another user: if that holder is *disabled* it is reclaimed with a forced re-bind, otherwise the holder is identified (by id) in a per-user error — see the `force_add` note in "Open questions / decisions". The same bind path (and conflict handling) is used in step 6. 5. **Update policy** — for each `(resolved, unifi_user)` in `diff.to_update_policy`: - `PUT /users/:id/access_policies` with body `{"access_policy_ids": []}`. 6. **Add** — for each `resolved` in `diff.to_add`: @@ -398,5 +398,5 @@ Test changes: extend `_write_minimal_valid` in `tests/test_config.py` to include - **Retry budget of 3 / inter-call delay of 75ms.** Both are constants in `client.py`. Promotable to config fields if real-world tuning demands. - **Whether to compress the CSV upload.** Not done; CSVs are kilobyte-scale. - **First-name placeholder for single-word display names.** `"—"` (em-dash) — chosen to be visibly distinct in the UniFi UI so an operator notices and edits the CiviCRM record. Open to bikeshed. -- **When `force_add: true` is used.** Never for a normal bind — door-sync doesn't displace an *active* user. It is used in exactly one case: reclaiming a card from a confirmed-**disabled** holder on a `CODE_CREDS_NFC_HAS_BIND_USER` conflict. The card can't be unbound from the disabled holder first (UniFi 404s card mutations on a deactivated user), so forcing the new bind is the only way to move it; door-sync gates this on having checked the holder's status in the fetch snapshot. An **active** holder (or one not in the snapshot) is left untouched and the bind fails as a per-user error naming the holder, so an operator resolves it deliberately. A *blanket* `force_add: true` remains rejected: it would clobber active admin-managed bindings. +- **When `force_add: true` is used.** Never for a normal bind — door-sync doesn't displace an *active* user. It is used in exactly one case: reclaiming a card from a confirmed-**disabled** holder on a `CODE_CREDS_NFC_HAS_BIND_USER` conflict. The card can't be unbound from the disabled holder first (UniFi 404s card mutations on a deactivated user), so forcing the new bind is the only way to move it; door-sync gates this on having checked the holder's status in the fetch snapshot. An **active** holder (or one not in the snapshot) is left untouched and the bind fails as a per-user error that identifies the holder by id (contact id, else UniFi user id — never the member's name; PII stays out of logs), so an operator resolves it deliberately. A *blanket* `force_add: true` remains rejected: it would clobber active admin-managed bindings. - **Audit-log entries for UniFi calls.** Out of scope; `audit.py` (a separate slice) owns audit logging. The orchestrator passes the diff and the result there. diff --git a/docs/superpowers/specs/2026-05-27-manual-test-plan.md b/docs/superpowers/specs/2026-05-27-manual-test-plan.md index 0240de8..8f98bd7 100644 --- a/docs/superpowers/specs/2026-05-27-manual-test-plan.md +++ b/docs/superpowers/specs/2026-05-27-manual-test-plan.md @@ -176,9 +176,32 @@ 1. In CiviCRM, expire or remove a test member's active membership (or clear their card ID). 2. Run `show-diff` — confirm `to_deactivate`. -3. Run `--once` — verify in UniFi admin: user is deactivated (not deleted, just inactive). +3. Run `--once` — verify in UniFi admin: the user's NFC card is **removed** (freed for reuse) and the user is deactivated (not deleted, just inactive). The card delete is issued *before* the status change. -### 4f. Full Sync +### 4f. Card Reuse and Reclaim + +Validates that a recycled physical card can move to a new member — the path that +previously crashed every cycle with `CODE_CREDS_NFC_HAS_BIND_USER`. + +**Recycled card, holder freed on deactivation:** + +1. Take member A holding card X in UniFi. Deactivate A (4e) and confirm card X was removed. +2. In CiviCRM, assign card X to a different member B. Run `show-diff` (B in `to_add` or `to_update_credential`), then `--once`. +3. Verify in UniFi admin: card X is bound to B, and B is active. + +**Reclaim from an already-disabled holder** (the production case — a card still +stuck on a user that was deactivated *before* this behavior shipped): + +4. Arrange member C **deactivated** in UniFi but still holding card Y (set this up in the UniFi admin UI if needed). +5. In CiviCRM, assign card Y to member D. Run `--once`. +6. Verify the operational log shows a single `WARNING ... reclaiming it for this member` line — redacted card (`****NNNN`) and `contact=`, **no member name** — and that UniFi admin now shows card Y bound to D. No alert/crash for D. + +**Active-holder guard** (door-sync must NOT displace an active user): + +7. Arrange card Z bound to an *active* UniFi account (e.g. a manually-enrolled admin). Assign card Z to a member in CiviCRM and run `--once`. +8. Verify the cycle records a per-user failure that identifies the holder by id (not name) and leaves card Z on the active account untouched. + +### 4g. Full Sync 1. Remove the tier rule restriction from step 4a — allow all membership types. 2. Run `show-diff` to review the full diff. @@ -188,6 +211,8 @@ ### Pass Criteria - All five write operations work correctly: add user, bind NFC card, assign policy, update credential, deactivate user. +- Deactivation frees the member's NFC card (the number becomes reusable). +- A recycled card moves to its new member; a card stuck on a disabled holder is reclaimed (single redacted warning, no alert); a card on an *active* holder is left untouched with an actionable per-user error. - Idempotency holds after each operation (re-run produces empty diff). - Audit log and state file reflect each cycle accurately. - No duplicate users or credentials created. From 4851b9132f88a60f3b79dc9bf30ae8037dd59cbb Mon Sep 17 00:00:00 2001 From: Ryan Morash Date: Mon, 29 Jun 2026 18:46:18 -0400 Subject: [PATCH 6/6] fix(unifi): refresh holder cache to DEACTIVATED after deactivation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the pre-deactivation card delete fails, the card stays bound to the user we then deactivate, but _holder_by_token still carries the ACTIVE snapshot taken at fetch time. A same-cycle reassignment of that number would read ACTIVE and refuse to reclaim — a spurious per-user failure. After the status change succeeds, refresh the contact's cached card holders to DEACTIVATED (user_id guarded), so the still-bound card is force-reclaimed correctly. Test: deactivation with a failing card delete, then a same-cycle reassignment of that card is reclaimed (force_add). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/door_sync/unifi/client.py | 26 ++++++++++- tests/test_unifi_client.py | 84 +++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 1 deletion(-) diff --git a/src/door_sync/unifi/client.py b/src/door_sync/unifi/client.py index 80db947..74b4020 100644 --- a/src/door_sync/unifi/client.py +++ b/src/door_sync/unifi/client.py @@ -17,7 +17,7 @@ import ssl import time from collections.abc import Callable, Iterable -from dataclasses import dataclass +from dataclasses import dataclass, replace from types import TracebackType from typing import Any from urllib.parse import urlsplit @@ -726,6 +726,26 @@ def _forget_holder_card(self, holder: _CardHolder, token: str) -> None: card for card in cards if str(card.get("token") or "") != token ] + def _mark_holder_deactivated(self, contact_id: int, user_id: str) -> None: + """Refresh a just-deactivated user's cached card holders to DEACTIVATED. + + ``_holder_by_token`` captures each holder's status at ``fetch_users()`` + time (ACTIVE for a user now being deactivated). If the pre-deactivation + card delete failed, the card is still bound; without this refresh a + same-cycle reassignment of that number would read the stale ACTIVE + snapshot and refuse to reclaim. The ``user_id`` guard avoids touching a + token already moved to a different holder. + + Args: + contact_id: The contact whose cached card holders are refreshed. + user_id: The deactivated user's id. + """ + for card in self._nfc_cards_by_contact.get(contact_id, []): + token = str(card.get("token") or "") + holder = self._holder_by_token.get(token) + if holder is not None and holder.user_id == user_id: + self._holder_by_token[token] = replace(holder, status="DEACTIVATED") + def _apply_update_policy(self, diff: Diff) -> None: for resolved, _unifi_user in diff.to_update_policy: user_id = self._unifi_user_id_by_contact.get(resolved.contact_id) @@ -787,6 +807,10 @@ def _apply_deactivate(self, diff: Diff) -> None: json={"status": "DEACTIVATED"}, ) time.sleep(self._INTER_CALL_DELAY_SECONDS) + # The user is now deactivated; keep the holder cache in step so a + # still-bound card (delete may have failed above) is force-reclaimed + # — not mis-read as ACTIVE — if reassigned later this cycle. + self._mark_holder_deactivated(unifi_user.contact_id, user_id) except UnifiClientError as exc: self._record_apply_failure(unifi_user.contact_id, exc) continue diff --git a/tests/test_unifi_client.py b/tests/test_unifi_client.py index 1319110..21e8912 100644 --- a/tests/test_unifi_client.py +++ b/tests/test_unifi_client.py @@ -1427,6 +1427,90 @@ def test_apply_bind_conflict_holder_with_null_status_is_not_reclaimed( assert "contact=8000" in str(exc_info.value) +def test_deactivate_with_failed_card_delete_allows_same_cycle_reclaim( + httpx_mock: HTTPXMock, + monkeypatch: pytest.MonkeyPatch, + make_client: Callable[..., UnifiClient], +) -> None: + """A failed card delete during deactivation still permits a same-cycle reclaim. + + The holder cache is refreshed to DEACTIVATED after the status change, so the + still-bound card is force-reclaimed rather than mis-read as the stale ACTIVE + snapshot captured at fetch time. + """ + monkeypatch.setattr("door_sync.unifi.client.time.sleep", lambda _: None) + + client = make_client() + + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/credentials/nfc_cards/tokens?page_num=1&page_size=100", + json=_card_tokens_page([(1234, "tok-1234")]), + ) + # H (5000) is ACTIVE and holds tok-1234 and is departing; M (7590) is being + # assigned that same card this cycle. + httpx_mock.add_response( + method="GET", + url="https://192.0.2.1:12445/api/v1/developer/users?page_num=1&page_size=100&expand[]=access_policy", + json=_users_page( + [ + _user_row(contact_id=5000, user_id="uuid-5000", nfc_token="tok-1234"), + _user_row( + contact_id=7590, user_id="uuid-7590", first_name="Member", last_name="7590" + ), + ] + ), + ) + fetched = client.fetch_users() + holder = next(u for u in fetched if u.contact_id == 5000) + target = next(u for u in fetched if u.contact_id == 7590) + + # The pre-deactivation card delete fails (error envelope) ... + httpx_mock.add_response( + method="DELETE", + url="https://192.0.2.1:12445/api/v1/developer/users/uuid-5000/nfc_cards/delete", + json={"code": "CODE_NOT_FOUND", "msg": "no-man zone", "data": None}, + ) + # ... but the deactivation status change succeeds. + httpx_mock.add_response( + method="PUT", + url="https://192.0.2.1:12445/api/v1/developer/users/uuid-5000", + json={"code": "SUCCESS", "msg": "success", "data": None}, + ) + # M's bind: rejected (card still on the now-deactivated H), then force-reclaimed. + httpx_mock.add_response( + method="PUT", + url="https://192.0.2.1:12445/api/v1/developer/users/uuid-7590/nfc_cards", + json={ + "code": "CODE_CREDS_NFC_HAS_BIND_USER", + "msg": "This NFC card is already assigned to another user.", + "data": None, + }, + ) + httpx_mock.add_response( + method="PUT", + url="https://192.0.2.1:12445/api/v1/developer/users/uuid-7590/nfc_cards", + json={"code": "SUCCESS", "msg": "success", "data": None}, + ) + + diff = _diff( + to_deactivate=(holder,), + to_update_credential=((_resolved(7590, card_id=1234), target),), + ) + # No raise: the still-bound card is reclaimed despite the failed delete. + client.apply(diff) + + binds = [ + _json.loads(r.content) + for r in httpx_mock.get_requests() + if r.method == "PUT" and r.url.path.endswith("/users/uuid-7590/nfc_cards") + ] + assert binds == [ + {"token": "tok-1234", "force_add": False}, + {"token": "tok-1234", "force_add": True}, + ] + + def test_apply_update_credential_swaps_card( httpx_mock: HTTPXMock, monkeypatch: pytest.MonkeyPatch, make_client: Callable[..., UnifiClient] ) -> None: