ewilliams28/turnstone
1
0
Fork 0
mirror of https://github.com/turnstonelabs/turnstone.git synced 2026-08-12 23:12:23 -06:00
Code Issues Packages Projects Releases Wiki Activity

fix: apply /review feedback on legacy URL cleanup

Browse Source 
Reviewer caught real misses on the consumer-swap claim:

- TypeScript SDK still defined and re-exported `CloseWorkstreamRequest`
  (types.ts + index.ts) — drop both. Now matches the Python-side
  removal.
- Four `tests/test_auth.py` cases (`test_write_full_token_ok`,
  `test_approve_full_token_ok`, `test_bearer_takes_precedence_over_cookie`,
  `test_cookie_full_on_write_ok`) were tautological after the legacy
  URL removal: they posted to `/api/send` / `/api/approve` and asserted
  `allowed is True`, but those paths now classify as `read` so a read
  token would also pass — they no longer tested the write/approve
  scope enforcement. Swap to path-keyed URLs to restore the original
  intent.
- `is_public_path("/api/send")` test renamed + retargeted to a
  path-keyed URL.

Doc-table drift the previous commit missed:

- `docs/security.md` path-to-scope mapping rewritten for the
  path-keyed verb family (write set, DELETE-on-/send dequeue,
  per-ws_id approve).
- `docs/architecture.md` scope-model row text swap from `/api/send`
  / `/api/approve` to the path-keyed equivalents.
- `docs/diagrams/01-system-context.puml` channel→server edge label
  swap.
- `docs/diagrams/15-auth-architecture.puml` scope class swap.

Cosmetic comment-only stragglers:

- `tests/test_session_worker.py` module docstring URL update.
- `tests/test_ratelimit.py` ~11 `/api/send` fixture-key strings
  retargeted to `/api/workstreams/abc/send` so the URL fixtures
  reflect the post-1.5 surface (rate limiter is path-agnostic; the
  swap is purely cosmetic).

4557 tests still passing under -m "not live"; ruff + mypy clean.
This commit is contained in:
Patrick Buckley
2026-04-26 21:06:25 -07:00
committed by Patrick Buckley
parent ad0e7ce6eb
commit d6e615d324
9 changed files with 27 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/architecture.md
+2 -2
View File
@@ -1100,8 +1100,8 @@ Three hierarchical scopes control endpoint access:
| Scope | Grants | Endpoints |
|-------|--------|-----------|
| `read` | SSE streams, workstream listing, history | GET endpoints |
| `write` | `read` + send, command, workstream create/close | POST to `/api/send`, `/api/command`, etc. |
| `approve` | `write` + tool approval, admin operations | POST to `/api/approve`, `/api/admin/*` |
| `write` | `read` + send, command, workstream create/close | POST to `/api/workstreams/{ws_id}/send`, `/api/command`, etc. |
| `approve` | `write` + tool approval, admin operations | POST to `/api/workstreams/{ws_id}/approve`, `/api/admin/*` |
### Middleware Flow
docs/diagrams/01-system-context.puml
+1 -1
View File
@@ -43,7 +43,7 @@ eval --> sqlite : SQLite
console --> server : HTTP proxy\n(hash-ring bucket lookup,\nproxy /node/{id}/* traffic)
channel --> server : HTTP + SSE\n(POST /v1/api/send,\nGET /v1/api/events)
channel --> server : HTTP + SSE\n(POST /v1/api/workstreams/{ws_id}/send,\nGET /v1/api/workstreams/{ws_id}/events)
' Notes
note right of console
docs/diagrams/15-auth-architecture.puml
+1 -1
View File
@@ -79,7 +79,7 @@ class "Scope Hierarchy" as SH <<scope>> {
--
GET → read
POST write paths → write
POST /api/approve → approve
POST /api/workstreams/{ws_id}/approve → approve
/api/admin/* → approve
}
docs/security.md
+5 -4
View File
@@ -67,10 +67,11 @@ Scopes are hierarchical — higher scopes imply all lower ones.
| Method | Path pattern | Required scope |
|--------|-------------|----------------|
| GET | Any protected path | `read` |
| POST | `/api/send`, `/api/plan`, `/api/command` | `write` |
| POST | `/api/workstreams/new`, `/api/workstreams/close` | `write` |
| POST | `/api/cluster/workstreams/new` | `write` |
| POST | `/api/approve` | `approve` |
| POST | `/api/plan`, `/api/command` | `write` |
| POST | `/api/workstreams/new`, `/api/cluster/workstreams/new` | `write` |
| POST | `/api/workstreams/{ws_id}/{send,cancel,close,delete,open,refresh-title,title,attachments}` | `write` |
| DELETE | `/api/workstreams/{ws_id}/send` (dequeue), `/api/workstreams/{ws_id}/attachments/{attachment_id}` | `write` |
| POST | `/api/workstreams/{ws_id}/approve` | `approve` |
| Any | `/api/admin/*` | `approve` |
Public paths bypass authentication entirely: `/`, `/health`, `/metrics`,
sdk/typescript/src/index.ts
-1
View File
@@ -84,7 +84,6 @@ export type {
CommandRequest,
CreateWorkstreamRequest,
CreateWorkstreamResponse,
CloseWorkstreamRequest,
WorkstreamInfo,
ListWorkstreamsResponse,
DashboardWorkstream,
sdk/typescript/src/types.ts
-4
View File
@@ -161,10 +161,6 @@ export interface CreateWorkstreamResponse {
attachment_ids?: string[];
}
export interface CloseWorkstreamRequest {
ws_id: string;
}
export interface WorkstreamInfo {
// Renamed `id` → `ws_id` and added kind/parent_ws_id/user_id in
// the Stage 2 list-verb lift. Pre-1.5 readers branching on
tests/test_auth.py
+6 -6
View File
@@ -53,8 +53,8 @@ class TestIsPublicPath:
def test_api_workstreams_not_public(self):
assert is_public_path("/api/workstreams") is False
def test_api_send_not_public(self):
assert is_public_path("/api/send") is False
def test_api_workstreams_send_not_public(self):
assert is_public_path("/api/workstreams/abc/send") is False
def test_api_cluster_overview_not_public(self):
assert is_public_path("/api/cluster/overview") is False
@@ -419,7 +419,7 @@ class TestCheckRequest:
def test_write_full_token_ok(self, full_jwt):
allowed, status, msg, _result = check_request(
"POST", "/api/send", full_jwt, jwt_secret=self._SECRET
"POST", "/api/workstreams/abc/send", full_jwt, jwt_secret=self._SECRET
)
assert allowed is True
assert status == 200
@@ -520,7 +520,7 @@ class TestCheckRequest:
def test_approve_full_token_ok(self, full_jwt):
allowed, status, msg, _result = check_request(
"POST", "/api/approve", full_jwt, jwt_secret=self._SECRET
"POST", "/api/workstreams/abc/approve", full_jwt, jwt_secret=self._SECRET
)
assert allowed is True
@@ -562,7 +562,7 @@ class TestCheckRequestWithCookie:
def test_bearer_takes_precedence_over_cookie(self, read_jwt, full_jwt):
allowed, status, _, _r = check_request(
"POST",
"/api/send",
"/api/workstreams/abc/send",
f"Bearer {full_jwt}",
cookie_header=f"turnstone_auth={read_jwt}",
jwt_secret=self._SECRET,
@@ -594,7 +594,7 @@ class TestCheckRequestWithCookie:
def test_cookie_full_on_write_ok(self, full_jwt):
allowed, status, _, _r = check_request(
"POST",
"/api/send",
"/api/workstreams/abc/send",
None,
cookie_header=f"turnstone_auth={full_jwt}",
jwt_secret=self._SECRET,
tests/test_ratelimit.py
+11 -11
View File
@@ -61,15 +61,15 @@ class TestRateLimiter:
def test_disabled_allows_everything(self):
limiter = RateLimiter(enabled=False, rate=1.0, burst=1)
for _ in range(100):
allowed, retry = limiter.check("1.2.3.4", "/api/send")
allowed, retry = limiter.check("1.2.3.4", "/api/workstreams/abc/send")
assert allowed is True
assert retry == 0.0
def test_exempt_paths_bypass(self):
limiter = RateLimiter(enabled=True, rate=1.0, burst=1)
# Exhaust the bucket on a normal path
limiter.check("1.2.3.4", "/api/send")
limiter.check("1.2.3.4", "/api/send")
limiter.check("1.2.3.4", "/api/workstreams/abc/send")
limiter.check("1.2.3.4", "/api/workstreams/abc/send")
# Exempt paths should still pass
allowed, retry = limiter.check("1.2.3.4", "/health")
@@ -84,18 +84,18 @@ class TestRateLimiter:
limiter = RateLimiter(enabled=True, rate=1.0, burst=1)
# Exhaust IP A
allowed_a, _ = limiter.check("10.0.0.1", "/api/send")
allowed_a, _ = limiter.check("10.0.0.1", "/api/workstreams/abc/send")
assert allowed_a is True
allowed_a, _ = limiter.check("10.0.0.1", "/api/send")
allowed_a, _ = limiter.check("10.0.0.1", "/api/workstreams/abc/send")
assert allowed_a is False
# IP B should still have its own bucket
allowed_b, _ = limiter.check("10.0.0.2", "/api/send")
allowed_b, _ = limiter.check("10.0.0.2", "/api/workstreams/abc/send")
assert allowed_b is True
def test_burst_then_reject(self):
limiter = RateLimiter(enabled=True, rate=10.0, burst=3)
results = [limiter.check("1.2.3.4", "/api/send")[0] for _ in range(5)]
results = [limiter.check("1.2.3.4", "/api/workstreams/abc/send")[0] for _ in range(5)]
assert results == [True, True, True, False, False]
def test_cleanup_removes_stale(self):
@@ -104,8 +104,8 @@ class TestRateLimiter:
limiter = RateLimiter(enabled=True, rate=10.0, burst=5)
# Create buckets for two IPs
limiter.check("10.0.0.1", "/api/send")
limiter.check("10.0.0.2", "/api/send")
limiter.check("10.0.0.1", "/api/workstreams/abc/send")
limiter.check("10.0.0.2", "/api/workstreams/abc/send")
# Advance time past max_age for both
mock_time.return_value = 5000.0
@@ -120,11 +120,11 @@ class TestRateLimiter:
mock_time.return_value = 1000.0
limiter = RateLimiter(enabled=True, rate=10.0, burst=5)
limiter.check("10.0.0.1", "/api/send")
limiter.check("10.0.0.1", "/api/workstreams/abc/send")
# Only 60s later — well within max_age
mock_time.return_value = 1060.0
limiter.check("10.0.0.2", "/api/send")
limiter.check("10.0.0.2", "/api/workstreams/abc/send")
mock_time.return_value = 1060.0
removed = limiter.cleanup(max_age=3600.0)
tests/test_session_worker.py
+1 -1
View File
@@ -1,7 +1,7 @@
"""Unit tests for ``turnstone.core.session_worker``.
The shared worker dispatch is load-bearing for both the interactive
``/v1/api/send`` HTTP handler and the coordinator
``/v1/api/workstreams/{ws_id}/send`` HTTP handler and the coordinator
``CoordinatorAdapter.send`` path. Tests cover the four invariants the
module must hold: