Defensive check on changeset resolve plus tests

CLAUDE.md

@@ -79,24 +79,42 @@ at all). Validated against the code:
 * **Export to TDEI** — no endpoint exists in this backend.
 * **Move Workspace PG→PG** — not possible; `WorkspacePatch` has no
   `tdeiProjectGroupId` field, so no route can change a workspace's project group.
-* **Validate Changeset** and **Edit POSM Element** — these go through the OSM
-  proxy catch-all (`api/main.py`), which gates *every* proxied operation on
+* **Edit POSM Element** — goes through the OSM proxy catch-all
+  (`api/main.py`), which gates *every* proxied operation on
   `isWorkspaceContributor` alone. There is no Validator- or Lead-level check on
-  proxied traffic.
+  proxied traffic. Raw changeset commits (proxied `PUT /api/0.6/changeset/...`)
+  are likewise Contributor-gated; the proxy only *tags* a contributor's new
+  changeset with `review_requested=yes` when the workspace has `autoFlagReview`
+  set — it does not enforce validation.
 
-**The Validator role grants nothing extra at this layer.**
-`isWorkspaceValidator` exists in `api/core/security.py` but no endpoint
-authorizes on it — it only appears in the `role` field of `WorkspaceResponse`.
-A Validator and a Contributor have identical permissions in this backend.
+**Enforced here, Validator-gated (`isWorkspaceLead || isWorkspaceValidator` →
+403).** This is a native FastAPI route, not proxied traffic. Leads (and POC via
+`isWorkspaceLead`) inherit it:
+
+| Capability | Endpoint |
+|---|---|
+| Resolve/Validate Changeset | PUT `/workspaces/{id}/changesets/{changeset_id}/resolve` |
+
+Resolving clears the `review_requested` tag and stamps `reviewed_by` with the
+reviewer's UUID. The gate is enforced both in the route
+(`api/src/osm/routes.py`) and, defensively, inside
+`OSMRepository.resolveChangeset` (`api/src/osm/repository.py`).
+
+**The Validator role grants exactly one thing at this layer:** the ability to
+resolve changesets via the endpoint above. Aside from that, a Validator and a
+Contributor have identical permissions in this backend. `isWorkspaceValidator`
+otherwise only appears in the `role` field of `WorkspaceResponse`.
 
 **"Contributor" and "Authenticated User With PG/Workspace Association" are the
 same gate.** `isWorkspaceContributor` simply checks whether the workspace is in
 one of the user's project groups (`accessibleWorkspaceIds`), i.e. PG/workspace
 association — so both rows collapse to the same check.
 
-If the Validator/Lead distinctions for changeset validation and TDEI export are
-required, they must be enforced downstream (`workspaces-openstreetmap-website/`,
-`workspaces-cgimap/`) — that has not been audited here.
+Changeset *resolution* is Validator/Lead-gated here (see above). But the
+Validator/Lead distinction on raw changeset *commits* and TDEI export is not
+enforced at this layer; if required, it must be enforced downstream
+(`workspaces-openstreetmap-website/`, `workspaces-cgimap/`) — that has not been
+audited here.
 
 ## Testing
 

api/src/osm/repository.py

@@ -1,7 +1,8 @@
 from sqlalchemy import text
 from sqlmodel.ext.asyncio.session import AsyncSession
 
-from api.core.exceptions import NotFoundException
+from api.core.exceptions import ForbiddenException, NotFoundException
+from api.core.security import UserInfo
 
 
 class OSMRepository:
@@ -50,10 +51,22 @@ async def getChangesetAdiff(self, workspace_id: int, changeset_id: int) -> list:
 
     async def resolveChangeset(
         self,
+        current_user: UserInfo,
         workspace_id: int,
         changeset_id: int,
-        reviewer_uuid: str,
     ) -> None:
+        # Defense in depth: resolving a changeset is a validator/lead
+        # capability. The route also enforces this, but gate here too so the
+        # repository cannot be misused from another call site.
+        if not current_user.isWorkspaceLead(
+            workspace_id
+        ) and not current_user.isWorkspaceValidator(workspace_id):
+            raise ForbiddenException(
+                "Only workspace leads and validators can resolve changesets"
+            )
+
+        reviewer_uuid = str(current_user.user_uuid)
+
         await self.session.execute(
             text(f"SET search_path TO 'workspace-{int(workspace_id)}', public")
         )

api/src/osm/routes.py

@@ -71,9 +71,7 @@ async def resolve_changeset(
     await repository_ws.getById(current_user, workspace_id)
 
     try:
-        await repository_osm.resolveChangeset(
-            workspace_id, changeset_id, str(current_user.user_uuid)
-        )
+        await repository_osm.resolveChangeset(current_user, workspace_id, changeset_id)
     except Exception as e:
         logger.error(
             f"Failed to resolve changeset {changeset_id}"

tests/integration/test_osm.py

@@ -0,0 +1,117 @@
+"""Integration tests for the /workspaces OSM routes (api/src/osm/routes.py).
+
+Each test drives a real HTTP request through the real route + repository,
+queueing simulated rows on the fake sessions.
+
+Focus: PUT /{id}/changesets/{cid}/resolve is gated to workspace leads and
+validators (403 otherwise). The gate is enforced both in the route and,
+defensively, inside ``OSMRepository.resolveChangeset`` -- so a contributor is
+rejected before any DB work, and the repository would reject a bad call site
+even if the route check were bypassed.
+"""
+
+import pytest
+
+from api.src.users.schemas import WorkspaceUserRoleType
+from tests.support import factories, fakes
+
+API = "/api/v1/workspaces"
+
+
+def _resolve_url(workspace_id=1, changeset_id=99):
+    return f"{API}/{workspace_id}/changesets/{changeset_id}/resolve"
+
+
+def _user_with_role(role, workspace_id=1):
+    return factories.make_user_info(osm_workspace_roles={workspace_id: [role]})
+
+
+# === PUT /{id}/changesets/{cid}/resolve ====================================
+
+
+async def test_resolve_changeset_validator_204(
+    client, login, task_session, osm_session
+):
+    login(_user_with_role(WorkspaceUserRoleType.VALIDATOR))
+    task_session.queue(fakes.rows(factories.make_workspace(id=1)))  # getById
+    osm_session.queue(fakes.affected(1), fakes.affected(1))  # DELETE, INSERT
+
+    response = await client.put(_resolve_url())
+
+    assert response.status_code == 204
+    assert osm_session.commits == 1  # resolveChangeset ran to completion
+
+
+async def test_resolve_changeset_lead_204(client, login, task_session, osm_session):
+    login(_user_with_role(WorkspaceUserRoleType.LEAD))
+    task_session.queue(fakes.rows(factories.make_workspace(id=1)))
+    osm_session.queue(fakes.affected(1), fakes.affected(1))
+
+    response = await client.put(_resolve_url())
+
+    assert response.status_code == 204
+    assert osm_session.commits == 1
+
+
+async def test_resolve_changeset_poc_204(client, login, task_session, osm_session):
+    # POC on the owning project group satisfies isWorkspaceLead.
+    login(
+        factories.make_user_info(
+            accessible_workspace_ids={factories.DEFAULT_PG_ID: [1]},
+            poc_group_ids=(factories.DEFAULT_PG_ID,),
+        )
+    )
+    task_session.queue(fakes.rows(factories.make_workspace(id=1)))
+    osm_session.queue(fakes.affected(1), fakes.affected(1))
+
+    response = await client.put(_resolve_url())
+
+    assert response.status_code == 204
+    assert osm_session.commits == 1
+
+
+async def test_resolve_changeset_contributor_403(
+    client, login, task_session, osm_session
+):
+    # A contributor (PG association, no validator/lead grant) is rejected
+    # before any DB work -- neither session should be touched.
+    login(
+        factories.make_user_info(
+            accessible_workspace_ids={factories.DEFAULT_PG_ID: [1]}
+        )
+    )
+
+    response = await client.put(_resolve_url())
+
+    assert response.status_code == 403
+    assert osm_session.commits == 0
+
+
+async def test_resolve_changeset_no_access_403(client, login):
+    # A user with no association to the workspace is likewise forbidden.
+    login()
+
+    response = await client.put(_resolve_url())
+
+    assert response.status_code == 403
+
+
+async def test_resolve_changeset_validator_of_other_workspace_403(client, login):
+    # Validator rights on workspace 2 do not authorize resolving on workspace 1.
+    login(_user_with_role(WorkspaceUserRoleType.VALIDATOR, workspace_id=2))
+
+    response = await client.put(_resolve_url(workspace_id=1))
+
+    assert response.status_code == 403
+
+
+async def test_resolve_changeset_unexpected_error_500(
+    error_client, login, task_session, osm_session
+):
+    login(_user_with_role(WorkspaceUserRoleType.VALIDATOR))
+    task_session.queue(fakes.rows(factories.make_workspace(id=1)))
+    osm_session.queue(fakes.raises(RuntimeError("db")))  # DELETE blows up
+
+    response = await error_client.put(_resolve_url())
+
+    assert response.status_code == 500