diff --git a/CLAUDE.md b/CLAUDE.md index 8e25c07..1e5e9d6 100644 --- a/CLAUDE.md +++ b/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 diff --git a/api/src/osm/repository.py b/api/src/osm/repository.py index 5ca1c9a..5a29b7e 100644 --- a/api/src/osm/repository.py +++ b/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") ) diff --git a/api/src/osm/routes.py b/api/src/osm/routes.py index 452b2d4..cadee1e 100644 --- a/api/src/osm/routes.py +++ b/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}" diff --git a/tests/integration/test_osm.py b/tests/integration/test_osm.py new file mode 100644 index 0000000..b4862d8 --- /dev/null +++ b/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