From 69a434bcdb04f52134df726b8b8d7db89613be29 Mon Sep 17 00:00:00 2001 From: Joshua Date: Wed, 23 Sep 2026 20:57:25 -0400 Subject: [PATCH 1/2] Accept assignee_child_ids on the admin definition edit route PATCH /api/v1/admin/quest-definitions/{id} now takes an optional assignee_child_ids list that replaces the definition's assignee set. The replace runs inside the core edit's single DAO transaction, so a rejected list (empty, duplicate, unknown or inactive child) returns 422 and writes nothing, including any other field in the same body. The existing single regeneration path rebuilds future instances. An omitted field leaves assignees unchanged. Task: 1c8096f6-643d-441b-a61a-892e0d5b3b59 Co-Authored-By: Claude Opus 5.5 --- api/routes_admin.py | 35 +- custom_components/nestquest/core/dao_rules.py | 52 ++- .../nestquest/core/quest_definitions.py | 32 +- tests/test_api_admin_questdef.py | 7 +- tests/test_api_admin_questdef_assignees.py | 366 ++++++++++++++++++ 5 files changed, 460 insertions(+), 32 deletions(-) create mode 100644 tests/test_api_admin_questdef_assignees.py diff --git a/api/routes_admin.py b/api/routes_admin.py index 6fc3009..adff79d 100644 --- a/api/routes_admin.py +++ b/api/routes_admin.py @@ -30,10 +30,10 @@ hand-rolled here), and the call goes to ``set_quest_definition_active``. The title policy, the window-name and due-time policy, the assignee checks, the whole-set window replacement and the never-hard-delete policy (deactivation is the only -removal path) all live in the core. Assignment is NOT settable -through the API: the core's edit path takes no assignees. The edit -route forwards only SUPPLIED fields (``exclude_unset``), like the -children edit. +removal path) all live in the core. The edit route forwards only +SUPPLIED fields (``exclude_unset``), like the children edit; a supplied +``assignee_child_ids`` list REPLACES the whole assignee set inside the +core edit's one transaction (and its one regeneration). The presence routes follow the same pattern over :mod:`nestquest_core.presence_management` — the ONE presence write @@ -182,8 +182,9 @@ deliberately narrow): set_active on an unknown id) is mapped to 404. - A core ``ValueError`` reporting a NON-EXISTENT DEFINITION (edit or set_active on an unknown id — the core prefixes that error with - ``definition_id:``) is mapped to 404. Create's rejected-assignee - error names a child from the BODY, not a path id, so it maps to 422. + ``definition_id:``) is mapped to 404. Create's and edit's + rejected-assignee error names a child from the BODY, not a path id, + so it maps to 422. - Every OTHER core ``ValueError`` — an empty display name, an explicit ``null`` colour/avatar_ref (clearing is not supported), a reorder list that is not a complete permutation of the children (missing, @@ -476,11 +477,12 @@ class AdminQuestDefinitionEditRequest(BaseModel): field stays unchanged. An explicit ``null`` ``description``/ ``icon`` IS passed and CLEARS the stored value (the core supports clearing here, unlike the children edit); an explicit ``null`` - ``rule`` or ``windows`` is passed and rejected by the core. A - supplied ``windows`` list REPLACES the whole window set, and - ``assignees`` are deliberately absent — assignment is not settable - through this route. An omitted (or ``null``) ``skip_on_away`` - keeps the stored value. + ``rule``, ``windows`` or ``assignee_child_ids`` is passed and + rejected by the core. A supplied ``windows`` list REPLACES the + whole window set, and a supplied ``assignee_child_ids`` list + REPLACES the whole assignee set (non-empty, every id an existing + active child — create's contract; a rejection changes nothing). An + omitted (or ``null``) ``skip_on_away`` keeps the stored value. """ title: str | None = None @@ -489,6 +491,7 @@ class AdminQuestDefinitionEditRequest(BaseModel): rule: AdminRuleRequest | None = None windows: list[str | tuple[str, str | None]] | None = None skip_on_away: StrictBool | None = None + assignee_child_ids: list[int] | None = None class AdminQuestDefinitionActiveRequest(BaseModel): @@ -1266,10 +1269,12 @@ async def admin_edit_quest_definition( field is never passed to :func:`nestquest_core.quest_definitions.edit_quest_definition` and stays unchanged; a supplied ``windows`` list replaces the WHOLE - window set in the core, and a supplied ``rule`` is rebuilt into a - ScheduleRule (:func:`_rule_from_request`, validated by the model). - An explicit ``null`` ``rule``/``windows`` reaches the core as None - and is rejected there (422), while an explicit ``null`` + window set in the core, a supplied ``assignee_child_ids`` list + replaces the WHOLE assignee set in the same core transaction, and a + supplied ``rule`` is rebuilt into a ScheduleRule + (:func:`_rule_from_request`, validated by the model). An explicit + ``null`` ``rule``/``windows``/``assignee_child_ids`` reaches the + core as None and is rejected there (422), while an explicit ``null`` ``description``/``icon`` clears the stored value — the core edit path supports clearing, unlike the children edit. An unknown definition id is mapped to 404 and any other core ``ValueError`` diff --git a/custom_components/nestquest/core/dao_rules.py b/custom_components/nestquest/core/dao_rules.py index 669569d..4f005d9 100644 --- a/custom_components/nestquest/core/dao_rules.py +++ b/custom_components/nestquest/core/dao_rules.py @@ -796,8 +796,9 @@ class QuestDefinitionsDao: rule: ScheduleRuleStorage | object = _UNSET, windows: list[tuple[str, str | None]] | object = _UNSET, skip_on_away: bool | object = _UNSET, + assignee_child_ids: list[int] | object = _UNSET, ) -> QuestDefinitionSnapshot: - """Edit a definition's metadata, rule and windows atomically. + """Edit a definition's metadata, rule, windows and assignees atomically. Mirrors :meth:`create_with_rule_and_windows`: everything runs inside ONE transaction under the connection lock, and the @@ -818,11 +819,15 @@ class QuestDefinitionsDao: are deleted and the given set re-inserted, so missing windows are added, changed due times updated and removed windows dropped. + - ``assignee_child_ids`` REPLACES the whole assignee set: every + id is validated as existing-and-active BEFORE any write (the + same check :meth:`add_assignee` applies), then links for + children no longer listed are removed and links for newly + listed children inserted; unchanged links are left alone. - Assignment is deliberately not settable here (multi-assignee is - managed by :meth:`add_assignee`/:meth:`remove_assignee`), and - activation is untouched. Raises ValueError when the definition - does not exist. Edits change future instances only: no + Activation is untouched. Raises ValueError when the definition + does not exist or an assignee is rejected; either way the + transaction rolls back and nothing is written. Edits change future instances only: no ``quest_instances`` or ``completion_events`` row is touched. """ if skip_on_away is not _UNSET: @@ -834,6 +839,9 @@ class QuestDefinitionsDao: raise ValueError( f"quest definition {definition_id} does not exist" ) + if assignee_child_ids is not _UNSET: + for child_id in assignee_child_ids: + await self._validate_assignable_child(child_id) assignments: list[str] = [] parameters: list[object] = [] for column, value in ( @@ -882,6 +890,10 @@ class QuestDefinitionsDao: "VALUES (?, ?, ?)", (definition_id, window, due_time), ) + if assignee_child_ids is not _UNSET: + await self._replace_assignees( + definition_id, assignee_child_ids + ) updated = await self.get(definition_id) assignees = await self.list_assignees(definition_id) window_records = await self.list_windows(definition_id) @@ -897,6 +909,36 @@ class QuestDefinitionsDao: windows=window_records, ) + async def _replace_assignees( + self, definition_id: int, child_ids: list[int] + ) -> None: + """Make the definition's assignee links exactly ``child_ids``. + + Removes the links for children not in ``child_ids`` and inserts + the missing ones. MUST be called inside the connection lock and + an open transaction, after the ids were validated as assignable. + """ + rows = await self._database.fetch_all( + "SELECT child_id FROM quest_definition_assignees " + "WHERE definition_id = ?", + (definition_id,), + ) + current = {row[0] for row in rows} + wanted = set(child_ids) + for child_id in sorted(current - wanted): + await self._database.execute( + "DELETE FROM quest_definition_assignees " + "WHERE definition_id = ? AND child_id = ?", + (definition_id, child_id), + ) + for child_id in child_ids: + if child_id not in current: + await self._database.execute( + "INSERT INTO quest_definition_assignees " + "(definition_id, child_id) VALUES (?, ?)", + (definition_id, child_id), + ) + async def _validate_rule_exists(self, schedule_rule_id: int) -> None: """Raise ValueError unless the rule exists. diff --git a/custom_components/nestquest/core/quest_definitions.py b/custom_components/nestquest/core/quest_definitions.py index 9e347e3..9918311 100644 --- a/custom_components/nestquest/core/quest_definitions.py +++ b/custom_components/nestquest/core/quest_definitions.py @@ -4,8 +4,9 @@ Sits between the Feature 09 service gate and the typed :class:`~.dao_rules.QuestDefinitionsDao`: callers get validation and the all-or-nothing create/edit here, and never touch the DAO SQL directly. :func:`create_quest_definition` is the create path, -:func:`edit_quest_definition` the edit path, :func:`assign_child` and -:func:`unassign_child` the assignment paths, and +:func:`edit_quest_definition` the edit path (which may also replace the +whole assignee set), :func:`assign_child` and +:func:`unassign_child` the single-child assignment paths, and :func:`set_quest_definition_active` the deactivate/reactivate path. There is no permission check here (that is Feature 09). @@ -286,10 +287,11 @@ async def edit_quest_definition( rule: ScheduleRule | object = _UNSET, windows: list[str | tuple[str, str | None]] | object = _UNSET, skip_on_away: bool | None = None, + assignee_child_ids: list[int] | object = _UNSET, today: datetime.date | None = None, horizon_days: int | None = None, ) -> CreatedQuestDefinition: - """Edit a quest definition's metadata, rule and windows atomically. + """Edit a quest definition's metadata, rule, windows and assignees atomically. Arguments default to the sentinel ``_UNSET`` meaning "leave this field alone". A provided ``title`` is validated like create's @@ -302,9 +304,14 @@ async def edit_quest_definition( a ``const.QUEST_WINDOWS`` name, optionally with a strict HH:MM due time), matching create's non-empty contract. A provided ``skip_on_away`` (a real bool) replaces the stored flag; ``None`` - leaves it unchanged. Assignment is NOT - settable here. Raises ValueError when the definition does not - exist or any argument is rejected. + leaves it unchanged. A provided ``assignee_child_ids`` REPLACES the + whole assignee set under create's contract (a non-empty list of + distinct plain ints, each an existing ACTIVE child — the DAO checks + that inside the edit's transaction): newly listed children are + assigned and unlisted ones unassigned. Raises ValueError when the + definition does not exist or any argument is rejected; a rejection + writes nothing, so the stored assignees (and every other field) + stay unchanged. Returns the updated definition together with its decoded rule, assignees and windows — all read back inside the DAO's transaction, @@ -348,6 +355,10 @@ async def edit_quest_definition( if windows is not _UNSET: window_specs = _normalize_windows(windows) + assignee_ids: list[int] | object = _UNSET + if assignee_child_ids is not _UNSET: + assignee_ids = _validate_assignee_ids(assignee_child_ids) + skip_on_away_value: bool | object = _UNSET if skip_on_away is not None: skip_on_away_value = _validate_skip_on_away(skip_on_away) @@ -362,14 +373,17 @@ async def edit_quest_definition( rule=rule_storage, windows=window_specs, skip_on_away=skip_on_away_value, + assignee_child_ids=assignee_ids, ) except ValueError as error: - # The DAO's missing-definition error names only the id; re-raise - # so the public contract names the field the caller passed (matching - # assign/unassign/deactivate). + # The DAO's missing-definition and assignee errors name only the + # id; re-raise so the public contract names the field the caller + # passed (matching create/assign/unassign/deactivate). message = str(error) if message.startswith("quest definition "): raise ValueError(f"definition_id: {message}") from error + if message.startswith("child "): + raise ValueError(f"assignee_child_ids: {message}") from error raise decoded_rule = schedule_rule_from_storage( diff --git a/tests/test_api_admin_questdef.py b/tests/test_api_admin_questdef.py index 9111fb2..3a0f398 100644 --- a/tests/test_api_admin_questdef.py +++ b/tests/test_api_admin_questdef.py @@ -18,8 +18,9 @@ caveat in api/nestquest_core.py): and windows exactly as posted. - ``PATCH /api/v1/admin/quest-definitions/{id}``: an edit PERSISTS the supplied fields and leaves omitted fields untouched (exclude_unset - semantics); a supplied ``windows`` list replaces the whole set; - assignment is not settable through the edit. + semantics); a supplied ``windows`` list replaces the whole set; an + omitted ``assignee_child_ids`` leaves the assignees untouched (the + supplied-list cases live in test_api_admin_questdef_assignees.py). - ``PATCH /api/v1/admin/quest-definitions/{id}/active`` with ``is_active: false`` sets the flag FALSE and the definition STILL EXISTS with its rule, assignees and windows (never hard-deleted); @@ -397,7 +398,7 @@ async def test_edit_persists_supplied_fields_and_leaves_omitted_untouched( ) assert stored.definition.icon == "toy-box" assert stored.rule.to_dict() == _weekly_rule_dict() - # Assignment is not settable through the edit route: untouched. + # assignee_child_ids was omitted: the assignees are untouched. assert sorted(child.id for child in stored.assignees) == [ada, bo] # A second edit touching only the rule leaves the metadata and the diff --git a/tests/test_api_admin_questdef_assignees.py b/tests/test_api_admin_questdef_assignees.py new file mode 100644 index 0000000..fc42d0c --- /dev/null +++ b/tests/test_api_admin_questdef_assignees.py @@ -0,0 +1,366 @@ +"""Admin-plane editable assignees on the definition edit route (task 1c8096f6). + +``PATCH /api/v1/admin/quest-definitions/{definition_id}`` accepts an +optional ``assignee_child_ids`` list that REPLACES the definition's +assignee set inside the core edit's ONE transaction +(:func:`nestquest_core.quest_definitions.edit_quest_definition`): + +- newly listed children are assigned and unlisted ones unassigned; the + response and the database both carry the resulting set; +- an empty list, a duplicate, an unknown child or an inactive child is + 422 and NOTHING is written — the stored assignees (and any other + field supplied in the same body) are unchanged; +- an omitted ``assignee_child_ids`` leaves the assignees unchanged; +- an assignment change regenerates the definition's future instances + through the existing core path; +- the route stays behind the router's ONE ``require_admin`` dependency + (absent credential 401, panel service token 403, non-admin JWT 403, + admin JWT 200). + +Every case is proven against the DATABASE, read through the SAME core +copy the app uses (``nestquest_core.*``; see api/nestquest_core.py). +""" +from __future__ import annotations + +import datetime +import importlib +from types import SimpleNamespace + +import pytest + +from tests.admin_jwt_harness import ( + ADMIN_KEY, + KID, + AdminRunner, + jwks_for, + make_token, +) + +_dao_rules = importlib.import_module("nestquest_core.dao_rules") +_dao_instances = importlib.import_module("nestquest_core.dao_instances") +_materialize = importlib.import_module("nestquest_core.materialize") + +#: The panel service token the harness app is configured with. +HARNESS_PANEL_TOKEN = "admin-test-panel-token" + + +def _admin_headers() -> dict[str, str]: + """The Authorization header of a valid nestquest-admins JWT.""" + return {"Authorization": f"Bearer {make_token()}"} + + +def _edit_path(definition_id: int) -> str: + """The edit route for one definition.""" + return f"/api/v1/admin/quest-definitions/{definition_id}" + + +@pytest.fixture +async def admin_assignees(temp_db_path: str) -> SimpleNamespace: + """An admin-plane app client plus its live database.""" + runner = AdminRunner(temp_db_path, jwks_for(ADMIN_KEY, KID)) + async with runner as client: + yield SimpleNamespace( + client=client, + database=runner.app.state.db.database, + ) + + +async def _create_children( + client: object, names: tuple[str, ...] +) -> list[int]: + """Create ACTIVE children through the admin children route.""" + ids = [] + for name in names: + created = await client.post( + "/api/v1/admin/children", + json={"display_name": name}, + headers=_admin_headers(), + ) + assert created.status_code == 201 + ids.append(created.json()["id"]) + return ids + + +async def _create_definition(client: object, assignee_ids: list[int]) -> int: + """POST a daily morning definition for the given assignees.""" + created = await client.post( + "/api/v1/admin/quest-definitions", + json={ + "title": "Brush teeth", + "rule": {"rule_type": "daily"}, + "assignee_child_ids": assignee_ids, + "windows": ["morning"], + }, + headers=_admin_headers(), + ) + assert created.status_code == 201 + return created.json()["id"] + + +async def _stored_assignee_ids( + database: object, definition_id: int +) -> set[int]: + """The definition's assignee ids as stored.""" + dao = _dao_rules.QuestDefinitionsDao(database) + return {child.id for child in await dao.list_assignees(definition_id)} + + +def _response_assignee_ids(response: object) -> set[int]: + """The assignee ids a definition payload reports.""" + return {assignee["id"] for assignee in response.json()["assignees"]} + + +# --- add / remove / replace ------------------------------------------------- + + +@pytest.mark.parametrize( + ("initial", "requested"), + [ + (("Ada",), ("Ada", "Bo")), # add a child + (("Ada", "Bo"), ("Ada",)), # remove a child + (("Ada", "Bo"), ("Cy",)), # replace the whole set + ], +) +async def test_edit_replaces_the_assignee_set( + admin_assignees: SimpleNamespace, + initial: tuple[str, ...], + requested: tuple[str, ...], +) -> None: + """A supplied list becomes exactly the stored and returned set.""" + client = admin_assignees.client + names = ("Ada", "Bo", "Cy") + ids = dict(zip(names, await _create_children(client, names))) + definition_id = await _create_definition( + client, [ids[name] for name in initial] + ) + wanted = {ids[name] for name in requested} + + response = await client.patch( + _edit_path(definition_id), + json={"assignee_child_ids": [ids[name] for name in requested]}, + headers=_admin_headers(), + ) + + assert response.status_code == 200 + assert _response_assignee_ids(response) == wanted + assert {a["display_name"] for a in response.json()["assignees"]} == set( + requested + ) + assert await _stored_assignee_ids( + admin_assignees.database, definition_id + ) == wanted + # The rest of the definition is untouched by an assignee-only edit. + assert response.json()["title"] == "Brush teeth" + assert [w["window"] for w in response.json()["windows"]] == ["morning"] + + +async def test_edit_with_assignees_and_other_fields_applies_both( + admin_assignees: SimpleNamespace, +) -> None: + """Assignees and metadata supplied together land in one edit.""" + client = admin_assignees.client + ada, bo = await _create_children(client, ("Ada", "Bo")) + definition_id = await _create_definition(client, [ada]) + + response = await client.patch( + _edit_path(definition_id), + json={"title": "Floss", "assignee_child_ids": [bo]}, + headers=_admin_headers(), + ) + + assert response.status_code == 200 + assert response.json()["title"] == "Floss" + assert _response_assignee_ids(response) == {bo} + dao = _dao_rules.QuestDefinitionsDao(admin_assignees.database) + assert (await dao.get(definition_id)).title == "Floss" + assert await _stored_assignee_ids( + admin_assignees.database, definition_id + ) == {bo} + + +# --- rejections leave the stored state unchanged ---------------------------- + + +@pytest.mark.parametrize( + ("case", "expected_detail"), + [ + ("empty", "assignee_child_ids must not be empty"), + ("unknown", "assignee_child_ids: child 999 does not exist"), + ("inactive", "assignee_child_ids: child {cy} is inactive"), + ("duplicate", "assignee_child_ids must not contain duplicate"), + ("null", "assignee_child_ids must be a list"), + ("mixed_unknown", "assignee_child_ids: child 999 does not exist"), + ("wrong_type", None), # the body model's own 422 + ], +) +async def test_rejected_assignee_list_is_422_and_changes_nothing( + admin_assignees: SimpleNamespace, + case: str, + expected_detail: str | None, +) -> None: + """Empty/unknown/inactive/malformed lists are 422 with no mutation. + + The same body also renames the definition, proving the whole edit + is all-or-nothing: neither the assignees nor the title change. + """ + client = admin_assignees.client + ada, bo, cy, di = await _create_children( + client, ("Ada", "Bo", "Cy", "Di") + ) + definition_id = await _create_definition(client, [ada, bo]) + deactivated = await client.patch( + f"/api/v1/admin/children/{cy}/active", + json={"is_active": False}, + headers=_admin_headers(), + ) + assert deactivated.status_code == 200 + + requested: object = { + "empty": [], + "unknown": [999], + "inactive": [ada, cy], + "duplicate": [ada, ada], + "null": None, + # A valid new child alongside an unknown one: di must NOT be + # assigned (all-or-nothing), and ada/bo must NOT be removed. + "mixed_unknown": [di, 999], + "wrong_type": ["Ada"], + }[case] + + response = await client.patch( + _edit_path(definition_id), + json={"title": "Renamed", "assignee_child_ids": requested}, + headers=_admin_headers(), + ) + + assert response.status_code == 422, case + if expected_detail is not None: + assert response.json()["detail"].startswith( + expected_detail.format(cy=cy) + ), response.json() + assert await _stored_assignee_ids( + admin_assignees.database, definition_id + ) == {ada, bo} + dao = _dao_rules.QuestDefinitionsDao(admin_assignees.database) + assert (await dao.get(definition_id)).title == "Brush teeth" + + +async def test_rejected_assignees_on_unknown_definition_is_404( + admin_assignees: SimpleNamespace, +) -> None: + """An unknown definition id is still 404 when assignees are supplied.""" + client = admin_assignees.client + (ada,) = await _create_children(client, ("Ada",)) + response = await client.patch( + _edit_path(999), + json={"assignee_child_ids": [ada]}, + headers=_admin_headers(), + ) + assert response.status_code == 404 + assert response.json()["detail"] == "Quest definition not found" + + +# --- omitted keeps ---------------------------------------------------------- + + +async def test_omitted_assignees_are_unchanged( + admin_assignees: SimpleNamespace, +) -> None: + """An edit without assignee_child_ids never touches the assignees.""" + client = admin_assignees.client + ada, bo = await _create_children(client, ("Ada", "Bo")) + definition_id = await _create_definition(client, [ada, bo]) + + response = await client.patch( + _edit_path(definition_id), + json={"title": "Floss"}, + headers=_admin_headers(), + ) + + assert response.status_code == 200 + assert _response_assignee_ids(response) == {ada, bo} + assert await _stored_assignee_ids( + admin_assignees.database, definition_id + ) == {ada, bo} + + +# --- regeneration ----------------------------------------------------------- + + +async def test_assignment_change_regenerates_future_instances( + admin_assignees: SimpleNamespace, +) -> None: + """Swapping the assignee moves the future open instances with it. + + The regeneration lives in the core edit path the route calls: the + definition's open instances at or after today are deleted and the + horizon re-materialized for the NEW assignee set. + """ + client = admin_assignees.client + database = admin_assignees.database + ada, bo = await _create_children(client, ("Ada", "Bo")) + definition_id = await _create_definition(client, [ada]) + + today = datetime.date.today() + start = today.isoformat() + end = (today + datetime.timedelta(days=7)).isoformat() + await _materialize.materialize(database, start, end, today=today) + instances = _dao_instances.QuestInstancesDao(database) + assert await instances.list_by_date_range(ada, start, end) + assert await instances.list_by_date_range(bo, start, end) == [] + + response = await client.patch( + _edit_path(definition_id), + json={"assignee_child_ids": [bo]}, + headers=_admin_headers(), + ) + + assert response.status_code == 200 + assert await instances.list_by_date_range(ada, start, end) == [] + regenerated = await instances.list_by_date_range(bo, start, end) + assert regenerated + assert {i.definition_id for i in regenerated} == {definition_id} + + +# --- authentication --------------------------------------------------------- + + +async def test_assignee_edit_auth_outcomes( + admin_assignees: SimpleNamespace, +) -> None: + """401 absent, 403 panel token, 403 non-admin, 200 admin.""" + client = admin_assignees.client + ada, bo = await _create_children(client, ("Ada", "Bo")) + definition_id = await _create_definition(client, [ada]) + body = {"assignee_child_ids": [bo]} + + absent = await client.patch(_edit_path(definition_id), json=body) + assert absent.status_code == 401 + + panel = await client.patch( + _edit_path(definition_id), + json=body, + headers={"Authorization": f"Bearer {HARNESS_PANEL_TOKEN}"}, + ) + assert panel.status_code == 403 + + non_admin_token = make_token(groups=("some-other-group",)) + non_admin = await client.patch( + _edit_path(definition_id), + json=body, + headers={"Authorization": f"Bearer {non_admin_token}"}, + ) + assert non_admin.status_code == 403 + + # None of the refused calls reached the handler. + assert await _stored_assignee_ids( + admin_assignees.database, definition_id + ) == {ada} + + admin = await client.patch( + _edit_path(definition_id), json=body, headers=_admin_headers() + ) + assert admin.status_code == 200 + assert await _stored_assignee_ids( + admin_assignees.database, definition_id + ) == {bo} -- 2.55.0 From 2c7ea48ee3a3e87edb078646d1e6d510510839c6 Mon Sep 17 00:00:00 2001 From: Joshua Date: Wed, 23 Sep 2026 21:04:50 -0400 Subject: [PATCH 2/2] Reject coerced assignee ids The create and edit quest-definition bodies parsed assignee_child_ids as lax list[int], so JSON true, 1.0 and "1" were coerced to child id 1 before the core's validator saw them. Use list[StrictInt] on both so such ids are a 422 and nothing is written. Co-Authored-By: Claude Opus 5.5 --- api/routes_admin.py | 15 ++++- tests/test_api_admin_questdef_assignees.py | 65 ++++++++++++++++++++++ 2 files changed, 77 insertions(+), 3 deletions(-) diff --git a/api/routes_admin.py b/api/routes_admin.py index adff79d..1c4de6b 100644 --- a/api/routes_admin.py +++ b/api/routes_admin.py @@ -252,7 +252,13 @@ import datetime from typing import Annotated, Literal, NoReturn from fastapi import APIRouter, Depends, HTTPException, Request, Response -from pydantic import BaseModel, StrictBool, field_validator, model_validator +from pydantic import ( + BaseModel, + StrictBool, + StrictInt, + field_validator, + model_validator, +) from api import transitions as api_transitions from api.auth import require_admin @@ -457,11 +463,13 @@ class AdminQuestDefinitionCreateRequest(BaseModel): date the child is absent). ``windows`` entries are a window name (``morning``) or a two-item ``[window, due_time]`` pair whose due time is a strict 24-hour HH:MM (or ``null`` for none) — both forms the core normalizes. + Assignee ids are strict JSON integers: a boolean, float or numeric + string is 422, never coerced to a child id. """ title: str rule: AdminRuleRequest - assignee_child_ids: list[int] + assignee_child_ids: list[StrictInt] windows: list[str | tuple[str, str | None]] description: str | None = None icon: str | None = None @@ -483,6 +491,7 @@ class AdminQuestDefinitionEditRequest(BaseModel): REPLACES the whole assignee set (non-empty, every id an existing active child — create's contract; a rejection changes nothing). An omitted (or ``null``) ``skip_on_away`` keeps the stored value. + Assignee ids are strict JSON integers, as on create. """ title: str | None = None @@ -491,7 +500,7 @@ class AdminQuestDefinitionEditRequest(BaseModel): rule: AdminRuleRequest | None = None windows: list[str | tuple[str, str | None]] | None = None skip_on_away: StrictBool | None = None - assignee_child_ids: list[int] | None = None + assignee_child_ids: list[StrictInt] | None = None class AdminQuestDefinitionActiveRequest(BaseModel): diff --git a/tests/test_api_admin_questdef_assignees.py b/tests/test_api_admin_questdef_assignees.py index fc42d0c..40c4b86 100644 --- a/tests/test_api_admin_questdef_assignees.py +++ b/tests/test_api_admin_questdef_assignees.py @@ -10,6 +10,8 @@ assignee set inside the core edit's ONE transaction - an empty list, a duplicate, an unknown child or an inactive child is 422 and NOTHING is written — the stored assignees (and any other field supplied in the same body) are unchanged; +- a JSON boolean, float or numeric string id is 422 on create and edit + (never coerced to an int) and nothing is written; - an omitted ``assignee_child_ids`` leaves the assignees unchanged; - an assignment change regenerates the definition's future instances through the existing core path; @@ -39,6 +41,9 @@ from tests.admin_jwt_harness import ( _dao_rules = importlib.import_module("nestquest_core.dao_rules") _dao_instances = importlib.import_module("nestquest_core.dao_instances") _materialize = importlib.import_module("nestquest_core.materialize") +_quest_definitions = importlib.import_module( + "nestquest_core.quest_definitions" +) #: The panel service token the harness app is configured with. HARNESS_PANEL_TOKEN = "admin-test-panel-token" @@ -245,6 +250,66 @@ async def test_rejected_assignee_list_is_422_and_changes_nothing( assert (await dao.get(definition_id)).title == "Brush teeth" +#: JSON ids the lax ``int`` parser would coerce (``true`` -> 1). +_COERCIBLE_IDS = [True, 1.0, "1"] + + +@pytest.mark.parametrize("coercible", _COERCIBLE_IDS) +async def test_edit_rejects_coercible_assignee_ids_and_changes_nothing( + admin_assignees: SimpleNamespace, coercible: object +) -> None: + """A bool, float or numeric-string id edit is 422; nothing changes.""" + client = admin_assignees.client + ada, bo = await _create_children(client, ("Ada", "Bo")) + # Assign bo only, so a coerced id 1 (ada) would be a visible change. + definition_id = await _create_definition(client, [bo]) + assert ada == 1 + + response = await client.patch( + _edit_path(definition_id), + json={"title": "Renamed", "assignee_child_ids": [coercible]}, + headers=_admin_headers(), + ) + + assert response.status_code == 422 + assert await _stored_assignee_ids( + admin_assignees.database, definition_id + ) == {bo} + dao = _dao_rules.QuestDefinitionsDao(admin_assignees.database) + assert (await dao.get(definition_id)).title == "Brush teeth" + + +@pytest.mark.parametrize("coercible", _COERCIBLE_IDS) +async def test_create_rejects_coercible_assignee_ids_and_persists_nothing( + admin_assignees: SimpleNamespace, coercible: object +) -> None: + """A bool, float or numeric-string id create is 422; nothing is written.""" + client = admin_assignees.client + ada, bo = await _create_children(client, ("Ada", "Bo")) + assert ada == 1 + existing_id = await _create_definition(client, [bo]) + + response = await client.post( + "/api/v1/admin/quest-definitions", + json={ + "title": "Tidy the den", + "rule": {"rule_type": "daily"}, + "assignee_child_ids": [coercible], + "windows": ["morning"], + }, + headers=_admin_headers(), + ) + + assert response.status_code == 422 + bundles = await _quest_definitions.list_active_definitions( + admin_assignees.database + ) + assert [bundle.definition.id for bundle in bundles] == [existing_id] + assert await _stored_assignee_ids( + admin_assignees.database, existing_id + ) == {bo} + + async def test_rejected_assignees_on_unknown_definition_is_404( admin_assignees: SimpleNamespace, ) -> None: -- 2.55.0