Skip to content

Commit 21e6aec

Browse files
authored
Merge pull request #65 from TaskarCenterAtUW/develop
Develop to Stage merge
2 parents 55e9963 + 2002bc4 commit 21e6aec

23 files changed

Lines changed: 1323 additions & 27 deletions

.claude/settings.json

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@
1010
"Bash(git config *)",
1111
"Bash(command -v gh)",
1212
"Bash(gh api *)",
13-
"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'\\)\\)\")"
13+
"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'\\)\\)\")",
14+
"Bash(grep -nA6 \"TENANT_BYPASSES: list\" api/main.py)",
15+
"Bash(grep -nA10 \"STRIP_REQUEST_HEADERS = \" api/main.py)"
1416
]
1517
}
1618
}
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
name: Trigger Deployment on Pull Request Merge
2+
on:
3+
pull_request:
4+
types: [closed]
5+
branches: [ develop, staging, production]
6+
permissions:
7+
contents: read
8+
actions: write
9+
jobs:
10+
on-merge:
11+
if: github.event.pull_request.merged == true
12+
runs-on: ubuntu-latest
13+
outputs:
14+
environment: ${{ steps.set-environment.outputs.environment }}
15+
steps:
16+
- name: Set Environment
17+
id: set-environment
18+
run: |
19+
case "${{ github.base_ref }}" in
20+
develop) echo "environment=develop" >> $GITHUB_OUTPUT ;;
21+
staging) echo "environment=staging" >> $GITHUB_OUTPUT ;;
22+
production) echo "environment=production" >> $GITHUB_OUTPUT ;;
23+
testing) echo "environment=testing" >> $GITHUB_OUTPUT ;;
24+
esac
25+
deploy:
26+
needs: on-merge
27+
runs-on: ubuntu-latest
28+
env:
29+
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
30+
steps:
31+
- name: Trigger Deployment Workflow
32+
run: |
33+
gh workflow run push-docker-image.yml \
34+
--repo ${{ github.repository }} \
35+
--ref ${{ github.base_ref }} \
36+
--field environment=${{ needs.on-merge.outputs.environment }}

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,3 +170,4 @@ docs/tasking-mvp/tasking-mvp.postman_environment.json
170170
docs/tasking-mvp/_enrich_postman.py
171171
docs/tasking-mvp/feature-coverage.md
172172
.idea/
173+
data/

CLAUDE.md

Lines changed: 106 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -79,24 +79,120 @@ at all). Validated against the code:
7979
* **Export to TDEI** — no endpoint exists in this backend.
8080
* **Move Workspace PG→PG** — not possible; `WorkspacePatch` has no
8181
`tdeiProjectGroupId` field, so no route can change a workspace's project group.
82-
* **Validate Changeset** and **Edit POSM Element**these go through the OSM
83-
proxy catch-all (`api/main.py`), which gates *every* proxied operation on
82+
* **Edit POSM Element**goes through the OSM proxy catch-all
83+
(`api/main.py`), which gates *every* proxied operation on
8484
`isWorkspaceContributor` alone. There is no Validator- or Lead-level check on
85-
proxied traffic.
85+
proxied traffic. Raw changeset commits (proxied `PUT /api/0.6/changeset/...`)
86+
are likewise Contributor-gated; the proxy only *tags* a contributor's new
87+
changeset with `review_requested=yes` when the workspace has `autoFlagReview`
88+
set — it does not enforce validation.
8689

87-
**The Validator role grants nothing extra at this layer.**
88-
`isWorkspaceValidator` exists in `api/core/security.py` but no endpoint
89-
authorizes on it — it only appears in the `role` field of `WorkspaceResponse`.
90-
A Validator and a Contributor have identical permissions in this backend.
90+
**Enforced here, Validator-gated (`isWorkspaceLead || isWorkspaceValidator`
91+
403).** This is a native FastAPI route, not proxied traffic. Leads (and POC via
92+
`isWorkspaceLead`) inherit it:
93+
94+
| Capability | Endpoint |
95+
|---|---|
96+
| Resolve/Validate Changeset | PUT `/workspaces/{id}/changesets/{changeset_id}/resolve` |
97+
98+
Resolving clears the `review_requested` tag and stamps `reviewed_by` with the
99+
reviewer's UUID. The gate is enforced both in the route
100+
(`api/src/osm/routes.py`) and, defensively, inside
101+
`OSMRepository.resolveChangeset` (`api/src/osm/repository.py`).
102+
103+
**The Validator role grants exactly one thing at this layer:** the ability to
104+
resolve changesets via the endpoint above. Aside from that, a Validator and a
105+
Contributor have identical permissions in this backend. `isWorkspaceValidator`
106+
otherwise only appears in the `role` field of `WorkspaceResponse`.
91107

92108
**"Contributor" and "Authenticated User With PG/Workspace Association" are the
93109
same gate.** `isWorkspaceContributor` simply checks whether the workspace is in
94110
one of the user's project groups (`accessibleWorkspaceIds`), i.e. PG/workspace
95111
association — so both rows collapse to the same check.
96112

97-
If the Validator/Lead distinctions for changeset validation and TDEI export are
98-
required, they must be enforced downstream (`workspaces-openstreetmap-website/`,
99-
`workspaces-cgimap/`) — that has not been audited here.
113+
Changeset *resolution* is Validator/Lead-gated here (see above). But the
114+
Validator/Lead distinction on raw changeset *commits* and TDEI export is not
115+
enforced at this layer; if required, it must be enforced downstream
116+
(`workspaces-openstreetmap-website/`, `workspaces-cgimap/`) — that has not been
117+
audited here.
118+
119+
## The OSM proxy layer: routing, auth, and user provisioning
120+
121+
This service is a reverse proxy in front of the OSM website (`osm-rails`) and
122+
cgimap. The non-obvious parts, learned the hard way:
123+
124+
### Deployment routing (workspaces-stack)
125+
126+
In `workspaces-stack`, the **api container (this backend) serves the public OSM
127+
host** (`osm.workspaces-<env>...`) alongside the API hosts — its traefik router
128+
rule is `Host(new-api) || Host(api) || Host(osm)`, and the `osm-web` /
129+
`osm-log-proxy` routers are commented out. So **every OSM call the frontend
130+
makes routes through this backend**: `validate_token`, the `X-Workspace` gate,
131+
and the CORS middleware all apply. The backend then proxies `/api/0.6/*` to
132+
`WS_OSM_HOST` (config default `http://osm-web` — the internal service, *not* the
133+
public domain). `osm-web`'s nginx splits traffic: changeset read/write subpaths
134+
to cgimap, everything else to osm-rails.
135+
136+
The frontend (`services/osm.ts`) calls the OSM host directly with
137+
`credentials: 'include'` + a Bearer TDEI token. Because it's *credentialed*
138+
CORS, `CORS_ORIGINS` must list the exact frontend origin (no `*`;
139+
`allow_credentials` is on). The stack supplies `WS_API_CORS_ORIGINS` as a **JSON
140+
array** (`["https://..."]`), while older config comma-split it — a JSON array
141+
would then become one malformed origin. `api/main.py` therefore reads
142+
`settings.cors_origins_list`, which parses **both** a JSON array and a
143+
comma-separated string (and `*`); set `CORS_ORIGINS` in either form.
144+
145+
### Two databases; `users` is owned by OSM Rails
146+
147+
`TASK_DATABASE_URL` (`alembic_task` tree) holds workspaces / tasking-manager
148+
tables; `OSM_DATABASE_URL` (`alembic_osm` tree) holds OSM data, `users`, and the
149+
`tasking_*` tables. The **`users` table is owned by the OSM Rails website**, not
150+
either alembic tree — the trees only FK to it, and integration tests stub it
151+
(`tests/integration/conftest.py`). The backend provisions `users` rows itself
152+
via raw SQL with `auth_provider='TDEI'` and `auth_uid = str(<JWT sub>)` (the OSM
153+
`auth_uid` **is** the token's `sub` claim).
154+
155+
### How OSM authenticates, and the TDEI token bridge
156+
157+
* **osm-rails** authenticates API calls *only* via **doorkeeper OAuth2**: it
158+
looks the bearer token up in `oauth_access_tokens` (`token`
159+
`resource_owner_id``users.id`). Doorkeeper stores tokens **plaintext**
160+
(`SecretStoring::Plain`), so the raw JWT matches directly. It has **no**
161+
TDEI/JWT auth path.
162+
* **cgimap** has both: the oauth2 lookup *and* a custom TDEI path
163+
(`get_user_id_for_tdei_token`, which verifies the JWT and matches
164+
`users.auth_uid`).
165+
166+
To make TDEI tokens work against osm-rails without forking it, the **token
167+
bridge** (`_bridge_token_to_osm` in `api/core/security.py`) mirrors a validated
168+
TDEI JWT into `oauth_access_tokens` — on the cache-miss path of `validate_token`
169+
(so ~once per token, not per request). Gated by `WS_OSM_TOKEN_BRIDGE_ENABLED`.
170+
It auto-provisions everything: a dedicated **system user** (`WS_OSM_SYSTEM_USER_*`)
171+
to own the doorkeeper `oauth_applications` row it creates (keyed by
172+
`WS_OSM_OAUTH_CLIENT_UID`; `oauth_applications.owner_id` is a NOT-NULL FK to
173+
`users`, hence the system user), the caller's `users` row, and the plaintext
174+
`oauth_access_tokens` row (`expires_in` from the JWT `exp`;
175+
`ON CONFLICT (token) DO UPDATE` so re-presenting reactivates it). On token
176+
rotation (new `jti`) the superseded token is revoked. Since the token then lives
177+
in `oauth_access_tokens`, both osm-rails and cgimap authenticate it via plain
178+
OAuth2 — cgimap's custom TDEI path becomes redundant.
179+
180+
### Provisioning gotcha: `pass_crypt` must be 8..255 chars
181+
182+
The OSM `User` model has `validates :pass_crypt, :length => 8..255`. When the
183+
backend provisions a `users` row, **`pass_crypt` must be 8–255 chars** (use a
184+
random throwaway — TDEI manages auth, so it's never used to log in). A too-short
185+
value (the old `'none'`) makes the user *invalid*. This is silent for **cgimap**
186+
operations — cgimap runs no Rails validations — but breaks any **osm-rails**
187+
operation that re-validates the author via `validates :author, :associated =>
188+
true`: notably `POST /api/0.6/changeset/{id}/comment` and note comments. The
189+
comment fails to save (no `id`), then rendering it throws *"Unable to serialize
190+
… without an id"*. Both provisioning paths (`_ensure_osm_user`,
191+
`_provision_users_from_tdei`) set a valid `pass_crypt`; migration
192+
`alembic_osm/versions/f3a7b9c1d2e4` heals legacy short rows. General principle:
193+
a backend-provisioned `users` row must satisfy OSM's `User` validations (also
194+
`display_name` 3..255 + unique, `email` present + unique) or Rails operations
195+
that touch it will fail even though cgimap operations succeed.
100196

101197
## Testing
102198

0 commit comments

Comments
 (0)