diff --git a/.claude/settings.json b/.claude/settings.json index fa7924c..ea52c82 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -10,7 +10,9 @@ "Bash(git config *)", "Bash(command -v gh)", "Bash(gh api *)", - "Bash(python3 -c \"import sys,json; d=json.load\\(sys.stdin\\); print\\('protected:', d.get\\('protected'\\)\\); p=d.get\\('protection',{}\\); print\\('required_pull_request_reviews:', 'yes' if p.get\\('required_pull_request_reviews'\\) else 'no'\\); print\\('enforce_admins:', \\(p.get\\('enforce_admins'\\) or {}\\).get\\('enabled'\\)\\)\")" + "Bash(python3 -c \"import sys,json; d=json.load\\(sys.stdin\\); print\\('protected:', d.get\\('protected'\\)\\); p=d.get\\('protection',{}\\); print\\('required_pull_request_reviews:', 'yes' if p.get\\('required_pull_request_reviews'\\) else 'no'\\); print\\('enforce_admins:', \\(p.get\\('enforce_admins'\\) or {}\\).get\\('enabled'\\)\\)\")", + "Bash(grep -nA6 \"TENANT_BYPASSES: list\" api/main.py)", + "Bash(grep -nA10 \"STRIP_REQUEST_HEADERS = \" api/main.py)" ] } } diff --git a/.github/workflows/trigger-deploy-on-merge.yml b/.github/workflows/trigger-deploy-on-merge.yml new file mode 100644 index 0000000..82265c8 --- /dev/null +++ b/.github/workflows/trigger-deploy-on-merge.yml @@ -0,0 +1,36 @@ +name: Trigger Deployment on Pull Request Merge +on: + pull_request: + types: [closed] + branches: [ develop, staging, production] +permissions: + contents: read + actions: write +jobs: + on-merge: + if: github.event.pull_request.merged == true + runs-on: ubuntu-latest + outputs: + environment: ${{ steps.set-environment.outputs.environment }} + steps: + - name: Set Environment + id: set-environment + run: | + case "${{ github.base_ref }}" in + develop) echo "environment=develop" >> $GITHUB_OUTPUT ;; + staging) echo "environment=staging" >> $GITHUB_OUTPUT ;; + production) echo "environment=production" >> $GITHUB_OUTPUT ;; + testing) echo "environment=testing" >> $GITHUB_OUTPUT ;; + esac + deploy: + needs: on-merge + runs-on: ubuntu-latest + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + steps: + - name: Trigger Deployment Workflow + run: | + gh workflow run push-docker-image.yml \ + --repo ${{ github.repository }} \ + --ref ${{ github.base_ref }} \ + --field environment=${{ needs.on-merge.outputs.environment }} \ No newline at end of file diff --git a/.gitignore b/.gitignore index 4516af1..d929d58 100644 --- a/.gitignore +++ b/.gitignore @@ -170,3 +170,4 @@ docs/tasking-mvp/tasking-mvp.postman_environment.json docs/tasking-mvp/_enrich_postman.py docs/tasking-mvp/feature-coverage.md .idea/ +data/ \ No newline at end of file diff --git a/CLAUDE.md b/CLAUDE.md index 8e25c07..d15dea5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -79,24 +79,120 @@ 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. + +## The OSM proxy layer: routing, auth, and user provisioning + +This service is a reverse proxy in front of the OSM website (`osm-rails`) and +cgimap. The non-obvious parts, learned the hard way: + +### Deployment routing (workspaces-stack) + +In `workspaces-stack`, the **api container (this backend) serves the public OSM +host** (`osm.workspaces-...`) alongside the API hosts — its traefik router +rule is `Host(new-api) || Host(api) || Host(osm)`, and the `osm-web` / +`osm-log-proxy` routers are commented out. So **every OSM call the frontend +makes routes through this backend**: `validate_token`, the `X-Workspace` gate, +and the CORS middleware all apply. The backend then proxies `/api/0.6/*` to +`WS_OSM_HOST` (config default `http://osm-web` — the internal service, *not* the +public domain). `osm-web`'s nginx splits traffic: changeset read/write subpaths +to cgimap, everything else to osm-rails. + +The frontend (`services/osm.ts`) calls the OSM host directly with +`credentials: 'include'` + a Bearer TDEI token. Because it's *credentialed* +CORS, `CORS_ORIGINS` must list the exact frontend origin (no `*`; +`allow_credentials` is on). The stack supplies `WS_API_CORS_ORIGINS` as a **JSON +array** (`["https://..."]`), while older config comma-split it — a JSON array +would then become one malformed origin. `api/main.py` therefore reads +`settings.cors_origins_list`, which parses **both** a JSON array and a +comma-separated string (and `*`); set `CORS_ORIGINS` in either form. + +### Two databases; `users` is owned by OSM Rails + +`TASK_DATABASE_URL` (`alembic_task` tree) holds workspaces / tasking-manager +tables; `OSM_DATABASE_URL` (`alembic_osm` tree) holds OSM data, `users`, and the +`tasking_*` tables. The **`users` table is owned by the OSM Rails website**, not +either alembic tree — the trees only FK to it, and integration tests stub it +(`tests/integration/conftest.py`). The backend provisions `users` rows itself +via raw SQL with `auth_provider='TDEI'` and `auth_uid = str()` (the OSM +`auth_uid` **is** the token's `sub` claim). + +### How OSM authenticates, and the TDEI token bridge + +* **osm-rails** authenticates API calls *only* via **doorkeeper OAuth2**: it + looks the bearer token up in `oauth_access_tokens` (`token` → + `resource_owner_id` → `users.id`). Doorkeeper stores tokens **plaintext** + (`SecretStoring::Plain`), so the raw JWT matches directly. It has **no** + TDEI/JWT auth path. +* **cgimap** has both: the oauth2 lookup *and* a custom TDEI path + (`get_user_id_for_tdei_token`, which verifies the JWT and matches + `users.auth_uid`). + +To make TDEI tokens work against osm-rails without forking it, the **token +bridge** (`_bridge_token_to_osm` in `api/core/security.py`) mirrors a validated +TDEI JWT into `oauth_access_tokens` — on the cache-miss path of `validate_token` +(so ~once per token, not per request). Gated by `WS_OSM_TOKEN_BRIDGE_ENABLED`. +It auto-provisions everything: a dedicated **system user** (`WS_OSM_SYSTEM_USER_*`) +to own the doorkeeper `oauth_applications` row it creates (keyed by +`WS_OSM_OAUTH_CLIENT_UID`; `oauth_applications.owner_id` is a NOT-NULL FK to +`users`, hence the system user), the caller's `users` row, and the plaintext +`oauth_access_tokens` row (`expires_in` from the JWT `exp`; +`ON CONFLICT (token) DO UPDATE` so re-presenting reactivates it). On token +rotation (new `jti`) the superseded token is revoked. Since the token then lives +in `oauth_access_tokens`, both osm-rails and cgimap authenticate it via plain +OAuth2 — cgimap's custom TDEI path becomes redundant. + +### Provisioning gotcha: `pass_crypt` must be 8..255 chars + +The OSM `User` model has `validates :pass_crypt, :length => 8..255`. When the +backend provisions a `users` row, **`pass_crypt` must be 8–255 chars** (use a +random throwaway — TDEI manages auth, so it's never used to log in). A too-short +value (the old `'none'`) makes the user *invalid*. This is silent for **cgimap** +operations — cgimap runs no Rails validations — but breaks any **osm-rails** +operation that re-validates the author via `validates :author, :associated => +true`: notably `POST /api/0.6/changeset/{id}/comment` and note comments. The +comment fails to save (no `id`), then rendering it throws *"Unable to serialize +… without an id"*. Both provisioning paths (`_ensure_osm_user`, +`_provision_users_from_tdei`) set a valid `pass_crypt`; migration +`alembic_osm/versions/f3a7b9c1d2e4` heals legacy short rows. General principle: +a backend-provisioned `users` row must satisfy OSM's `User` validations (also +`display_name` 3..255 + unique, `email` present + unique) or Rails operations +that touch it will fail even though cgimap operations succeed. ## Testing diff --git a/README.md b/README.md index 8ffa3f5..b76ca45 100644 --- a/README.md +++ b/README.md @@ -6,11 +6,125 @@ This is a combination API backend for workspaces, providing /workspaces* methods the OSM API ("openstreetmap website") plus OSM CGI-map (the C-accelerated methods) that enforces authorization and authentication based on a TDEI/Keycloak JWT token (see main.py for this proxy logic). +## What the proxy must provide for osm-rails / osm-web + +This backend is the **only entry point** to the OSM tier (osm-rails + cgimap, +behind `osm-web`): in the deployment, the public OSM host routes to this +container, which proxies `/api/0.6/*` to `WS_OSM_HOST` (default +`http://osm-web`). For the OSM services to work, the proxy must uphold the +following contract. `CLAUDE.md` has the full rationale. + +1. **Bridge TDEI auth into OSM's OAuth2.** osm-rails authenticates the API *only* + via doorkeeper OAuth2 (`oauth_access_tokens`); it has no TDEI/JWT path. So on + token validation the backend mirrors the TDEI JWT into `oauth_access_tokens` + in the OSM DB (the "token bridge" in `api/core/security.py`), and forwards the + incoming `Authorization: Bearer ` header unchanged. Then osm-rails and + cgimap authenticate the token via plain OAuth2. Controlled by + `WS_OSM_TOKEN_BRIDGE_ENABLED` (on by default) — with it off, osm-rails returns + **401** for TDEI tokens. The backend auto-creates the doorkeeper application + (and a system user to own it), so no manual OSM setup is required. + +2. **Provision valid OSM `users` rows.** The backend creates OSM `users` rows for + TDEI users (`auth_provider='TDEI'`, `auth_uid` = the JWT `sub`). These must + satisfy OSM's `User` validations — in particular `pass_crypt` length 8..255. + A too-short value is invisible to cgimap but makes osm-rails operations that + re-validate the user fail (e.g. posting a changeset comment or a note), + surfacing as *"Unable to serialize … without an id"*. The + `alembic_osm` migration `*_heal_short_tdei_pass_crypt` repairs legacy rows. + +3. **Carry workspace tenancy.** Workspace-scoped OSM requests must include an + `X-Workspace: ` header. The proxy authorizes it against the caller's + workspaces and forwards it (it is *not* stripped) so cgimap/osm-rails scope to + the `workspace-` schema. A few paths are exempt (`TENANT_BYPASSES` in + `api/main.py`): workspace create/delete (`PUT`/`DELETE /api/0.6/workspaces/{id}`) + and user provisioning during sign-in (`PUT /api/0.6/user/{uid}`). + +4. **Set the proxy headers.** The proxy rewrites `Host` to the OSM host and sets + `X-Real-IP` / `X-Forwarded-For` / `-Host` / `-Proto`, while stripping + hop-by-hop headers and any spoofed forwarding headers from the client. It does + *not* strip `Authorization` or `X-Workspace`. + +5. **Connectivity.** `WS_OSM_HOST` must reach `osm-web`, and the backend needs + **both** `OSM_DATABASE_URL` and `TASK_DATABASE_URL` — the token bridge and + user provisioning write to the OSM database. + ## Branch Index * ```develop``` merge your work here; keep this up to date with the "development" environment / dev tag * ```staging``` keep this up to date with the "staging" environment / stage tag * ```production``` keep this up to date with the "production" environment / prod tag + +## Deployment architecture + +The deployed system is defined by [`docker-compose.az.yml`](docker-compose.az.yml). It runs the +**application tier** as four containers; the **data tier** (Postgres/PostGIS) is external, managed +Azure Database for PostgreSQL, not part of this compose file. + +When deployed by `workspaces-stack` this model also holds. + +``` + client (TDEI/Keycloak JWT) + │ + ▼ :8000 + ┌──────────────────────────────────────┐ + │ workspaces-backend │ this repo — FastAPI front door. + │ authn/authz + OSM reverse proxy │ Serves /api/v1/*, proxies the rest. + └───┬──────────────────────────────┬────┘ + WS_OSM_HOST TASK_DATABASE_URL + (→ osm-rails) OSM_DATABASE_URL + │ │ + ▼ │ + ┌──────────────┐ │ + │ osm-rails │ OSM website (Rails); the single OSM entry point. + │ :3000 │ Serves the API/UI and fronts cgimap for the + └──────┬───────┘ performance-critical /api/0.6 calls. + │ (internal) │ + ▼ │ + ┌──────────────┐ │ + │ osm-cgimap │ C-accelerated /api/0.6 (map, changeset bulk) + │ :8000 │ │ + └──────────────┘ │ + │ + osm-rails-worker (rake jobs:work) │ background jobs + │ │ + │ backend, rails, cgimap, worker all connect to ▼ + ┌─────────────────────────────────────────────────────────────────┐ + │ data tier — Azure Postgres (external, PostGIS) │ + │ opensidewalks-${ENV}.postgres.database.azure.com:5432 │ + │ • workspaces-tasks-${ENV} TASK db (alembic_task; backend) │ + │ • workspaces-osm-${ENV} OSM db (alembic_osm; all four) │ + └─────────────────────────────────────────────────────────────────┘ +``` + +### Services + +| Service | Image | Role | +|---|---|---| +| `workspaces-backend` | `workspaces-backend-v2:${ENV}` | This repo. The only host-exposed service (`8000:8000`). Validates the TDEI/Keycloak JWT, enforces workspace authorization, serves `/api/v1/*`, and proxies everything else to the OSM tier. Connects to **both** databases. | +| `osm-rails` | `workspaces-osm-rails-v2:${ENV}` | The OpenStreetMap website (Rails) — the **single OSM entry point** the backend proxies to (`WS_OSM_HOST`). Serves the OSM API/UI and fronts cgimap for the heavy `/api/0.6` calls. Connects to the OSM db. | +| `osm-cgimap` | `workspaces-osm-cgimap-v2:${ENV}` | C++ reimplementation of the performance-critical OSM `0.6` calls (map queries, changeset upload/download), sitting behind `osm-rails`. Tuned here for large imports (`CGIMAP_MAX_*`). Connects to the OSM db. | +| `osm-rails-worker` | `workspaces-osm-rails-v2:${ENV}` | Background job runner (`rake jobs:work`) for the Rails app. Connects to the OSM db. | + +### Two databases + +The backend holds two connections, and the two alembic trees target them independently (see +`CLAUDE.md` and `api/utils/migrations.py`): + +* **TASK db** (`TASK_DATABASE_URL` → `workspaces-tasks-${ENV}`) — the workspaces + tasking-manager + schema, built by the `alembic_task` tree. Only the backend connects here. +* **OSM db** (`OSM_DATABASE_URL` → `workspaces-osm-${ENV}`) — OSM data plus `users` and the + `tasking_*` tables, built by the `alembic_osm` tree. The backend, cgimap, rails, and the worker + all connect here. + +On startup (outside of pytest) the backend runs `alembic -n task upgrade head` and +`alembic -n osm upgrade head`, applying each tree to its database. + +### Environment templating + +Every image tag, database name/user, and server host is parameterized by `${ENV}` +(`dev` / `stage` / `prod`), and secrets are injected from the shell environment +(`${WS_TASKS_DB_PASS}`, `${WS_OSM_DB_PASS}`, `${WS_OSM_SECRET_KEY_BASE}`). Branches map to these +environments — see the Branch Index below. ## To start on your local machine for dev work @@ -40,3 +154,65 @@ uvx pyright --pythonpath .venv/bin/python api tests uv run black api tests && uv run isort api tests ``` +## Development with local environment + +Use the file `docker-compose.local.yml` to build and deploy local code changes. This allows you to run the entire system at once instead of +connecting to existing Databases. + +### Initial setup for development local environment + +Step 1: Login to azure docker + +The docker compose relies on images in `opensidewalksdev` azure container registry. Make sure your docker system is logged into it before pulling the images and trying to run the containers. + +Docker login command: + +`docker login opensidewalksdev.azurecr.io -u opensidewalksdev ` + +Password needs to be obtained from Azure portal + +Step 2: Run docker compose for the first time + +Use the following command to start the containers first time + +`docker compose --file docker-compose.local.yml up --build` + +Step 3: Run the migration scripts. + +You will observe that only `osm-rails` component seems to work but the backend and other services may be down. this is because the database migrations on the base osm database are not done. To do the base migrations, do the following: + +- Connect to the `osm-rails` container. If you are using docker hub for desktop, just go to the exec section of the container. + If you want to use command line, execute the command `docker exec -it /bin/bash` where `container_name` is the name of osm-rails container +- Execute the migration script in the /bin/bash with `bundle exec rails db:migrate` +- The above command runs the migration script for databases + +Step 4: Add `workspaces-tasks-local` database in postgresql + +Workspaces backend relies on an additional database. This is needed for some older migrations code. + +- Connect to `database` container. +- Run the following set of commands one by one + +```shell +psql --username postgres +create database "workspaces-tasks-local"; +exit; +psql --username postgres --dbname "workspaces-tasks-local"; +create extension if not exists postgis; + +``` + +Step 5: Restart the docker compose again + +- `docker compose --file docker-compose.local.yml down` +- `docker compose --file docker-compose.local.yml up --build` + + + +### Commands to start and stop the docker compose + +`docker compose --file docker-compose.local.yml up --build -d` + +`docker compose --file docker-compose.local.yml down` + +Backend code will be available at `http://localhost:8000` diff --git a/alembic_osm/versions/a92361f527ef_custom_imagery_and_description.py b/alembic_osm/versions/a92361f527ef_custom_imagery_and_description.py new file mode 100644 index 0000000..6f1bc64 --- /dev/null +++ b/alembic_osm/versions/a92361f527ef_custom_imagery_and_description.py @@ -0,0 +1,35 @@ +"""custom_imagery_and_description + +Revision ID: a92361f527ef +Revises: f3a7b9c1d2e4 +Create Date: 2026-07-14 15:08:42.142553 + +""" + +from typing import Sequence, Union + +import sqlalchemy as sa +from alembic import op +from sqlalchemy.dialects.postgresql import JSONB + +# revision identifiers, used by Alembic. +revision: str = "a92361f527ef" +down_revision: Union[str, None] = "f3a7b9c1d2e4" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.add_column( + "tasking_projects", + sa.Column("custom_imagery", JSONB(astext_type=sa.Text()), nullable=True), + ) + op.add_column( + "tasking_projects", + sa.Column("description", sa.String(length=10000), nullable=True), + ) + + +def downgrade() -> None: + op.drop_column("tasking_projects", "custom_imagery") + op.drop_column("tasking_projects", "description") diff --git a/alembic_osm/versions/f3a7b9c1d2e4_heal_short_tdei_pass_crypt.py b/alembic_osm/versions/f3a7b9c1d2e4_heal_short_tdei_pass_crypt.py new file mode 100644 index 0000000..5ff6bca --- /dev/null +++ b/alembic_osm/versions/f3a7b9c1d2e4_heal_short_tdei_pass_crypt.py @@ -0,0 +1,47 @@ +"""heal short TDEI-provisioned users.pass_crypt + +TDEI users are provisioned by the backend via raw SQL with a placeholder +``pass_crypt``. Older rows used a 4-char value (``"none"``), which violates OSM's +``pass_crypt`` length 8..255 ``User`` validation. That makes the user invalid, +and Rails operations that re-validate the author via ``validates :author, +:associated => true`` (posting a changeset comment or a note comment) then fail +to save. Replace any too-short value with a random throwaway -- TDEI manages +auth, so ``pass_crypt`` is never used to authenticate. + +Revision ID: f3a7b9c1d2e4 +Revises: 5303f61d7b3a +Create Date: 2026-07-11 00:00:00.000000 + +""" + +from typing import Sequence, Union + +from alembic import op +from sqlalchemy import inspect, text + +# revision identifiers, used by Alembic. +revision: str = "f3a7b9c1d2e4" +down_revision: Union[str, None] = "5303f61d7b3a" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + bind = op.get_bind() + # `users` is owned by the OSM Rails website, not this tree; guard so the + # migration is a no-op if it hasn't been created yet. + if not inspect(bind).has_table("users"): + return + + # Idempotent: re-running only touches rows that are still too short. + op.execute( + text( + "UPDATE users SET pass_crypt = md5(random()::text) " + "WHERE auth_provider = 'TDEI' AND length(pass_crypt) < 8" + ) + ) + + +def downgrade() -> None: + # The original short values are unrecoverable (and were invalid anyway). + pass diff --git a/api/core/config.py b/api/core/config.py index c4c6bde..c6554c7 100644 --- a/api/core/config.py +++ b/api/core/config.py @@ -1,3 +1,5 @@ +import json + from pydantic_settings import BaseSettings, SettingsConfigDict @@ -10,11 +12,13 @@ class Settings(BaseSettings): PROJECT_NAME: str = "Workspaces API" - # JSON array of allowed CORS origins. For example: - # - # ["https://workspaces.example.com", "https://leaderboard.example.com"] - # - CORS_ORIGINS: list[str] = [] + # Allowed CORS origins. Accepts either a comma-separated list OR a JSON + # array (the deployment stack historically supplies a JSON array), plus "*" + # for all origins. Read via `cors_origins_list`, not this raw string. + # Examples: + # CORS_ORIGINS="https://workspaces.example.com,https://leaderboard.example.com" + # CORS_ORIGINS='["https://workspaces.example.com"]' + CORS_ORIGINS: str = "" TASK_DATABASE_URL: str = ( "postgresql+asyncpg://user:pass@localhost:5432/tasking_manager" @@ -40,11 +44,59 @@ class Settings(BaseSettings): # proxy destination--"osm-web" is a virtual docker network endpoint WS_OSM_HOST: str = "http://osm-web" + # OSM token bridge: when enabled, a validated TDEI token is mirrored into the + # OSM database's `oauth_access_tokens` table so osm-rails (doorkeeper) and + # cgimap authenticate it via their standard OAuth2 path -- no custom JWT + # handling needed in those services. The backend also auto-creates the + # doorkeeper `oauth_applications` row (keyed by WS_OSM_OAUTH_CLIENT_UID and + # owned by the dedicated system user below) that these tokens belong to, so + # no manual OSM setup is required. + WS_OSM_TOKEN_BRIDGE_ENABLED: bool = True + + # Stable client id (uid) for the auto-created doorkeeper application. Point + # this at an existing application's uid to reuse it instead of creating one. + WS_OSM_OAUTH_CLIENT_UID: str = "workspaces-backend" + + # Scopes granted to the application and mirrored tokens. Must cover the OSM + # API operations the frontend performs; see `lib/oauth.rb` in the OSM website. + WS_OSM_OAUTH_SCOPES: str = ( + "read_prefs write_prefs write_api write_changeset_comments " + "read_gpx write_gpx write_notes" + ) + + # Dedicated OSM `users` row that owns the auto-created doorkeeper application + # (satisfies oauth_applications.owner_id, a NOT-NULL FK to users). It never + # signs in; these values just need to be stable and unique among users. + WS_OSM_SYSTEM_USER_AUTH_UID: str = "workspaces-backend-system" + WS_OSM_SYSTEM_USER_DISPLAY_NAME: str = "Workspaces Backend (system)" + WS_OSM_SYSTEM_USER_EMAIL: str = "workspaces-backend-system@tdei.us" + SENTRY_DSN: str = "" + @property + def cors_origins_list(self) -> list[str]: + """Allowed CORS origins as a list. + + Tolerates both formats the deployment has used: a JSON array + (``["https://a","https://b"]``) and a comma-separated string + (``https://a,https://b``). ``"*"`` is passed through as-is. + """ + raw = self.CORS_ORIGINS.strip() + if not raw: + return [] + if raw.startswith("["): + try: + parsed = json.loads(raw) + except json.JSONDecodeError: + parsed = None + if isinstance(parsed, list): + return [str(o).strip() for o in parsed if str(o).strip()] + return [o.strip() for o in raw.split(",") if o.strip()] + model_config = SettingsConfigDict( env_file=".env", env_file_encoding="utf-8", + extra="ignore", # ignore unknown environment variables ) diff --git a/api/core/security.py b/api/core/security.py index 0ff32e2..020abf8 100644 --- a/api/core/security.py +++ b/api/core/security.py @@ -1,3 +1,5 @@ +import secrets +import time from enum import StrEnum from uuid import UUID @@ -19,6 +21,8 @@ # @test: Test that the methods on the UserInfo class return the correct values for a given set of project groups and workspace roles # @test: Test that any failed network requests are handled gracefully # @test: Test that the caching mechanism works correctly and evicts entries when roles change +# @test: Test that when WS_OSM_TOKEN_BRIDGE_ENABLED, a validated token is mirrored into oauth_access_tokens with the doorkeeper application + system-owner user + caller user auto-provisioned and expires_in from the JWT exp; and is a no-op when disabled +# @test: Test that re-presenting a token reactivates its OSM row (revoked_at cleared, expiry refreshed) and that a rotated (superseded) token is revoked, both gated on WS_OSM_TOKEN_BRIDGE_ENABLED # Set up logger for this module logger = get_logger(__name__) @@ -316,6 +320,14 @@ async def validate_token( logger.info("Token validation cache hit") return cached logger.info("Token validation cache miss: token rotated") + # The old token is superseded; revoke its OSM row so it stops + # authenticating against osm-rails/cgimap before its own exp. + # TODO: this assumes a single active token per user. Truly concurrent + # sessions (multiple valid jtis for one user) will flap -- each request + # revokes the other session's OSM token and reactivates its own via the + # DO UPDATE upsert. To support multi-session, track multiple active jtis + # per user (cache + revoke set) instead of a single cached token. + await _revoke_osm_token(osm_db_session, cached.credentials) del _user_info_cache[user_uuid] # Cache miss: fetch TDEI roles and DB data: @@ -327,6 +339,189 @@ async def validate_token( return user_info +async def _ensure_osm_user( + session: AsyncSession, + *, + auth_uid: str, + email: str | None, + display_name: str, +) -> int | None: + """Idempotently provision an OSM ``users`` row (auth_provider ``TDEI``) and + return its id. ``email`` is synthesised when absent -- the column is UNIQUE + and NOT NULL.""" + await session.execute( + text( + "INSERT INTO users (auth_uid, email, display_name, auth_provider, " + "status, pass_crypt, data_public, email_valid, terms_seen, " + "creation_time, terms_agreed, tou_agreed) " + "VALUES (:auth_uid, :email, :name, 'TDEI', 'active', :pass_crypt, " + "true, true, true, (now() at time zone 'utc'), " + "(now() at time zone 'utc'), (now() at time zone 'utc')) " + "ON CONFLICT (auth_uid) DO NOTHING" + ), + { + "auth_uid": auth_uid, + "email": email or f"{auth_uid}@tdei.invalid", + "name": display_name, + # OSM's User model validates pass_crypt length 8..255. TDEI manages + # auth, so this is a throwaway that just satisfies that rule (a too- + # short value makes the user invalid and breaks Rails operations + # that re-validate it via `validates :author, :associated => true`, + # e.g. posting a changeset comment). + "pass_crypt": secrets.token_hex(16), + }, + ) + row = ( + await session.execute( + text( + "SELECT id FROM users " + "WHERE auth_provider = 'TDEI' AND auth_uid = :auth_uid" + ), + {"auth_uid": auth_uid}, + ) + ).first() + return row[0] if row else None + + +async def _ensure_osm_oauth_application(session: AsyncSession) -> int | None: + """Ensure the doorkeeper application the mirrored tokens belong to exists, + creating it (owned by a dedicated system user) if needed. Idempotent by the + configured client uid; returns the application id.""" + owner_id = await _ensure_osm_user( + session, + auth_uid=settings.WS_OSM_SYSTEM_USER_AUTH_UID, + email=settings.WS_OSM_SYSTEM_USER_EMAIL, + display_name=settings.WS_OSM_SYSTEM_USER_DISPLAY_NAME, + ) + if owner_id is None: + return None + await session.execute( + text( + "INSERT INTO oauth_applications (owner_type, owner_id, name, uid, " + "secret, redirect_uri, scopes, confidential, created_at, updated_at) " + "VALUES ('User', :owner_id, :name, :uid, :secret, " + "'urn:ietf:wg:oauth:2.0:oob', :scopes, true, " + "(now() at time zone 'utc'), (now() at time zone 'utc')) " + "ON CONFLICT (uid) DO NOTHING" + ), + { + "owner_id": owner_id, + "name": "Workspaces Backend", + "uid": settings.WS_OSM_OAUTH_CLIENT_UID, + # Never used (tokens are inserted directly rather than issued via the + # OAuth flow), but the column is NOT NULL. + "secret": secrets.token_hex(32), + "scopes": settings.WS_OSM_OAUTH_SCOPES, + }, + ) + row = ( + await session.execute( + text("SELECT id FROM oauth_applications WHERE uid = :uid"), + {"uid": settings.WS_OSM_OAUTH_CLIENT_UID}, + ) + ).first() + return row[0] if row else None + + +async def _bridge_token_to_osm( + session: AsyncSession, + *, + user_uuid: UUID, + user_name: str, + email: str | None, + token: str, + exp: int | None, +) -> None: + """Mirror a validated TDEI token into the OSM database so osm-rails + (doorkeeper) and cgimap authenticate it through their standard OAuth2 path, + with no custom JWT handling required in those services. + + Auto-provisions everything it needs: the doorkeeper ``oauth_applications`` + row (owned by a dedicated system user), the caller's ``users`` row, and a + plaintext ``oauth_access_tokens`` row -- no manual OSM setup. Doorkeeper + stores tokens plaintext (``SecretStoring::Plain``), so the raw JWT matches on + lookup for both osm-rails and cgimap; ``expires_in`` tracks the JWT's ``exp``. + + No-op unless ``WS_OSM_TOKEN_BRIDGE_ENABLED``. Best-effort: a failure here + must not break token validation -- the ``/api/v1`` routes keep working; only + the proxied OSM calls would 401 until the row exists. + """ + if not settings.WS_OSM_TOKEN_BRIDGE_ENABLED: + return + + try: + app_id = await _ensure_osm_oauth_application(session) + if app_id is None: + return + user_id = await _ensure_osm_user( + session, auth_uid=str(user_uuid), email=email, display_name=user_name + ) + if user_id is None: + return + + expires_in = max(0, exp - int(time.time())) if exp else None + await session.execute( + text( + "INSERT INTO oauth_access_tokens (application_id, " + "resource_owner_id, token, scopes, created_at, expires_in) " + "VALUES (:app_id, :user_id, :token, :scopes, " + "(now() at time zone 'utc'), :expires_in) " + # Re-presenting a token reactivates it: refresh the expiry to + # track this validation and clear any prior revocation, so a + # token revoked on rotation heals if it is used again. + "ON CONFLICT (token) DO UPDATE SET " + "resource_owner_id = EXCLUDED.resource_owner_id, " + "scopes = EXCLUDED.scopes, " + "created_at = EXCLUDED.created_at, " + "expires_in = EXCLUDED.expires_in, " + "revoked_at = NULL" + ), + { + "app_id": app_id, + "user_id": user_id, + "token": token, + "scopes": settings.WS_OSM_OAUTH_SCOPES, + "expires_in": expires_in, + }, + ) + await session.commit() + except Exception as e: + # Never fail auth on a bridge error; the OSM row just won't exist yet. + logger.warning( + "Failed to bridge TDEI token into OSM oauth_access_tokens: %s", e + ) + await session.rollback() + + +async def _revoke_osm_token(session: AsyncSession, token: str) -> None: + """Revoke a superseded token's OSM ``oauth_access_tokens`` row (best-effort). + + Called when a user's token rotates (new ``jti``) so the previous JWT stops + authenticating against osm-rails/cgimap before its own ``exp`` rather than + lingering until it expires. No-op unless the bridge is enabled. + + Note: OSM/cgimap are reachable only *through* this proxy, and every request + first passes ``validate_token`` -- so a TDEI-revoked token is already + rejected upstream. This revocation is defense-in-depth for the superseded + token and keeps the OSM token store tidy. + """ + if not settings.WS_OSM_TOKEN_BRIDGE_ENABLED: + return + try: + await session.execute( + text( + "UPDATE oauth_access_tokens " + "SET revoked_at = (now() at time zone 'utc') " + "WHERE token = :token AND revoked_at IS NULL" + ), + {"token": token}, + ) + await session.commit() + except Exception as e: + logger.warning("Failed to revoke superseded OSM token: %s", e) + await session.rollback() + + async def _validate_token_uncached( token: str, user_uuid: UUID, @@ -427,6 +622,17 @@ async def _validate_token_uncached( osmRoles[i["workspace_id"]].append(i["role"]) r.osmWorkspaceRoles = osmRoles + # Mirror this validated token into the OSM DB so proxied OSM/cgimap calls + # authenticate via the standard OAuth2 path. No-op unless configured. + await _bridge_token_to_osm( + osm_db_session, + user_uuid=user_uuid, + user_name=r.user_name, + email=payload.get("email"), + token=token, + exp=payload.get("exp"), + ) + logger.info("Finished validation of token") return r diff --git a/api/main.py b/api/main.py index d537b72..c3708b3 100644 --- a/api/main.py +++ b/api/main.py @@ -97,7 +97,7 @@ async def lifespan(_app: FastAPI): app.add_middleware( CORSMiddleware, - allow_origins=settings.CORS_ORIGINS, + allow_origins=settings.cors_origins_list, allow_credentials=True, allow_methods=["*"], allow_headers=["*"], 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/api/src/tasking/projects/dtos.py b/api/src/tasking/projects/dtos.py index 488e981..01057a8 100644 --- a/api/src/tasking/projects/dtos.py +++ b/api/src/tasking/projects/dtos.py @@ -47,6 +47,8 @@ class ProjectCreateRequest(WireModel): lock_timeout_hours: int = PydField(default=8, ge=1, le=720) aoi: Optional[AoiInput] = None role_assignments: list[ProjectRoleAssignment] = PydField(default_factory=list) + custom_imagery: Optional[dict[str, Any]] = PydField(default=None) + description: Optional[str] = PydField(default=None, max_length=10_000) @field_validator("name") @classmethod @@ -68,6 +70,8 @@ class ProjectUpdateRequest(WireModel): instructions: Optional[str] = PydField(default=None, max_length=10_000) lock_timeout_hours: Optional[int] = PydField(default=None, ge=1, le=720) review_required: Optional[bool] = None + custom_imagery: Optional[dict[str, Any]] = PydField(default=None) + description: Optional[str] = PydField(default=None, max_length=10_000) class ProjectResponse(WireModel): @@ -87,6 +91,8 @@ class ProjectResponse(WireModel): created_by_name: Optional[str] = None created_at: datetime updated_at: datetime + custom_imagery: Optional[Any] = None + description: Optional[str] = None class ProjectListItem(WireModel): @@ -114,6 +120,10 @@ class ProjectListResponse(WireModel): pagination: Pagination +class ProjectNameValidationResponse(WireModel): + exists: bool + + # --------------------------------------------------------------------------- # AOI response shape (canonical Feature wrapping a MultiPolygon) # --------------------------------------------------------------------------- @@ -184,6 +194,7 @@ class SelfProjectRolesResponse(WireModel): "ProjectCreateRequest", "ProjectListItem", "ProjectListResponse", + "ProjectNameValidationResponse", "ProjectResponse", "ProjectRoleAddRequest", "ProjectRoleAssignment", diff --git a/api/src/tasking/projects/repository.py b/api/src/tasking/projects/repository.py index 3aa6820..95875c1 100644 --- a/api/src/tasking/projects/repository.py +++ b/api/src/tasking/projects/repository.py @@ -1,6 +1,7 @@ from __future__ import annotations import json +import secrets from datetime import datetime from typing import Any from uuid import UUID @@ -242,6 +243,8 @@ def _to_response(project: TaskingProject, task_count: int = 0) -> ProjectRespons created_by_name=project.created_by_name, created_at=project.created_at, updated_at=project.updated_at, + custom_imagery=project.custom_imagery, # type: ignore[arg-type] + description=project.description, # type: ignore[arg-type] ) async def _provision_users_from_tdei( @@ -313,13 +316,18 @@ async def _provision_users_from_tdei( await self.session.execute( text( "INSERT INTO users (auth_uid, email, display_name, auth_provider, status, pass_crypt, data_public, email_valid, terms_seen, creation_time, terms_agreed, tou_agreed) " - "VALUES (:uid, :email, :name, 'TDEI', 'active', 'none', true, true, true, (now() at time zone 'utc'), (now() at time zone 'utc'), (now() at time zone 'utc')) " + "VALUES (:uid, :email, :name, 'TDEI', 'active', :pass_crypt, true, true, true, (now() at time zone 'utc'), (now() at time zone 'utc'), (now() at time zone 'utc')) " "ON CONFLICT (auth_uid) DO NOTHING" ), params={ "uid": uid, "email": member.email, "name": member.display_name, + # OSM validates pass_crypt length 8..255; a too-short value + # makes the user invalid and breaks Rails ops that re-validate + # the author (changeset/note comments). TDEI manages auth, so + # this is a throwaway. + "pass_crypt": secrets.token_hex(16), }, ) resolved.add(uid) @@ -462,6 +470,22 @@ async def list_projects( pagination=Pagination(page=page, page_size=page_size, total=total), ) + async def project_name_exists(self, workspace_id: int, name: str) -> bool: + result = await self.session.execute( + select(func.count()) + .select_from(TaskingProject) + .where( + (TaskingProject.workspace_id == workspace_id) + & (TaskingProject.name == name) + & ( + TaskingProject.deleted_at.is_( # pyright: ignore[reportAttributeAccessIssue, reportOptionalMemberAccess] + None + ) + ) + ) + ) + return int(result.scalar() or 0) > 0 + async def create( self, workspace_id: int, @@ -525,6 +549,8 @@ async def create( lock_timeout_hours=body.lock_timeout_hours, created_by=current_user.user_uuid, created_by_name=current_user.user_name, + custom_imagery=body.custom_imagery, # type: ignore[arg-type] + description=body.description, # type: ignore[arg-type] ) if body.aoi is not None: geom = _aoi_to_shapely(body.aoi) @@ -626,6 +652,10 @@ async def patch( updates["lock_timeout_hours"] = body.lock_timeout_hours if body.review_required is not None: updates["review_required"] = body.review_required + if body.custom_imagery is not None: + updates["custom_imagery"] = body.custom_imagery # type: ignore[arg-type] + if body.description is not None: + updates["description"] = body.description if updates: updates["updated_at"] = datetime.now() diff --git a/api/src/tasking/projects/routes.py b/api/src/tasking/projects/routes.py index d4979b0..4d531d0 100644 --- a/api/src/tasking/projects/routes.py +++ b/api/src/tasking/projects/routes.py @@ -12,6 +12,7 @@ AoiFeature, ProjectCreateRequest, ProjectListResponse, + ProjectNameValidationResponse, ProjectResponse, ProjectRoleAddRequest, ProjectRoleItem, @@ -128,6 +129,26 @@ async def create_project( ) +@router.get("/validate-name", response_model=ProjectNameValidationResponse) +async def validate_project_name( + workspace_id: int, + name: str = Query(..., min_length=1, max_length=255), + current_user: UserInfo = Depends(validate_token), + workspace_repo: WorkspaceRepository = Depends(get_workspace_repo), + project_repo: TaskingProjectRepository = Depends(get_project_repo), +): + await assert_workspace_visible(workspace_id, current_user, workspace_repo) + normalized_name = name.strip() + if not normalized_name: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail="name cannot be blank", + ) + return ProjectNameValidationResponse( + exists=await project_repo.project_name_exists(workspace_id, normalized_name) + ) + + @router.get("/{project_id}", response_model=ProjectResponse) async def get_project( workspace_id: int, diff --git a/api/src/tasking/projects/schemas.py b/api/src/tasking/projects/schemas.py index 901d5f6..85fc0b2 100644 --- a/api/src/tasking/projects/schemas.py +++ b/api/src/tasking/projects/schemas.py @@ -10,6 +10,7 @@ from pydantic import Field as PydField from sqlalchemy import Column from sqlalchemy import Enum as SAEnum +from sqlalchemy.dialects.postgresql import JSONB from sqlmodel import Field, SQLModel # --------------------------------------------------------------------------- @@ -94,6 +95,13 @@ class TaskingProject(SQLModel, table=True): ) deleted_at: Optional[datetime] = None + # Custom Imagery data for the project. Stored as JSONB in the database and converted to / from a Pydantic model in the repository layer. + custom_imagery: Optional[Any] = Field( + default=None, sa_column=Column(JSONB, nullable=True, default=None) + ) + + description: Optional[str] = Field(default=None, nullable=True) + # --------------------------------------------------------------------------- # Project role enum + table diff --git a/docker-compose.local.yml b/docker-compose.local.yml new file mode 100644 index 0000000..8bfeae3 --- /dev/null +++ b/docker-compose.local.yml @@ -0,0 +1,71 @@ +services: + database: + image: postgis/postgis:16-3.4 + environment: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: testing + POSTGRES_DB: workspaces-osm-local + ports: + - 5432:5432 + volumes: + - ./data/db:/var/lib/postgresql/data + healthcheck: + test: ["CMD-SHELL", "pg_isready -U postgres -d workspaces-osm-local"] + interval: 10s + timeout: 5s + retries: 5 + osm-cgimap: + image: opensidewalksdev.azurecr.io/workspaces-osm-cgimap:dev + environment: + CGIMAP_INSTANCES: 10 + CGIMAP_HOST: database + CGIMAP_USERNAME: postgres + CGIMAP_PASSWORD: testing + CGIMAP_DBNAME: workspaces-osm-local + CGIMAP_MAX_CHANGESET_ELEMENTS: 100000000 # max features per import or save + CGIMAP_MAX_PAYLOAD: 1000000000 # max size of dataset uploads + CGIMAP_MAP_NODES: 100000000 # max number of nodes per requeset + CGIMAP_MAP_AREA: 1 # max area per request in square degrees + ports: + - 8000 + osm-rails: + image: opensidewalksdev.azurecr.io/workspaces-osm-rails:dev + environment: + RAILS_ENV: production + SECRET_KEY_BASE: osm_secret_key + WS_OSM_DB_HOST: database + WS_OSM_DB_USER: postgres + WS_OSM_DB_PASS: testing + WS_OSM_DB_NAME: workspaces-osm-local + stdin_open: true + tty: true + ports: + - 3000 + command: ["bundle", "exec", "rails", "s", "-p", "3000", "-b", "0.0.0.0"] + + osm-rails-worker: + image: opensidewalksdev.azurecr.io/workspaces-osm-rails:dev + environment: + RAILS_ENV: production + SECRET_KEY_BASE: osm_secret_key + WS_OSM_DB_HOST: database + WS_OSM_DB_USER: postgres + WS_OSM_DB_PASS: testing + WS_OSM_DB_NAME: workspaces-osm-local + stdin_open: true + tty: true + command: ["bundle", "exec", "rake", "jobs:work"] + + backend: + build: . + container_name: workspaces-backend + volumes: + - .:/app + ports: + - 8000:8000 + environment: + TDEI_BACKEND_URL: ${TDEI_BACKEND_URL} + TDEI_OIDC_REALM: ${TDEI_OIDC_REALM} + TDEI_OIDC_URL: ${TDEI_OIDC_URL} + OSM_DATABASE_URL: postgresql+asyncpg://postgres:testing@database:5432/workspaces-osm-local + TASK_DATABASE_URL: postgresql+asyncpg://postgres:testing@database:5432/workspaces-tasks-local \ No newline at end of file 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 diff --git a/tests/integration/test_projects_flow.py b/tests/integration/test_projects_flow.py index c1575de..088c614 100644 --- a/tests/integration/test_projects_flow.py +++ b/tests/integration/test_projects_flow.py @@ -206,6 +206,38 @@ async def test_outsider_404s_on_list( assert r.status_code == 404 +class TestProjectNameValidation: + async def test_validate_name_false_then_true( + self, client, as_lead, seeded_workspace_id + ): + """validate-name reports false before create and true after create for same workspace.""" + path = f"{API.format(wid=seeded_workspace_id)}/validate-name" + + r = await client.get(path, params={"name": "name-check"}) + assert r.status_code == 200, r.text + assert r.json() == {"exists": False} + + r = await client.post( + API.format(wid=seeded_workspace_id), + json={"name": "name-check"}, + ) + assert r.status_code == 201, r.text + + r = await client.get(path, params={"name": "name-check"}) + assert r.status_code == 200, r.text + assert r.json() == {"exists": True} + + async def test_validate_name_outsider_404( + self, client, as_outsider, seeded_workspace_id + ): + """Outsider receives 404 from tenancy gate on validate-name.""" + r = await client.get( + f"{API.format(wid=seeded_workspace_id)}/validate-name", + params={"name": "anything"}, + ) + assert r.status_code == 404 + + # --------------------------------------------------------------------------- # Workflow 3b — error mapping (constraint violations → precise HTTP status). # --------------------------------------------------------------------------- diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 20ac3f5..e9fc3ad 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -175,6 +175,14 @@ async def get(self, workspace_id: int, project_id: int): raise NotFoundException(f"Project {project_id} not found") return self._response(p) + async def project_name_exists(self, workspace_id: int, name: str) -> bool: + return any( + p["workspace_id"] == workspace_id + and p["name"] == name + and p["deleted_at"] is None + for p in self._projects.values() + ) + async def patch(self, workspace_id, project_id, body, current_user): p_resp = await self.get(workspace_id, project_id) p = self._projects[project_id] diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index 3a06d3b..cc96fa0 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -14,7 +14,7 @@ def test_defaults_loaded_when_env_unset(): s = Settings(_env_file=None) # type: ignore[call-arg] # pydantic-settings init kwarg assert s.PROJECT_NAME == "Workspaces API" - assert s.CORS_ORIGINS == [] + assert s.CORS_ORIGINS == "" assert s.DEBUG is False assert s.SENTRY_DSN == "" assert s.WS_OSM_HOST == "http://osm-web" @@ -38,18 +38,55 @@ def test_env_vars_override_members(monkeypatch): def test_cors_origins_parsed_from_json_env(monkeypatch): - monkeypatch.setenv("CORS_ORIGINS", '["https://a.example", "https://b.example"]') + monkeypatch.setenv("CORS_ORIGINS", "https://a.example,https://b.example") s = Settings(_env_file=None) # type: ignore[call-arg] # pydantic-settings init kwarg - assert s.CORS_ORIGINS == ["https://a.example", "https://b.example"] + assert s.CORS_ORIGINS == "https://a.example,https://b.example" + + +def test_cors_origins_list_comma_separated(monkeypatch): + monkeypatch.setenv("CORS_ORIGINS", "https://a.example, https://b.example") + s = Settings(_env_file=None) # type: ignore[call-arg] # pydantic-settings init kwarg + assert s.cors_origins_list == ["https://a.example", "https://b.example"] + + +def test_cors_origins_list_json_array(monkeypatch): + # The deployment stack supplies a JSON array; it must parse, not become one + # malformed bracketed origin. + monkeypatch.setenv("CORS_ORIGINS", '["https://a.example","https://b.example"]') + s = Settings(_env_file=None) # type: ignore[call-arg] # pydantic-settings init kwarg + assert s.cors_origins_list == ["https://a.example", "https://b.example"] + + +def test_cors_origins_list_single_json_array(monkeypatch): + monkeypatch.setenv("CORS_ORIGINS", '["https://only.example"]') + s = Settings(_env_file=None) # type: ignore[call-arg] # pydantic-settings init kwarg + assert s.cors_origins_list == ["https://only.example"] + + +def test_cors_origins_list_empty_and_wildcard(monkeypatch): + s = Settings(_env_file=None) # type: ignore[call-arg] # pydantic-settings init kwarg + assert s.cors_origins_list == [] + + monkeypatch.setenv("CORS_ORIGINS", "*") + s = Settings(_env_file=None) # type: ignore[call-arg] # pydantic-settings init kwarg + assert s.cors_origins_list == ["*"] + + +def test_cors_origins_list_malformed_json_falls_back_to_comma(monkeypatch): + # A value that starts with "[" but isn't valid JSON should not crash; fall + # back to comma-splitting rather than raising. + monkeypatch.setenv("CORS_ORIGINS", "[not json") + s = Settings(_env_file=None) # type: ignore[call-arg] # pydantic-settings init kwarg + assert s.cors_origins_list == ["[not json"] def test_value_types_and_formats(): s = Settings(_env_file=None) # type: ignore[call-arg] # pydantic-settings init kwarg assert isinstance(s.PROJECT_NAME, str) - assert isinstance(s.CORS_ORIGINS, list) + assert isinstance(s.CORS_ORIGINS, str) assert isinstance(s.DEBUG, bool) # empty-string default is preserved (not coerced to None): assert s.SENTRY_DSN == "" diff --git a/tests/unit/test_project_routes.py b/tests/unit/test_project_routes.py index 1620aba..792103a 100644 --- a/tests/unit/test_project_routes.py +++ b/tests/unit/test_project_routes.py @@ -1,5 +1,7 @@ from __future__ import annotations +import pytest + API = "/api/v1/workspaces/{wid}/tasking/projects" @@ -68,6 +70,17 @@ async def test_create_blank_name_422( ) assert r.status_code == 422 + @pytest.mark.parametrize("bad_value", ["not-an-object", 123, True, ["x"]]) + async def test_create_rejects_non_object_custom_imagery_422( + self, client, as_lead, seeded_workspace_id, fake_repos, bad_value + ): + """custom_imagery must be a JSON object; scalar/array values are rejected (422).""" + r = await client.post( + API.format(wid=seeded_workspace_id), + json={"name": "Pilot", "custom_imagery": bad_value}, + ) + assert r.status_code == 422 + async def test_get_404_when_missing( self, client, as_lead, seeded_workspace_id, fake_repos ): @@ -105,6 +118,24 @@ async def test_patch_name(self, client, as_lead, seeded_workspace_id, fake_repos assert r.status_code == 200 assert r.json()["name"] == "after" + @pytest.mark.parametrize("bad_value", ["not-an-object", 123, False, ["x"]]) + async def test_patch_rejects_non_object_custom_imagery_422( + self, client, as_lead, seeded_workspace_id, fake_repos, bad_value + ): + """PATCH rejects custom_imagery when it is not a JSON object (422).""" + pid = ( + await client.post( + API.format(wid=seeded_workspace_id), + json={"name": "before"}, + ) + ).json()["id"] + + r = await client.patch( + f"{API.format(wid=seeded_workspace_id)}/{pid}", + json={"custom_imagery": bad_value}, + ) + assert r.status_code == 422 + async def test_soft_delete_204_then_404( self, client, as_lead, seeded_workspace_id, fake_repos ): @@ -137,6 +168,55 @@ async def test_duplicate_name_409( assert r.status_code == 409 +class TestProjectNameValidation: + async def test_validate_name_returns_false_when_missing( + self, client, as_contributor, seeded_workspace_id, fake_repos + ): + """Validation returns exists=false when no active project uses the name.""" + r = await client.get( + f"{API.format(wid=seeded_workspace_id)}/validate-name", + params={"name": "new-project"}, + ) + assert r.status_code == 200 + assert r.json() == {"exists": False} + + async def test_validate_name_returns_true_when_exists( + self, client, as_lead, seeded_workspace_id, fake_repos + ): + """Validation returns exists=true when an active project with same name exists.""" + await client.post( + API.format(wid=seeded_workspace_id), + json={"name": "pilot-check"}, + ) + + r = await client.get( + f"{API.format(wid=seeded_workspace_id)}/validate-name", + params={"name": "pilot-check"}, + ) + assert r.status_code == 200 + assert r.json() == {"exists": True} + + async def test_validate_name_blank_rejected_422( + self, client, as_contributor, seeded_workspace_id, fake_repos + ): + """Whitespace-only name is rejected with 422.""" + r = await client.get( + f"{API.format(wid=seeded_workspace_id)}/validate-name", + params={"name": " "}, + ) + assert r.status_code == 422 + + async def test_validate_name_outsider_404( + self, client, as_outsider, seeded_workspace_id, fake_repos + ): + """Tenancy gate hides workspace existence for outsiders.""" + r = await client.get( + f"{API.format(wid=seeded_workspace_id)}/validate-name", + params={"name": "pilot-check"}, + ) + assert r.status_code == 404 + + # --------------------------------------------------------------------------- # Lifecycle gates # --------------------------------------------------------------------------- diff --git a/tests/unit/test_token_bridge.py b/tests/unit/test_token_bridge.py new file mode 100644 index 0000000..20e338f --- /dev/null +++ b/tests/unit/test_token_bridge.py @@ -0,0 +1,219 @@ +"""Tests for the OSM token bridge in ``api/core/security.py``. + +``_bridge_token_to_osm`` mirrors a validated TDEI token into the OSM database's +``oauth_access_tokens`` table so osm-rails (doorkeeper) and cgimap authenticate +it through their standard OAuth2 path. It auto-provisions everything it needs: + +- a dedicated *system* ``users`` row that owns the doorkeeper application, +- the doorkeeper ``oauth_applications`` row (idempotent by the client uid), +- the caller's ``users`` row (owns the token), +- the plaintext ``oauth_access_tokens`` row (expires_in from the JWT exp). + +We cover: the no-op when disabled, the full provisioning chain when enabled, +the wiring of ids between rows, best-effort failure handling, and revocation. +""" + +import time +from typing import cast +from uuid import uuid4 + +from sqlmodel.ext.asyncio.session import AsyncSession + +import api.core.security as security +from api.core.config import settings + + +class RecordingSession: + """Async session stand-in that records (sql, params) and answers the two id + lookups: ``users`` (system vs caller, by auth_uid) and ``oauth_applications``.""" + + def __init__(self, system_user_id=1, caller_user_id=42, app_id=7): + self.calls: list[tuple[str, dict]] = [] + self.commits = 0 + self.rollbacks = 0 + self._system_user_id = system_user_id + self._caller_user_id = caller_user_id + self._app_id = app_id + + async def execute(self, statement, params=None): + params = params or {} + sql = str(statement) + self.calls.append((sql, params)) + + class _R: + def __init__(self, rows): + self._rows = rows + + def first(self): + return self._rows[0] if self._rows else None + + if "SELECT id FROM users" in sql: + if params.get("auth_uid") == settings.WS_OSM_SYSTEM_USER_AUTH_UID: + return _R([(self._system_user_id,)]) + return _R([(self._caller_user_id,)]) + if "SELECT id FROM oauth_applications" in sql: + return _R([(self._app_id,)]) + return _R([]) + + async def commit(self): + self.commits += 1 + + async def rollback(self): + self.rollbacks += 1 + + def sql_for(self, needle): + return [c for c in self.calls if needle in c[0]] + + +async def test_bridge_is_noop_when_disabled(monkeypatch): + """With the bridge disabled the bridge touches no DB.""" + monkeypatch.setattr(settings, "WS_OSM_TOKEN_BRIDGE_ENABLED", False) + session = RecordingSession() + + await security._bridge_token_to_osm( + cast(AsyncSession, session), + user_uuid=uuid4(), + user_name="alice", + email="alice@example.com", + token="tok", + exp=None, + ) + + assert session.calls == [] + assert session.commits == 0 + + +async def test_bridge_provisions_app_users_and_token(monkeypatch): + """Enabled: system user -> application -> caller user -> mirrored token, + with the ids wired between rows.""" + monkeypatch.setattr(settings, "WS_OSM_TOKEN_BRIDGE_ENABLED", True) + monkeypatch.setattr(settings, "WS_OSM_OAUTH_CLIENT_UID", "workspaces-backend") + monkeypatch.setattr(settings, "WS_OSM_OAUTH_SCOPES", "write_api read_prefs") + uid = uuid4() + exp = int(time.time()) + 3600 + session = RecordingSession(system_user_id=1, caller_user_id=42, app_id=7) + + await security._bridge_token_to_osm( + cast(AsyncSession, session), + user_uuid=uid, + user_name="alice", + email="alice@example.com", + token="jwt-token", + exp=exp, + ) + + # System user provisioned (owns the app) and caller user provisioned. + user_inserts = session.sql_for("INSERT INTO users") + inserted_auth_uids = {c[1]["auth_uid"] for c in user_inserts} + assert settings.WS_OSM_SYSTEM_USER_AUTH_UID in inserted_auth_uids + assert str(uid) in inserted_auth_uids + + # Application created idempotently by uid, owned by the system user (id 1). + app_inserts = session.sql_for("INSERT INTO oauth_applications") + assert app_inserts and "ON CONFLICT (uid)" in app_inserts[0][0] + assert app_inserts[0][1]["uid"] == "workspaces-backend" + assert app_inserts[0][1]["owner_id"] == 1 + assert app_inserts[0][1]["scopes"] == "write_api read_prefs" + + # Token mirrored: app id from the lookup, resource_owner = caller (42), + # self-healing upsert, exp-derived TTL. + token_inserts = session.sql_for("oauth_access_tokens") + assert token_inserts + sql, params = token_inserts[0] + assert "DO UPDATE" in sql and "revoked_at = NULL" in sql + assert params["app_id"] == 7 + assert params["user_id"] == 42 + assert params["token"] == "jwt-token" + assert params["scopes"] == "write_api read_prefs" + assert 3590 <= params["expires_in"] <= 3600 + + assert session.commits == 1 + assert session.rollbacks == 0 + + +async def test_bridge_synthesises_email_when_absent(monkeypatch): + """A caller without an email claim still provisions (email is UNIQUE/NOT NULL).""" + monkeypatch.setattr(settings, "WS_OSM_TOKEN_BRIDGE_ENABLED", True) + uid = uuid4() + session = RecordingSession() + + await security._bridge_token_to_osm( + cast(AsyncSession, session), + user_uuid=uid, + user_name="alice", + email=None, + token="jwt-token", + exp=None, + ) + + caller_insert = next( + c for c in session.sql_for("INSERT INTO users") if c[1]["auth_uid"] == str(uid) + ) + assert caller_insert[1]["email"] == f"{uid}@tdei.invalid" + + params = session.sql_for("oauth_access_tokens")[0][1] + assert params["expires_in"] is None + + +async def test_bridge_is_best_effort_on_db_error(monkeypatch): + """A DB failure must not propagate out of validation; it rolls back.""" + monkeypatch.setattr(settings, "WS_OSM_TOKEN_BRIDGE_ENABLED", True) + + class BoomSession(RecordingSession): + async def execute(self, statement, params=None): + raise RuntimeError("db down") + + session = BoomSession() + + await security._bridge_token_to_osm( + cast(AsyncSession, session), + user_uuid=uuid4(), + user_name="alice", + email=None, + token="tok", + exp=None, + ) + + assert session.rollbacks == 1 + assert session.commits == 0 + + +async def test_revoke_is_noop_when_disabled(monkeypatch): + """With the bridge off, revoking a rotated token touches no DB.""" + monkeypatch.setattr(settings, "WS_OSM_TOKEN_BRIDGE_ENABLED", False) + session = RecordingSession() + + await security._revoke_osm_token(cast(AsyncSession, session), "old-token") + + assert session.calls == [] + assert session.commits == 0 + + +async def test_revoke_marks_only_the_superseded_token(monkeypatch): + """Revocation flips revoked_at for exactly the given token, if not already.""" + monkeypatch.setattr(settings, "WS_OSM_TOKEN_BRIDGE_ENABLED", True) + session = RecordingSession() + + await security._revoke_osm_token(cast(AsyncSession, session), "old-token") + + updates = session.sql_for("UPDATE oauth_access_tokens") + assert updates and "revoked_at" in updates[0][0] + assert "WHERE token = :token AND revoked_at IS NULL" in updates[0][0] + assert updates[0][1]["token"] == "old-token" + assert session.commits == 1 + + +async def test_revoke_is_best_effort_on_db_error(monkeypatch): + """A DB failure during revocation must not propagate; it rolls back.""" + monkeypatch.setattr(settings, "WS_OSM_TOKEN_BRIDGE_ENABLED", True) + + class BoomSession(RecordingSession): + async def execute(self, statement, params=None): + raise RuntimeError("db down") + + session = BoomSession() + + await security._revoke_osm_token(cast(AsyncSession, session), "tok") + + assert session.rollbacks == 1 + assert session.commits == 0