From f5db83f26c0ba76f2b9ca6a312859ab56df5ebbf Mon Sep 17 00:00:00 2001 From: bonamin Date: Wed, 30 Sep 2026 00:44:34 +0300 Subject: [PATCH] fix(devices): sync users.device_serials when PUT /api/devices/{id} changes user_list DeviceUpdate accepts user_list, so a device PUT could add or remove users without touching their device_serials - leaving the MQTT app ACL stale (a removed user would keep access; an added user would be denied). update_device now diffs the old vs new user_list and, in the same atomic batch as the device write, ArrayUnion/ArrayRemoves the device's serial on each added/removed user, then invalidates their MQTT ACL cache entries. Dangling user references are skipped (updating a missing doc would fail the whole batch). PUTs without user_list take the old single-update path. Covered by tests/test_device_serials_sync.py (fake Firestore). Co-Authored-By: Claude Opus 5.5 --- backend/devices/service.py | 35 ++++++- backend/tests/test_device_serials_sync.py | 118 ++++++++++++++++++++++ 2 files changed, 152 insertions(+), 1 deletion(-) create mode 100644 backend/tests/test_device_serials_sync.py diff --git a/backend/devices/service.py b/backend/devices/service.py index b836e85..29e7c10 100644 --- a/backend/devices/service.py +++ b/backend/devices/service.py @@ -230,12 +230,45 @@ def update_device(device_doc_id: str, data: DeviceUpdate) -> DeviceInDB: update_data[key] = _deep_merge(existing[key], update_data[key]) update_data = _restore_timestamps(update_data) - doc_ref.update(update_data) + + if "user_list" not in update_data: + doc_ref.update(update_data) + else: + # A user_list change must also update each affected user's + # device_serials (MQTT ACL), in the same atomic batch. + from users import service as users_service + + old_ids = {_user_list_entry_id(e) for e in (existing.get("user_list") or [])} - {""} + new_ids = {_user_list_entry_id(e) for e in update_data["user_list"]} - {""} + serial = users_service.device_serial_of(existing) + + batch = db.batch() + batch.update(doc_ref, update_data) + touched = [] + for user_id in (old_ids ^ new_ids): + user_ref = db.collection("users").document(user_id) + user_doc = user_ref.get() + if not user_doc.exists: + continue # dangling reference — nothing to sync + users_service.stage_device_serial_link(batch, user_ref, serial, linked=user_id in new_ids) + touched.append(user_doc.to_dict()) + batch.commit() + for user_data in touched: + users_service.invalidate_mqtt_acl_cache(user_data) updated_doc = doc_ref.get() return _doc_to_device(updated_doc) +def _user_list_entry_id(entry) -> str: + """user_list entries are DocumentReferences, "users/{id}" paths or raw doc IDs.""" + if isinstance(entry, DocumentReference): + return entry.id + if isinstance(entry, str): + return entry.strip().split("/")[-1] + return "" + + def get_device_users(device_doc_id: str) -> list[dict]: """Get users assigned to a device from the device_users sub-collection. diff --git a/backend/tests/test_device_serials_sync.py b/backend/tests/test_device_serials_sync.py new file mode 100644 index 0000000..93f0582 --- /dev/null +++ b/backend/tests/test_device_serials_sync.py @@ -0,0 +1,118 @@ +""" +update_device must keep users.device_serials in sync when a PUT changes the +device's user_list (the MQTT ACL reads device_serials). Firestore is faked. +""" + +import pytest +from google.cloud.firestore_v1 import ArrayRemove, ArrayUnion + +from devices import service as devices_service +from devices.models import DeviceUpdate +from mqtt import app_users + +SERIAL = "BSVSPR-26C13X-STD01R-X7KQA" + + +class _Snap: + def __init__(self, ref, data): + self.reference = ref + self.id = ref.id + self._data = data + self.exists = data is not None + + def to_dict(self): + return dict(self._data) if self._data is not None else None + + +class _Ref: + def __init__(self, db, collection, doc_id): + self._db, self._col, self.id = db, collection, doc_id + + def get(self): + return _Snap(self, self._db.docs[self._col].get(self.id)) + + def update(self, data): + self._db.apply(self._col, self.id, data) + + +class _Batch: + def __init__(self, db): + self._db, self.ops = db, [] + + def update(self, ref, data): + self.ops.append((ref, data)) + + def commit(self): + for ref, data in self.ops: + ref.update(data) + self._db.commits += 1 + + +class _Collection: + def __init__(self, db, name): + self._db, self._name = db, name + + def document(self, doc_id): + return _Ref(self._db, self._name, doc_id) + + +class FakeDB: + def __init__(self): + self.docs = {"devices": {}, "users": {}} + self.commits = 0 + + def collection(self, name): + return _Collection(self, name) + + def batch(self): + return _Batch(self) + + def apply(self, col, doc_id, data): + doc = self.docs[col][doc_id] + for key, val in data.items(): + if isinstance(val, ArrayUnion): + doc[key] = list(doc.get(key) or []) + [v for v in val.values if v not in (doc.get(key) or [])] + elif isinstance(val, ArrayRemove): + doc[key] = [v for v in (doc.get(key) or []) if v not in val.values] + else: + doc[key] = val + + +@pytest.fixture +def db(monkeypatch): + fake = FakeDB() + fake.docs["devices"]["dev1"] = { + "serial_number": SERIAL, "mfg_status": "claimed", "user_list": ["users/alice", "users/bob"], + } + fake.docs["users"]["alice"] = {"uid": "alice", "device_serials": [SERIAL]} + fake.docs["users"]["bob"] = {"uid": "bob", "device_serials": [SERIAL, "OTHER"]} + fake.docs["users"]["carol"] = {"uid": "carol"} + monkeypatch.setattr(devices_service, "get_db", lambda: fake) + return fake + + +def test_put_user_list_syncs_added_and_removed_users(db, monkeypatch): + invalidated = [] + monkeypatch.setattr(app_users, "invalidate", invalidated.append) + + devices_service.update_device("dev1", DeviceUpdate(user_list=["users/alice", "users/carol"])) + + assert db.docs["devices"]["dev1"]["user_list"] == ["users/alice", "users/carol"] + assert db.docs["users"]["alice"]["device_serials"] == [SERIAL] # unchanged + assert db.docs["users"]["bob"]["device_serials"] == ["OTHER"] # removed + assert db.docs["users"]["carol"]["device_serials"] == [SERIAL] # added + assert sorted(invalidated) == ["bob", "carol"] + assert db.commits == 1 # one atomic batch + + +def test_put_user_list_ignores_dangling_user_refs(db): + devices_service.update_device("dev1", DeviceUpdate(user_list=["users/alice", "users/bob", "users/ghost"])) + assert db.docs["devices"]["dev1"]["user_list"] == ["users/alice", "users/bob", "users/ghost"] + assert "ghost" not in db.docs["users"] + + +def test_put_without_user_list_does_not_touch_users(db): + devices_service.update_device("dev1", DeviceUpdate(device_name="Church")) + assert db.docs["devices"]["dev1"]["device_name"] == "Church" + assert db.commits == 0 + assert db.docs["users"]["bob"]["device_serials"] == [SERIAL, "OTHER"]