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.md b/docs/architecture.md index 6450ab3..8d89273 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -252,6 +252,13 @@ 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 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 *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/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 28e3079..a10dc40 100644 --- a/docs/superpowers/specs/2026-05-22-unifi-client-design.md +++ b/docs/superpowers/specs/2026-05-22-unifi-client-design.md @@ -149,12 +149,12 @@ 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`: 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: - 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. -- **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. +- **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. diff --git a/src/door_sync/unifi/client.py b/src/door_sync/unifi/client.py index 2edcd6f..74b4020 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, replace 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,21 @@ 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 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 + employee_number: str + status: str + + class UnifiClient: """Read+write UniFi Access local-API client. @@ -124,6 +144,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 +312,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 +414,37 @@ 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 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. + """ + cards = row.get("nfc_cards") or [] + if not isinstance(cards, list): + return + holder = _CardHolder( + user_id=str(row.get("id") or ""), + employee_number=str(row.get("employee_number") or ""), + status=str(row.get("status") or ""), + ) + for card in cards: + if not isinstance(card, dict): + continue + token = str(card.get("token") or "") + 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,158 @@ 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}, + 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 _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": force_add}, + ) + + def _bind_nfc_card(self, user_id: str, token: str, card_id: int) -> None: + """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: + + * If the holder is a *disabled* UniFi user, the card is theirs only as + 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. + + 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 logs/errors. + + Raises: + 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._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; 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._put_bind_card(user_id, token, force_add=True) + # 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 = ( + _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 _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 _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: @@ -593,16 +785,35 @@ 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", f"/api/v1/developer/users/{user_id}", 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 - 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 +1052,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 +1121,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 +1282,35 @@ 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. + + 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. + """ + status = holder.status or "unknown status" + emp = holder.employee_number + if emp.isascii() and emp.isdigit() and int(emp) > 0: + return f"contact={emp} ({status})" + return f"a non-sync UniFi user (id={holder.user_id or '?'}, {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..21e8912 100644 --- a/tests/test_unifi_client.py +++ b/tests/test_unifi_client.py @@ -1119,6 +1119,398 @@ 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 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() + + # 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],))) + + 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"} + 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_active_holder_names_and_raises( + httpx_mock: HTTPXMock, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, + make_client: Callable[..., UnifiClient], +) -> None: + """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. 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")]), + ) + # 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( + [ + admin_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) + + # 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),))) + + # 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 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 (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( + 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, + }, + ) + # ... 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", + 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),))) + + # 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 = [ + _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}, + ] + # 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_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: @@ -1876,6 +2268,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,9 +2292,10 @@ 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) frees its card (while active) then sets + # status, update_credential(101 DELETE then PUT card), update_policy(102). assert write_path_methods == [ + ("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"),