diff --git a/CHANGELOG.md b/CHANGELOG.md index b59144f4..bff0a007 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,122 @@ project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). --- +## [Unreleased] + +Tenant isolation and safe filters on top of core's tenant columns (`tenant_columns=` on +`PgCheckpointer` / `PgVectorStore`) and filter language (`tenxgraph.storage.sql_filter`). +Needs the `10xgraph` release that ships them. + +### Breaking + +- **Security:** `RoleBasedAuthorizationBackend(isolation=...)` (and `authorization.isolation` + in `10xgraph.json` for the `rbac` backend) raises `ValueError` for anything other than + `"owner"`, `"none"` or a `tenxgraph.core.authz.Isolation` member, so the server refuses to + boot. An unknown value such as `"owners"` used to become `"owner"` silently. +- **Security:** `resolve_isolation_scope` gives a backend without an `isolation_scope` method + `"owner"` (the `AuthorizationBackend` default, fail closed). It used to give `"none"`. +- An `isolation_scope(user)` result that is an unknown string now fails the request (403): + core's `build_authz` raises for it instead of turning it into `"none"`. +- Client `filters` on an extra column whose type is not one of the six built-in types + (`jsonb`, `text[]`, `numeric(10,2)`, ...) accept only `is_null`; anything else is a 422 with + core's message. + +### Added + +- `POST /v1/store/memories/list` takes `filters`, validated like search and forget (a tenant + column or `user_id` is a 422, reserved owner keys are stripped). Filters only narrow the + list; the isolation scope still applies. Without `filters` the store call is unchanged. +- `AuthorizationBackend.isolation_scope(user)` is called per request with the authenticated + user and may return `"owner"`, `"none"` or a list of columns (`"user_id"` and declared + tenant columns, e.g. `["org_id"]` for an org admin). The result is stamped into the trusted + `authz` block for core. Backends that define `isolation_scope(self)` with no argument keep + working (the signature decides how it is called). A malformed column list is a 403, never + "no isolation". +- `GET /v1/threads?filters=`: column conditions on `thread_name`, `created_at`, + `updated_at` and the checkpointer's extra columns, validated with core's `validate_filters` + before the listing runs. Bad JSON, an unknown column or operator, a tenant column or + `user_id`, and a checkpointer without filterable columns (in-memory, SQLite) are 422 with a + message that never echoes the input and survives production. +- Store routes (`/v1/store/search`, `/v1/store/memories/forget`) validate `filters` with the + same validator when the store has a `filter_schema` (`PgVectorStore`): a tenant column, + `user_id` or a bad condition is a 422 before the store (or the embedding call) runs. Stores + without one (`QdrantStore`) are unchanged. + +### Changed + +- `OwnershipAuthorizationBackend` (and `RoleBasedAuthorizationBackend`) compare a thread with + the caller on every column the caller's scope picks, through + `BaseCheckpointer.aget_thread_scope` (`aget_thread_owner` for checkpointers without it). + Two tenants with the same `user_id` can no longer reach each other's threads. The caller's + values come only from the authenticated user (`config["user"]`); a picked column the thread + or the caller lacks, or one the checkpointer does not declare, denies. +- With `isolation: "none"`, `RoleBasedAuthorizationBackend` now lets any authenticated user + reach an existing thread (no scope column to compare), matching the unscoped storage that + setting already gave. Unowned threads keep the create-only rule. +- The ownership cache holds the thread's scope values. Without tenant columns the shared cache + entry keeps its old layout (`owner:id:`, plain owner); with them it is a JSON object + under `owner:scope:`, so an owner-only entry is never read as a full scope. A cached + entry missing a compared column is looked up again. A thread deleted by someone other than + its owner (an org admin) leaves a tombstone that blocks everyone. +- With tenant columns declared, cached thread scopes live in the cache store for 5 minutes + (`TENANT_L2_TTL_SECONDS`) instead of an hour, so a delete whose cache eviction failed leaves + the old scope readable for minutes, not an hour. Without tenant columns the TTL stays 1 h. + +### Security + +- Uploaded files are scoped by tenant columns as well as `user_id`. With tenant columns + declared on the bound checkpointer (`PgCheckpointer(tenant_columns=...)`), an upload records + the caller's values (from `config["user"]` only, converted with the column types) in the + file's metadata under `tenant`, and every file path requires them to match the caller's: + download, `/info`, `/url`, `DELETE`, `GET /v1/files`, `graph://media/` and `file_id` + references in invoke, stream, WebSocket, AG-UI, `PUT state` and `POST messages`, thread + links and the per-user quota. Before, `alice` in `org-b` could list, read and reference the + files of `alice` in `org-a`, and both shared one quota. Fails closed: an identity without a + tenant value gets 403 on upload and 404 on reads, and a file with no recorded tenant values + (uploaded before this release, or ownerless) is denied while tenant columns are declared, + whatever `MEDIA_REQUIRE_OWNER` says. To keep such files, backfill their metadata with + `{"tenant": {"": ""}}` (canonical text, e.g. lowercase UUID). Without tenant + columns nothing changes and no key is written. + +### Fixed + +- Deleting a thread now releases the files linked to it by the thread's owner, read with + `aget_thread_scope` before the delete, as well as the deleter's. Before, an org admin (a scope + without `user_id`) or a `none`-scope deleter released only their own files, so the member's + linked files stayed behind. `media.delete_with_thread` still decides whether a file left + with no thread is deleted. +- `auth` `{"method": "custom", "path": "module:attr"}` now loads a top-level module in the + project folder (`auth_app:TokenAuth`), a dotted package path and a file path form + (`auth/custom.py:Cls`). Before, `auth_app:TokenAuth` failed with "Custom auth path does not + exist" and `auth_app.py:TokenAuth` with "No module named auth_app.py". A path that truly + does not exist still raises the same error naming it. +- `POST /v1/store/search`: `retrieval_strategy` and `distance_metric` now default to `None`, so the store's own settings apply. They were forced to `similarity` / `cosine`, which broke a text-only or HYBRID `PgVectorStore` and a Qdrant store with another distance. + +- **Security:** a store bound from the `store` key in `10xgraph.json` skipped core's + checkpointer/store tenant column check (it never goes through `compile()`). The loader now + runs `check_tenant_columns` against the graph's checkpointer (and the one bound in the + container): `MODE=production` refuses the boot, other modes warn. New key + `store_allow_tenant_mismatch` (bool, default `false`) opts out, like + `compile(allow_tenant_mismatch=True)`, for a store holding only shared data. It is in the + config editor's Memory store section. +- **Security:** the thread delete tombstone exempted the deleter by `user_id` only, so the same + `user_id` in another tenant could recreate a just-deleted id while another worker still + cached the old tenant's scope. With tenant columns the tombstone stores the deleter's + `user_id` and tenant values (canonical JSON under `owner:tomb:`) and only a caller + matching all of them is exempt. `OwnershipAuthorizationBackend.evict` takes the deleting + `user=`; `CheckpointerService.delete_thread` passes it when the backend accepts it. Without + tenant columns the tombstone keeps its plain `user_id` layout. A plain tombstone written by + an older worker during a rolling deploy blocks everyone when tenant columns are declared. +- A cache store error while evicting a deleted thread's scope or writing its tombstone was + swallowed at debug level. It is now a warning, `ThreadOwnershipResolver.evict` raises + `OwnershipCacheEvictionError`, and `DELETE /v1/threads/{thread_id}` reports `"cache"` in + `cleanup_failed`. +- Stale docs said AG-UI and `/v1/graph/live` pass `client_thread_ids=True`; they follow + `runs.thread_ids` like REST since G-113. The `client_thread_ids` authorize context flag stays + for custom callers. + +--- + ## [1.0.0rc4] - 2026-11-01 The fourth release candidate of `10xgraph-api` 1.0. It requires `10xgraph` 1.0.0rc4 diff --git a/CLAUDE.md b/CLAUDE.md index 47926f48..94fef42d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -93,10 +93,11 @@ Parsed by `tenxgraph_api/src/app/core/config/graph_config.py`. Supported keys: | `checkpointer` | `"module:attr"` -> a `BaseCheckpointer` | | `injectq` | `"module:attr"` -> an InjectQ container | | `store` | `"module:attr"` -> a `BaseStore` | +| `store_allow_tenant_mismatch` | Boolean, default false. The loader runs core `check_tenant_columns` on the store bound from `store` against the graph checkpointer (and a container-bound one): a tenant-column mismatch refuses to boot under `MODE=production` and warns otherwise. `true` logs the mismatch at INFO and boots (for a store that holds only shared data, such as a RAG knowledge base) | | `cache_store` | One object, `graph_config.CacheStoreConfig`: `type` (`memory` default \| `redis` \| `custom`), `url` (type redis only, optional: empty uses the `REDIS_URL` environment variable, a set value wins; `$VAR` / `${VAR}` expanded when the store is built, unset variable is a config error; `RedisCacheStore`, also Valkey, Dragonfly, KeyDB), `path` (type custom only, required; `"module:attr"` -> a `BaseCacheStore` instance or zero-arg factory), `rate_limit` (object, see below), `token_revocation` (see its row). No key or no `type`: `InMemoryCacheStore` (per process). Unknown keys are config errors listing the valid ones. Nothing else picks the store (`REDIS_URL` alone does not). Selected and bound at boot by `core/cache_store/loader.py::load_and_bind_cache_store` (one INFO line `Cache store: (, )`, e.g. `cache_store.url`), closed once in the lifespan; type redis without the `redis` package refuses to boot. Holds the run locks and caps, idempotency, ownership cache L2, token revocation and rate-limit counters. Top-level `redis`, `rate_limit` and `token_revocation` keys are config errors at `GraphConfig` load (`check_cache_store_settings`) saying where the setting goes | | `ag_ui` | `{"enabled": bool}`, default off. Mounts `POST /v1/ag-ui` (AG-UI protocol, for CopilotKit and other AG-UI clients). Needs the `ag-ui` extra | | `websocket` | `enabled` (`false` leaves out `/v1/graph/ws` and `/v1/graph/live`; `true` without `websockets`/`wsproto` refuses to boot; unset keeps them and warns if no library; `routers/setup_router.py::_graph_router`), `max_connections` (1000), `max_connections_per_user` (10), `realtime_models` (models a `/v1/graph/live` client may pick), `realtime_client_overrides` (init options a client may set; default `voice`, `modalities`, `vad`; add `system_prompt` only for trusted callers) | -| `runs` | `RunsConfig`: `thread_ids` (`server` default = only existing ids may be named on REST invoke/stream, `/v1/graph/ws` and checkpointer writes, a new thread is started by omitting the id; enforced by the ownership/rbac backends via `OwnershipAuthorizationBackend(server_thread_ids=...)`. AG-UI and `/v1/graph/live` are exempt: they pass `client_thread_ids=True` to `authorize` because their frontends make their own ids, and the caller owns a new one. `client` = any request may name a new thread, explicit opt-in), `config_keys` / `state_keys` (allow-lists for request config and `initial_state`/PUT state; 422 on other keys; `thread_id`, `run_id` always allowed), `max_concurrent_per_user` (HTTP invoke/stream/AG-UI runs in flight, default 10, 429 over it; `routers/graph/run_limits.py`), `timeout` (seconds; invoke 504, streams end with an error chunk), `max_parallel_tool_calls` (positive int, default unset = core's default 10 or env `TENXGRAPH_MAX_PARALLEL_TOOL_CALLS`; set into every run's config in `_prepare_input`, reserved from clients) | +| `runs` | `RunsConfig`: `thread_ids` (`server` default = only existing ids may be named on REST invoke/stream, `/v1/graph/ws` and checkpointer writes, a new thread is started by omitting the id; enforced by the ownership/rbac backends via `OwnershipAuthorizationBackend(server_thread_ids=...)`. AG-UI and `/v1/graph/live` follow the same rule (a blank or `new` id gets a server id). `client` = any request may name a new thread, explicit opt-in), `config_keys` / `state_keys` (allow-lists for request config and `initial_state`/PUT state; 422 on other keys; `thread_id`, `run_id` always allowed), `max_concurrent_per_user` (HTTP invoke/stream/AG-UI runs in flight, default 10, 429 over it; `routers/graph/run_limits.py`), `timeout` (seconds; invoke 504, streams end with an error chunk), `max_parallel_tool_calls` (positive int, default unset = core's default 10 or env `TENXGRAPH_MAX_PARALLEL_TOOL_CALLS`; set into every run's config in `_prepare_input`, reserved from clients) | | `cache_store.token_revocation` | `true`, `false`, `null` or `{"enabled", "token_lifetime", "user_ttl"}` (`graph_config.TokenRevocationConfig`; a string is a config error). Read through `GraphConfig.token_revocation`. Revocations live in the cache store (`core/auth/revocation.py`: `revoke_token`, `revoke_user`, `is_revoked`), checked by `RequirePermission` (401), before every `/v1/graph/ws` run, and before and every 30 s during a `/v1/graph/live` session (`REALTIME_RECHECK_INTERVAL`, close 1008); cache store errors fail closed (503), and an unreachable cache store refuses the boot | | `media` | `delete_with_thread` (default true): delete an uploaded file when the last message or thread that references it is deleted. Links are recorded in the file's `threads` metadata (`MediaService.link_threads` / `unlink_threads`); files without a `threads` key (uploaded before link tracking) and stores without `update_metadata` (core older than the release that added it) are never auto-deleted. `false` still records links. Parsed by `graph_config.MediaConfig` | | `graph_introspection` | `"open"` or `"scoped"` (default scoped in production): `GET /v1/graph`, `/v1/graph/tools`, `/v1/graph:StateSchema` need an explicit `graph:read` scope (`RequirePermission(..., introspection=True)`) | @@ -125,9 +126,23 @@ valid ones (`RATE_LIMIT_KEYS`). `allow_all` only with auth off in development (no identity, so nothing to authorize). Selection lives in `loader._resolve_authorization_backend`; `RequirePermission` passes `resource_id` (thread_id from path or body) to `authorize`. -- **Isolation defaults to owner.** `AuthorizationBackend.isolation_scope()` returns `"owner"`, so a - custom backend with only `authorize()` still scopes threads and memories per user; return - `"none"` on purpose to share (`DefaultAuthorizationBackend` / `allow_all` does). +- **Isolation defaults to owner.** `AuthorizationBackend.isolation_scope(user)` is called per request and + returns `"owner"` by default, so a custom backend with only `authorize()` still scopes threads and + memories per user; return `"none"` on purpose to share (`DefaultAuthorizationBackend` / `allow_all` + does), or a list of `user_id` and declared tenant columns (e.g. `["org_id"]` for an org admin). + Backends with the old no-argument `isolation_scope(self)` still work (signature decides, no + retry on `TypeError`). `core.authz.Isolation` is the `OWNER` / `NONE` StrEnum; an unknown scope + raises (`RoleBasedAuthorizationBackend(isolation=...)` raises at construction, so the server + refuses to boot). The result is stamped in `config["user"]["authz"]`. Tenant column values come + only from `config["user"]` (the `BaseAuth` result or JWT claims). +- **Ownership compares scope columns.** `OwnershipAuthorizationBackend` allows a thread when its + values (`BaseCheckpointer.aget_thread_scope`, cached by `ThreadOwnershipResolver` with the tenant + values) equal the caller's on every column the scope picks; a scope naming a column the + checkpointer does not declare denies (fails closed). +- **Client `filters`.** `GET /v1/threads?filters=` (at most 8192 chars) and the store routes' + `filters` are checked by `core/auth/client_filters.py` with core's `sql_filter` validator before + any DB work: malformed JSON, non-filterable or tenant columns / `user_id`, a bad operator, or a + checkpointer without filterable columns answer 422 (`ClientInputError`), never echoing values. - **Token claims are validated.** `JwtAuth` requires `user_id` to be a non-empty string or an int (stored as a string), else 401. `token_revoked` checks `jti` even with an odd `user_id`. Scopes come from `authorization.scopes_from_claims`: the `scope` claim (space-separated) and @@ -209,7 +224,12 @@ valid ones (`RATE_LIMIT_KEYS`). `GET /v1/files/{file_id}`, `/{file_id}/info`, `/{file_id}/url`, `DELETE /v1/files/{file_id}` (owner only, 404 otherwise, scope `files:delete`, added in `authorization.FILES_DELETE_SCOPES` until core lists it), `GET /v1/config/multimodal`. Deleting a message or thread also releases - the files it referenced (`CheckpointerService.delete_message` / `delete_thread`). + the files it referenced (`CheckpointerService.delete_message` / `delete_thread`; a thread delete + releases the thread owner's files too, read with `aget_thread_scope` before the delete). With + tenant columns on the bound checkpointer, files are tenant-scoped: upload records + `metadata["tenant"]` (canonical values from `config["user"]`, `authorization.identity_values`), + and access, list, quota, `graph://media/` / `file_id` references and `link_threads` require a + match on every tenant column (`media.tenant_matches`); a file without tenant values is denied. - **AG-UI** (`tags=["AG-UI"]`, only when `ag_ui.enabled`): `POST /v1/ag-ui`. `routers/ag_ui/`: `converter.py` (RunAgentInput -> new user/tool messages only; the checkpoint is the record), `event_mapper.py` (StreamChunk -> AG-UI events), `service.py` (runs `GraphService.stream_chunks`). diff --git a/JOBS_PLAN.md b/JOBS_PLAN.md new file mode 100644 index 00000000..effd40e0 --- /dev/null +++ b/JOBS_PLAN.md @@ -0,0 +1,130 @@ +# Jobs and cron (TaskIQ): sprint plan + +Status: planned, nothing coded. Each sprint ends with a manual test the user runs before the next one starts. + +## Goal + +Opt-in background jobs and cron for production, using native TaskIQ. Developers write plain `@broker.task` functions. 10xGraph owns the broker, loads the same graph and InjectQ container in the worker, and adds HTTP and webhook triggers plus build support. + +```python +from tenxgraph_api.jobs import broker + +@broker.task(http=True, retry_on_error=True, max_retries=5) +async def summarize_interview(interview_id: str): ... + +@broker.task(schedule=[{"cron": "0 2 * * *"}]) +async def nightly_cleanup(): ... +``` + +```json +"worker": { + "jobs": "jobs", + "broker": "redis", + "url": null +} +``` + +There is no `worker` section by default. Without it no routes are added, no worker runs and TaskIQ is not imported. TaskIQ comes only with `pip install "10xgraph-api[worker]"`. + +### `worker` section + +The `worker` section is separate from `cache_store`. Jobs can use their own Redis or Redis database. + +| Key | Meaning | +|---|---| +| `jobs` (required) | Module to import, so its `@broker.task` functions are registered. | +| `broker` | `"redis"` (default) builds a `RedisStreamBroker`. `"module:attr"` points at any TaskIQ broker the user built (RabbitMQ, NATS, Kafka and so on); 10xGraph uses it as is. | +| `url` | Redis URL for the built-in broker, with `$VAR` and `${VAR}` expanded. It is ignored when `broker` is `"module:attr"`. | + +The URL is resolved in this order: `worker.url`, then the `WORKER_REDIS_URL` env var, then `REDIS_URL`. A user with one Redis sets only `REDIS_URL`, and it is used everywhere. A user who wants jobs on a separate Redis or database sets `WORKER_REDIS_URL`. + +With a custom broker, tasks are written against the user's own broker object, not `tenxgraph_api.jobs.broker`. Everything else (loader, HTTP labels, build) works the same. + +## Known blocker + +`taskiq-redis` 1.2.4 requires `redis>=8.0.0,<9`, but our `redis` extra pins `redis>=5.0.7,<7`. Sprint 0 must settle this, either by raising our pin (which must be checked against core's Redis usage) or by pinning an older taskiq-redis. + +## Sprint 0: spike (no product code) + +In a scratch venv with `taskiq` 0.13.0 and `taskiq-redis` 1.2.4, check the following: + +- [ ] How to list a broker's registered tasks and find one by name. Build detection and the HTTP route both depend on this. +- [ ] Custom labels such as `http=True` reach the task's labels without errors. +- [ ] `SmartRetryMiddleware` works with `RedisStreamBroker` (retry, delay, max retries). +- [ ] When the worker is killed mid-task, the task is redelivered (stream acks). +- [ ] Graceful shutdown: SIGTERM finishes running tasks within `--shutdown-timeout`. +- [ ] `TaskiqScheduler` with `LabelScheduleSource` fires a cron task once. +- [ ] The redis version conflict above is resolved. + +**Exit:** a short findings note. The plan is updated where reality differs. + +## Sprint 1: worker core + +- [ ] Add the `[worker]` extra: `taskiq`, `taskiq-redis`, with versions from sprint 0. +- [ ] Add the `worker` config section (`jobs`, `broker`, `url`), parsed in `graph_config.py`. Unknown keys are config errors, the same as other sections. +- [ ] `tenxgraph_api.jobs.broker`: a lazily configured `RedisStreamBroker` with retry middleware and a result backend (with expiry). Its URL is `worker.url`, then `WORKER_REDIS_URL`, then `REDIS_URL`. It is independent of `cache_store`. +- [ ] Support `broker: "module:attr"` by loading the user's TaskIQ broker and checking that it is a TaskIQ broker. +- [ ] Replace the legacy, fully commented-out TaskIQ stubs in `src/app/worker.py` and `src/app/tasks/`. They come from an old template (Tortoise, `fastapi_injector`) and nothing imports them. +- [ ] `10xgraph worker` command. It loads env, then `load_container`, then `load_graph`, then the `jobs` module, then runs the TaskIQ worker. +- [ ] InjectQ resolution inside tasks, so `Inject[...]` works the same as in graph code. +- [ ] Startup error when the `worker` section is set but the extra is missing. +- [ ] Tests: unit tests plus one integration test against real Redis. + +**User test:** +1. Run `10xgraph worker` on the example project. +2. Call `.kiq()` from a script. +3. Confirm the task gets an injected dependency and can invoke the graph. +4. Confirm a failing task retries. +5. Kill the worker mid-task and confirm the task reruns. + +## Sprint 2: HTTP trigger + +- [ ] Add `POST /v1/jobs/{name}`, only for tasks labeled `http=True`. + - Uses the existing auth plus an `AuthorizationBackend` check on resource `jobs`, action `trigger`. + - Validates the body against the task signature. + - Calls `.kiq()` and returns `202` with the `job_id`. +- [ ] Add `GET /v1/jobs/runs/{job_id}` for status and result, read from the result backend. +- [ ] Add `GET /v1/jobs` to list the tasks exposed over HTTP. +- [ ] Unknown tasks and unlabeled tasks return 404. + +**User test:** +1. Call the endpoint with curl, with and without a token. +2. Send a bad body and confirm it is rejected. +3. Poll the status endpoint until the result arrives. + +## Sprint 3: webhook trigger + +- [ ] Add a `webhook=Verifier` label, which uses signature checks instead of user auth. +- [ ] Ship a built-in HMAC verifier with the secret read from env. +- [ ] Add a base class for custom verifiers, such as LiveKit's. +- [ ] Return 401 on a bad signature. The endpoint enqueues only and never runs the job inline. + +**User test:** +1. Send a signed request and an unsigned one. +2. Run the LiveKit-style example: webhook, then summarize job. + +## Sprint 4: cron and build + +- [ ] Add the `10xgraph scheduler` command (loader plus `TaskiqScheduler` with `LabelScheduleSource`). +- [ ] `10xgraph build` imports the jobs module and reads the broker's tasks: + - No `worker` section: `api` only. + - Tasks: `api` and `worker`. + - Any task with a `schedule` label: also `scheduler`, with replicas pinned to 1. +- [ ] Document the one-scheduler rule. + +**User test:** +1. Build three sample projects (no jobs, jobs only, cron) and check the compose files. +2. Run the cron project and confirm the job fires once per tick. + +## Later (not scheduled) + +- Admin-only schedule edit (`ListRedisScheduleSource` plus `PATCH /v1/jobs/{name}/schedule`). +- Redis event triggers. +- Docs page and a client SDK method for triggering a job and reading its status. + +## Rules carried through every sprint + +- Jobs are registered only in code. HTTP can trigger a job but never register one. +- Delivery is at least once, so idempotency is the developer's job (key the work by, for example, `interview_id`). This goes in the docs from sprint 1. +- Cron jobs run as `system_user_id`. HTTP jobs run as the caller. +- Chat streaming and WebSocket code stay untouched. diff --git a/graph/dummy_store.py b/graph/dummy_store.py index e0fb8b81..c4bd11c3 100644 --- a/graph/dummy_store.py +++ b/graph/dummy_store.py @@ -7,7 +7,7 @@ substring match with a fake similarity score — enough to see the UI work. Referenced in 10xgraph.json as ``"store": "graph.dummy_store:store"``. Swap for a -real ``QdrantStore``/``Mem0Store`` when embeddings + a vector DB are available. +real ``QdrantStore``/``PgVectorStore`` when embeddings + a vector DB are available. """ from __future__ import annotations diff --git a/pyproject.toml b/pyproject.toml index d9efbc77..5b42733d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -398,3 +398,10 @@ dev = [ "gunicorn>=23,<27", "uvicorn-worker>=0.4,<1", ] + +# TEMPORARY: develop and test against unreleased core (PgVectorStore, tenant columns, +# sql_filter, Isolation, aget_thread_scope). uv sources are not written into the built +# wheel, so published packages still depend on 10xgraph from PyPI. +# Remove this block at the 1.0.0rc5 release and pin "10xgraph>=1.0.0rc5,<2.0" instead. +[tool.uv.sources] +10xgraph = { git = "https://github.com/10xGraph/10xGraph", branch = "main" } diff --git a/tenxgraph_api/cli/config_editor/schema.py b/tenxgraph_api/cli/config_editor/schema.py index 4abca85b..baa65b20 100644 --- a/tenxgraph_api/cli/config_editor/schema.py +++ b/tenxgraph_api/cli/config_editor/schema.py @@ -111,6 +111,7 @@ "observability", "injectq", "store", + "store_allow_tenant_mismatch", "checkpointer", "thread_name_generator", "remote_tools", @@ -962,6 +963,17 @@ "placeholder": "graph.dependencies:store", "help": "module:attr of a BaseStore instance.", }, + { + "path": ["store_allow_tenant_mismatch"], + "label": "Allow tenant column mismatch", + "type": "bool", + "default": False, + "help": ( + "The server refuses to start under MODE=production (and warns otherwise) " + "when this store and the checkpointer declare different tenant columns. " + "Turn on only for a store that holds shared data, such as a knowledge base." + ), + }, ], }, { diff --git a/tenxgraph_api/cli/config_editor/validation.py b/tenxgraph_api/cli/config_editor/validation.py index f8baffc2..fe873f65 100644 --- a/tenxgraph_api/cli/config_editor/validation.py +++ b/tenxgraph_api/cli/config_editor/validation.py @@ -391,6 +391,11 @@ def _check_wiring(config: dict, out: _Collector, base_dir: Path | None) -> None: if value is None or value == "": continue _check_import_path(key, value, out, base_dir) + mismatch = config.get("store_allow_tenant_mismatch") + if mismatch is not None and not isinstance(mismatch, bool): + out.error( + "store_allow_tenant_mismatch", "store_allow_tenant_mismatch must be true or false." + ) if config.get("checkpointer"): out.warning( "checkpointer", diff --git a/tenxgraph_api/cli/templates/skills/10xgraph/SKILL.md b/tenxgraph_api/cli/templates/skills/10xgraph/SKILL.md index 88f01af6..0fec4e8b 100644 --- a/tenxgraph_api/cli/templates/skills/10xgraph/SKILL.md +++ b/tenxgraph_api/cli/templates/skills/10xgraph/SKILL.md @@ -37,7 +37,7 @@ Treat https://10xgraph.com/ as the first source of truth for public package name - Threads and checkpointing: `references/checkpointing-and-threads.md` - Dependency injection (InjectQ): `references/dependency-injection.md` - Multimodal files and media stores: `references/media-and-files.md` - - Long-term memory stores (MemoryConfig, QdrantStore, Mem0Store): `references/memory-and-store.md` + - Long-term memory stores (MemoryConfig, PgVectorStore, QdrantStore) and multi-tenant tenant columns: `references/memory-and-store.md` - Streaming, StreamChunk, SSE, ResponseGranularity: `references/streaming.md` - Stream emitter for tool progress updates: `references/stream-emitter.md` - Observability hooks, validators, and runtime jumps: `references/callbacks-and-command.md` diff --git a/tenxgraph_api/cli/templates/skills/10xgraph/references/architecture.md b/tenxgraph_api/cli/templates/skills/10xgraph/references/architecture.md index 5abbf8c9..104c66e2 100644 --- a/tenxgraph_api/cli/templates/skills/10xgraph/references/architecture.md +++ b/tenxgraph_api/cli/templates/skills/10xgraph/references/architecture.md @@ -25,7 +25,7 @@ Use this when deciding where a change belongs or explaining how 10xGraph package | `tenxgraph.core.realtime` | `LiveInputQueue`, `RealtimeConfig`, `RealtimeEvent` (Gemini Live audio-to-audio) | | `tenxgraph.prebuilt.tools` | `fetch_url`, `safe_calculator`, `file_read`, `file_write`, `file_search`, `google_web_search`, `vertex_ai_search`, `make_user_memory_tool`, `make_agent_memory_tool`, `create_handoff_tool` | | `tenxgraph.storage.checkpointer` | `InMemoryCheckpointer`, `PgCheckpointer` | -| `tenxgraph.storage.store` | `QdrantStore`, `Mem0Store` | +| `tenxgraph.storage.store` | `PgVectorStore`, `QdrantStore`, `MemoryConfig` | | `tenxgraph.storage.media` | `InMemoryMediaStore`, `LocalFileMediaStore`, `CloudMediaStore` | | `tenxgraph.runtime` | Publisher adapters (SSE, A2A, Kafka, RabbitMQ, Redis Pub/Sub) | | `tenxgraph.utils` | `ResponseGranularity`, `CallbackManager`, `tool` decorator | diff --git a/tenxgraph_api/cli/templates/skills/10xgraph/references/auth-and-authorization.md b/tenxgraph_api/cli/templates/skills/10xgraph/references/auth-and-authorization.md index dc6047e3..bf01ef1e 100644 --- a/tenxgraph_api/cli/templates/skills/10xgraph/references/auth-and-authorization.md +++ b/tenxgraph_api/cli/templates/skills/10xgraph/references/auth-and-authorization.md @@ -84,7 +84,10 @@ class MyAuthz(AuthorizationBackend): resource_id: str | None = None, **context, ) -> bool: ... # required; False -> 403 - def isolation_scope(self) -> str: # optional; "owner" | "none" (default "owner") + def isolation_scope(self, user: dict): # optional; per request. Default "owner" + # "owner" | "none" | list of column names, or the tenxgraph.core.authz.Isolation members + if user.get("role") == "org_admin": + return ["org_id"] # every row of the caller's org (needs tenant_columns=["org_id"]) return "owner" def scopes_for(self, user: dict) -> list[str] | None: # optional; None = permissive @@ -92,7 +95,12 @@ class MyAuthz(AuthorizationBackend): ``` Subclass `OwnershipAuthorizationBackend` to inherit cached owner-only checks and override only -what you need. +what you need. Its thread check compares the same scope columns, so an org admin can open every +thread of the org (via `BaseCheckpointer.aget_thread_scope`). A backend written as +`isolation_scope(self)` (no `user`) still works. An unknown scope raises `ValueError` (never a +silent "none"): use `"owner"`, `"none"`, `Isolation.OWNER` / `Isolation.NONE` or a list of +`user_id` and declared tenant column names; a list naming an undeclared column raises too. +A picked tenant column the user lacks raises, it is never dropped from the filter. ### Scopes @@ -121,15 +129,17 @@ space-separated string) and matches exact names; a claim of the wrong type is a ``` Maps `user["roles"]` to scopes on top of owner isolation. `"*"` -> all scopes. `default_scopes` -granted to everyone. `isolation`: `"owner"` | `"none"`. +granted to everyone. `isolation`: `"owner"` | `"none"` (any other value stops the server at boot). ### Data isolation (config["authz"]) API checks decide access; the storage layer (checkpointer, store) enforces isolation from a trusted policy. After a successful check, `RequirePermission` stamps -`user["authz"] = {user_id, scope, scopes}` where `scope` = backend `isolation_scope()`. Services +`user["authz"] = {user_id, scope, scopes}` where `scope` = backend `isolation_scope(user)`. Services copy the trusted `user` into `config["user"]`, so the core library reads `config["user"]["authz"]` -and scopes rows to the caller (`owner`) or not (`none`). Set server-side only; not client-forgeable. +and scopes rows to the caller (`owner`), not at all (`none`), or by the listed columns. Set server-side only; not client-forgeable. + +Tenant and extra column values (`org_id`, `project_id`, `public_id`) come only from `config["user"]`: return them from `BaseAuth.authenticate()` (or put them in the JWT claims; the decoded token is the user). Never read them from request config, metadata, state or model output. Server-owned config keys (`RESERVED_CONFIG_KEYS` in `core/auth/request_config.py`: `authz`, `user`, `user_id`, `remote_tools`, `file_tool_root`, `workspace_root`, `tool_timeout`, `node_timeout`, `max_parallel_tool_calls`, `collection`, plus any `_`-prefixed key) are dropped from client config by `client_config` on every route. They are ignored, not refused (debug log). A client never picks a Qdrant `collection`; see memory-and-store.md. diff --git a/tenxgraph_api/cli/templates/skills/10xgraph/references/checkpointing-and-threads.md b/tenxgraph_api/cli/templates/skills/10xgraph/references/checkpointing-and-threads.md index c85c7b1e..70aba6df 100644 --- a/tenxgraph_api/cli/templates/skills/10xgraph/references/checkpointing-and-threads.md +++ b/tenxgraph_api/cli/templates/skills/10xgraph/references/checkpointing-and-threads.md @@ -14,12 +14,12 @@ At minimum, checkpointed calls need: config = {"thread_id": "conv-1"} ``` -Use `user_id` for multi-tenant scoping where the backend supports it. +Use `user_id` for multi-tenant scoping where the backend supports it. For organizations and projects, `PgCheckpointer(tenant_columns=..., extra_columns=...)` adds columns to `threads`; see the multi-tenant section of `memory-and-store.md`. ## Backends - `InMemoryCheckpointer`: development/tests/single-process only. State is lost on restart and not shared across workers. -- `PgCheckpointer`: production backend using PostgreSQL for durable data and Redis for fast cache. Requires setup before use. +- `PgCheckpointer`: production backend using PostgreSQL for durable data and Redis for fast cache. Requires setup before use. Optional `tenant_columns`, `extra_columns` and `row_level_security` (the in-memory and SQLite checkpointers raise if given them). Use production storage for load-balanced API deployments. diff --git a/tenxgraph_api/cli/templates/skills/10xgraph/references/memory-and-store.md b/tenxgraph_api/cli/templates/skills/10xgraph/references/memory-and-store.md index 02287425..ac22e4f3 100644 --- a/tenxgraph_api/cli/templates/skills/10xgraph/references/memory-and-store.md +++ b/tenxgraph_api/cli/templates/skills/10xgraph/references/memory-and-store.md @@ -28,23 +28,55 @@ Writes run in the background. A near-identical memory (similarity score 0.95 or ## Backends +- `PgVectorStore` (extra `pgvector`; Postgres 14+, pgvector extension 0.7.0+): memories in one Postgres table, next to the `PgCheckpointer` tables. `from tenxgraph.storage.store import PgVectorStore, OpenAIEmbedding`. - `QdrantStore`: vector database backend, local or cloud. -- `Mem0Store`: managed memory service backend. Embedding helpers such as `OpenAIEmbedding` and factories like `create_local_qdrant_store` exist in the store package where available. +### PgVectorStore + +```python +store = PgVectorStore(embedding=OpenAIEmbedding(), dsn="postgresql://user:pass@host:5432/app") +await store.asetup() # extension, table, indexes; safe to repeat +``` + +- `embedding=None` gives a text-only store (no `embedding` column, no extension needed, default strategy `TEXT`). +- Strategies: `SIMILARITY`, `TEXT` (`ts_rank`), `HYBRID` (Reciprocal Rank Fusion, `hybrid_weights`, `rrf_k`). `RetrievalStrategy` lives in `tenxgraph.storage.store.store_schema`. +- Settings in three levels, most specific wins: store constructor, then `MemoryConfig` / `UserMemoryConfig` / `AgentMemoryConfig` `retrieval_strategy` (scope beats `MemoryConfig`), then the `asearch(...)` call. `QdrantStore` raises for `TEXT` / `HYBRID`. +- `on_embedding_error="fallback"` (default): a write saves text without an embedding and a search runs as `TEXT`, with a warning; `await store.abackfill_embeddings()` fills the gaps. `"raise"` propagates. +- Other options: `pool` (never closed by the store), `schema`, `table`, `collection`, `distance_metric`, `text_search_config`, `tenant_columns`, `extra_columns`, `row_level_security`. + +### Multi-tenant columns (PgCheckpointer and PgVectorStore) + +Declare the same `tenant_columns` (isolation: `text`, `bigint`, `integer`, `uuid`; required by default) and optional `extra_columns` (data: any Postgres type) on both: + +```python +TENANT = {"org_id": "uuid", "project_id": "text"} +checkpointer = PgCheckpointer(postgres_dsn=..., redis_url=..., tenant_columns=TENANT, extra_columns={"plan": "text"}) +store = PgVectorStore(embedding=..., dsn=..., tenant_columns=TENANT, extra_columns={"plan": "text"}) +``` + +Trusted-source rule: tenant and extra column values come only from `config["user"]`. `user` is a reserved config key: clients cannot set it. Your auth class fills it (`BaseAuth.authenticate()` result or JWT claims). Return `org_id`, `project_id`, `public_id` there. Never read them from request config, metadata, state or model output. + +- Isolation scope = which columns a row must match: `"owner"` (`user_id` + every tenant column), `"none"`, or a list such as `["org_id"]`, chosen per user by `isolation_scope(user)` on the authorization backend (see auth-and-authorization.md). Unknown scopes raise. +- `compile()` raises `GraphError` `GRAPH_TENANT_001` under `MODE=production` (warns otherwise) when the checkpointer and store declare different tenant columns; `compile(allow_tenant_mismatch=True)` opts out (for example a shared knowledge-base store). +- Filters: `{"col": value}` or `{"col": {"op": value}}` on extra columns and built-ins (`created_at`, `thread_name`, `memory_type`, `category`, ...). Operators: text `eq ne in nin like ilike is_null`; integer/bigint/timestamptz `eq ne gt gte lt lte in nin is_null`; boolean/uuid `eq ne in nin is_null`; other extra types `is_null` only. `ne`/`nin` match NULL rows. Limits: 10 conditions, 100 items per `in`, 256 characters per string. Tenant columns and `user_id` are rejected. `GET /v1/threads?filters=` and the store routes return 422 for a bad filter. +- Old rows have NULL tenant values and are invisible to scopes that pick those columns until backfilled. +- `row_level_security=True` adds native Postgres RLS on top (one transaction per call; superusers and `BYPASSRLS` roles skip it). +- `like '%x%'` scans the caller's rows; add a `pg_trgm` index for contains-search. + ## Collections -Keep kinds of data apart (user, shared agent/system, company data) with Qdrant collections. +Keep kinds of data apart (user, shared agent/system, company data) with collections. - A client can never choose a collection. The API drops `collection` from client config on every route (invoke, stream, `/v1/graph/ws`, AG-UI, store routes), ignored and logged at debug, like `user_id`, `user`, `authz`, `file_tool_root`. -- Precedence: 1) `UserMemoryConfig(collection=...)` / `AgentMemoryConfig(collection=...)` (a name or a function of the run config; always wins), 2) `collection` in a run config built by developer code, 3) `QdrantStore(collection=...)` default (a name or a function of the config; default `"agentflow_memories"`). With a function default, `asetup()` creates nothing; collections are created on first write or search. `RAGAgent`: `store_config={"collection": ...}` or `store_config_builder` returning `collection`. +- Precedence: 1) `UserMemoryConfig(collection=...)` / `AgentMemoryConfig(collection=...)` (a name or a function of the run config; always wins), 2) `collection` in a run config built by developer code, 3) the store's `collection=` default (`QdrantStore` or `PgVectorStore`; a name or a function of the config; `QdrantStore` default `"agentflow_memories"`). With a function default, `asetup()` creates nothing; collections are created on first write or search. `RAGAgent`: `store_config={"collection": ...}` or `store_config_builder` returning `collection`. - `/v1/store/...` routes always use the store default (and call it if it is a function, with the authenticated `user` in the config). Keep user memory in the default collection; put agent memory and RAG in named collections. - Company data: either one shared collection with owner `system_user_id(f"company:{tenant_id}")`, or one collection per company: `UserMemoryConfig(collection=company_collection)` where the function reads `(cfg.get("user") or {}).get("tenant_id")` and raises if it is missing (with auth off there is no `user`). For RAG: `store_config_builder=lambda cfg: {"user_id": system_user_id(f"company:{t}"), "collection": f"company_{t}"}` with `t` read the same way. Read identity from `cfg["user"]` or `cfg["user_id"]`, never from state. ## Wiring -- Pass `MemoryConfig(store=..., retrieval_mode=...)` to `Agent`. Optional: `limit`, `score_threshold`, `max_tokens`, `inject_system_prompt`, `user_memory=UserMemoryConfig(...)`, `agent_memory=AgentMemoryConfig(...)`. +- Pass `MemoryConfig(store=..., retrieval_mode=...)` to `Agent`. Optional: `limit`, `score_threshold`, `retrieval_strategy`, `max_tokens`, `inject_system_prompt`, `user_memory=UserMemoryConfig(...)`, `agent_memory=AgentMemoryConfig(...)`. - In `postload` mode the Agent needs a `ToolNode` (or the name of a `ToolNode` node); memory setup registers the tools there. `preload` needs no ToolNode for memory. - Pass the store to `compile(store=...)` too. - The graph-level API (`MemoryIntegration`, `create_memory_preload_node`, `get_memory_system_prompt`) and the `memory_tool` function were removed. Do not use them. @@ -61,7 +93,7 @@ Store API routes include: - `DELETE /v1/store/memories/{memory_id}` - `POST /v1/store/memories/forget` -Forget deletes only what matches `memory_type`, `category` and `filters` (all combined). `Mem0Store` cannot narrow and rejects those fields. `query` is capped at 8,000 characters and `content` at 100,000 (`422` above). Client `options` accept only `infer` and `replace_metadata`. +Forget deletes only what matches `memory_type`, `category` and `filters` (all combined). `query` is capped at 8,000 characters and `content` at 100,000 (`422` above). Client `options` accept only `infer` and `replace_metadata`. TypeScript client methods include `storeMemory`, `searchMemory`, `getMemory`, `updateMemory`, `deleteMemory`, `listMemories`, and `forgetMemories`. diff --git a/tenxgraph_api/cli/templates/skills/10xgraph/references/prebuilt-agents-and-tools.md b/tenxgraph_api/cli/templates/skills/10xgraph/references/prebuilt-agents-and-tools.md index 842ccc48..32ef2059 100644 --- a/tenxgraph_api/cli/templates/skills/10xgraph/references/prebuilt-agents-and-tools.md +++ b/tenxgraph_api/cli/templates/skills/10xgraph/references/prebuilt-agents-and-tools.md @@ -252,7 +252,7 @@ agent = RAGAgent(store=store, agent=Agent(model="gpt-4o"), reranker=CohereRerank Constructor options: `store`, `agent`, `reranker`, `top_k`, `top_n`, `retrieval_strategy`, `score_threshold`, `store_config`, `store_config_builder`, plus `state`, `context_manager`, `publisher`, `id_generator`, `container`. - `store_config` is a fixed dict passed to every `store.asearch()` call (shared knowledge base). -- `store_config_builder(run_config) -> dict` builds the store config per run instead, for example `store_config_builder=lambda cfg: {"user_id": cfg["user_id"]}`. `QdrantStore` reads `user_id`, `thread_id` and `collection`. Identity must come from the authenticated run config, never from state. +- `store_config_builder(run_config) -> dict` builds the store config per run instead, for example `store_config_builder=lambda cfg: {"user_id": cfg["user_id"]}`. `QdrantStore` reads `user_id`, `thread_id` and `collection`. Identity must come from the authenticated run config, never from state. With a tenant-column `PgVectorStore`, `RAGAgent` forwards only `config["user"]` (never `config["authz"]`) to the store; for a knowledge base shared across tenants use a store without tenant columns and `compile(allow_tenant_mismatch=True)`. The retrieved documents are numbered and fenced in a `` block that is added to the latest user message of the model request only, before the user's text. The saved user message is not changed and the documents are not saved in state. The wrapped `agent` must accept `execute(state, config, *, request_blocks=None)` (the built-in `Agent` does), otherwise `RAGAgent` raises `TypeError` when there are documents. diff --git a/tenxgraph_api/src/app/core/auth/authorization.py b/tenxgraph_api/src/app/core/auth/authorization.py index 4c20c289..ad74bdde 100644 --- a/tenxgraph_api/src/app/core/auth/authorization.py +++ b/tenxgraph_api/src/app/core/auth/authorization.py @@ -5,10 +5,14 @@ to add resource-level access control to their 10xGraph applications. """ +import inspect import logging from abc import ABC, abstractmethod +from collections.abc import Sequence from typing import TYPE_CHECKING, Any +from tenxgraph.core.authz import Isolation + if TYPE_CHECKING: from tenxgraph_api.src.app.core.cache_store import BaseCacheStore @@ -78,6 +82,38 @@ def scopes_from_claims(user: Any) -> list[str] | None: return sorted(granted) +def _accepts_user(fn: Any) -> bool: + """Whether ``isolation_scope`` takes the user (the 1.0 form) or nothing (older backends).""" + try: + params = inspect.signature(fn).parameters.values() + except (TypeError, ValueError): + return False + return any( + p.kind + in ( + inspect.Parameter.POSITIONAL_ONLY, + inspect.Parameter.POSITIONAL_OR_KEYWORD, + inspect.Parameter.VAR_POSITIONAL, + ) + for p in params + ) + + +def resolve_isolation_scope(backend: Any, user: dict[str, Any] | None) -> str | list[str]: + """Call ``backend.isolation_scope`` for this caller. + + ``isolation_scope(user)`` is the current form; backends written before it take no + argument, so the signature decides how it is called (never a retry on ``TypeError``, + which could hide a bug inside the method). A backend without the method gets + ``"owner"``, the :class:`AuthorizationBackend` default (fail closed): sharing data takes + an explicit ``"none"``. + """ + fn = getattr(backend, "isolation_scope", None) + if not callable(fn): + return "owner" + return fn(user) if _accepts_user(fn) else fn() + + class AuthorizationBackend(ABC): """ Abstract base class for authorization backends. @@ -86,11 +122,11 @@ class AuthorizationBackend(ABC): for their 10xGraph applications. The authorize method is called before any resource operation to check if the user has permission. - Storage isolation is separate from ``authorize``: ``isolation_scope()`` decides whether - threads and memories are scoped to the calling user. It defaults to ``"owner"`` (each user - sees only their own data), so a backend that implements only ``authorize`` is isolated. - Return ``"none"`` from ``isolation_scope`` on purpose to let every authenticated user see - the same data. + Storage isolation is separate from ``authorize``: ``isolation_scope(user)`` decides which + columns threads and memories are scoped by for this caller. It defaults to ``"owner"`` + (``user_id`` plus every declared tenant column), so a backend that implements only + ``authorize`` is isolated. Return ``"none"`` on purpose to let every authenticated user + see the same data, or a column list such as ``["org_id"]`` for an org admin. Example: class MyAuthorizationBackend(AuthorizationBackend): @@ -101,9 +137,11 @@ async def authorize(self, user, resource, action, resource_id=None, **context): # Add custom logic here return False - def isolation_scope(self) -> str: + def isolation_scope(self, user): # "owner" (the default): each user sees only their own threads and memories. - # Return "none" only when users are meant to share data. + # An org admin sees every thread in their org. + if user.get("role") == "org_admin": + return ["org_id"] return "owner" """ @@ -133,15 +171,26 @@ async def authorize( Exception: Can raise exceptions for auth failures or errors """ - def isolation_scope(self) -> str: - """The data-isolation policy this backend implies, sent to storage per request. + def isolation_scope(self, user: dict[str, Any] | None = None) -> str | list[str]: + """The data-isolation policy for this caller, sent to storage per request. + + Called once per request with the authenticated ``user`` (the dict your auth class + returned). The API stamps the result (server-side, non-hijackable) into the trusted + ``authz`` block (``tenxgraph.core.authz.build_authz``) so the checkpointer and store + scope storage the *same* way this backend decides access, and the built-in ownership + backends compare the same columns when a request names a thread. - The API stamps this (server-side, non-hijackable) into ``config["policy"]`` so the - checkpointer scopes storage the *same* way this backend decides access -- no - contradiction between "allow_all" authz and a data layer that still filters. + - ``"owner"`` -> ``["user_id", *tenant_columns]``: only the caller's own rows. + - ``"none"`` -> ``[]``: storage is not scoped (any authenticated user sees everything). + - a list of ``"user_id"`` and declared tenant column names (``tenant_columns=`` on + ``PgCheckpointer`` / ``PgVectorStore``): rows matching the caller's values on those + columns, e.g. ``["org_id"]`` for an org admin. Naming a column the storage backend + does not declare fails the request. - - ``"owner"`` -> storage is scoped to the caller's ``user_id`` (owner-only). - - ``"none"`` -> storage is not scoped (any authenticated user sees everything). + The caller's values come only from ``user`` (``config["user"]``), never from request + config. A picked tenant column the user lacks fails the request rather than being + dropped. Backends written before 1.0 define ``isolation_scope(self)`` with no + argument; they keep working. Defaults to ``"owner"`` (fail closed): a custom backend that implements only ``authorize`` still isolates users. Override and return ``"none"`` to share data on @@ -202,18 +251,26 @@ async def authorize( # Only check if user is authenticated (has user_id) return bool(user.get("user_id")) - def isolation_scope(self) -> str: + def isolation_scope(self, user: dict[str, Any] | None = None) -> str | list[str]: # allow_all shares data on purpose: authenticated == authorized, no per-user scoping. return "none" class OwnershipAuthorizationBackend(AuthorizationBackend): - """Object-level authorization: a user may only touch threads they own. + """Object-level authorization: a user may only touch threads in their isolation scope. This is the built-in "secure" backend. It enforces the rule most multi-tenant apps want without the developer writing any code: *a thread can be read, mutated, stopped or fixed only by the user who owns it.* Ownership is resolved from the - configured checkpointer, whose thread registry records the owning ``user_id``. + configured checkpointer, whose thread registry records the owning ``user_id`` and, with + ``PgCheckpointer(tenant_columns=...)``, the thread's tenant values. + + "Owns" follows :meth:`isolation_scope`: a thread is accessible when its values equal the + caller's on every column the caller's scope picks (``"owner"`` = ``user_id`` and every + tenant column; a list such as ``["org_id"]`` lets an org admin reach every thread of the + org; ``"none"`` picks no column, so any existing thread). The caller's values come from + the authenticated user only. A picked column the thread or the caller has no value for + denies. Decision table (for the thread-scoped resources ``graph`` and ``checkpointer``): @@ -223,29 +280,30 @@ class OwnershipAuthorizationBackend(AuthorizationBackend): - The thread has no owner yet -> allow only the actions that create it (``graph:invoke``, ``graph:stream``, ``checkpointer:write``); the caller becomes its owner. With ``server_thread_ids`` (``runs.thread_ids == "server"``) even those are denied: new - threads must be started without an id, so the server picks it. The two routes whose - clients make their own ids (AG-UI and ``/v1/graph/live``) pass ``client_thread_ids=True`` - in the authorize context, which lifts that one refusal for them. Every other + threads must be started without an id, so the server picks it. Every route, AG-UI + and ``/v1/graph/live`` included, follows this rule. Every other action (read, delete, stop, fix) is denied: there is nothing of the caller's to act on, and an ownerless thread may still hold data written before owners were recorded. - - The thread exists and the caller owns it -> allow. - - The thread exists and is owned by someone else -> **deny, for every action** - including ``invoke`` and ``stream``. This is the whole point: you cannot read, - continue, stop, fix, or run another user's thread. + - The thread exists and matches the caller on every scope column -> allow. + - The thread exists and does not match -> **deny, for every action** including + ``invoke`` and ``stream``. This is the whole point: you cannot read, continue, stop, + fix, or run another user's (or another tenant's) thread. Resources other than ``graph``/``checkpointer`` (store, files) are allowed through: they enforce their own per-user scoping (the store keys on ``user_id``; media checks file ownership in ``MediaService``). Ownership is resolved through a :class:`ThreadOwnershipResolver` in front of - :meth:`BaseCheckpointer.aget_thread_owner`. Ownership is immutable, so the resolver - caches it (in-process, plus the shared cache store when one is configured) -- after the - first lookup a check is an in-memory hit, not a database round-trip per request. Pass the - API's ``BaseCacheStore`` as ``cache``; the backend uses it but never closes it. + :meth:`BaseCheckpointer.aget_thread_scope` (``aget_thread_owner`` for a checkpointer + without it). A thread's values are immutable, so the resolver caches them (in-process, + plus the shared cache store when one is configured) -- after the first lookup a check is + an in-memory hit, not a database round-trip per request. Pass the API's + ``BaseCacheStore`` as ``cache``; the backend uses it but never closes it. This backend fails closed: with no checkpointer, or one that cannot resolve ownership (``aget_thread_owner`` raises ``NotImplementedError``), every thread-scoped request is - denied. The loader refuses to start in that setup (see + denied, and so is a scope naming a tenant column the checkpointer does not declare or + answer. The loader refuses to start without an owner lookup (see :func:`assert_ownership_supported`), so this only guards a misconfiguration that slipped past it. """ @@ -265,7 +323,7 @@ class OwnershipAuthorizationBackend(AuthorizationBackend): } ) - def isolation_scope(self) -> str: + def isolation_scope(self, user: dict[str, Any] | None = None) -> str | list[str]: # Owner-only access -> storage must be scoped to the caller too. return "owner" @@ -313,20 +371,73 @@ def _get_resolver(self) -> Any | None: ThreadOwnershipResolver, ) - self._resolver = ThreadOwnershipResolver(cp.aget_thread_owner, cache=self._cache) + self._resolver = ThreadOwnershipResolver( + scope_lookup(cp), cache=self._cache, tenant_columns=self._tenant_columns() + ) return self._resolver - def evict(self, thread_id: str | int, owner: str | int | None = None) -> Any: + def _tenant_columns(self) -> tuple[str, ...]: + """The checkpointer's declared tenant column names (empty for most checkpointers).""" + return tenant_column_names(self.checkpointer) + + def _scope_columns(self, user: dict[str, Any]) -> tuple[str, ...] | None: + """The columns this caller's isolation scope compares, or None to deny. + + Resolved exactly as storage resolves the stamped block + (``tenxgraph.core.authz.isolation_columns``), against the checkpointer's tenant + columns. A scope storage could not apply (an unknown string, a column the + checkpointer does not declare) denies instead of guessing. + """ + from tenxgraph.core.authz import SCOPE_NONE, SCOPE_OWNER, build_authz, isolation_columns + + scope = resolve_isolation_scope(self, user) + try: + if isinstance(scope, str) and scope not in (SCOPE_OWNER, SCOPE_NONE): + raise ValueError(f"unknown isolation scope {scope!r}") + if not isinstance(scope, str | list | tuple): + raise ValueError(f"isolation scope must be a string or a list, got {scope!r}") + block = build_authz(user.get("user_id"), scope=scope) + return isolation_columns({"authz": block}, self._tenant_columns()) + except ValueError as exc: + logger.error( + "%s.isolation_scope returned a scope the checkpointer cannot apply (%s); " + "denying thread access.", + type(self).__name__, + exc, + ) + return None + + def evict( + self, + thread_id: str | int, + owner: str | int | None = None, + *, + user: dict[str, Any] | None = None, + ) -> Any: """Invalidate a thread's cached ownership (call on thread deletion). - ``owner`` is the deleting user; they may recreate the id straight away. Returns the - resolver's ``evict`` coroutine when a resolver exists, so callers may ``await`` it; - returns None when there is nothing to evict. + ``owner`` is the deleting user's id; ``user`` the deleting authenticated user. With + ``user`` the tombstone records the deleter's ``user_id`` and tenant values, and only a + caller matching all of them may recreate the id straight away. Returns the resolver's + ``evict`` coroutine when a resolver exists, so callers may ``await`` it (it raises + ``OwnershipCacheEvictionError`` when the cache store could not be updated); returns + None when there is nothing to evict. """ resolver = self._get_resolver() if resolver is None: return None - return resolver.evict(thread_id, owner) + who: Any = owner + if user is not None and user.get("user_id") is not None: + who = self._identity_values(user) + return resolver.evict(thread_id, who) + + def _identity_values(self, user: dict[str, Any]) -> dict[str, str | None]: + """The caller's ``user_id`` and tenant column values as canonical text (None if unset). + + Converted with the checkpointer's declared column types, the same way + :meth:`_matches` reads them, so a tombstone compares like an ownership check. + """ + return identity_values(self.checkpointer, user) async def authorize( self, @@ -360,27 +471,34 @@ async def authorize( ) return False + columns = self._scope_columns(user) + if columns is None: + return False + return await self._authorize_ownership( resolver, str(resource_id), - str(user_id), + user, + columns, resource, action, client_thread_ids=context.get("client_thread_ids") is True, ) - async def _authorize_ownership( + async def _authorize_ownership( # noqa: PLR0911 self, resolver: Any, resource_id: str, - user_id: str, + user: dict[str, Any], + columns: tuple[str, ...], resource: str, action: str, *, client_thread_ids: bool = False, ) -> bool: + user_id = str(user.get("user_id")) try: - owner = await resolver.owner_of(resource_id) + thread_scope = await _resolve_scope(resolver, resource_id, columns) except NotImplementedError: logger.error( "Checkpointer %s cannot resolve thread ownership; denying %s:%s. Use a " @@ -402,9 +520,9 @@ async def _authorize_ownership( return False # No owner yet: only an action that creates the thread may proceed, and it makes the - # caller the owner. Otherwise the caller must be the owner, for EVERY action. - if owner is None: - if await self._recently_deleted(resolver, resource_id, user_id): + # caller the owner. Otherwise the caller must match the thread, for EVERY action. + if thread_scope is None: + if await self._recently_deleted(resolver, resource_id, user_id, user): # Another worker may still cache the deleted thread's owner; handing the id to # a new owner now would let that stale entry reach into the new thread. logger.info( @@ -416,8 +534,8 @@ async def _authorize_ownership( return False if self.server_thread_ids and not client_thread_ids: # New threads get server-picked ids; a client may not name one into being, - # so nobody can claim an id ahead of the user it was meant for. AG-UI and - # realtime clients make their own ids and are exempt (``client_thread_ids``). + # so nobody can claim an id ahead of the user it was meant for. No built-in + # route passes ``client_thread_ids``; it stays for custom callers. logger.info( "Refusing %s:%s on unknown thread %s: runs.thread_ids is 'server'", resource, @@ -426,34 +544,162 @@ async def _authorize_ownership( ) return False return (resource, action) in self.CREATE_ACTIONS - return str(owner) == user_id + if self._matches(thread_scope, user, columns): + return True + missing = [column for column in columns if column not in thread_scope] + if missing: + logger.error( + "Checkpointer %s did not return %s for thread %s, which the isolation scope " + "compares; denying %s:%s. Declare the column with tenant_columns= on " + "PgCheckpointer or return it from aget_thread_scope.", + type(self.checkpointer).__name__, + ", ".join(missing), + resource_id, + resource, + action, + ) + return False - @staticmethod - async def _recently_deleted(resolver: Any, resource_id: str, user_id: str) -> bool: + def _matches( + self, thread_scope: dict[str, Any], user: dict[str, Any], columns: tuple[str, ...] + ) -> bool: + """Whether the thread's values equal the caller's on every scope column. + + The caller's values come from the authenticated user only (``config["user"]`` once a + service copies it), converted with the column's declared type when the checkpointer + declares it. A column with no value on either side never matches. + """ + from tenxgraph.storage.columns import ColumnSet, user_value + + from tenxgraph_api.src.app.core.auth.ownership_resolver import canonical_value + + specs = getattr(self.checkpointer, "columns", None) + if not isinstance(specs, ColumnSet): + specs = None # a custom checkpointer: compare the values as text + for column in columns: + expected = thread_scope.get(column) + if expected is None: + return False + value = user_value(user, column) + if value is None: + return False + if column != "user_id": + spec = specs.get(column) if specs is not None else None + if spec is not None: + try: + value = spec.convert(value) + except ValueError: + return False + if value is None: + return False + if canonical_value(value) != expected: + return False + return True + + async def _recently_deleted( + self, resolver: Any, resource_id: str, user_id: str, user: dict[str, Any] | None = None + ) -> bool: check = getattr(resolver, "recently_deleted", None) if not callable(check): return False + from tenxgraph_api.src.app.core.auth.ownership_resolver import ThreadOwnershipResolver + + # The built-in resolver compares user_id and tenant values; a custom one gets the id. + caller: Any = user_id + if user is not None and isinstance(resolver, ThreadOwnershipResolver): + caller = self._identity_values(user) try: - return bool(await check(resource_id, user_id)) + return bool(await check(resource_id, caller)) except Exception as exc: # pragma: no cover - the resolver already swallows errors logger.debug("Tombstone check failed for thread %s: %s", resource_id, exc) return False +def tenant_column_names(checkpointer: Any) -> tuple[str, ...]: + """``checkpointer.tenant_column_names`` as a tuple (empty when it declares none).""" + names = getattr(checkpointer, "tenant_column_names", None) + if not isinstance(names, Sequence) or isinstance(names, str): + return () + return tuple(str(name) for name in names) + + +def identity_values(checkpointer: Any, user: Any) -> dict[str, str | None]: + """``user``'s ``user_id`` and tenant column values as canonical text (None if unset). + + The tenant columns are the ones ``checkpointer`` declares. Each value is read from the + authenticated user only (``config["user"]``) and converted with the column's declared + type, as an ownership check compares it; a value the type rejects becomes None. Used by + ownership tombstones and by uploaded-file tenant checks (``MediaService``). + """ + from tenxgraph.storage.columns import ColumnSet, user_value + + from tenxgraph_api.src.app.core.auth.ownership_resolver import canonical_value + + specs = getattr(checkpointer, "columns", None) + if not isinstance(specs, ColumnSet): + specs = None + values: dict[str, str | None] = {"user_id": canonical_value(user_value(user, "user_id"))} + for column in tenant_column_names(checkpointer): + value = user_value(user, column) + spec = specs.get(column) if specs is not None else None + if value is not None and spec is not None: + try: + value = spec.convert(value) + except ValueError: + value = None + values[column] = canonical_value(value) + return values + + +def _has_scope_lookup(checkpointer: Any) -> bool: + return inspect.iscoroutinefunction(getattr(checkpointer, "aget_thread_scope", None)) + + +def scope_lookup(checkpointer: Any) -> Any: + """The checkpointer's thread scope lookup: ``aget_thread_scope``, else ``aget_thread_owner``. + + ``BaseCheckpointer.aget_thread_scope`` answers ``{"user_id": owner}`` through + ``aget_thread_owner`` by default and ``PgCheckpointer`` adds its tenant values. A + checkpointer (or an older core) without it falls back to ``aget_thread_owner``, which the + resolver reads as ``{"user_id": owner}``; scopes that pick tenant columns then deny. + """ + if _has_scope_lookup(checkpointer): + return checkpointer.aget_thread_scope + return checkpointer.aget_thread_owner + + +async def _resolve_scope( + resolver: Any, thread_id: str, columns: tuple[str, ...] +) -> dict[str, Any] | None: + """The thread's scope through ``resolver``; a custom resolver with only ``owner_of`` works.""" + scope_of = getattr(resolver, "scope_of", None) + if callable(scope_of): + return await scope_of(thread_id, columns) + from tenxgraph_api.src.app.core.auth.ownership_resolver import to_scope + + return to_scope(await resolver.owner_of(thread_id)) + + def supports_owner_lookup(checkpointer: Any) -> bool: - """Whether ``checkpointer`` can answer ``aget_thread_owner``. + """Whether ``checkpointer`` can answer ``aget_thread_owner`` (or ``aget_thread_scope``). - A :class:`BaseCheckpointer` subclass that does not override it inherits the base method, - which always raises ``NotImplementedError``. Other objects are taken at their word if they - have the method. + A :class:`BaseCheckpointer` subclass that overrides neither inherits base methods that + always raise ``NotImplementedError``. Other objects are taken at their word if they have + one of the methods. """ if checkpointer is None: return False from tenxgraph.storage.checkpointer import BaseCheckpointer if isinstance(checkpointer, BaseCheckpointer): - return type(checkpointer).aget_thread_owner is not BaseCheckpointer.aget_thread_owner - return callable(getattr(checkpointer, "aget_thread_owner", None)) + cls = type(checkpointer) + if cls.aget_thread_owner is not BaseCheckpointer.aget_thread_owner: + return True + base_scope = getattr(BaseCheckpointer, "aget_thread_scope", None) + return base_scope is not None and getattr(cls, "aget_thread_scope", None) is not base_scope + return callable(getattr(checkpointer, "aget_thread_owner", None)) or _has_scope_lookup( + checkpointer + ) def assert_ownership_supported(backend: AuthorizationBackend, checkpointer: Any) -> None: @@ -477,6 +723,25 @@ def assert_ownership_supported(backend: AuthorizationBackend, checkpointer: Any) ) +def _isolation_shortcut(isolation: Any) -> str: + """``"owner"`` or ``"none"`` for an exact string or ``Isolation`` member; else raise. + + An unknown value (``"owners"``, ``None``, ``True``) raises ``ValueError`` at construction, + so a server with a typo in ``authorization.isolation`` refuses to boot instead of + quietly picking an isolation level. + """ + if isinstance(isolation, str): + try: + return Isolation(isolation).value + except ValueError: + pass + valid = ", ".join(repr(m.value) for m in Isolation) + raise ValueError( + f"Unknown authorization isolation {isolation!r}: use {valid} " + "(tenxgraph.core.authz.Isolation)" + ) + + class RoleBasedAuthorizationBackend(OwnershipAuthorizationBackend): """RBAC: map a user's roles to scopes, on top of owner-only object isolation. @@ -488,7 +753,9 @@ class RoleBasedAuthorizationBackend(OwnershipAuthorizationBackend): gets every scope in :func:`all_scopes`. - **Isolation** (which rows are visible) -- inherited from :class:`OwnershipAuthorizationBackend` (owner-only threads), configurable via - ``isolation``. + ``isolation``: ``"owner"`` (default) or ``"none"``, or an ``Isolation`` member. Any + other value raises ``ValueError`` when the backend is built, so the server refuses + to boot rather than guess. Example:: @@ -506,7 +773,7 @@ def __init__( role_scopes: dict[str, Any] | None = None, *, default_scopes: Any = (), - isolation: str = "owner", + isolation: str | Isolation = "owner", checkpointer: Any | None = None, resolver: Any | None = None, cache: "BaseCacheStore | None" = None, @@ -520,9 +787,9 @@ def __init__( ) self._role_scopes = {str(r): list(s) for r, s in (role_scopes or {}).items()} self._default_scopes = list(default_scopes) - self._isolation = isolation if isolation in ("owner", "none") else "owner" + self._isolation = _isolation_shortcut(isolation) - def isolation_scope(self) -> str: + def isolation_scope(self, user: dict[str, Any] | None = None) -> str | list[str]: return self._isolation @staticmethod diff --git a/tenxgraph_api/src/app/core/auth/client_filters.py b/tenxgraph_api/src/app/core/auth/client_filters.py new file mode 100644 index 00000000..e8590f53 --- /dev/null +++ b/tenxgraph_api/src/app/core/auth/client_filters.py @@ -0,0 +1,146 @@ +"""Client ``filters`` for thread listing and the store routes, checked before storage runs. + +Filters are core's safe filter language (``tenxgraph.storage.sql_filter``): equality, ranges, +``like`` / ``ilike``, ``in`` and null checks on declared columns only, every value a bind +parameter. Core validates them again when it builds the query; checking here first turns a bad +filter into a 422 before any database work, with core's message, which names columns and +operators but never echoes values or SQL. + +Tenant columns and ``user_id`` are isolation columns: the server sets them from the caller's +identity (``config["user"]``), so a filter naming one is refused (422), never applied. +""" + +from __future__ import annotations + +import json +from collections.abc import Iterator +from contextlib import contextmanager +from typing import Any + +from tenxgraph_api.src.app.core.auth.request_config import ClientInputError, structure_error + + +try: # core >= 1.0.0 (safe filters); older cores have no column filters at all + from tenxgraph.storage.sql_filter import ( + MAX_CONDITIONS, + FilterError, + FilterSchema, + validate_filters, + ) +except ImportError: # pragma: no cover - exercised only against an older core + MAX_CONDITIONS = 10 + FilterError = None # type: ignore[assignment,misc] + FilterSchema = None # type: ignore[assignment,misc] + validate_filters = None # type: ignore[assignment] + + +# Longest ``GET /v1/threads?filters=...`` value accepted. Ten conditions with long values and an +# ``in`` list fit well inside it; anything longer is refused before it is parsed. +MAX_FILTERS_PARAM_LENGTH = 8192 + +_NOT_AN_OBJECT = "filters must be a JSON object mapping column names to conditions" + + +def _reject_constant(_name: str) -> Any: + raise ValueError("non-finite number") + + +def parse_filters_param(raw: str | None) -> dict[str, Any] | None: + """Parse the ``filters`` query parameter (a JSON object) or raise ``ClientInputError``. + + ``None`` or an empty string means no filters. The message never repeats the input. + """ + if raw is None or not raw.strip(): + return None + if len(raw) > MAX_FILTERS_PARAM_LENGTH: + raise ClientInputError(f"filters is longer than {MAX_FILTERS_PARAM_LENGTH} characters") + try: + value = json.loads(raw, parse_constant=_reject_constant) + except (ValueError, RecursionError) as exc: + raise ClientInputError(f"filters is not valid JSON; {_NOT_AN_OBJECT}") from exc + if not isinstance(value, dict): + raise ClientInputError(_NOT_AN_OBJECT) + if (error := structure_error(value, "filters")) is not None: + raise ClientInputError(error) + return value or None + + +def _filter_schema(backend: Any) -> Any: + """``backend.filter_schema`` when it is a core ``FilterSchema``, else None (no column + filters: in-memory and SQLite checkpointers, QdrantStore, custom backends).""" + if FilterSchema is None: + return None + schema = getattr(backend, "filter_schema", None) + return schema if isinstance(schema, FilterSchema) else None + + +def _client_error(exc: Exception) -> ClientInputError: + # FilterError messages are written to be safe for a client (see sql_filter's docstring). + return ClientInputError(str(exc)) + + +def check_thread_filters(checkpointer: Any, filters: dict[str, Any] | None) -> None: + """Validate ``GET /v1/threads`` filters against the checkpointer's filter schema. + + Raises: + ClientInputError: The filters are malformed, name a column that is not filterable + (tenant columns and ``user_id`` included), or the checkpointer has no filterable + columns (the in-memory and SQLite checkpointers). + """ + if not filters: + return + schema = _filter_schema(checkpointer) + if schema is None: + raise ClientInputError( + f"filters are not supported by {type(checkpointer).__name__}; thread filters need " + "a checkpointer with filterable columns (PgCheckpointer)" + ) + try: + validate_filters(filters, schema) + except FilterError as exc: # type: ignore[misc] + raise _client_error(exc) from exc + + +def check_store_filters(store: Any, filters: Any) -> None: + """Validate store ``filters`` against ``store.filter_schema`` when the store has one. + + A store with a filter schema (``PgVectorStore``) takes column conditions on its + filterable columns and plain equality on any other key, which it matches against the + memory's JSON metadata. The column conditions are validated here with core's validator; + the store checks the metadata keys itself (a ``FilterError`` it raises is mapped by + :func:`filter_errors_as_client_errors`). A store without a schema (``QdrantStore``) keeps + its own filter handling unchanged. + + Raises: + ClientInputError: A column condition is not allowed, a tenant column or ``user_id`` + is named, or there are too many conditions. + """ + schema = _filter_schema(store) + if schema is None or not filters: + return + if not isinstance(filters, dict): + raise ClientInputError(_NOT_AN_OBJECT) + if len(filters) > MAX_CONDITIONS: + raise ClientInputError(f"Too many filter conditions: at most {MAX_CONDITIONS}") + columns = { + key: value + for key, value in filters.items() + if schema.get(key) is not None or schema.is_isolation(key) + } + try: + validate_filters(columns, schema) + except FilterError as exc: # type: ignore[misc] + raise _client_error(exc) from exc + + +@contextmanager +def filter_errors_as_client_errors() -> Iterator[None]: + """Turn a ``FilterError`` raised by storage into a ``ClientInputError`` (422, kept in + production). Any other error passes through unchanged.""" + if FilterError is None: + yield + return + try: + yield + except FilterError as exc: + raise _client_error(exc) from exc diff --git a/tenxgraph_api/src/app/core/auth/ownership_resolver.py b/tenxgraph_api/src/app/core/auth/ownership_resolver.py index 0e6a6821..5d08d8ec 100644 --- a/tenxgraph_api/src/app/core/auth/ownership_resolver.py +++ b/tenxgraph_api/src/app/core/auth/ownership_resolver.py @@ -5,6 +5,13 @@ after the first lookup an authorization check costs nothing (an in-process hit) instead of a database round-trip per request. +What is cached is the thread's **scope**: ``{"user_id": ..., : ...}`` from +``BaseCheckpointer.aget_thread_scope`` (``PgCheckpointer(tenant_columns=...)`` adds the tenant +values; every other checkpointer answers ``{"user_id": owner}``). Values are kept as canonical +strings (:func:`canonical_value`) so they survive the shared cache and compare the same way +whatever type the database returned. The tenant values are set at thread creation and never +change either, so the same caching rules hold. + Cache tiers: - **L1**: a bounded in-process LRU (per worker) with a short TTL. @@ -18,11 +25,14 @@ also leaves a **tombstone** for one L1 TTL (in L1, and in L2 when a shared cache store is configured), and :meth:`ThreadOwnershipResolver.recently_deleted` lets the authorization backend refuse to create a thread under that id until every worker's stale entry has expired. -Without a shared cache store the tombstone only exists on the worker that handled the delete; -run multi-worker deployments with the shared L2 tier. The L2 TTL covers an eviction that -failed to reach the cache store. - -Only *positive* results (a known owner) are cached. A ``None`` result (the thread does not +The deleter is exempt only when the caller matches the deleter on ``user_id`` and every tenant +column (a stale entry elsewhere then names the same identity). Without a shared cache store the +tombstone only exists on the worker that handled the delete; run multi-worker deployments with +the shared L2 tier. The L2 TTL bounds an eviction that failed to reach the cache store; that +failure is logged as a warning and raised from :meth:`ThreadOwnershipResolver.evict` (as +:class:`OwnershipCacheEvictionError`) so the delete reports it. + +Only *positive* results (a known thread) are cached. A ``None`` result (the thread does not exist yet) is **never** cached: a nonexistent thread can become owned by the very next request, and a cached ``None`` would let a later caller be treated as the creator of a thread someone else just made (a time-of-check/time-of-use hole). @@ -31,10 +41,13 @@ from __future__ import annotations import asyncio +import json import logging import time from collections import OrderedDict -from collections.abc import Awaitable, Callable +from collections.abc import Awaitable, Callable, Iterable, Mapping +from datetime import UTC, datetime +from typing import Any from tenxgraph_api.src.app.core.cache_store import BaseCacheStore from tenxgraph_api.src.app.core.cache_store.loader import is_in_memory @@ -43,29 +56,86 @@ logger = logging.getLogger("tenxgraph_api.authorization") -# Sentinel signalling the backing checkpointer cannot resolve ownership at all -# (``aget_thread_owner`` raised ``NotImplementedError``). Propagated so the caller can -# degrade gracefully instead of treating every thread as unowned. -OwnerLookup = Callable[[str], Awaitable["str | int | None"]] +# Backing lookup: ``(thread_id) -> owner | scope dict | None``. A scope dict is what +# ``aget_thread_scope`` returns; a bare owner is what ``aget_thread_owner`` returns (read as +# ``{"user_id": owner}``). It may raise ``NotImplementedError`` when the checkpointer cannot +# resolve ownership at all; that propagates so the caller can fail closed. +OwnerLookup = Callable[[str], Awaitable[Any]] +ThreadScope = dict[str, str | None] DEFAULT_L1_TTL_SECONDS = 30.0 DEFAULT_L2_TTL_SECONDS = 3600 +# L2 TTL for scope entries when tenant columns are declared. A thread's scope is immutable, so +# the TTL only matters when a delete's L2 eviction failed (cache store outage): the stale +# scope then outlives the thread for up to this long. Tenant deployments are the ones where a +# stale entry crosses a tenant boundary, so they get a short window. Five minutes keeps the L2 +# useful (a busy thread is looked up in the database at most once per five minutes for the +# whole cluster, one primary-key read) while cutting the exposure from an hour to minutes. +TENANT_L2_TTL_SECONDS = 300 + + +class OwnershipCacheEvictionError(RuntimeError): + """A thread delete could not evict or tombstone the thread in the shared cache store. + + The L1 entry and the local tombstone are already handled. Another worker may still read + the old scope from the cache store until its L2 TTL ends. + """ + + +def canonical_value(value: Any) -> str | None: + """The text a scope value is cached and compared as (``None`` stays ``None``). + + Datetimes are compared in UTC, booleans as ``true``/``false``, everything else (text, + integers, UUIDs) by ``str()``. Both sides of a comparison go through this, after the + caller's value was converted with the column's declared type. + """ + if value is None: + return None + if isinstance(value, bool): + return "true" if value else "false" + if isinstance(value, datetime): + if value.tzinfo is not None: + value = value.astimezone(UTC) + return value.isoformat() + return str(value) + + +def to_scope(result: Any) -> ThreadScope | None: + """Normalise a lookup result into a scope dict of canonical values, or None (no thread). + + A mapping must carry a ``user_id`` (a thread always has an owner); one without it is + treated as "no such thread" rather than guessed at. + """ + if result is None: + return None + if isinstance(result, Mapping): + if result.get("user_id") is None: + return None + return {str(key): canonical_value(value) for key, value in result.items()} + return {"user_id": canonical_value(result)} class ThreadOwnershipResolver: - """Resolve (and cache) the owning ``user_id`` for a ``thread_id``. + """Resolve (and cache) a thread's scope: its owning ``user_id`` and tenant values. Args: - lookup: async ``(thread_id) -> user_id | None`` backing lookup (typically - ``checkpointer.aget_thread_owner``). May raise ``NotImplementedError`` if the - checkpointer cannot resolve ownership; that propagates uncached. - cache: the API's cache store, the shared L2 tier (keys ``owner:id:`` and - ``owner:tomb:``). None or an ``InMemoryCacheStore`` means L1 only. Cache - errors degrade to the DB lookup and never fail a request. The resolver never - closes it. + lookup: async backing lookup, ``(thread_id) -> scope dict | owner | None`` + (typically ``checkpointer.aget_thread_scope``, or ``aget_thread_owner`` for a + checkpointer without it). May raise ``NotImplementedError`` if the checkpointer + cannot resolve ownership; that propagates uncached. + cache: the API's cache store, the shared L2 tier. None or an ``InMemoryCacheStore`` + means L1 only. Cache errors degrade to the DB lookup and never fail a request. + The resolver never closes it. Keys: ``owner:id:`` (the owner as plain + text, the layout older releases wrote and read) when there are no tenant + columns, ``owner:scope:`` (a JSON object) when there are, and + ``owner:tomb:`` for delete markers. max_size: L1 LRU capacity (entries). l1_ttl: seconds an L1 entry is trusted; bounds staleness after a delete elsewhere. - l2_ttl: seconds an L2 entry lives in the cache store. + l2_ttl: seconds an L2 entry lives in the cache store. Default: ``DEFAULT_L2_TTL_SECONDS`` + (1 h), or ``TENANT_L2_TTL_SECONDS`` (5 min) when ``tenant_columns`` is non-empty. + tenant_columns: the checkpointer's declared tenant columns. Non-empty switches the L2 + entries to the JSON ``owner:scope:`` key, so a plain owner entry (from an older + release or from before the columns were declared) is never read as a full scope. """ def __init__( @@ -75,92 +145,187 @@ def __init__( cache: BaseCacheStore | None = None, max_size: int = 10_000, l1_ttl: float = DEFAULT_L1_TTL_SECONDS, - l2_ttl: int = DEFAULT_L2_TTL_SECONDS, + l2_ttl: int | None = None, + tenant_columns: Iterable[str] = (), ) -> None: self._lookup = lookup + self._tenant_columns = tuple(tenant_columns) + if l2_ttl is None: + l2_ttl = TENANT_L2_TTL_SECONDS if self._tenant_columns else DEFAULT_L2_TTL_SECONDS # An in-memory store lives in this process, like L1: caching in it too adds nothing. self._cache = None if is_in_memory(cache) else cache self._max_size = max(1, max_size) self._l1_ttl = l1_ttl self._l2_ttl = l2_ttl - # key -> (owner, monotonic expiry) - self._l1: OrderedDict[str, tuple[str, float]] = OrderedDict() - # key -> (monotonic expiry, deleting owner or "") of the tombstone a delete left here - self._tombstones: OrderedDict[str, tuple[float, str]] = OrderedDict() + # key -> (thread scope, monotonic expiry) + self._l1: OrderedDict[str, tuple[ThreadScope, float]] = OrderedDict() + # key -> (monotonic expiry, deleter scope or None when unknown) of a delete's tombstone + self._tombstones: OrderedDict[str, tuple[float, ThreadScope | None]] = OrderedDict() self._lock = asyncio.Lock() # ------------------------------------------------------------------ public - async def owner_of(self, thread_id: str | int) -> str | None: - """Return the owning ``user_id`` for ``thread_id``, or None if it has no owner. + async def scope_of( + self, thread_id: str | int, columns: Iterable[str] = () + ) -> ThreadScope | None: + """Return the thread's scope (canonical values), or None if the thread does not exist. + + ``columns`` are the columns the caller's isolation scope compares. A cached entry + missing one of them (written before that tenant column was declared) is not trusted: + the thread is looked up again and the cache refreshed. A column the checkpointer does + not answer stays missing, and the caller treats that as no match. Raises: NotImplementedError: if the backing checkpointer cannot resolve ownership. """ key = str(thread_id) + needed = tuple(columns) cached = await self._l1_get(key) - if cached is not None: - return cached + if cached is not None and _covers(cached, needed): + return dict(cached) from_l2 = await self._l2_get(key) - if from_l2 is not None: + if from_l2 is not None and _covers(from_l2, needed): await self._l1_put(key, from_l2) - return from_l2 + return dict(from_l2) - owner = await self._lookup(key) # may raise NotImplementedError -> propagate - if owner is None: + # may raise NotImplementedError -> propagate + scope = to_scope(await self._lookup(key)) + if scope is None: return None # never cache negatives (TOCTOU on new threads) - owner_str = str(owner) - await self._l1_put(key, owner_str) - await self._l2_set(key, owner_str) - return owner_str + await self._l1_put(key, scope) + await self._l2_set(key, scope) + return dict(scope) - async def evict(self, thread_id: str | int, owner: str | int | None = None) -> None: - """Drop any cached owner for ``thread_id`` and tombstone it (call on thread delete). + async def owner_of(self, thread_id: str | int) -> str | None: + """Return the owning ``user_id`` for ``thread_id``, or None if it has no owner. - ``owner`` is the user who deleted it (the owner, since the delete was authorized); that - user may recreate the id at once, because a stale cache entry can only point at them. + Raises: + NotImplementedError: if the backing checkpointer cannot resolve ownership. + """ + scope = await self.scope_of(thread_id) + return None if scope is None else scope.get("user_id") + + async def evict( + self, thread_id: str | int, owner: str | int | Mapping[str, Any] | None = None + ) -> None: + """Drop any cached scope for ``thread_id`` and tombstone it (call on thread delete). + + ``owner`` identifies who deleted it: a ``user_id``, or a mapping with ``user_id`` and + the deleter's tenant column values (what the authorization backend passes). That + caller may recreate the id at once when it is the thread's owner, because a stale cache + entry can only point at the same identity. With tenant columns declared the exemption + needs every tenant value, so a bare ``user_id`` blocks everyone. + + Raises: + OwnershipCacheEvictionError: the cache store could not delete the cached scope or + write the tombstone. Everything else (L1, the local tombstone, the other L2 + writes) is done first; the failure is also logged as a warning. """ key = str(thread_id) - owner_str = "" if owner is None else str(owner) + deleter = self._identity(owner) async with self._lock: cached = self._l1.pop(key, None) - if not owner_str and cached is not None: - owner_str = cached[0] - self._tombstones[key] = (time.monotonic() + self._l1_ttl, owner_str) + cached_scope = cached[0] if cached is not None else None + if deleter is None and cached_scope is not None: + deleter = self._identity(cached_scope) + elif ( + deleter is not None + and cached_scope is not None + and any( + column in cached_scope and cached_scope[column] != deleter.get(column) + for column in self._identity_columns() + ) + ): + # Someone other than the owner deleted it (a tenant-wide scope such as an org + # admin, or the same user_id in another tenant). A stale entry elsewhere still + # names the old owner, so the deleter gets no head start: the tombstone blocks + # everyone. + deleter = None + self._tombstones[key] = (time.monotonic() + self._l1_ttl, deleter) self._tombstones.move_to_end(key) while len(self._tombstones) > self._max_size: self._tombstones.popitem(last=False) - await self._l2_delete(key) - await self._l2_tombstone(key, owner_str) + failed = not await self._l2_delete(key) + failed = not await self._l2_tombstone(key, deleter) or failed + if failed: + logger.warning( + "Ownership cache eviction for thread %s did not reach the cache store; another " + "worker may keep the old scope for up to %d s.", + key, + self._l2_ttl, + ) + raise OwnershipCacheEvictionError(f"ownership cache eviction failed for thread {key}") - async def recently_deleted(self, thread_id: str | int, user_id: str | None = None) -> bool: - """Whether ``thread_id`` was deleted within the last L1 TTL by someone other than - ``user_id``. + def _identity_columns(self) -> tuple[str, ...]: + return ("user_id", *self._tenant_columns) - Until then another worker may still cache the old owner, so the id must not be handed - to a new owner yet. The deleting owner is the exception: a stale entry naming them - grants nothing they do not already own. A tombstone with no recorded owner blocks - everyone. + def _identity(self, who: str | int | Mapping[str, Any] | None) -> ThreadScope | None: + """A deleter or caller as canonical ``{user_id, : value}``, or None. + + A mapping keeps ``user_id`` and the declared tenant columns (canonical values); a bare + id becomes ``{"user_id": id}``. None, or no ``user_id``, is None (unknown). + """ + if who is None: + return None + if isinstance(who, Mapping): + user_id = who.get("user_id") + if user_id is None or user_id == "": + return None + return { + column: canonical_value(who[column]) + for column in self._identity_columns() + if column in who + } + text = str(who) + return {"user_id": text} if text else None + + def _exempt(self, deleter: ThreadScope | None, caller: ThreadScope | None) -> bool: + """Whether ``caller`` is the deleter on ``user_id`` and every declared tenant column.""" + if not deleter or not caller or not deleter.get("user_id"): + return False + return all( + column in deleter and column in caller and deleter[column] == caller[column] + for column in self._identity_columns() + ) + + async def _l2_delete(self, key: str) -> bool: + # Both layouts: an older worker may have written the plain owner entry. + ok = await self._l2_delete_key(self._owner_key(key)) + return await self._l2_delete_key(self._scope_key(key)) and ok + + async def recently_deleted( + self, thread_id: str | int, user_id: str | int | Mapping[str, Any] | None = None + ) -> bool: + """Whether ``thread_id`` was deleted within the last L1 TTL by someone other than + the caller. + + ``user_id`` is the caller: a ``user_id``, or a mapping with ``user_id`` and the caller's + tenant column values (canonical text). Until the tombstone expires another worker may + still cache the old scope, so the id must not be handed to a new owner yet. The deleting + owner is the exception when the caller matches them on ``user_id`` and every declared + tenant column: a stale entry naming them grants nothing they do not already own. A + tombstone with no recorded deleter blocks everyone. """ key = str(thread_id) + caller = self._identity(user_id) async with self._lock: entry = self._tombstones.get(key) if entry is not None: - expires_at, owner = entry + expires_at, deleter = entry if time.monotonic() < expires_at: - return not (owner and user_id is not None and owner == str(user_id)) + return not self._exempt(deleter, caller) del self._tombstones[key] - shared_owner = await self._l2_tombstone_owner(key) - if shared_owner is None: + found, shared = await self._l2_tombstone_deleter(key) + if not found: return False - return not (shared_owner and user_id is not None and shared_owner == str(user_id)) + return not self._exempt(shared, caller) # ------------------------------------------------------------------ L1 - async def _l1_get(self, key: str) -> str | None: + async def _l1_get(self, key: str) -> ThreadScope | None: async with self._lock: entry = self._l1.get(key) if entry is None: @@ -172,9 +337,9 @@ async def _l1_get(self, key: str) -> str | None: self._l1.move_to_end(key) # mark most-recently-used return owner - async def _l1_put(self, key: str, owner: str) -> None: + async def _l1_put(self, key: str, scope: ThreadScope) -> None: async with self._lock: - self._l1[key] = (owner, time.monotonic() + self._l1_ttl) + self._l1[key] = (dict(scope), time.monotonic() + self._l1_ttl) self._l1.move_to_end(key) while len(self._l1) > self._max_size: self._l1.popitem(last=False) # evict least-recently-used @@ -191,61 +356,120 @@ def _owner_key(key: str) -> str: def _tombstone_key(key: str) -> str: return f"owner:tomb:{key_part(key)}" - async def _l2_get(self, key: str) -> str | None: + @staticmethod + def _scope_key(key: str) -> str: + return f"owner:scope:{key_part(key)}" + + async def _l2_get(self, key: str) -> ThreadScope | None: if self._cache is None: return None + cache_key = self._scope_key(key) if self._tenant_columns else self._owner_key(key) try: - value = await self._cache.get(self._owner_key(key)) + value = await self._cache.get(cache_key) except Exception as exc: # degrade to DB; never fail the request logger.debug("Ownership L2 get failed for %s: %s", key, exc) return None if value is None: return None - return _text(value) + if not self._tenant_columns: + return {"user_id": _text(value)} + return _decode_scope(value) - async def _l2_set(self, key: str, owner: str) -> None: + async def _l2_set(self, key: str, scope: ThreadScope) -> None: if self._cache is None: return + if self._tenant_columns: + cache_key = self._scope_key(key) + payload = json.dumps(scope, sort_keys=True, separators=(",", ":")).encode() + else: + cache_key = self._owner_key(key) + payload = (scope.get("user_id") or "").encode() try: - # evict() removes the entry on delete; the TTL covers an eviction that failed. - await self._cache.set(self._owner_key(key), owner.encode(), ttl=self._l2_ttl) + # evict() removes the entry on delete; the TTL bounds an eviction that failed. + await self._cache.set(cache_key, payload, ttl=self._l2_ttl) except Exception as exc: logger.debug("Ownership L2 set failed for %s: %s", key, exc) - async def _l2_tombstone(self, key: str, owner: str) -> None: + def _encode_tombstone(self, deleter: ThreadScope | None) -> bytes: + """The shared tombstone value: ``-`` (unknown deleter), the plain ``user_id`` (no tenant + columns, the layout older releases wrote and read) or canonical JSON of the deleter's + ``user_id`` and tenant values. + """ + if not deleter or not deleter.get("user_id"): + return b"-" + if not self._tenant_columns: + return str(deleter["user_id"]).encode() + return json.dumps(deleter, sort_keys=True, separators=(",", ":")).encode() + + def _decode_tombstone(self, value: object) -> ThreadScope | None: + """The deleter in a shared tombstone, or None when unknown (blocks everyone). + + With tenant columns declared, a plain ``user_id`` (written by an older release during a + rolling deploy) carries no tenant values, so it exempts nobody. + """ + text = _text(value) + if text == "-" or not text: + return None + if not self._tenant_columns: + return {"user_id": text} + return _decode_scope(text) or {"user_id": text} + + async def _l2_tombstone(self, key: str, deleter: ThreadScope | None) -> bool: + """Write the shared tombstone; False when the cache store failed.""" if self._cache is None: - return + return True try: await self._cache.set( self._tombstone_key(key), - owner.encode() or b"-", + self._encode_tombstone(deleter), ttl=max(1, int(self._l1_ttl + 0.999)), ) except Exception as exc: - logger.debug("Ownership L2 tombstone failed for %s: %s", key, exc) + logger.warning("Ownership L2 tombstone failed for %s: %s", key, exc) + return False + return True - async def _l2_tombstone_owner(self, key: str) -> str | None: - """The deleting owner in a shared tombstone ("" when unknown), or None when none.""" + async def _l2_tombstone_deleter(self, key: str) -> tuple[bool, ThreadScope | None]: + """``(found, deleter)`` from a shared tombstone; deleter None when unknown.""" if self._cache is None: - return None + return False, None try: value = await self._cache.get(self._tombstone_key(key)) except Exception as exc: # degrade to "not deleted"; L1 already answered for us logger.debug("Ownership L2 tombstone read failed for %s: %s", key, exc) - return None + return False, None if value is None: - return None - text = _text(value) - return "" if text == "-" else text + return False, None + return True, self._decode_tombstone(value) - async def _l2_delete(self, key: str) -> None: + async def _l2_delete_key(self, cache_key: str) -> bool: + """Delete one L2 key; False when the cache store failed.""" if self._cache is None: - return + return True try: - await self._cache.delete(self._owner_key(key)) + await self._cache.delete(cache_key) except Exception as exc: - logger.debug("Ownership L2 delete failed for %s: %s", key, exc) + logger.warning("Ownership L2 delete failed for %s: %s", cache_key, exc) + return False + return True def _text(value: object) -> str: return value.decode() if isinstance(value, bytes | bytearray) else str(value) + + +def _covers(scope: Mapping[str, Any], columns: Iterable[str]) -> bool: + return all(column in scope for column in columns) + + +def _decode_scope(value: object) -> ThreadScope | None: + """A JSON scope entry from the cache store, or None (a miss) when it is not one.""" + try: + data = json.loads(_text(value)) + except (ValueError, UnicodeDecodeError): + return None + if not isinstance(data, dict) or not isinstance(data.get("user_id"), str): + return None + if not all(isinstance(k, str) and (v is None or isinstance(v, str)) for k, v in data.items()): + return None + return data diff --git a/tenxgraph_api/src/app/core/auth/permissions.py b/tenxgraph_api/src/app/core/auth/permissions.py index 1696f926..00508381 100644 --- a/tenxgraph_api/src/app/core/auth/permissions.py +++ b/tenxgraph_api/src/app/core/auth/permissions.py @@ -19,6 +19,7 @@ from tenxgraph_api.src.app.core.auth.authorization import ( AuthorizationBackend, ScopeClaimError, + resolve_isolation_scope, scopes_from_claims, ) from tenxgraph_api.src.app.core.auth.identity_cache import recall_identity @@ -258,7 +259,7 @@ def __init__( self.introspection = introspection self.explicit_scope = explicit_scope - async def __call__( # noqa: PLR0912 + async def __call__( # noqa: PLR0912, PLR0915 self, connection: HTTPConnection, response: Response, @@ -373,16 +374,27 @@ async def __call__( # noqa: PLR0912 # this user into config["user"], so the isolation policy + scopes reach every # downstream call (checkpointer, store, graph execution). Overwrites anything the # client sent -- not hijackable. + # The isolation scope is chosen per caller: ``isolation_scope(user)`` (older backends + # take no argument; see ``resolve_isolation_scope``). if isinstance(user, dict) and user.get("user_id"): from tenxgraph.core.authz import build_authz - scope_fn = getattr(authz, "isolation_scope", None) - iso_scope = scope_fn() if callable(scope_fn) else "none" - user["authz"] = build_authz( - user["user_id"], - scope=iso_scope, - scopes=granted_scopes if granted_scopes is not None else [], - ) + iso_scope = resolve_isolation_scope(authz, user) + try: + user["authz"] = build_authz( + user["user_id"], + scope=iso_scope, + scopes=granted_scopes if granted_scopes is not None else [], + ) + except ValueError as exc: + # A malformed scope (an unknown string such as "owners", or a bad column + # list) must never turn into "no isolation": refuse. + logger.error( + "isolation_scope of %s returned an invalid scope: %s", + type(authz).__name__, + exc, + ) + _reject(connection, 403, f"Not authorized to {self.action} {self.resource}") # Log successful auth/authz (with sanitized user info) logger.debug( diff --git a/tenxgraph_api/src/app/core/config/graph_config.py b/tenxgraph_api/src/app/core/config/graph_config.py index d5f4b683..40339ae5 100644 --- a/tenxgraph_api/src/app/core/config/graph_config.py +++ b/tenxgraph_api/src/app/core/config/graph_config.py @@ -828,6 +828,16 @@ def injectq_path(self) -> str | None: def store_path(self) -> str | None: return self.data.get("store", None) + @property + def store_allow_tenant_mismatch(self) -> bool: + """``store_allow_tenant_mismatch`` (default false): the ``store`` key's opt-out of the + checkpointer/store tenant column check, like ``compile(allow_tenant_mismatch=True)``. + """ + value = self.data.get("store_allow_tenant_mismatch", False) + if value is None: + return False + return _parse_bool(value, field="store_allow_tenant_mismatch") + @property def cache_store(self) -> CacheStoreConfig: """``cache_store`` key, parsed (:class:`CacheStoreConfig`); the memory store when absent. diff --git a/tenxgraph_api/src/app/loader.py b/tenxgraph_api/src/app/loader.py index ffe93eb5..3c7480f1 100644 --- a/tenxgraph_api/src/app/loader.py +++ b/tenxgraph_api/src/app/loader.py @@ -2,6 +2,7 @@ import inspect import logging import os +import sys import weakref from collections.abc import Callable from pathlib import Path @@ -498,21 +499,23 @@ def _load_custom_auth(path: str | None) -> BaseAuth: if not path: raise ValueError("Custom auth requires a 'path' in auth_config.") - # Extract file path before the ':' for existence check - module_or_path = path.split(":", 1)[0] if ":" in path else path + module_or_path, sep, attr = path.partition(":") - # Simple handling: if it appears to be a filesystem path, use it; otherwise - # convert dotted module path to a file path like src/auth/custom_auth.py - if os.path.sep in module_or_path or module_or_path.endswith(".py"): - file_path = Path(module_or_path) - elif "." in module_or_path and os.path.sep not in module_or_path: - file_path = Path(module_or_path.replace(".", os.path.sep) + ".py") + # Accept a file path form ("auth/custom.py:Cls") as well as a module form + # ("auth_app:Cls", "pkg.mod:Cls"). Both become a dotted module name resolved from the + # import base (cwd plus sys.path, which holds the project folder), like ``agent``. + if module_or_path.endswith(".py"): + module_name = module_or_path[:-3].replace("\\", "/").strip("/").replace("/", ".") else: - file_path = Path(module_or_path) + module_name = module_or_path + rel = Path(*module_name.split(".")) if module_name else Path(module_or_path) - if not file_path.exists(): + bases = [Path.cwd(), *(Path(p) for p in sys.path if p)] + found = any((base / rel).with_suffix(".py").exists() or (base / rel).is_dir() for base in bases) + if not found: raise ValueError(f"Custom auth path does not exist: {module_or_path}") + path = f"{module_name}{sep}{attr}" auth = load_auth(path) if auth is None: # load_auth only returns None for an empty path, checked above raise ValueError("Custom auth requires a 'path' in auth_config.") @@ -562,13 +565,20 @@ def _resolve_authorization_backend( backend_name = (authorization.get("backend") or authorization.get("type") or "").lower() if backend_name in ("rbac", "role_based", "roles"): logger.info("Using RoleBasedAuthorizationBackend (config-driven roles).") - return RoleBasedAuthorizationBackend( - role_scopes=authorization.get("roles") or authorization.get("role_scopes") or {}, - default_scopes=authorization.get("default_scopes") or (), - isolation=authorization.get("isolation", "owner"), - cache=cache, - server_thread_ids=server_thread_ids, - ) + try: + return RoleBasedAuthorizationBackend( + role_scopes=authorization.get("roles") + or authorization.get("role_scopes") + or {}, + default_scopes=authorization.get("default_scopes") or (), + # Unknown values (a typo, null) raise: the server must not boot with a + # guessed isolation level. + isolation=authorization.get("isolation", "owner"), + cache=cache, + server_thread_ids=server_thread_ids, + ) + except ValueError as exc: + raise ValueError(f"authorization.isolation in 10xgraph.json: {exc}") from exc raise ValueError( f"Unknown authorization backend config: {authorization!r}. For RBAC use " '{"backend": "rbac", "roles": {...}}.' @@ -717,6 +727,56 @@ def warn_per_process_state(container: InjectQ) -> None: ) +def check_store_tenant_columns( + graph: CompiledGraph | None, + container: InjectQ, + store: BaseStore | None, + *, + allow_mismatch: bool = False, +) -> None: + """Run core's checkpointer/store tenant column check for the ``store`` key's store. + + ``compile()`` runs it for the store passed to it, honouring ``allow_tenant_mismatch``; the + store from ``10xgraph.json`` never goes through ``compile()``. It is compared with the + graph's checkpointer and with the checkpointer bound in the container (when different). + Same rule as core: ``MODE=production`` refuses the boot (``GraphError``), any other mode + logs a warning. ``allow_mismatch`` (``store_allow_tenant_mismatch`` in ``10xgraph.json``) + never refuses and logs the mismatch at INFO. Skipped on a core without the check, and when + the store is the one the graph was compiled with (``compile()`` already checked it). + """ + if store is None: + return + try: + from tenxgraph.core.graph.utils.tenant_check import check_tenant_columns + except ImportError: # core older than tenant columns: nothing to compare + return + if graph is not None and getattr(graph, "_store", None) is store: + return + checkpointers: list[object] = [] + for candidate in ( + getattr(graph, "_checkpointer", None) if graph is not None else None, + container.try_get(BaseCheckpointer), + ): + if candidate is not None and all(candidate is not seen for seen in checkpointers): + checkpointers.append(candidate) + try: + takes_flag = "allow_mismatch" in inspect.signature(check_tenant_columns).parameters + except (TypeError, ValueError): # pragma: no cover - defensive + takes_flag = False + for checkpointer in checkpointers: + if takes_flag: + check_tenant_columns(checkpointer, store, allow_mismatch=allow_mismatch) + elif allow_mismatch: + logger.info( + "store_allow_tenant_mismatch is set: not comparing the tenant columns of %s " + "and %s.", + type(checkpointer).__name__, + type(store).__name__, + ) + else: + check_tenant_columns(checkpointer, store) + + async def attach_all_modules( config: GraphConfig, container: InjectQ, @@ -756,6 +816,14 @@ async def attach_all_modules( # router reports "Store is not configured" even when 10xgraph.json sets one. store = load_store(config.store_path) container.bind_instance(BaseStore, store, allow_none=True) + # compile() compares the graph's checkpointer and store tenant columns (D6); a store bound + # from 10xgraph.json skips compile(), so run the same check here. + check_store_tenant_columns( + graph, + container, + store, + allow_mismatch=getattr(config, "store_allow_tenant_mismatch", False) is True, + ) # Bind the in-memory telemetry store so the graph service can record run # events and the /v1/observability endpoints can reconstruct traces. diff --git a/tenxgraph_api/src/app/routers/checkpointer/router.py b/tenxgraph_api/src/app/routers/checkpointer/router.py index 144f04eb..415f2a9e 100644 --- a/tenxgraph_api/src/app/routers/checkpointer/router.py +++ b/tenxgraph_api/src/app/routers/checkpointer/router.py @@ -1,11 +1,12 @@ """Checkpointer router module.""" -from typing import Any +from typing import Annotated, Any -from fastapi import APIRouter, Depends, HTTPException, Request, status +from fastapi import APIRouter, Depends, HTTPException, Query, Request, status from injectq.integrations import InjectAPI from tenxgraph.core.state import Message +from tenxgraph_api.src.app.core.auth.client_filters import parse_filters_param from tenxgraph_api.src.app.core.auth.permissions import RequirePermission from tenxgraph_api.src.app.core.auth.request_config import client_config from tenxgraph_api.src.app.core.config.settings import get_settings @@ -469,6 +470,18 @@ async def list_threads( search: str | None = None, offset: int | None = None, limit: int | None = None, + filters: Annotated[ + str | None, + Query( + description=( + "JSON object of column conditions, e.g. " + '{"plan": "pro", "created_at": {"gte": "2026-01-01T00:00:00+00:00"}}. ' + "Columns: thread_name, created_at, updated_at and the checkpointer's declared " + "extra columns. Tenant columns and user_id are refused. Needs a checkpointer " + "with filterable columns (PgCheckpointer); 422 otherwise." + ), + ), + ] = None, service: CheckpointerService = InjectAPI(CheckpointerService), user: dict[str, Any] = Depends(RequirePermission("checkpointer", "read")), ): @@ -493,6 +506,7 @@ async def list_threads( search, offset, _clamp_limit(limit), + filters=parse_filters_param(filters), ) return success_response( diff --git a/tenxgraph_api/src/app/routers/checkpointer/services/checkpointer_service.py b/tenxgraph_api/src/app/routers/checkpointer/services/checkpointer_service.py index 3b4b7095..a2007b09 100644 --- a/tenxgraph_api/src/app/routers/checkpointer/services/checkpointer_service.py +++ b/tenxgraph_api/src/app/routers/checkpointer/services/checkpointer_service.py @@ -1,3 +1,4 @@ +import inspect from collections.abc import Awaitable, Callable from typing import Any @@ -8,6 +9,10 @@ from tenxgraph.storage.checkpointer import BaseCheckpointer from tenxgraph_api.src.app.core import logger +from tenxgraph_api.src.app.core.auth.client_filters import ( + check_thread_filters, + filter_errors_as_client_errors, +) from tenxgraph_api.src.app.core.auth.request_config import ( ClientInputError, client_config, @@ -36,6 +41,19 @@ from tenxgraph_api.src.app.utils.parse_output import parse_state_output +def _accepts_user_kwarg(func: Callable[..., Any]) -> bool: + """Whether ``func`` takes a ``user`` keyword (or ``**kwargs``).""" + try: + params = inspect.signature(func).parameters.values() + except (TypeError, ValueError): + return False + return any( + p.kind is inspect.Parameter.VAR_KEYWORD + or (p.name == "user" and p.kind is not inspect.Parameter.POSITIONAL_ONLY) + for p in params + ) + + class CheckpointerUnavailableError(HTTPException, ValueError): """Raised when the checkpointer service has not been configured.""" @@ -102,7 +120,7 @@ async def _check_media_references(self, messages: list[Any], user: dict) -> None user_id = user.get("user_id") if not user_id or not messages: return - await check_media_references(messages, self._media_service(), str(user_id)) + await check_media_references(messages, self._media_service(), str(user_id), user) def _check_media_urls(self, messages: list[Any]) -> None: """External media URLs in written messages get the same policy check as graph input @@ -124,7 +142,50 @@ async def _link_media(self, messages: list[Any], cfg: dict[str, Any], user: dict return keys = [k for m in messages for k in internal_media_keys(m)] if keys: - await media.link_threads(keys, str(thread_id), str(user_id)) + await media.link_threads(keys, str(thread_id), str(user_id), user) + + async def _file_owners( + self, media: Any, thread_id: Any, cfg: dict[str, Any], user: dict + ) -> list[tuple[str, dict[str, str | None]]]: + """Identities whose files linked to the thread are released when it is deleted. + + The thread's owner (``user_id`` plus tenant values, read before the delete through + ``aget_thread_scope``, else ``aget_thread_owner``) and the deleter. An org admin or a + ``none``-scope deleter thus releases the member's files, not only their own. A + failed lookup leaves only the deleter. Never raises. + """ + from tenxgraph_api.src.app.core.auth.authorization import scope_lookup + from tenxgraph_api.src.app.core.auth.ownership_resolver import to_scope + + owners: list[tuple[str, dict[str, str | None]]] = [] + try: + columns = media.tenant_columns() + scope = to_scope(await scope_lookup(self.checkpointer)(thread_id)) + if scope is not None and scope.get("user_id") is not None: + owners.append((str(scope["user_id"]), {c: scope.get(c) for c in columns})) + except Exception as exc: + logger.debug("Thread owner lookup for file cleanup failed: %s", exc) + caller = cfg.get("user_id") + if caller is not None: + try: + identity = (str(caller), media.caller_tenant(user)) + except Exception as exc: + logger.debug("Caller tenant lookup for file cleanup failed: %s", exc) + else: + if identity not in owners: + owners.append(identity) + return owners + + @staticmethod + async def _release_owned_files( + media: Any, thread_id: Any, owners: list[tuple[str, dict[str, str | None]]] + ) -> bool: + """``release_thread`` for each owner; False when any file could not be released.""" + released_all = True + for owner_id, tenant in owners: + if await media.release_thread(str(thread_id), owner_id, tenant): + released_all = False + return released_all def _link_tracking_media(self) -> Any: """The media service when it can track thread links, else ``None``. Never raises.""" @@ -441,12 +502,26 @@ async def list_threads( search: str | None = None, offset: int | None = None, limit: int | None = None, + filters: dict[str, Any] | None = None, ) -> ThreadsListResponseSchema: + """List the caller's threads, narrowed by ``filters`` (validated first, 422 when bad). + + ``filters`` only narrow: core ANDs them after the isolation conditions, which come from + the trusted ``config["user"]``, never from the client. + """ cfg = self._config({}, user) - res = await self.checkpointer.alist_threads(cfg, search, offset, limit) - has_more = await _has_more( - len(res), offset, limit, lambda o: self.checkpointer.alist_threads(cfg, search, o, 1) - ) + check_thread_filters(self.checkpointer, filters) + # Only pass filters when there are some, so a custom checkpointer written before + # ``alist_threads`` took ``filters`` keeps working for plain listings. + extra: dict[str, Any] = {"filters": filters} if filters else {} + with filter_errors_as_client_errors(): + res = await self.checkpointer.alist_threads(cfg, search, offset, limit, **extra) + has_more = await _has_more( + len(res), + offset, + limit, + lambda o: self.checkpointer.alist_threads(cfg, search, o, 1, **extra), + ) return ThreadsListResponseSchema( threads=[t.model_dump() for t in res], has_more=has_more, @@ -478,6 +553,8 @@ async def delete_thread( except Exception as exc: incomplete.append("files") logger.warning("File cleanup skipped for thread %s: %s", log_safe(thread_id), exc) + # Whose files to release, read while the thread still exists. + owners = await self._file_owners(media, thread_id, cfg, user) if media is not None else [] res = await self.checkpointer.aclean_thread(cfg) if ( thread_keys @@ -487,7 +564,8 @@ async def delete_thread( incomplete.append("files") # Files still linked to the thread through messages that are gone (cleared or replaced # state) are not in `thread_keys`; release them by their own metadata. - if media is not None and await media.release_thread(str(thread_id), cfg.get("user_id")): + # Owned by the thread's owner or the deleter, with their tenant values. + if media is not None and not await self._release_owned_files(media, thread_id, owners): incomplete.append("files") # Clean telemetry traces if TelemetryStore is bound in InjectQ @@ -514,7 +592,12 @@ async def delete_thread( authz = current_container().try_get(AuthorizationBackend) evict = getattr(authz, "evict", None) if callable(evict): - pending = evict(str(thread_id), user.get("user_id")) + # The built-in backend takes the whole user, so the delete tombstone records + # the deleter's tenant values too; a custom evict(thread_id, owner) gets the id. + if _accepts_user_kwarg(evict): + pending = evict(str(thread_id), user.get("user_id"), user=user) + else: + pending = evict(str(thread_id), user.get("user_id")) if pending is not None: await pending except Exception as exc: diff --git a/tenxgraph_api/src/app/routers/graph/services/graph_service.py b/tenxgraph_api/src/app/routers/graph/services/graph_service.py index 92e8fd61..b2139aee 100644 --- a/tenxgraph_api/src/app/routers/graph/services/graph_service.py +++ b/tenxgraph_api/src/app/routers/graph/services/graph_service.py @@ -854,6 +854,7 @@ async def _prepare_input( graph_input: GraphInputSchema, user_id: str | None = None, server_config: dict[str, Any] | None = None, + user: dict[str, Any] | None = None, ): is_new_thread = False config = client_config(graph_input.config) @@ -888,17 +889,19 @@ async def _prepare_input( # External media URLs first: the checks are cheap and need no network. self.check_media_urls(graph_input.messages) # Preprocess multimodal messages (resolve file_id → cached text, etc.) + # ``user`` (the authenticated identity) carries the tenant values files are scoped by. preprocessed = await preprocess_multimodal_messages( graph_input.messages, self.media_service, user_id, + user=user, ) # The accepted messages now use these files: record the thread on each (owned files # only; the ownership check above already ran). if user_id and self.media_service is not None: keys = [k for m in preprocessed for k in internal_media_keys(m)] if keys: - await self.media_service.link_threads(keys, str(thread_id), str(user_id)) + await self.media_service.link_threads(keys, str(thread_id), str(user_id), user) input_data: dict = { "messages": preprocessed, } @@ -945,7 +948,9 @@ async def invoke_graph( # noqa: PLR0912, PLR0915 logger.debug("Invoking graph with input: %s", log_safe(graph_input.messages)) # Prepare the input - input_data, config, meta = await self._prepare_input(graph_input, user.get("user_id")) + input_data, config, meta = await self._prepare_input( + graph_input, user.get("user_id"), user=user + ) thread_lock = await self._lock_new_thread(meta) # add user inside config config["user"] = user @@ -1161,7 +1166,7 @@ async def stream_chunks( # noqa: PLR0912, PLR0915 # Prepare the config input_data, config, meta = await self._prepare_input( - graph_input, user.get("user_id"), server_config + graph_input, user.get("user_id"), server_config, user=user ) thread_lock = await self._lock_new_thread(meta) # add user inside config diff --git a/tenxgraph_api/src/app/routers/graph/services/multimodal_preprocessor.py b/tenxgraph_api/src/app/routers/graph/services/multimodal_preprocessor.py index 08ff2554..9998bf8a 100644 --- a/tenxgraph_api/src/app/routers/graph/services/multimodal_preprocessor.py +++ b/tenxgraph_api/src/app/routers/graph/services/multimodal_preprocessor.py @@ -151,14 +151,20 @@ def media_file_ids(message: Message | dict[str, Any]) -> list[str]: return ids -async def _ensure_access(media_service: MediaService, file_id: str, user_id: str) -> None: +async def _ensure_access( + media_service: MediaService, + file_id: str, + user_id: str, + user: dict[str, Any] | None = None, +) -> None: """Ownership check with one answer for missing and foreign files. Both become the same ``ValueError`` (a 422 on invoke), so the response never tells a - caller whether another user's file exists. + caller whether another user's (or another tenant's) file exists. ``user`` is the + authenticated identity, whose tenant values the file must also match. """ try: - await media_service.ensure_can_access(file_id, user_id) + await media_service.ensure_can_access(file_id, user_id, user) except (KeyError, PermissionError) as exc: raise ClientInputError(f"File not found: {file_id}") from exc @@ -167,9 +173,13 @@ async def check_media_references( messages: list[Message] | list[dict[str, Any]], media_service: MediaService | None, user_id: str, + user: dict[str, Any] | None = None, ) -> None: """Run the ownership check on every internal media reference a client sent. + ``user`` is the authenticated identity: with tenant columns declared, each file must + record the same tenant values as well as the same ``user_id``. + A client may reference ``graph://media/`` directly instead of a ``file_id``; the graph resolves both the same way, so both must be checked. Also used for messages written straight into a thread, since the next run resolves those references too. @@ -184,7 +194,7 @@ async def check_media_references( "configured on this server." ) for key in keys: - await _ensure_access(media_service, key, user_id) + await _ensure_access(media_service, key, user_id, user) async def _get_cached_extraction(media_service: MediaService, file_id: str) -> str | None: @@ -206,7 +216,10 @@ async def _get_cached_extraction(media_service: MediaService, file_id: str) -> s async def _offload_inline( - media_service: MediaService, messages: list[Message], user_id: str | None + media_service: MediaService, + messages: list[Message], + user_id: str | None, + user: dict[str, Any] | None = None, ) -> None: """Move large inline base64 blobs into the media store before anything copies them into state. @@ -214,13 +227,14 @@ async def _offload_inline( """ offload = getattr(media_service, "offload_inline", None) if inspect.iscoroutinefunction(offload): - await offload(messages, user_id) + await offload(messages, user_id, user) async def preprocess_multimodal_messages( messages: list[Message], media_service: MediaService | None, user_id: str | None = None, + user: dict[str, Any] | None = None, ) -> list[Message]: """Resolve file_id references in messages before graph execution. @@ -233,15 +247,16 @@ async def preprocess_multimodal_messages( the MediaRefResolver can pick it up at LLM-call time. With an authenticated ``user_id``, every file the messages reference -- by ``file_id`` - or by an internal ``graph://media/`` URL -- must belong to that user. + or by an internal ``graph://media/`` URL -- must belong to that user (and, with tenant + columns declared, to the tenant values of ``user``, the authenticated identity). ``media_service`` being None skips the rewriting, but not the ownership rule. """ if user_id: - await check_media_references(messages, media_service, user_id) + await check_media_references(messages, media_service, user_id, user) if media_service is None: return messages - await _offload_inline(media_service, messages, user_id) + await _offload_inline(media_service, messages, user_id, user) processed: list[Message] = [] for msg in messages: @@ -255,7 +270,7 @@ async def preprocess_multimodal_messages( and block.media.file_id ): if user_id: - await _ensure_access(media_service, block.media.file_id, user_id) + await _ensure_access(media_service, block.media.file_id, user_id, user) cached = await _get_cached_extraction(media_service, block.media.file_id) if cached: new_content.append(TextBlock(text=cached)) @@ -265,7 +280,7 @@ async def preprocess_multimodal_messages( if hasattr(block, "media") and block.media.kind == "file_id" and block.media.file_id: fid = block.media.file_id if user_id: - await _ensure_access(media_service, fid, user_id) + await _ensure_access(media_service, fid, user_id, user) # Convert file_id → graph://media/ URL reference if not is_internal_media_url(block.media.url): block.media.kind = "url" diff --git a/tenxgraph_api/src/app/routers/media/__init__.py b/tenxgraph_api/src/app/routers/media/__init__.py index 1a750244..51fb3ffd 100644 --- a/tenxgraph_api/src/app/routers/media/__init__.py +++ b/tenxgraph_api/src/app/routers/media/__init__.py @@ -18,6 +18,7 @@ from tenxgraph.storage.media.security import check_image_pixels from tenxgraph.storage.media.storage.base import BaseMediaStore +from tenxgraph_api.src.app.core.auth.authorization import identity_values, tenant_column_names from tenxgraph_api.src.app.core.auth.request_config import ClientInputError from tenxgraph_api.src.app.core.config.graph_config import MediaConfig from tenxgraph_api.src.app.core.config.media_settings import MediaSettings, MediaStorageType @@ -35,6 +36,35 @@ # Metadata key listing the threads whose messages reference a file. Written empty at upload # (so a file without the key is known to predate link tracking and is never auto-deleted). THREADS_KEY = "threads" +# Metadata key holding the uploader's tenant values: ``{column: canonical text}`` for every +# tenant column the bound checkpointer declares (``PgCheckpointer(tenant_columns=...)``). +# Written only when tenant columns are declared; the values come from ``config["user"]``. +TENANT_KEY = "tenant" + + +def tenant_matches(metadata: dict[str, Any], tenant: dict[str, str | None]) -> bool: + """Whether a file's recorded tenant values equal ``tenant`` on every column. + + ``tenant`` is the caller's values from :meth:`MediaService.caller_tenant`: empty when no + tenant columns are declared (always a match). Fails closed: a caller missing a value, or + a file with no recorded value for a column (uploaded before tenant tracking), never + matches. + """ + if not tenant: + return True + recorded = metadata.get(TENANT_KEY) + if not isinstance(recorded, dict): + return False + for column, value in tenant.items(): + stored = recorded.get(column) + if value is None or stored is None or str(stored) != value: + return False + return True + + +class MissingTenantError(PermissionError): + """The authenticated user has no value for a declared tenant column.""" + # In-process caches in front of the shared checkpointer cache. Bounded so a stream of new # uploads cannot grow a worker's memory without limit. @@ -200,15 +230,27 @@ def _create_document_pipeline(settings: MediaSettings) -> DocumentPipeline: class _OwnedStore: """Wraps a media store so offloaded blobs get the same metadata as an upload.""" - def __init__(self, store: BaseMediaStore, user_id: str | None, track_threads: bool): + def __init__( + self, + store: BaseMediaStore, + user_id: str | None, + track_threads: bool, + tenant: dict[str, str | None] | None = None, + ): self._store = store self._user_id = user_id self._track_threads = track_threads + self._tenant = tenant or {} async def store(self, data: bytes, mime_type: str, metadata: dict[str, Any] | None = None): meta: dict[str, Any] = {"created_at": int(time.time()), **(metadata or {})} if self._user_id: + if any(value is None for value in self._tenant.values()): + # Stored unscoped, no one in the tenant could read it back: refuse instead. + raise ClientInputError("the authenticated user has no tenant value") meta["owner_id"] = str(self._user_id) + if self._tenant: + meta[TENANT_KEY] = dict(self._tenant) if self._track_threads: meta[THREADS_KEY] = [] return await self._store.store(data, mime_type, metadata=meta) @@ -264,12 +306,45 @@ def checkpointer(self) -> BaseCheckpointer | None: def max_size_bytes(self) -> int: return int(self._settings.MEDIA_MAX_SIZE_MB * 1024 * 1024) + # ------------------------------------------------------------------ + # Tenant scope + # ------------------------------------------------------------------ + + def tenant_columns(self) -> tuple[str, ...]: + """Tenant columns the bound checkpointer declares (empty when none or no checkpointer).""" + try: + checkpointer = self.checkpointer + except AttributeError: # a bare stand-in built without __init__ + return () + return tenant_column_names(checkpointer) + + def caller_tenant(self, user: dict[str, Any] | None) -> dict[str, str | None]: + """The caller's value for each declared tenant column, as canonical text. + + Read from the authenticated user (``config["user"]``) only and converted with the + column's declared type, exactly as thread ownership compares them + (``authorization.identity_values``). Empty when no tenant columns are declared; a + missing or unconvertible value is None, which never matches a file. + """ + columns = self.tenant_columns() + if not columns: + return {} + values = identity_values(self.checkpointer, user or {}) + return {column: values.get(column) for column in columns} + + @staticmethod + def _owned_by(metadata: dict[str, Any], user_id: Any, tenant: dict[str, str | None]) -> bool: + """Whether the file records ``user_id`` as owner and ``tenant`` as its tenant values.""" + return str(metadata.get("owner_id")) == str(user_id) and tenant_matches(metadata, tenant) + # ------------------------------------------------------------------ # File operations # ------------------------------------------------------------------ - async def ensure_can_access(self, file_id: str, user_id: str | None) -> None: - """Reject access to a file owned by a different user. + async def ensure_can_access( + self, file_id: str, user_id: str | None, user: dict[str, Any] | None = None + ) -> None: + """Reject access to a file owned by a different user (or tenant). Files had no owner concept at all: `get_file(file_id)` returned bytes to any authenticated caller who knew (or guessed) the id. Ownership is recorded in @@ -278,9 +353,16 @@ async def ensure_can_access(self, file_id: str, user_id: str | None) -> None: Legacy files uploaded before ownership existed, or while auth was off, carry no ``owner_id``. ``MEDIA_REQUIRE_OWNER`` decides what happens to them: denied (the production default) or allowed with a warning (the development default). + + With tenant columns declared on the checkpointer, the file's recorded tenant values + must also equal the caller's (``user``, the authenticated identity) on every column: + the same ``user_id`` in two tenants is two identities. A caller without a value, and + a file with no recorded tenant values (uploaded before tenant tracking or without an + owner), are denied. """ if not user_id: return + tenant = self.caller_tenant(user) try: metadata = await self.store.get_metadata(file_id) @@ -293,7 +375,7 @@ async def ensure_can_access(self, file_id: str, user_id: str | None) -> None: owner_id = metadata.get("owner_id") if owner_id is None: - if self._settings.MEDIA_REQUIRE_OWNER: + if tenant or self._settings.MEDIA_REQUIRE_OWNER: raise PermissionError(f"File has no recorded owner: {file_id}") logger.warning( "File %s has no recorded owner (uploaded before ownership tracking); " @@ -302,7 +384,7 @@ async def ensure_can_access(self, file_id: str, user_id: str | None) -> None: ) return - if str(owner_id) != str(user_id): + if not self._owned_by(metadata, user_id, tenant): # Deliberately not "you don't own this" -- do not confirm the id exists. logger.warning( "User %s attempted to access file %s owned by another user", @@ -317,11 +399,17 @@ async def upload_file( filename: str, mime_type: str, user_id: str | None = None, + user: dict[str, Any] | None = None, ) -> dict[str, Any]: """Upload a file: store binary + optionally extract text. Records the uploader as the file's owner, so it cannot later be read by a - different user who happens to know the id. + different user who happens to know the id. With tenant columns declared, the + uploader's tenant values (from ``user``, the authenticated identity) are recorded + too, under ``TENANT_KEY``. + + Raises: + MissingTenantError: Tenant columns are declared and ``user`` lacks a value. Returns a dict with file_id, mime_type, size_bytes, filename, extracted_text (nullable), and url. @@ -349,7 +437,8 @@ async def upload_file( except DocumentTooLargeError as exc: raise ClientInputError(str(exc)) from exc - await self._check_quota(user_id, len(data)) + tenant = self._upload_tenant(user_id, user) + await self._check_quota(user_id, len(data), tenant) # Store binary in the configured MediaStore. The owner is written into the # file's own metadata, so ownership survives as long as the file does -- @@ -357,6 +446,8 @@ async def upload_file( file_metadata: dict[str, Any] = {"filename": filename, "created_at": int(time.time())} if user_id: file_metadata["owner_id"] = str(user_id) + if tenant: + file_metadata[TENANT_KEY] = tenant if self.supports_links: file_metadata[THREADS_KEY] = [] @@ -397,11 +488,31 @@ async def upload_file( return result - async def _check_quota(self, user_id: str | None, incoming_bytes: int) -> None: + def _upload_tenant( + self, user_id: str | None, user: dict[str, Any] | None + ) -> dict[str, str | None]: + """The tenant values an upload records (empty without a user or tenant columns). + + Raises: + MissingTenantError: Tenant columns are declared and ``user`` lacks a value. + """ + tenant = self.caller_tenant(user) if user_id else {} + if any(value is None for value in tenant.values()): + raise MissingTenantError("The authenticated user has no value for a tenant column") + return tenant + + async def _check_quota( + self, + user_id: str | None, + incoming_bytes: int, + tenant: dict[str, str | None] | None = None, + ) -> None: """Refuse an upload that would take ``user_id`` past the per-user file or byte quota. Does nothing without a user, with both quotas off, or when the store cannot list its - files (a warning is logged once). Counts only files that record this user as owner. + files (a warning is logged once). Counts only files that record this user as owner + and, with tenant columns declared, the same tenant values (``tenant``), so the same + ``user_id`` in two tenants has two quotas. """ max_files = self._settings.MEDIA_MAX_FILES_PER_USER max_bytes = int(self._settings.MEDIA_MAX_USER_STORAGE_MB * 1024 * 1024) @@ -426,7 +537,7 @@ async def _check_quota(self, user_id: str | None, incoming_bytes: int) -> None: except Exception as exc: logger.debug("Skipping unreadable file %s in quota count: %s", log_safe(key), exc) continue - if meta and str(meta.get("owner_id")) == str(user_id): + if meta and self._owned_by(meta, user_id, tenant or {}): count += 1 used += int(meta.get("size_bytes") or 0) if max_files and count >= max_files: @@ -437,20 +548,29 @@ async def _check_quota(self, user_id: str | None, incoming_bytes: int) -> None: "of files per user" ) - async def offload_inline(self, messages: list[Any], user_id: str | None = None) -> None: + async def offload_inline( + self, + messages: list[Any], + user_id: str | None = None, + user: dict[str, Any] | None = None, + ) -> None: """Move large inline base64 media in ``messages`` into the store, in place. Uses core ``ensure_media_offloaded`` with ``MEDIA_INLINE_MAX_BYTES`` as the threshold (0 turns it off). Offloaded files are owned by ``user_id`` and carry the same metadata - as an upload, so the caller can link them to the thread and they are deleted with it. + as an upload (tenant values included), so the caller can link them to the thread and + they are deleted with it. Raises: - ClientInputError: The blob is not valid base64 or exceeds ``MEDIA_MAX_SIZE_MB``. + ClientInputError: The blob is not valid base64 or exceeds ``MEDIA_MAX_SIZE_MB``, + or a blob must be stored while tenant columns are declared and ``user`` + lacks a value (nothing to scope the offloaded file to). """ threshold = self._settings.MEDIA_INLINE_MAX_BYTES if threshold <= 0: return - store = _OwnedStore(self.store, user_id, self.supports_links) + tenant = self.caller_tenant(user) if user_id else {} + store = _OwnedStore(self.store, user_id, self.supports_links, tenant) try: for message in messages: await ensure_media_offloaded( @@ -523,11 +643,15 @@ async def _modify_metadata( updates = fn(dict(metadata)) return await self._update_metadata(key, updates) if updates else True - async def link_threads(self, keys: list[str], thread_id: str, user_id: str) -> None: + async def link_threads( + self, keys: list[str], thread_id: str, user_id: str, user: dict[str, Any] | None = None + ) -> None: """Record ``thread_id`` on each file the caller owns that an accepted message references. - Never raises: a failure only means the file is not tracked, so it is never auto-deleted. - Files without a ``threads`` list (uploaded before link tracking) are left alone. + Owning means the same ``user_id`` and, with tenant columns declared, the same tenant + values (``user``). Never raises: a failure only means the file is not tracked, so it + is never auto-deleted. Files without a ``threads`` list (uploaded before link + tracking) are left alone. """ if not keys: return @@ -539,13 +663,14 @@ async def link_threads(self, keys: list[str], thread_id: str, user_id: str) -> N "are not linked to threads and never deleted automatically." ) return + tenant = self.caller_tenant(user) for key in dict.fromkeys(keys): try: def add_link(metadata: dict[str, Any]) -> dict[str, Any] | None: threads = metadata.get(THREADS_KEY) if ( - str(metadata.get("owner_id")) != str(user_id) + not self._owned_by(metadata, user_id, tenant) or not isinstance(threads, list) or thread_id in threads ): @@ -591,9 +716,18 @@ def drop_link(metadata: dict[str, Any]) -> dict[str, Any] | None: logger.warning("Could not release file %s from a thread: %s", log_safe(key), exc) return failed - async def release_thread(self, thread_id: str, user_id: str | None) -> list[str]: + async def release_thread( + self, + thread_id: str, + user_id: str | None, + tenant: dict[str, str | None] | None = None, + ) -> list[str]: """Release every file ``user_id`` linked to ``thread_id``, whatever the messages say. + ``tenant`` is the owner's tenant values (canonical text per declared column); only + files recording them are released. None with tenant columns declared matches nothing. + A thread delete calls this for the thread's owner and for the deleter. + ``unlink_threads`` works from the keys a thread's messages reference. A file linked through a message that is gone since (cleared state, replaced messages) would stay linked to a deleted thread forever; this finds those by scanning the store's metadata @@ -604,6 +738,8 @@ async def release_thread(self, thread_id: str, user_id: str | None) -> list[str] """ if not self.supports_links or user_id is None: return [] + if tenant is None: + tenant = dict.fromkeys(self.tenant_columns()) try: all_keys = await self._keys_of(user_id) except NotImplementedError: @@ -625,7 +761,7 @@ async def release_thread(self, thread_id: str, user_id: str | None) -> list[str] metadata is not None and isinstance(threads, list) and thread_id in threads - and str(metadata.get("owner_id")) == str(user_id) + and self._owned_by(metadata, user_id, tenant) ): linked.append(key) failed.extend(await self.unlink_threads(linked, thread_id)) @@ -677,10 +813,17 @@ async def _forget_cached(self, file_id: str, mime_type: str | None) -> None: ) async def list_files( - self, user_id: str | None, offset: int = 0, limit: int = 50 + self, + user_id: str | None, + offset: int = 0, + limit: int = 50, + user: dict[str, Any] | None = None, ) -> tuple[list[dict[str, Any]], bool]: """One page of the files ``user_id`` owns, newest first, and whether more follow. + With tenant columns declared, only files recording the caller's tenant values + (``user``) are listed. + Files with no recorded owner are not listed for an identified user. With no ``user_id`` (authentication off) every file is listed, matching ``ensure_can_access``. Reads each stored file's metadata, so the cost grows with the number of files in the store. @@ -689,6 +832,7 @@ async def list_files( NotImplementedError: The media store cannot enumerate its files. """ keys = await self._keys_of(user_id) + tenant = self.caller_tenant(user) if user_id else {} owned: list[dict[str, Any]] = [] for key in keys: try: @@ -698,7 +842,7 @@ async def list_files( continue if not meta: continue - if user_id and str(meta.get("owner_id")) != str(user_id): + if user_id and not self._owned_by(meta, user_id, tenant): continue owned.append( { @@ -715,10 +859,12 @@ async def list_files( page = owned[offset : offset + limit] return page, offset + limit < len(owned) - async def get_file(self, file_id: str, user_id: str | None = None) -> tuple[bytes, str]: + async def get_file( + self, file_id: str, user_id: str | None = None, user: dict[str, Any] | None = None + ) -> tuple[bytes, str]: """Retrieve file bytes and MIME type.""" if user_id: - await self.ensure_can_access(file_id, user_id) + await self.ensure_can_access(file_id, user_id, user) return await self.store.retrieve(file_id) async def local_file(self, file_id: str) -> tuple[Path, str] | None: @@ -744,10 +890,12 @@ async def local_file(self, file_id: str) -> tuple[Path, str] | None: return None return (path, metadata["mime_type"]) if path.is_file() else None - async def get_file_info(self, file_id: str, user_id: str | None = None) -> dict[str, Any]: + async def get_file_info( + self, file_id: str, user_id: str | None = None, user: dict[str, Any] | None = None + ) -> dict[str, Any]: """Return metadata about a stored file.""" if user_id: - await self.ensure_can_access(file_id, user_id) + await self.ensure_can_access(file_id, user_id, user) metadata = await self.store.get_metadata(file_id) if metadata is None: raise KeyError(f"File not found: {file_id}") diff --git a/tenxgraph_api/src/app/routers/media/router.py b/tenxgraph_api/src/app/routers/media/router.py index f2dd7f42..41e3858e 100644 --- a/tenxgraph_api/src/app/routers/media/router.py +++ b/tenxgraph_api/src/app/routers/media/router.py @@ -15,7 +15,7 @@ from tenxgraph_api.src.app.core.auth.request_config import client_error_detail from tenxgraph_api.src.app.core.config.media_settings import get_media_settings from tenxgraph_api.src.app.core.utils.log_sanitizer import log_safe -from tenxgraph_api.src.app.routers.media import MediaService +from tenxgraph_api.src.app.routers.media import MediaService, MissingTenantError from tenxgraph_api.src.app.routers.media.schemas import ( FileAccessUrlResponse, FileDeleteResponse, @@ -213,7 +213,15 @@ async def upload_file( try: # Record the uploader as the owner, so the file cannot later be read by a # different user who knows the id. - result = await service.upload_file(data, file.filename, mime, user_id=user.get("user_id")) + result = await service.upload_file( + data, file.filename, mime, user_id=user.get("user_id"), user=user + ) + except MissingTenantError: + # Tenant columns are declared but the identity carries no value: the file could not + # be scoped to a tenant, so it is refused rather than stored unscoped. + raise HTTPException( + status_code=403, detail="The authenticated user has no tenant value for files" + ) except ValueError as exc: raise HTTPException(status_code=413, detail=client_error_detail(exc)) @@ -243,7 +251,7 @@ async def list_files( ): user_id = user.get("user_id") try: - items, has_more = await service.list_files(user_id, offset, limit) + items, has_more = await service.list_files(user_id, offset, limit, user=user) except NotImplementedError: raise HTTPException(status_code=501, detail="The configured media store cannot list files") payload = FileListResponse( @@ -271,12 +279,12 @@ async def get_file( try: # Authentication alone is not enough: without this, ANY authenticated user # who knew a file_id could download another user's file. - await service.ensure_can_access(file_id, user.get("user_id")) + await service.ensure_can_access(file_id, user.get("user_id"), user) local = await service.local_file(file_id) if local is not None: # stream from disk instead of loading the whole file path, mime_type = local return _file_download_response(file_id, path, mime_type) - data, mime_type = await service.get_file(file_id, user.get("user_id")) + data, mime_type = await service.get_file(file_id, user.get("user_id"), user) except KeyError: raise HTTPException(status_code=404, detail="File not found") except PermissionError: @@ -301,8 +309,8 @@ async def get_file_info( user: dict[str, Any] = Depends(RequirePermission("files", "read")), ): try: - await service.ensure_can_access(file_id, user.get("user_id")) - info = await service.get_file_info(file_id, user.get("user_id")) + await service.ensure_can_access(file_id, user.get("user_id"), user) + info = await service.get_file_info(file_id, user.get("user_id"), user) except KeyError: raise HTTPException(status_code=404, detail="File not found") except PermissionError: @@ -326,7 +334,7 @@ async def get_file_access_url( # Especially important here: this hands out a *signed direct URL*, which # bypasses the API entirely. Handing one to a non-owner would leak the file # even after this check was added elsewhere. - await service.ensure_can_access(file_id, user.get("user_id")) + await service.ensure_can_access(file_id, user.get("user_id"), user) info = await service.get_file_info(file_id) except KeyError: raise HTTPException(status_code=404, detail="File not found") @@ -361,7 +369,7 @@ async def delete_file( user: dict[str, Any] = Depends(RequirePermission("files", "delete")), ): try: - await service.ensure_can_access(file_id, user.get("user_id")) + await service.ensure_can_access(file_id, user.get("user_id"), user) deleted = await service.delete_file(file_id) except (KeyError, PermissionError, ValueError): # 404, not 403: do not confirm that someone else's file_id exists. diff --git a/tenxgraph_api/src/app/routers/store/router.py b/tenxgraph_api/src/app/routers/store/router.py index 77df685f..a3ad39d2 100644 --- a/tenxgraph_api/src/app/routers/store/router.py +++ b/tenxgraph_api/src/app/routers/store/router.py @@ -100,7 +100,7 @@ async def list_memories( cfg = payload.config or {} opts = payload.options result = await service.list_memories( - cfg, user, limit=payload.limit, options=opts, offset=payload.offset + cfg, user, limit=payload.limit, options=opts, offset=payload.offset, filters=payload.filters ) return success_response(result, request) diff --git a/tenxgraph_api/src/app/routers/store/schemas/store_schemas.py b/tenxgraph_api/src/app/routers/store/schemas/store_schemas.py index ebba2a35..c5dbdc89 100644 --- a/tenxgraph_api/src/app/routers/store/schemas/store_schemas.py +++ b/tenxgraph_api/src/app/routers/store/schemas/store_schemas.py @@ -89,13 +89,13 @@ class SearchMemorySchema(BaseConfigSchema): default=None, description="Additional store-specific filters.", ) - retrieval_strategy: RetrievalStrategy = Field( - default=RetrievalStrategy.SIMILARITY, - description="Retrieval strategy used by the backend store.", + retrieval_strategy: RetrievalStrategy | None = Field( + default=None, + description="Retrieval strategy. Omit to use the store's configured default.", ) - distance_metric: DistanceMetric = Field( - default=DistanceMetric.COSINE, - description="Distance metric applied during similarity search.", + distance_metric: DistanceMetric | None = Field( + default=None, + description="Distance metric. Omit to use the store's configured default.", ) max_tokens: int = Field( default=4000, @@ -138,6 +138,10 @@ class ListMemoriesSchema(BaseConfigSchema): le=MAX_LIST_OFFSET, description="Number of memories to skip, for paging. Use next_offset from the response.", ) + filters: BoundedDict | None = Field( + default=None, + description="Additional filters that narrow the list; the isolation scope still applies.", + ) class ForgetMemorySchema(BaseConfigSchema): diff --git a/tenxgraph_api/src/app/routers/store/services/store_service.py b/tenxgraph_api/src/app/routers/store/services/store_service.py index 33e0579f..be9d5f1a 100644 --- a/tenxgraph_api/src/app/routers/store/services/store_service.py +++ b/tenxgraph_api/src/app/routers/store/services/store_service.py @@ -8,6 +8,10 @@ from tenxgraph.storage.store import BaseStore from tenxgraph_api.src.app.core import logger +from tenxgraph_api.src.app.core.auth.client_filters import ( + check_store_filters, + filter_errors_as_client_errors, +) from tenxgraph_api.src.app.core.auth.request_config import client_config from tenxgraph_api.src.app.routers.store.schemas.store_schemas import ( ForgetMemorySchema, @@ -32,15 +36,15 @@ def __init__(self) -> None: # Memory fields the store owns. Client metadata is kept, but these keys are dropped from it: -# user_id and thread_id decide who sees a memory, and memory_id is the id Mem0 reports back. +# user_id and thread_id decide who sees a memory, and memory_id is the memory's own id. SYSTEM_METADATA_KEYS = frozenset( {"content", "user_id", "thread_id", "memory_type", "category", "timestamp", "memory_id"} ) -# Store keyword options a client may set. The built-in stores read only these (``infer`` in -# Mem0Store.astore, ``replace_metadata`` in QdrantStore.aupdate). Anything else is dropped so a -# client cannot pass arbitrary keyword arguments to the store; ``memory_id`` is never allowed -# because ``astore`` upserts, so a client-chosen id could replace an existing memory. +# Store keyword options a client may set (``replace_metadata`` is read by QdrantStore.aupdate). +# Anything else is dropped so a client cannot pass arbitrary keyword arguments to the store; +# ``memory_id`` is never allowed because ``astore`` upserts, so a client-chosen id could replace +# an existing memory. CLIENT_OPTION_KEYS = frozenset({"infer", "replace_metadata"}) @@ -131,20 +135,23 @@ async def search_memories( store = self._get_store() cfg = self._config(payload.config, user) options = _client_options(payload.options) - - results = await store.asearch( - cfg, - payload.query, - memory_type=payload.memory_type, - category=payload.category, - limit=payload.limit, - score_threshold=payload.score_threshold, - filters=_client_filters(payload.filters), - retrieval_strategy=payload.retrieval_strategy, - distance_metric=payload.distance_metric, - max_tokens=payload.max_tokens, - **options, - ) + # Validated before anything runs (no embedding call for a bad filter); 422 when bad. + check_store_filters(store, payload.filters) + + with filter_errors_as_client_errors(): + results = await store.asearch( + cfg, + payload.query, + memory_type=payload.memory_type, + category=payload.category, + limit=payload.limit, + score_threshold=payload.score_threshold, + filters=_client_filters(payload.filters), + retrieval_strategy=payload.retrieval_strategy, + distance_metric=payload.distance_metric, + max_tokens=payload.max_tokens, + **options, + ) return MemorySearchResponseSchema(results=results) async def get_memory( @@ -166,12 +173,21 @@ async def list_memories( limit: int = 100, options: dict[str, Any] | None = None, offset: int = 0, + filters: dict[str, Any] | None = None, ) -> MemoryListResponseSchema: """One page of memories. The store has no offset, so it is asked for ``offset + limit + 1`` rows and the page is cut from those; the extra row only tells ``has_more``.""" store = self._get_store() cfg = self._config(config, user) - rows = await store.aget_all(cfg, limit=offset + limit + 1, **_client_options(options)) + check_store_filters(store, filters) + list_kwargs: dict[str, Any] = {} + client_filters = _client_filters(filters) + if client_filters: + list_kwargs["filters"] = client_filters + with filter_errors_as_client_errors(): + rows = await store.aget_all( + cfg, limit=offset + limit + 1, **list_kwargs, **_client_options(options) + ) page = list(rows[offset : offset + limit]) has_more = len(rows) > offset + limit return MemoryListResponseSchema( @@ -219,6 +235,7 @@ async def forget_memory( store = self._get_store() cfg = self._config(payload.config, user) options = _client_options(payload.options) + check_store_filters(store, payload.filters) forget_kwargs: dict[str, Any] = { "memory_type": payload.memory_type, "category": payload.category, @@ -227,5 +244,6 @@ async def forget_memory( # Remove None values before forwarding to the store forget_kwargs = {k: v for k, v in forget_kwargs.items() if v is not None} forget_kwargs.update(options) - result = await store.aforget_memory(cfg, **forget_kwargs) + with filter_errors_as_client_errors(): + result = await store.aforget_memory(cfg, **forget_kwargs) return MemoryOperationResponseSchema(success=True, data=result) diff --git a/tests/integration_tests/test_media_tenant_isolation.py b/tests/integration_tests/test_media_tenant_isolation.py new file mode 100644 index 00000000..676a76b5 --- /dev/null +++ b/tests/integration_tests/test_media_tenant_isolation.py @@ -0,0 +1,334 @@ +"""Uploaded files are scoped by tenant columns as well as ``user_id``. + +The same ``user_id`` in two tenants (``alice`` in ``org-a`` and in ``org-b``) is two identities: +neither may list, read, sign, delete, reference or link the other's files, and each has its own +quota. The tenant values come from the authenticated identity (``config["user"]``) for the +tenant columns the bound checkpointer declares; a file records them at upload. + +Also covered: a legacy file with no recorded tenant values is denied while tenant columns are +declared, typed columns compare after conversion (``uuid`` case), nothing changes without +tenant columns, and deleting a member's thread as an org admin releases the member's files. +""" + +# ruff: noqa: S101, PLR2004, SLF001 + +from __future__ import annotations + +import json +from pathlib import Path +from typing import Any +from uuid import UUID + +import anyio +import pytest +from fastapi.testclient import TestClient +from injectq import InjectQ +from tenxgraph.core.graph import CompiledGraph, StateGraph +from tenxgraph.core.state import AgentState, Message, TextBlock +from tenxgraph.storage.checkpointer import BaseCheckpointer, InMemoryCheckpointer +from tenxgraph.storage.columns import parse_columns +from tenxgraph.utils.constants import END +from tenxgraph.utils.thread_info import ThreadInfo + +from tenxgraph_api.src.app.core.auth.authorization import AuthorizationBackend +from tenxgraph_api.src.app.core.auth.base_auth import BaseAuth +from tenxgraph_api.src.app.core.auth.request_config import ClientInputError +from tenxgraph_api.src.app.core.config.graph_config import GraphConfig +from tenxgraph_api.src.app.core.config.media_settings import MediaSettings, MediaStorageType +from tenxgraph_api.src.app.routers.checkpointer.router import router as checkpointer_router +from tenxgraph_api.src.app.routers.graph.router import router as graph_router +from tenxgraph_api.src.app.routers.graph.services.multimodal_preprocessor import ( + check_media_references, +) +from tenxgraph_api.src.app.routers.media import TENANT_KEY, MediaService +from tenxgraph_api.src.app.routers.media.router import router as media_router + +from .conftest import build_app, make_client +from .test_tenant_isolation import TenantAuth, TenantAuthz, TenantCheckpointer, _as, _seed_cfg + + +ALICE_A = _as("alice", "org-a") +ALICE_B = _as("alice", "org-b") +SECRET_A = b"secret-of-alice-in-org-a" + + +class _Harness: + """Media, graph and checkpointer routes over a checkpointer with ``org_id`` declared.""" + + def __init__(self, tmp_path: Path, checkpointer: Any = None, **media_settings: Any) -> None: + settings = MediaSettings( + _env_file=None, + MEDIA_STORAGE_TYPE=MediaStorageType.MEMORY, + MEDIA_REQUIRE_OWNER=True, + **media_settings, + ) # type: ignore[call-arg] + self.checkpointer = checkpointer if checkpointer is not None else TenantCheckpointer() + self.media = MediaService(settings=settings, checkpointer=self.checkpointer) + + config_file = tmp_path / "10xgraph.json" + config_file.write_text( + json.dumps({"agent": "x:y", "auth": {"method": "custom", "path": "x:y"}}) + ) + config = GraphConfig(str(config_file)) + + graph = StateGraph(AgentState()) + graph.add_node( + "MAIN", lambda state, config: Message(role="assistant", content=[TextBlock(text="ok")]) + ) + graph.set_entry_point("MAIN") + graph.add_edge("MAIN", END) + compiled = graph.compile(checkpointer=self.checkpointer) + + authz = TenantAuthz(checkpointer=self.checkpointer) + bindings = { + MediaService: self.media, + BaseAuth: TenantAuth(), + AuthorizationBackend: authz, + GraphConfig: config, + BaseCheckpointer: self.checkpointer, + CompiledGraph: compiled, + } + active = InjectQ.get_instance() + for kind, value in bindings.items(): + active.bind_instance(kind, value, allow_none=True) + app = build_app( + routers=[media_router, graph_router, checkpointer_router], + authz=authz, + checkpointer=self.checkpointer, + extra_bindings=bindings, + ) + self.client: TestClient = make_client(app) + + def upload(self, headers: dict[str, str], data: bytes = SECRET_A) -> Any: + return self.client.post( + "/v1/files/upload", + files={"file": ("note.txt", data, "text/plain")}, + headers=headers, + ) + + def upload_key(self, headers: dict[str, str], data: bytes = SECRET_A) -> str: + resp = self.upload(headers, data) + assert resp.status_code == 200, resp.text + return resp.json()["data"]["file_id"] + + def metadata(self, key: str) -> dict[str, Any] | None: + return anyio.run(self.media.store.get_metadata, key) + + +@pytest.fixture +def h(tmp_path): + with InjectQ.test_mode(): + yield _Harness(tmp_path) + + +def _image(key: str) -> dict: + return { + "role": "user", + "content": [{"type": "image", "media": {"kind": "url", "url": f"graph://media/{key}"}}], + } + + +def _listed(h: _Harness, headers: dict[str, str]) -> list[str]: + resp = h.client.get("/v1/files", headers=headers) + assert resp.status_code == 200, resp.text + return [f["file_id"] for f in resp.json()["data"]["files"]] + + +# --------------------------------------------------------------------------- media routes + + +def test_upload_records_the_callers_tenant_values(h): + key = h.upload_key(ALICE_A) + meta = h.metadata(key) + assert meta["owner_id"] == "alice" + assert meta[TENANT_KEY] == {"org_id": "org-a"} + + +def test_same_user_id_in_another_tenant_cannot_reach_the_file(h): + key = h.upload_key(ALICE_A) + for path in (f"/v1/files/{key}", f"/v1/files/{key}/info", f"/v1/files/{key}/url"): + resp = h.client.get(path, headers=ALICE_B) + assert resp.status_code == 404, f"{path} -> {resp.status_code}" + assert SECRET_A.decode() not in resp.text + assert h.client.delete(f"/v1/files/{key}", headers=ALICE_B).status_code == 404 + assert _listed(h, ALICE_B) == [] + + # Within the tenant every route works, and the delete above did nothing. + own = h.client.get(f"/v1/files/{key}", headers=ALICE_A) + assert own.status_code == 200 and own.content == SECRET_A + assert h.client.get(f"/v1/files/{key}/info", headers=ALICE_A).status_code == 200 + assert h.client.get(f"/v1/files/{key}/url", headers=ALICE_A).status_code == 200 + assert _listed(h, ALICE_A) == [key] + assert h.client.delete(f"/v1/files/{key}", headers=ALICE_A).status_code == 200 + assert h.metadata(key) is None + + +def test_listing_keeps_the_two_tenants_apart(h): + key_a = h.upload_key(ALICE_A) + key_b = h.upload_key(ALICE_B, b"secret-of-alice-in-org-b") + assert _listed(h, ALICE_A) == [key_a] + assert _listed(h, ALICE_B) == [key_b] + + +def test_upload_without_a_tenant_value_is_refused(h): + resp = h.upload(_as("alice")) + assert resp.status_code == 403 + assert h.client.get("/v1/files", headers=_as("alice")).json()["data"]["files"] == [] + + +def test_a_caller_without_a_tenant_value_cannot_read_the_file(h): + key = h.upload_key(ALICE_A) + assert h.client.get(f"/v1/files/{key}", headers=_as("alice")).status_code == 404 + + +def test_quotas_are_counted_per_tenant(tmp_path): + with InjectQ.test_mode(): + h = _Harness(tmp_path, MEDIA_MAX_FILES_PER_USER=1) + h.upload_key(ALICE_A) + # The same user_id in org-b has its own quota. + h.upload_key(ALICE_B) + over = h.upload(ALICE_A) + assert over.status_code == 413 + assert "Upload limit" in over.text + + +# --------------------------------------------------------------------------- references + + +def test_invoke_cannot_reference_the_other_tenants_file(h): + key = h.upload_key(ALICE_A) + body = {"messages": [_image(key)]} + denied = h.client.post("/v1/graph/invoke", json=body, headers=ALICE_B) + assert denied.status_code in (400, 422), denied.text + assert SECRET_A.decode() not in denied.text + allowed = h.client.post("/v1/graph/invoke", json=body, headers=ALICE_A) + assert allowed.status_code == 200, allowed.text + + +def test_stream_cannot_reference_the_other_tenants_file(h): + key = h.upload_key(ALICE_A) + body = {"messages": [_image(key)]} + denied = h.client.post("/v1/graph/stream", json=body, headers=ALICE_B) + assert "File not found" in denied.text or denied.status_code in (400, 422) + assert SECRET_A.decode() not in denied.text + + +def test_put_state_cannot_reference_the_other_tenants_file(h): + key = h.upload_key(ALICE_A) + cfg = _seed_cfg("t-b", "alice", "org-b") + anyio.run(h.checkpointer.aput_thread, cfg, ThreadInfo(thread_id="t-b", user_id="alice")) + body = {"state": {"context": [_image(key)]}} + resp = h.client.put("/v1/threads/t-b/state", json=body, headers=ALICE_B) + assert resp.status_code in (400, 422), resp.text + + +@pytest.mark.asyncio +async def test_reference_checks_and_links_compare_tenant_values(h): + key = h.upload_key(ALICE_A) + message = Message.model_validate(_image(key)) + user_a = {"user_id": "alice", "org_id": "org-a"} + user_b = {"user_id": "alice", "org_id": "org-b"} + + with pytest.raises(ClientInputError, match="not found"): + await check_media_references([message], h.media, "alice", user_b) + with pytest.raises(ClientInputError, match="not found"): + await check_media_references([message], h.media, "alice") # no identity: no tenant + await check_media_references([message], h.media, "alice", user_a) + + await h.media.link_threads([key], "t1", "alice", user_b) + assert (await h.media.store.get_metadata(key))["threads"] == [] + await h.media.link_threads([key], "t1", "alice", user_a) + assert (await h.media.store.get_metadata(key))["threads"] == ["t1"] + + +# --------------------------------------------------------------------------- legacy and types + + +@pytest.mark.asyncio +async def test_a_file_without_tenant_values_is_denied_under_tenant_columns(h): + """Uploaded before tenant tracking: no recorded tenant, so no tenant may use it.""" + key = await h.media.store.store(b"legacy", "text/plain", metadata={"owner_id": "alice"}) + user_a = {"user_id": "alice", "org_id": "org-a"} + with pytest.raises(PermissionError): + await h.media.ensure_can_access(key, "alice", user_a) + assert await h.media.list_files("alice", user=user_a) == ([], False) + + +@pytest.mark.asyncio +async def test_an_ownerless_file_is_denied_under_tenant_columns_even_in_development(tmp_path): + with InjectQ.test_mode(): + h = _Harness(tmp_path) + h.media._settings.MEDIA_REQUIRE_OWNER = False + key = await h.media.store.store(b"legacy", "text/plain", metadata={}) + with pytest.raises(PermissionError): + await h.media.ensure_can_access(key, "alice", {"user_id": "alice", "org_id": "org-a"}) + + +class _UuidCheckpointer(InMemoryCheckpointer): + def __init__(self) -> None: + super().__init__() + self.columns = parse_columns({"org_id": "uuid"}, table="threads") + self.tenant_column_names = self.columns.tenant_names + + +@pytest.mark.asyncio +async def test_typed_tenant_columns_compare_after_conversion(tmp_path): + org = "6F9619FF-8B86-D011-B42D-00CF4FC964FF" + with InjectQ.test_mode(): + h = _Harness(tmp_path, checkpointer=_UuidCheckpointer()) + result = await h.media.upload_file( + b"x", "a.txt", "text/plain", user_id="alice", user={"user_id": "alice", "org_id": org} + ) + key = result["file_id"] + meta = await h.media.store.get_metadata(key) + assert meta[TENANT_KEY] == {"org_id": str(UUID(org))} + # Lower case and UUID objects are the same tenant; a malformed value is none. + await h.media.ensure_can_access(key, "alice", {"user_id": "alice", "org_id": org.lower()}) + await h.media.ensure_can_access(key, "alice", {"user_id": "alice", "org_id": UUID(org)}) + with pytest.raises(PermissionError): + await h.media.ensure_can_access(key, "alice", {"user_id": "alice", "org_id": "nope"}) + + +@pytest.mark.asyncio +async def test_no_tenant_columns_records_no_tenant_and_checks_the_owner_only(tmp_path): + with InjectQ.test_mode(): + h = _Harness(tmp_path, checkpointer=InMemoryCheckpointer()) + result = await h.media.upload_file(b"x", "a.txt", "text/plain", user_id="alice") + key = result["file_id"] + assert TENANT_KEY not in await h.media.store.get_metadata(key) + await h.media.ensure_can_access(key, "alice") + await h.media.ensure_can_access(key, "alice", {"user_id": "alice", "org_id": "any"}) + with pytest.raises(PermissionError): + await h.media.ensure_can_access(key, "bob") + + +# --------------------------------------------------------------------------- thread delete + + +def test_org_admin_thread_delete_releases_the_members_files(h): + """The admin's own id owns nothing here: the release follows the thread's owner.""" + key = h.upload_key(ALICE_A) + cfg = _seed_cfg("t-a", "alice", "org-a") + anyio.run(h.checkpointer.aput_thread, cfg, ThreadInfo(thread_id="t-a", user_id="alice")) + # Linked through a message that is gone since: only the metadata scan finds it. + anyio.run(h.media.link_threads, [key], "t-a", "alice", {"user_id": "alice", "org_id": "org-a"}) + assert h.metadata(key)["threads"] == ["t-a"] + + admin = _as("carol", "org-a", "org_admin") + resp = h.client.request("DELETE", "/v1/threads/t-a", json={}, headers=admin) + assert resp.status_code == 200, resp.text + assert h.metadata(key) is None + + +def test_thread_delete_keeps_the_same_user_ids_file_in_another_tenant(h): + """A file of alice@org-b linked to the same thread id is not alice@org-a's to release.""" + key_b = h.upload_key(ALICE_B, b"secret-of-alice-in-org-b") + anyio.run( + h.media.link_threads, [key_b], "t-a", "alice", {"user_id": "alice", "org_id": "org-b"} + ) + cfg = _seed_cfg("t-a", "alice", "org-a") + anyio.run(h.checkpointer.aput_thread, cfg, ThreadInfo(thread_id="t-a", user_id="alice")) + + resp = h.client.request("DELETE", "/v1/threads/t-a", json={}, headers=ALICE_A) + assert resp.status_code == 200, resp.text + assert h.metadata(key_b)["threads"] == ["t-a"] diff --git a/tests/integration_tests/test_server_thread_ids.py b/tests/integration_tests/test_server_thread_ids.py index 31823905..9245622a 100644 --- a/tests/integration_tests/test_server_thread_ids.py +++ b/tests/integration_tests/test_server_thread_ids.py @@ -2,7 +2,8 @@ REST invoke/stream and the checkpointer routes refuse an unknown client-named id (the same 403 as a taken id, so existence is not revealed); omitting the id still starts a thread. -AG-UI and ``/v1/graph/live`` pass ``client_thread_ids`` and keep accepting frontend ids. +AG-UI and ``/v1/graph/live`` follow the same rule. The ``client_thread_ids`` authorize context +flag is kept for custom callers; no built-in route passes it. """ # ruff: noqa: S101, PLR2004 diff --git a/tests/integration_tests/test_tenant_isolation.py b/tests/integration_tests/test_tenant_isolation.py new file mode 100644 index 00000000..63b000a6 --- /dev/null +++ b/tests/integration_tests/test_tenant_isolation.py @@ -0,0 +1,737 @@ +"""Tenant isolation through the API: per-user isolation scopes, thread scopes and filters. + +Two tenants hold a user with the same ``user_id`` (``alice`` in ``org-a`` and in ``org-b``). +Every route family must keep them apart, the tenant always coming from the authenticated +identity (``config["user"]``) and never from request config or ``filters``. + +The checkpointer and store here are small in-memory stand-ins for ``PgCheckpointer`` / +``PgVectorStore`` with ``tenant_columns=["org_id"]``. They resolve values and isolation with +the real core helpers (``ColumnSet.resolve``, ``isolation_columns``, ``isolation_conditions``, +``validate_filters``), so what is tested is the API's side of the core contract. +""" + +from __future__ import annotations + +import json +from typing import Any +from unittest.mock import AsyncMock +from uuid import uuid4 + +import anyio +import pytest +from fastapi import APIRouter, Depends, FastAPI, HTTPException, Request, Response +from injectq import InjectQ +from injectq.integrations.fastapi import setup_fastapi +from tenxgraph.core.authz import build_authz, isolation_columns +from tenxgraph.core.state import AgentState, Message +from tenxgraph.storage.checkpointer import BaseCheckpointer, InMemoryCheckpointer +from tenxgraph.storage.columns import isolation_conditions, parse_columns +from tenxgraph.storage.sql_filter import FilterError, FilterSchema, validate_filters +from tenxgraph.storage.store import BaseStore, MemorySearchResult +from tenxgraph.utils.thread_info import ThreadInfo + +from tenxgraph_api.src.app.core.auth.authorization import ( + AuthorizationBackend, + OwnershipAuthorizationBackend, +) +from tenxgraph_api.src.app.core.auth.base_auth import BaseAuth +from tenxgraph_api.src.app.core.auth.permissions import RequirePermission +from tenxgraph_api.src.app.core.config.graph_config import GraphConfig +from tenxgraph_api.src.app.core.config.settings import get_settings +from tenxgraph_api.src.app.core.config.setup_middleware import setup_middleware +from tenxgraph_api.src.app.core.exceptions.handle_errors import init_errors_handler +from tenxgraph_api.src.app.routers.checkpointer.router import router as checkpointer_router +from tenxgraph_api.src.app.routers.store.router import router as store_router + +from .conftest import make_client + + +HTTP_OK = 200 +HTTP_FORBIDDEN = 403 +HTTP_UNPROCESSABLE = 422 + + +# --------------------------------------------------------------------------- +# Auth, authorization and storage stand-ins +# --------------------------------------------------------------------------- + + +class TenantAuth(BaseAuth): + """Identity from headers: ``X-Test-User``, ``X-Test-Org`` and ``X-Test-Role``.""" + + def authenticate(self, request: Request, response: Response, credential: Any) -> dict: + user_id = request.headers.get("X-Test-User") + if not user_id: + raise HTTPException(status_code=401, detail="X-Test-User header required") + user: dict[str, Any] = {"user_id": user_id} + if org := request.headers.get("X-Test-Org"): + user["org_id"] = org + if role := request.headers.get("X-Test-Role"): + user["role"] = role + return user + + +class TenantAuthz(OwnershipAuthorizationBackend): + """Owner scope for everyone; an org admin sees every thread and memory of the org.""" + + def isolation_scope(self, user=None): + if user and user.get("role") == "org_admin": + return ["org_id"] + return "owner" + + +class _AuthOn: + def auth_config(self) -> str: + return "custom" + + +def _seed_cfg(thread_id: str, user_id: str, org_id: str) -> dict[str, Any]: + user = {"user_id": user_id, "org_id": org_id} + user["authz"] = build_authz(user_id, scope="owner") + return {"thread_id": thread_id, "user_id": user_id, "user": user} + + +class TenantCheckpointer(InMemoryCheckpointer): + """``InMemoryCheckpointer`` plus ``org_id`` (tenant) and ``plan`` (extra) columns.""" + + def __init__(self) -> None: + super().__init__() + self.columns = parse_columns(["org_id"], ["plan"], table="threads") + self.tenant_column_names = self.columns.tenant_names + self.filter_schema = FilterSchema.for_threads(self.columns) + self.rows: dict[str, dict[str, Any]] = {} + self.scope_lookups = 0 + self.list_calls: list[Any] = [] + + async def aput_thread(self, config, thread_info): + # Values come from config["user"] only and are set once, at creation. + values = self.columns.resolve(config, write=True) + self.rows.setdefault(str(config["thread_id"]), values) + return await super().aput_thread(config, thread_info) + + async def aget_thread_scope(self, thread_id): + self.scope_lookups += 1 + owner = await self.aget_thread_owner(thread_id) + if owner is None: + return None + return {"user_id": owner, "org_id": self.rows.get(str(thread_id), {}).get("org_id")} + + def _visible(self, config, thread_id) -> bool: + picked = isolation_columns(config, self.tenant_column_names) + conditions = isolation_conditions( + self.columns.scope_specs(picked), self.columns.resolve(config) + ) + row = self.rows.get(str(thread_id), {}) + return all(row.get(spec.name) == value for spec, value in conditions) + + async def aget_thread(self, config): + if not self._visible(config, config.get("thread_id")): + return None + return await super().aget_thread(config) + + async def aget_state(self, config): + if not self._visible(config, config.get("thread_id")): + return None + return await super().aget_state(config) + + async def aget_state_cache(self, config): + if not self._visible(config, config.get("thread_id")): + return None + return await super().aget_state_cache(config) + + async def alist_messages(self, config, search=None, offset=None, limit=None): + if not self._visible(config, config.get("thread_id")): + return [] + return await super().alist_messages(config, search, offset, limit) + + async def alist_threads(self, config, search=None, offset=None, limit=None, filters=None): + self.list_calls.append(filters) + conditions = validate_filters(filters, self.filter_schema) + threads = await super().alist_threads(config, search, offset, limit) + kept = [] + for thread in threads: + row = self.rows.get(str(thread.thread_id), {}) + if not self._visible(config, thread.thread_id): + continue + if all(c.op == "eq" and row.get(c.column.name) == c.value for c in conditions): + kept.append(thread) + return kept + + +class TenantStore: + """A store with ``org_id`` (tenant) and ``topic`` (extra) columns, like ``PgVectorStore``.""" + + def __init__(self) -> None: + self.columns = parse_columns(["org_id"], ["topic"], table="memories") + self.tenant_column_names = self.columns.tenant_names + self.filter_schema = FilterSchema.for_memories(self.columns) + self.rows: dict[str, dict[str, Any]] = {} + self.calls: list[str] = [] + + def _match(self, config, row) -> bool: + picked = isolation_columns(config, self.tenant_column_names) + if picked is None: + picked = ("user_id", *self.tenant_column_names) + if "user_id" in picked and row["user_id"] != str(config.get("user_id")): + return False + conditions = isolation_conditions( + self.columns.scope_specs(picked), self.columns.resolve(config) + ) + return all(row[spec.name] == value for spec, value in conditions) + + def _narrow(self, rows, filters): + filters = filters or {} + columns = {k: v for k, v in filters.items() if self.filter_schema.get(k) is not None} + conditions = validate_filters(columns, self.filter_schema) + return [r for r in rows if all(r.get(c.column.name) == c.value for c in conditions)] + + @staticmethod + def _result(row) -> MemorySearchResult: + return MemorySearchResult( + id=row["id"], content=row["content"], user_id=row["user_id"], metadata={} + ) + + async def astore(self, config, content, memory_type=None, category=None, metadata=None, **kw): + self.calls.append("astore") + values = self.columns.resolve(config, write=True) + memory_id = str(uuid4()) + self.rows[memory_id] = { + "id": memory_id, + "user_id": str(config["user_id"]), + "content": str(content), + **values, + "topic": (metadata or {}).get("topic"), + } + return memory_id + + async def asearch(self, config, query, *, filters=None, limit=10, **kw): + self.calls.append("asearch") + rows = [r for r in self.rows.values() if self._match(config, r)] + return [self._result(r) for r in self._narrow(rows, filters)][:limit] + + async def aget(self, config, memory_id, **kw): + self.calls.append("aget") + row = self.rows.get(memory_id) + return self._result(row) if row and self._match(config, row) else None + + async def aget_all(self, config, limit=100, **kw): + self.calls.append("aget_all") + return [self._result(r) for r in self.rows.values() if self._match(config, r)][:limit] + + async def aupdate(self, config, memory_id, content, metadata=None, **kw): + self.calls.append("aupdate") + row = self.rows.get(memory_id) + if not row or not self._match(config, row): + return False + row["content"] = str(content) + return True + + async def adelete(self, config, memory_id, **kw): + self.calls.append("adelete") + row = self.rows.get(memory_id) + if not row or not self._match(config, row): + return False + del self.rows[memory_id] + return True + + async def aforget_memory(self, config, filters=None, **kw): + self.calls.append("aforget_memory") + gone = [ + r["id"] for r in self._narrow(self.rows.values(), filters) if self._match(config, r) + ] + for memory_id in gone: + del self.rows[memory_id] + return len(gone) + + +# --------------------------------------------------------------------------- +# App +# --------------------------------------------------------------------------- + + +def _probe_router() -> APIRouter: + """Stand-ins for invoke / stream: the real permission check, then the stamped authz.""" + probe = APIRouter() + + @probe.post("/v1/graph/invoke") + async def _invoke(payload: dict, user: dict = Depends(RequirePermission("graph", "invoke"))): + return {"authz": user.get("authz")} + + @probe.post("/v1/graph/stream") + async def _stream(payload: dict, user: dict = Depends(RequirePermission("graph", "stream"))): + return {"authz": user.get("authz")} + + return probe + + +def _app( + *, + checkpointer: Any = None, + store: Any = None, + authz: AuthorizationBackend | None = None, +) -> tuple[Any, TenantCheckpointer]: + checkpointer = checkpointer if checkpointer is not None else TenantCheckpointer() + app = FastAPI() + setup_middleware(app) + init_errors_handler(app) + container = InjectQ() + container.bind_instance(GraphConfig, _AuthOn()) + container.bind_instance(BaseAuth, TenantAuth()) + container.bind_instance( + AuthorizationBackend, + authz if authz is not None else TenantAuthz(checkpointer=checkpointer), + ) + container.bind_instance(BaseCheckpointer, checkpointer) + container.bind_instance(BaseStore, store if store is not None else TenantStore()) + setup_fastapi(container, app) + for router in (checkpointer_router, store_router, _probe_router()): + app.include_router(router) + return make_client(app), checkpointer + + +def _as(user: str, org: str | None = None, role: str | None = None) -> dict[str, str]: + headers = {"X-Test-User": user} + if org: + headers["X-Test-Org"] = org + if role: + headers["X-Test-Role"] = role + return headers + + +ALICE_A = _as("alice", "org-a") +ALICE_B = _as("alice", "org-b") + + +def _seed_two_tenants(cp: TenantCheckpointer) -> None: + """``alice`` owns ``t-a`` in org-a and ``t-b`` in org-b; ``bob`` owns ``t-bob`` in org-a.""" + for thread_id, user, org, plan in ( + ("t-a", "alice", "org-a", "pro"), + ("t-b", "alice", "org-b", "pro"), + ("t-bob", "bob", "org-a", "free"), + ): + cfg = _seed_cfg(thread_id, user, org) + cfg["user"]["plan"] = plan + anyio.run(cp.aput_thread, cfg, ThreadInfo(thread_id=thread_id, user_id=user)) + anyio.run(cp.aput_state, cfg, AgentState(context=[Message.text_message(f"hi {org}")])) + anyio.run(cp.aput_messages, cfg, [Message.text_message(f"secret of {org}")]) + + +def _listed(client, headers, **params) -> set[str]: + resp = client.get("/v1/threads", headers=headers, params=params) + assert resp.status_code == HTTP_OK, resp.text + return {t["thread_id"] for t in resp.json()["data"]["threads"]} + + +# --------------------------------------------------------------------------- +# Threads: two tenants, the same user_id +# --------------------------------------------------------------------------- + + +def test_thread_listing_keeps_two_tenants_with_the_same_user_id_apart(): + client, cp = _app() + _seed_two_tenants(cp) + + assert _listed(client, ALICE_A) == {"t-a"} + assert _listed(client, ALICE_B) == {"t-b"} + + +@pytest.mark.parametrize( + "path", ["/v1/threads/{t}", "/v1/threads/{t}/state", "/v1/threads/{t}/messages"] +) +def test_thread_reads_refuse_the_other_tenant(path): + client, cp = _app() + _seed_two_tenants(cp) + + assert client.get(path.format(t="t-a"), headers=ALICE_A).status_code == HTTP_OK + assert client.get(path.format(t="t-b"), headers=ALICE_B).status_code == HTTP_OK + assert client.get(path.format(t="t-b"), headers=ALICE_A).status_code == HTTP_FORBIDDEN + assert client.get(path.format(t="t-a"), headers=ALICE_B).status_code == HTTP_FORBIDDEN + + +def test_thread_writes_and_deletes_refuse_the_other_tenant(): + client, cp = _app() + _seed_two_tenants(cp) + + put = client.put("/v1/threads/t-b/state", json={"state": {}}, headers=ALICE_A) + assert put.status_code == HTTP_FORBIDDEN + delete = client.request("DELETE", "/v1/threads/t-b", json={}, headers=ALICE_A) + assert delete.status_code == HTTP_FORBIDDEN + assert "t-b" in cp.rows + assert _listed(client, ALICE_B) == {"t-b"} + + +@pytest.mark.parametrize("route", ["/v1/graph/invoke", "/v1/graph/stream"]) +def test_runs_refuse_a_thread_of_the_other_tenant(route): + client, cp = _app() + _seed_two_tenants(cp) + + assert client.post(route, json={"thread_id": "t-a"}, headers=ALICE_A).status_code == HTTP_OK + assert ( + client.post(route, json={"thread_id": "t-b"}, headers=ALICE_A).status_code == HTTP_FORBIDDEN + ) + assert ( + client.post(route, json={"config": {"thread_id": "t-a"}}, headers=ALICE_B).status_code + == HTTP_FORBIDDEN + ) + + +def test_a_caller_without_the_tenant_value_is_refused(): + """The owner scope picks org_id; an identity without one never matches (fail closed).""" + client, cp = _app() + _seed_two_tenants(cp) + + assert client.get("/v1/threads/t-a", headers=_as("alice")).status_code == HTTP_FORBIDDEN + + +def test_org_admin_scope_reaches_every_thread_of_the_org_only(): + client, cp = _app() + _seed_two_tenants(cp) + admin_a = _as("carol", "org-a", "org_admin") + + assert client.get("/v1/threads/t-a/state", headers=admin_a).status_code == HTTP_OK + assert client.get("/v1/threads/t-bob/messages", headers=admin_a).status_code == HTTP_OK + assert client.get("/v1/threads/t-b/state", headers=admin_a).status_code == HTTP_FORBIDDEN + assert _listed(client, admin_a) == {"t-a", "t-bob"} + + stamped = client.post("/v1/graph/invoke", json={}, headers=admin_a).json()["authz"] + assert stamped["scope"] == ["org_id"] + + +def test_thread_scope_lookups_are_cached(): + client, cp = _app() + _seed_two_tenants(cp) + + for _ in range(4): + client.get("/v1/threads/t-a/state", headers=ALICE_A) + client.get("/v1/threads/t-a/state", headers=ALICE_B) + assert cp.scope_lookups == 1 + + +# --------------------------------------------------------------------------- +# Client config can never choose the tenant +# --------------------------------------------------------------------------- + + +FORGED_CONFIG = { + "user": {"user_id": "alice", "org_id": "org-b"}, + "org_id": "org-b", + "authz": {"user_id": "alice", "scope": "none", "scopes": []}, +} + + +def test_client_config_user_and_tenant_keys_do_not_reach_stored_thread_values(): + client, cp = _app() + _seed_two_tenants(cp) + + resp = client.put( + "/v1/threads/t-new/state", + json={"config": FORGED_CONFIG, "state": {}}, + headers=ALICE_A, + ) + assert resp.status_code == HTTP_OK, resp.text + assert cp.rows["t-new"]["org_id"] == "org-a" + # The forged values reached neither the stored row nor anyone's view of it. + assert client.get("/v1/threads/t-new/state", headers=ALICE_B).status_code == HTTP_FORBIDDEN + assert _listed(client, ALICE_B) == {"t-b"} + assert _listed(client, ALICE_A) == {"t-a", "t-new"} + + +def test_client_config_cannot_widen_a_read(): + client, cp = _app() + _seed_two_tenants(cp) + + resp = client.put( + "/v1/threads/t-a/state", + json={"config": FORGED_CONFIG, "state": {}}, + headers=ALICE_A, + ) + assert resp.status_code == HTTP_OK + # The listing is driven by the identity alone. + assert _listed(client, ALICE_A) == {"t-a"} + + +def test_client_config_tenant_keys_do_not_reach_the_store(): + store = TenantStore() + client, _ = _app(store=store) + + resp = client.post( + "/v1/store/memories", + json={"content": "note", "config": FORGED_CONFIG}, + headers=ALICE_A, + ) + assert resp.status_code == HTTP_OK, resp.text + (row,) = store.rows.values() + assert row["org_id"] == "org-a" + found = client.post( + "/v1/store/search", json={"query": "note", "config": FORGED_CONFIG}, headers=ALICE_B + ) + assert found.status_code == HTTP_OK + assert found.json()["data"]["results"] == [] + + +# --------------------------------------------------------------------------- +# Store routes: two tenants, the same user_id +# --------------------------------------------------------------------------- + + +def test_store_routes_keep_two_tenants_with_the_same_user_id_apart(): + store = TenantStore() + client, _ = _app(store=store) + + created = client.post("/v1/store/memories", json={"content": "a-note"}, headers=ALICE_A) + memory_id = created.json()["data"]["memory_id"] + + def ids(resp) -> list[str]: + data = resp.json()["data"] + return [m["id"] for m in data.get("results", data.get("memories", []))] + + search = {"query": "note"} + assert ids(client.post("/v1/store/search", json=search, headers=ALICE_A)) == [memory_id] + assert ids(client.post("/v1/store/search", json=search, headers=ALICE_B)) == [] + assert ids(client.post("/v1/store/memories/list", json={}, headers=ALICE_B)) == [] + got = client.post(f"/v1/store/memories/{memory_id}", json={}, headers=ALICE_B) + assert got.json()["data"]["memory"] is None + upd = client.put(f"/v1/store/memories/{memory_id}", json={"content": "x"}, headers=ALICE_B) + assert upd.json()["data"]["data"] is False + gone = client.request("DELETE", f"/v1/store/memories/{memory_id}", json={}, headers=ALICE_B) + assert gone.json()["data"]["data"] is False + forget = client.post("/v1/store/memories/forget", json={}, headers=ALICE_B) + assert forget.json()["data"]["data"] == 0 + assert list(store.rows) == [memory_id] + assert store.rows[memory_id]["content"] == "a-note" + + +# --------------------------------------------------------------------------- +# filters on GET /v1/threads +# --------------------------------------------------------------------------- + + +def test_thread_filters_narrow_the_listing(): + client, cp = _app() + _seed_two_tenants(cp) + admin_a = _as("carol", "org-a", "org_admin") + + assert _listed(client, admin_a, filters=json.dumps({"plan": "pro"})) == {"t-a"} + assert _listed(client, admin_a, filters=json.dumps({"plan": {"eq": "free"}})) == {"t-bob"} + assert cp.list_calls[-1] == {"plan": {"eq": "free"}} + assert _listed(client, admin_a, filters="") == {"t-a", "t-bob"} + + +@pytest.mark.parametrize( + ("filters", "message"), + [ + ({"org_id": "org-b"}, "isolation column"), + ({"user_id": "alice"}, "isolation column"), + ({"plan": {"gt": "a"}}, "not supported"), + ({"nope": 1}, "Unknown filter column"), + ({"plan": {"in": []}}, ""), + ], +) +def test_bad_thread_filters_are_422_before_listing(filters, message): + client, cp = _app() + _seed_two_tenants(cp) + + resp = client.get("/v1/threads", headers=ALICE_A, params={"filters": json.dumps(filters)}) + assert resp.status_code == HTTP_UNPROCESSABLE, resp.text + assert message in resp.json()["error"]["message"] + assert cp.list_calls == [] + + +@pytest.mark.parametrize( + "raw", ["{not json", "[1, 2]", '"plan"', "NaN", '{"plan": NaN}', "[" * 5000 + "]" * 5000] +) +def test_unparseable_thread_filters_are_422(raw): + client, cp = _app() + + resp = client.get("/v1/threads", headers=ALICE_A, params={"filters": raw}) + assert resp.status_code == HTTP_UNPROCESSABLE, resp.text + assert "filters" in resp.json()["error"]["message"] + assert "not json" not in resp.json()["error"]["message"] + assert cp.list_calls == [] + + +def test_thread_filters_need_a_checkpointer_with_filterable_columns(): + cp = InMemoryCheckpointer() + client, _ = _app(checkpointer=cp, authz=TenantAuthz(checkpointer=cp)) + + assert client.get("/v1/threads", headers=ALICE_A).status_code == HTTP_OK + resp = client.get( + "/v1/threads", headers=ALICE_A, params={"filters": json.dumps({"plan": "pro"})} + ) + assert resp.status_code == HTTP_UNPROCESSABLE + assert "InMemoryCheckpointer" in resp.json()["error"]["message"] + + +def test_thread_filter_messages_survive_production(monkeypatch): + settings = get_settings() + monkeypatch.setattr(settings, "MODE", "production") + monkeypatch.setattr(settings, "ORIGINS", "https://app.example.com") + client, _ = _app() + + resp = client.get( + "/v1/threads", headers=ALICE_A, params={"filters": json.dumps({"org_id": "x"})} + ) + assert resp.status_code == HTTP_UNPROCESSABLE + assert "isolation column" in resp.json()["error"]["message"] + + +# --------------------------------------------------------------------------- +# filters on the store routes +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("route", "body"), + [ + ("/v1/store/search", {"query": "q", "filters": {"org_id": "org-b"}}), + ("/v1/store/search", {"query": "q", "filters": {"user_id": "bob"}}), + ("/v1/store/search", {"query": "q", "filters": {"topic": {"bogus": 1}}}), + ("/v1/store/search", {"query": "q", "filters": {f"k{i}": i for i in range(11)}}), + ("/v1/store/memories/forget", {"filters": {"org_id": "org-b"}}), + ("/v1/store/memories/forget", {"filters": {"created_at": {"gte": "yesterday"}}}), + ], +) +def test_bad_store_filters_are_422_before_the_store_runs(route, body): + store = TenantStore() + client, _ = _app(store=store) + + resp = client.post(route, json=body, headers=ALICE_A) + assert resp.status_code == HTTP_UNPROCESSABLE, resp.text + assert store.calls == [] + + +def test_store_filters_on_columns_and_metadata_pass_through(): + store = TenantStore() + client, _ = _app(store=store) + for topic in ("billing", "support"): + client.post( + "/v1/store/memories", + json={"content": topic, "metadata": {"topic": topic}}, + headers=ALICE_A, + ) + + resp = client.post( + "/v1/store/search", + json={"query": "q", "filters": {"topic": "billing", "source": "chat"}}, + headers=ALICE_A, + ) + assert resp.status_code == HTTP_OK, resp.text + assert [r["content"] for r in resp.json()["data"]["results"]] == ["billing"] + + +def test_a_filter_error_raised_by_the_store_is_a_422(): + class _Strict(TenantStore): + async def asearch(self, config, query, **kw): + raise FilterError("Unsupported filter on a metadata key") + + client, _ = _app(store=_Strict()) + resp = client.post( + "/v1/store/search", json={"query": "q", "filters": {"x": [1]}}, headers=ALICE_A + ) + assert resp.status_code == HTTP_UNPROCESSABLE + assert "metadata key" in resp.json()["error"]["message"] + + +def test_a_store_without_a_filter_schema_keeps_its_own_filters(): + store = AsyncMock(spec=BaseStore) + store.asearch.return_value = [] + client, _ = _app(store=store) + + resp = client.post( + "/v1/store/search", + json={"query": "q", "filters": {"org_id": "x", "user_id": "bob", "AND": [{"a": 1}]}}, + headers=ALICE_A, + ) + assert resp.status_code == HTTP_OK, resp.text + # Unchanged behaviour: reserved owner keys are dropped, everything else is forwarded. + assert store.asearch.await_args.kwargs["filters"] == {"org_id": "x", "AND": [{"a": 1}]} + + +# --------------------------------------------------------------------------- +# isolation_scope: per-user form, old no-arg form, bad scopes +# --------------------------------------------------------------------------- + + +def test_old_no_arg_isolation_scope_backend_still_works(): + class _Legacy(OwnershipAuthorizationBackend): + def isolation_scope(self): # pre-1.0 signature: no user + return "owner" + + cp = TenantCheckpointer() + client, _ = _app(checkpointer=cp, authz=_Legacy(checkpointer=cp)) + _seed_two_tenants(cp) + + stamped = client.post("/v1/graph/invoke", json={}, headers=ALICE_A).json()["authz"] + assert stamped["scope"] == "owner" + assert client.get("/v1/threads/t-a/state", headers=ALICE_A).status_code == HTTP_OK + assert client.get("/v1/threads/t-b/state", headers=ALICE_A).status_code == HTTP_FORBIDDEN + assert _listed(client, ALICE_A) == {"t-a"} + + +def test_old_no_arg_isolation_scope_on_a_plain_backend_still_works(): + class _Shared(AuthorizationBackend): + async def authorize(self, user, resource, action, resource_id=None, **ctx): + return True + + def isolation_scope(self): + return "none" + + client, _ = _app(authz=_Shared()) + stamped = client.post("/v1/graph/invoke", json={}, headers=ALICE_A).json()["authz"] + assert stamped["scope"] == "none" + + +def test_isolation_scope_receives_the_authenticated_user(): + seen: list[dict] = [] + + class _Recording(AuthorizationBackend): + async def authorize(self, user, resource, action, resource_id=None, **ctx): + return True + + def isolation_scope(self, user): + seen.append(dict(user)) + return ["user_id", "org_id"] + + client, _ = _app(authz=_Recording()) + stamped = client.post("/v1/graph/invoke", json={}, headers=ALICE_A).json()["authz"] + assert stamped["scope"] == ["user_id", "org_id"] + assert seen[-1]["org_id"] == "org-a" + + +def test_a_malformed_scope_list_is_refused_not_turned_into_no_isolation(): + class _Broken(AuthorizationBackend): + async def authorize(self, user, resource, action, resource_id=None, **ctx): + return True + + def isolation_scope(self, user): + return ["org id; drop"] + + client, _ = _app(authz=_Broken()) + assert client.get("/v1/threads", headers=ALICE_A).status_code == HTTP_FORBIDDEN + + +def test_a_scope_column_the_checkpointer_does_not_declare_denies_thread_access(): + class _Team(TenantAuthz): + def isolation_scope(self, user=None): + return ["team_id"] + + cp = TenantCheckpointer() + client, _ = _app(checkpointer=cp, authz=_Team(checkpointer=cp)) + _seed_two_tenants(cp) + + assert client.get("/v1/threads/t-a/state", headers=ALICE_A).status_code == HTTP_FORBIDDEN + + +def test_a_checkpointer_that_cannot_answer_tenant_values_fails_closed(): + """Declares org_id but answers only the owner (the base aget_thread_scope).""" + + class _OwnerOnly(TenantCheckpointer): + async def aget_thread_scope(self, thread_id): + owner = await self.aget_thread_owner(thread_id) + return None if owner is None else {"user_id": owner} + + cp = _OwnerOnly() + client, _ = _app(checkpointer=cp, authz=TenantAuthz(checkpointer=cp)) + _seed_two_tenants(cp) + + assert client.get("/v1/threads/t-a/state", headers=ALICE_A).status_code == HTTP_FORBIDDEN diff --git a/tests/unit_tests/store/test_store_schemas.py b/tests/unit_tests/store/test_store_schemas.py index 8754348d..576f55b3 100644 --- a/tests/unit_tests/store/test_store_schemas.py +++ b/tests/unit_tests/store/test_store_schemas.py @@ -116,8 +116,9 @@ def test_default_values(self): """Test default values.""" schema = SearchMemorySchema(query="test") assert schema.limit == 10 - assert schema.retrieval_strategy == RetrievalStrategy.SIMILARITY - assert schema.distance_metric == DistanceMetric.COSINE + # None = the store's own configured strategy and metric (core three-level settings) + assert schema.retrieval_strategy is None + assert schema.distance_metric is None assert schema.max_tokens == 4000 def test_missing_query_raises_error(self): diff --git a/tests/unit_tests/store/test_store_service.py b/tests/unit_tests/store/test_store_service.py index 4ef89101..7a1ebbfb 100644 --- a/tests/unit_tests/store/test_store_service.py +++ b/tests/unit_tests/store/test_store_service.py @@ -556,3 +556,53 @@ async def test_forget_drops_reserved_keys(self, store_service, mock_store, mock_ "source": "chat", "OR": [{"tag": "x"}], } + + +@pytest.mark.asyncio +class TestListMemoriesFilters: + """``filters`` on list: validated like search and forget, passed to ``aget_all``.""" + + @pytest.fixture + def schema_store(self, mock_store): + from tenxgraph.storage.columns import parse_columns + from tenxgraph.storage.sql_filter import FilterSchema + + mock_store.filter_schema = FilterSchema.for_memories( + parse_columns(["org_id"], {"plan": "text"}) + ) + mock_store.aget_all.return_value = [] + return mock_store + + async def test_valid_filters_reach_aget_all(self, store_service, schema_store, mock_user): + await store_service.list_memories( + {}, mock_user, filters={"plan": "pro", "source": "chat", "OR": [{"source": "x"}, {"app_id": "b"}]} + ) + kwargs = schema_store.aget_all.call_args.kwargs + assert kwargs["filters"] == {"plan": "pro", "source": "chat", "OR": [{"source": "x"}]} + + @pytest.mark.parametrize("filters", [{"org_id": "x"}, {"user_id": "victim"}]) + async def test_isolation_keys_give_422(self, store_service, schema_store, mock_user, filters): + from tenxgraph_api.src.app.core.auth.request_config import ClientInputError + + with pytest.raises(ClientInputError): + await store_service.list_memories({}, mock_user, filters=filters) + schema_store.aget_all.assert_not_called() + + async def test_bad_operator_gives_422(self, store_service, schema_store, mock_user): + from tenxgraph_api.src.app.core.auth.request_config import ClientInputError + + with pytest.raises(ClientInputError): + await store_service.list_memories( + {}, mock_user, filters={"plan": {"nope": 1}} + ) + schema_store.aget_all.assert_not_called() + + async def test_no_filters_keeps_old_call(self, store_service, mock_store, mock_user): + mock_store.aget_all.return_value = [] + await store_service.list_memories({}, mock_user, limit=5) + assert "filters" not in mock_store.aget_all.call_args.kwargs + + async def test_store_without_schema_passes_filters(self, store_service, mock_store, mock_user): + mock_store.aget_all.return_value = [] + await store_service.list_memories({}, mock_user, filters={"tag": "x"}) + assert mock_store.aget_all.call_args.kwargs["filters"] == {"tag": "x"} diff --git a/tests/unit_tests/test_client_filter_types.py b/tests/unit_tests/test_client_filter_types.py new file mode 100644 index 00000000..8a125d71 --- /dev/null +++ b/tests/unit_tests/test_client_filter_types.py @@ -0,0 +1,50 @@ +"""Client filters on extra columns of non-table types (jsonb, text[], ...) allow only is_null. + +The API validates with core's ``validate_filters``; this checks the rule reaches the client as +a 422-style ``ClientInputError`` with core's message. +""" + +from __future__ import annotations + +import pytest +from tenxgraph.storage.columns import parse_columns +from tenxgraph.storage.sql_filter import FilterSchema + +from tenxgraph_api.src.app.core.auth.client_filters import ( + check_store_filters, + check_thread_filters, +) +from tenxgraph_api.src.app.core.auth.request_config import ClientInputError + + +COLUMNS = parse_columns(["org_id"], {"settings": "jsonb", "tags": "text[]", "plan": "text"}) + + +class _Checkpointer: + filter_schema = FilterSchema.for_threads(COLUMNS) + + +class _Store: + filter_schema = FilterSchema.for_memories(COLUMNS) + + +@pytest.mark.parametrize( + "filters", + [{"settings": {"eq": "x"}}, {"tags": ["a"]}, {"tags": {"in": ["a"]}}], +) +def test_other_operators_rejected(filters): + with pytest.raises(ClientInputError, match="supports only is_null"): + check_thread_filters(_Checkpointer(), filters) + with pytest.raises(ClientInputError, match="supports only is_null"): + check_store_filters(_Store(), filters) + + +def test_is_null_and_known_types_pass(): + filters = {"settings": {"is_null": True}, "tags": {"is_null": False}, "plan": "pro"} + check_thread_filters(_Checkpointer(), filters) + check_store_filters(_Store(), filters) + + +def test_tenant_column_still_refused(): + with pytest.raises(ClientInputError, match="isolation column"): + check_thread_filters(_Checkpointer(), {"org_id": {"is_null": True}}) diff --git a/tests/unit_tests/test_loader.py b/tests/unit_tests/test_loader.py index 2ca999fb..a4bb3aa1 100644 --- a/tests/unit_tests/test_loader.py +++ b/tests/unit_tests/test_loader.py @@ -523,3 +523,50 @@ def test_web_concurrency_above_one_warns_even_in_development(monkeypatch, caplog assert not caplog.records loader.warn_per_process_state(_warn_container(None, InMemoryCacheStore())) assert "The cache store is in-memory" in caplog.text + + +class TestCustomAuthPathForms: + """``auth.path`` resolves like the other module:attr keys (project folder on sys.path).""" + + AUTH_SRC = ( + "from tenxgraph_api import BaseAuth\n" + "class TokenAuth(BaseAuth):\n" + " async def authenticate(self, request, response, credential):\n" + " return {'user_id': 'u'}\n" + ) + + @pytest.fixture + def project(self, tmp_path, monkeypatch): + import sys + + (tmp_path / "auth_app.py").write_text(self.AUTH_SRC) + pkg = tmp_path / "authpkg" + pkg.mkdir() + (pkg / "__init__.py").write_text("") + (pkg / "token_auth.py").write_text(self.AUTH_SRC) + monkeypatch.chdir(tmp_path) + monkeypatch.syspath_prepend(str(tmp_path)) + yield tmp_path + for name in ("auth_app", "authpkg", "authpkg.token_auth"): + sys.modules.pop(name, None) + + @pytest.mark.parametrize( + "path", + [ + "auth_app:TokenAuth", + "auth_app.py:TokenAuth", + "authpkg.token_auth:TokenAuth", + "authpkg/token_auth.py:TokenAuth", + ], + ) + def test_path_forms_load(self, project, path): + from tenxgraph_api.src.app.loader import _load_custom_auth + + assert _load_custom_auth(path).__class__.__name__ == "TokenAuth" + + @pytest.mark.parametrize("path", ["missing_mod:Cls", "pkg.missing:Cls", "nope/x.py:Cls"]) + def test_missing_module_names_path(self, project, path): + from tenxgraph_api.src.app.loader import _load_custom_auth + + with pytest.raises(ValueError, match="Custom auth path does not exist"): + _load_custom_auth(path) diff --git a/tests/unit_tests/test_media_ref_ownership.py b/tests/unit_tests/test_media_ref_ownership.py index 988757c0..4aa6c9e1 100644 --- a/tests/unit_tests/test_media_ref_ownership.py +++ b/tests/unit_tests/test_media_ref_ownership.py @@ -27,7 +27,7 @@ def _service(owners: dict[str, str]) -> MagicMock: """A media service whose ownership check knows the given ``file -> owner`` map.""" - async def ensure_can_access(file_id, user_id): + async def ensure_can_access(file_id, user_id, user=None): if file_id not in owners: raise KeyError(file_id) if owners[file_id] != user_id: @@ -50,7 +50,7 @@ async def test_internal_url_of_another_users_file_is_rejected(): await preprocess_multimodal_messages( [_image("graph://media/victim-file")], service, "attacker" ) - service.ensure_can_access.assert_awaited_once_with("victim-file", "attacker") + service.ensure_can_access.assert_awaited_once_with("victim-file", "attacker", None) @pytest.mark.asyncio @@ -60,7 +60,7 @@ async def test_legacy_internal_url_of_another_users_file_is_rejected(): await preprocess_multimodal_messages( [_image("agentflow://media/victim-file")], service, "attacker" ) - service.ensure_can_access.assert_awaited_once_with("victim-file", "attacker") + service.ensure_can_access.assert_awaited_once_with("victim-file", "attacker", None) @pytest.mark.asyncio diff --git a/tests/unit_tests/test_media_router.py b/tests/unit_tests/test_media_router.py index d0eeb9ca..98c8d693 100644 --- a/tests/unit_tests/test_media_router.py +++ b/tests/unit_tests/test_media_router.py @@ -142,7 +142,7 @@ async def test_get_file_returns_response(self, mock_service, mock_user): user=mock_user, ) - mock_service.get_file.assert_called_once_with("file-1", "user-123") + mock_service.get_file.assert_called_once_with("file-1", "user-123", mock_user) assert result.body == b"file content" assert result.media_type == "text/plain" @@ -189,7 +189,7 @@ async def test_get_file_info_calls_service( user=mock_user, ) - mock_service.get_file_info.assert_called_once_with("file-1", "user-123") + mock_service.get_file_info.assert_called_once_with("file-1", "user-123", mock_user) @pytest.mark.asyncio async def test_get_file_info_handles_not_found(self, mock_request, mock_service, mock_user): diff --git a/tests/unit_tests/test_rbac.py b/tests/unit_tests/test_rbac.py index 44df6100..b68b953f 100644 --- a/tests/unit_tests/test_rbac.py +++ b/tests/unit_tests/test_rbac.py @@ -82,7 +82,33 @@ def test_single_role_string_accepted(): def test_isolation_defaults_to_owner_and_is_configurable(): assert _backend().isolation_scope() == "owner" assert _backend(isolation="none").isolation_scope() == "none" - assert _backend(isolation="bogus").isolation_scope() == "owner" # invalid -> owner + + + +@pytest.mark.parametrize("bad", ["bogus", "owners", "OWNER", None, True, ["user_id"]]) +def test_unknown_isolation_raises_at_construction(bad): + """A typo must stop the server from booting, never pick an isolation level quietly.""" + with pytest.raises(ValueError, match="Unknown authorization isolation"): + _backend(isolation=bad) + + +def test_isolation_accepts_enum_members(): + from tenxgraph.core.authz import Isolation + + assert _backend(isolation=Isolation.NONE).isolation_scope() == "none" + assert _backend(isolation=Isolation.OWNER).isolation_scope() == "owner" + assert type(_backend(isolation=Isolation.NONE).isolation_scope()) is str + + +def test_backend_without_isolation_scope_gets_owner(): + """A duck-typed backend with no isolation_scope method fails closed (owner).""" + from tenxgraph_api.src.app.core.auth.authorization import resolve_isolation_scope + + class _Bare: + async def authorize(self, *a, **k): + return True + + assert resolve_isolation_scope(_Bare(), {"user_id": "u1"}) == "owner" @pytest.mark.asyncio @@ -113,3 +139,12 @@ def test_loader_rejects_unknown_backend_dict(): with pytest.raises(ValueError): _resolve_authorization_backend({"backend": "nope"}) + + +@pytest.mark.parametrize("bad", ["owners", None]) +def test_loader_refuses_unknown_isolation(bad): + """10xgraph.json with a bad authorization.isolation stops the server from booting.""" + from tenxgraph_api.src.app.loader import _resolve_authorization_backend + + with pytest.raises(ValueError, match=r"authorization\.isolation in 10xgraph\.json"): + _resolve_authorization_backend({"backend": "rbac", "roles": ROLES, "isolation": bad}) diff --git a/tests/unit_tests/test_resource_id_rule.py b/tests/unit_tests/test_resource_id_rule.py index 153c0f64..f6dad611 100644 --- a/tests/unit_tests/test_resource_id_rule.py +++ b/tests/unit_tests/test_resource_id_rule.py @@ -63,7 +63,7 @@ async def stop(body: dict[str, Any], user: dict[str, Any] = guard): GOOD_IDS = [ "1234567890123456789", # Snowflake "3f2b8c1e-9d4a-4c1b-8f6e-0a1b2c3d4e5f", # UUID - "mem0:user@example.com_1.2-3", + "org:user@example.com_1.2-3", "a" * 128, ] diff --git a/tests/unit_tests/test_sprint7_review_fixes.py b/tests/unit_tests/test_sprint7_review_fixes.py new file mode 100644 index 00000000..368632b3 --- /dev/null +++ b/tests/unit_tests/test_sprint7_review_fixes.py @@ -0,0 +1,458 @@ +"""Regression tests for the Sprint 7 store review findings A2, A3, A4 and A6. + +A2: the D6 tenant column check runs for the store bound from ``10xgraph.json``. +A3: a delete tombstone exempts only a caller matching the deleter on every tenant column. +A4: a failed L2 eviction is visible, and tenant scope entries use a short L2 TTL. +""" + +# ruff: noqa: S101, PLR2004 + +from __future__ import annotations + +import json +import logging +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from injectq import InjectQ +from tenxgraph.core.exceptions import GraphError +from tenxgraph.storage.checkpointer import BaseCheckpointer, InMemoryCheckpointer +from tenxgraph.storage.columns import parse_columns + +from tenxgraph_api.src.app.core.auth.authorization import OwnershipAuthorizationBackend +from tenxgraph_api.src.app.core.auth.ownership_resolver import ( + DEFAULT_L2_TTL_SECONDS, + TENANT_L2_TTL_SECONDS, + OwnershipCacheEvictionError, + ThreadOwnershipResolver, +) +from tenxgraph_api.src.app.routers.checkpointer.services.checkpointer_service import ( + CheckpointerService, +) + + +class _Cache: + """A shared cache store double (both workers see the same dict).""" + + def __init__(self, *, fail_delete: bool = False, fail_set: bool = False) -> None: + self.d: dict[str, bytes] = {} + self.ttls: dict[str, int | None] = {} + self.fail_delete = fail_delete + self.fail_set = fail_set + + async def get(self, key): + return self.d.get(key) + + async def set(self, key, value, ttl=None, mode="always"): + if self.fail_set and key.startswith("owner:tomb:"): + raise ConnectionError("cache store down") + self.d[key] = value + self.ttls[key] = ttl + return True + + async def delete(self, key): + if self.fail_delete: + raise ConnectionError("cache store down") + self.d.pop(key, None) + + +class _TenantCheckpointer: + """A checkpointer double with an ``org_id`` tenant column; threads live in ``db``.""" + + tenant_column_names = ("org_id",) + columns = parse_columns({"org_id": "text"}) + + def __init__(self) -> None: + self.db: dict[str, dict] = {} + + async def aget_thread_scope(self, thread_id): + return self.db.get(thread_id) + + async def aget_thread_owner(self, thread_id): + return (self.db.get(thread_id) or {}).get("user_id") + + +A = {"user_id": "u1", "org_id": "A"} +B = {"user_id": "u1", "org_id": "B"} + + +def _workers(cache=None, cp=None): + cache = cache if cache is not None else _Cache() + cp = cp if cp is not None else _TenantCheckpointer() + w1 = OwnershipAuthorizationBackend(cp, cache=cache, server_thread_ids=False) + w2 = OwnershipAuthorizationBackend(cp, cache=cache, server_thread_ids=False) + return w1, w2, cp, cache + + +# --------------------------------------------------------------------------- +# A3: tombstone keyed on the deleter's full scope +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_same_user_id_in_another_tenant_cannot_take_a_just_deleted_id(): + """tomb.py: u1@A deletes T on W2; u1@B must not recreate T while W1 still caches u1@A.""" + w1, w2, cp, _ = _workers() + cp.db["T"] = dict(A) + assert await w1.authorize(A, "checkpointer", "read", "T") is True + assert await w2.authorize(A, "checkpointer", "delete", "T") is True + del cp.db["T"] + await w2.evict("T", "u1", user=A) + + assert await w2.authorize(B, "graph", "invoke", "T") is False + assert await w1.authorize(B, "graph", "invoke", "T") is False # through the L2 tombstone + # The deleter itself (same user_id and tenant) may recreate it at once, on any worker. + assert await w2.authorize(A, "graph", "invoke", "T") is True + assert await w1.authorize(A, "graph", "invoke", "T") is True + + +@pytest.mark.asyncio +async def test_tombstone_stores_the_deleter_scope_as_canonical_json(): + _, w2, cp, cache = _workers() + cp.db["T"] = dict(A) + assert await w2.authorize(A, "checkpointer", "delete", "T") is True + await w2.evict("T", "u1", user=A) + assert json.loads(cache.d["owner:tomb:T"]) == {"org_id": "A", "user_id": "u1"} + + +@pytest.mark.asyncio +async def test_bare_user_id_tombstone_blocks_everyone_with_tenant_columns(): + """A plain-owner tombstone (an older worker during a rolling deploy) exempts nobody.""" + w1, _, _, cache = _workers() + cache.d["owner:tomb:T"] = b"u1" + assert await w1.authorize(A, "graph", "invoke", "T") is False + assert await w1.authorize(B, "graph", "invoke", "T") is False + + +@pytest.mark.asyncio +async def test_plain_owner_tombstones_unchanged_without_tenant_columns(): + """No tenant columns: the tombstone stays the plain user_id older releases read.""" + cache = _Cache() + deleting = ThreadOwnershipResolver(AsyncMock(return_value=None), cache=cache) + other = ThreadOwnershipResolver(AsyncMock(return_value=None), cache=cache) + await deleting.evict("T", "alice") + assert cache.d["owner:tomb:T"] == b"alice" + assert await other.recently_deleted("T", "alice") is False + assert await other.recently_deleted("T", "bob") is True + cache.d["owner:tomb:U"] = b"alice" # written by an older release + assert await other.recently_deleted("U", "alice") is False + assert await other.recently_deleted("U", {"user_id": "alice"}) is False + + +@pytest.mark.asyncio +async def test_resolver_exempts_only_a_full_scope_match(): + r = ThreadOwnershipResolver(AsyncMock(return_value=None), tenant_columns=("org_id",)) + await r.evict("T", {"user_id": "u1", "org_id": "A"}) + assert await r.recently_deleted("T", {"user_id": "u1", "org_id": "A"}) is False + assert await r.recently_deleted("T", {"user_id": "u1", "org_id": "B"}) is True + assert await r.recently_deleted("T", "u1") is True # no tenant values: not a match + assert await r.recently_deleted("T") is True + + +@pytest.mark.asyncio +async def test_deleter_from_another_tenant_gets_no_head_start(): + """Same user_id, other tenant (a ["user_id"] scope) deleting: the tombstone blocks all.""" + r = ThreadOwnershipResolver( + AsyncMock(return_value={"user_id": "u1", "org_id": "A"}), tenant_columns=("org_id",) + ) + await r.scope_of("T", ("user_id", "org_id")) + await r.evict("T", {"user_id": "u1", "org_id": "B"}) + assert await r.recently_deleted("T", {"user_id": "u1", "org_id": "B"}) is True + assert await r.recently_deleted("T", {"user_id": "u1", "org_id": "A"}) is True + + +# --------------------------------------------------------------------------- +# A4: failed L2 eviction is visible; short L2 TTL with tenant columns +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_failed_l2_eviction_raises_and_warns(caplog): + cache = _Cache() + r = ThreadOwnershipResolver( + AsyncMock(return_value={"user_id": "u1", "org_id": "A"}), + cache=cache, + tenant_columns=("org_id",), + ) + await r.scope_of("T", ("user_id", "org_id")) + cache.fail_delete = True + with ( + caplog.at_level(logging.WARNING, logger="tenxgraph_api.authorization"), + pytest.raises(OwnershipCacheEvictionError), + ): + await r.evict("T", {"user_id": "u1", "org_id": "A"}) + assert "did not reach the cache store" in caplog.text + # L1 and the local tombstone are still handled before the error. + assert "T" not in r._l1 + assert await r.recently_deleted("T", {"user_id": "u1", "org_id": "B"}) is True + + +@pytest.mark.asyncio +async def test_failed_tombstone_write_raises(): + r = ThreadOwnershipResolver(AsyncMock(return_value=None), cache=_Cache(fail_set=True)) + with pytest.raises(OwnershipCacheEvictionError): + await r.evict("T", "alice") + + +@pytest.mark.asyncio +async def test_thread_delete_reports_a_failed_cache_eviction(monkeypatch): + cp = _TenantCheckpointer() + cp.db["T"] = dict(A) + cache = _Cache() + backend = OwnershipAuthorizationBackend(cp, cache=cache) + assert await backend.authorize(A, "checkpointer", "delete", "T") is True + cache.fail_delete = True + + container = MagicMock() + container.try_get.return_value = backend + monkeypatch.setattr( + "tenxgraph_api.src.app.core.utils.container.current_container", lambda: container + ) + telemetry = MagicMock() + telemetry.try_get.return_value = None + monkeypatch.setattr("injectq.InjectQ.get_instance", lambda: telemetry) + monkeypatch.setattr(CheckpointerService, "_media_service", staticmethod(lambda: None)) + + service = CheckpointerService.__new__(CheckpointerService) + service.checkpointer = MagicMock(aclean_thread=AsyncMock(return_value=None)) + service.settings = MagicMock() + res = await service.delete_thread({"thread_id": "T"}, dict(A), "T") + assert res.success is False + assert res.data["cleanup_failed"] == ["cache"] + + +@pytest.mark.asyncio +async def test_thread_delete_passes_the_deleter_scope(monkeypatch): + """delete_thread hands the whole user to the built-in backend (A3 end to end).""" + w1, w2, cp, _ = _workers() + cp.db["T"] = dict(A) + assert await w1.authorize(A, "checkpointer", "read", "T") is True + assert await w2.authorize(A, "checkpointer", "delete", "T") is True + + container = MagicMock() + container.try_get.return_value = w2 + monkeypatch.setattr( + "tenxgraph_api.src.app.core.utils.container.current_container", lambda: container + ) + telemetry = MagicMock() + telemetry.try_get.return_value = None + monkeypatch.setattr("injectq.InjectQ.get_instance", lambda: telemetry) + monkeypatch.setattr(CheckpointerService, "_media_service", staticmethod(lambda: None)) + service = CheckpointerService.__new__(CheckpointerService) + + async def clean(_cfg): + cp.db.pop("T", None) + + service.checkpointer = MagicMock(aclean_thread=clean) + service.settings = MagicMock() + res = await service.delete_thread({"thread_id": "T"}, dict(A), "T") + assert res.success is True + # The deleting worker (L1 evicted) and a fresh worker (shared tombstone only). + w3 = OwnershipAuthorizationBackend(cp, cache=w1._cache, server_thread_ids=False) + for worker in (w2, w3): + assert await worker.authorize(B, "graph", "invoke", "T") is False + assert await worker.authorize(A, "graph", "invoke", "T") is True + + +@pytest.mark.asyncio +async def test_custom_evict_without_user_keyword_still_works(monkeypatch): + calls = [] + + class Backend: + def evict(self, thread_id, owner=None): + calls.append((thread_id, owner)) + + container = MagicMock() + container.try_get.return_value = Backend() + monkeypatch.setattr( + "tenxgraph_api.src.app.core.utils.container.current_container", lambda: container + ) + telemetry = MagicMock() + telemetry.try_get.return_value = None + monkeypatch.setattr("injectq.InjectQ.get_instance", lambda: telemetry) + monkeypatch.setattr(CheckpointerService, "_media_service", staticmethod(lambda: None)) + service = CheckpointerService.__new__(CheckpointerService) + service.checkpointer = MagicMock(aclean_thread=AsyncMock(return_value=None)) + service.settings = MagicMock() + res = await service.delete_thread({"thread_id": "T"}, dict(A), "T") + assert res.success is True + assert calls == [("T", "u1")] + + +@pytest.mark.asyncio +async def test_l2_ttl_is_short_with_tenant_columns(): + lookup = AsyncMock(return_value={"user_id": "u1", "org_id": "A"}) + tenant_cache, plain_cache = _Cache(), _Cache() + await ThreadOwnershipResolver(lookup, cache=tenant_cache, tenant_columns=("org_id",)).scope_of( + "T", ("user_id", "org_id") + ) + await ThreadOwnershipResolver(lookup, cache=plain_cache).scope_of("T") + assert tenant_cache.ttls["owner:scope:T"] == TENANT_L2_TTL_SECONDS == 300 + assert plain_cache.ttls["owner:id:T"] == DEFAULT_L2_TTL_SECONDS + explicit = _Cache() + await ThreadOwnershipResolver( + lookup, cache=explicit, tenant_columns=("org_id",), l2_ttl=42 + ).scope_of("T", ("user_id", "org_id")) + assert explicit.ttls["owner:scope:T"] == 42 + + +# --------------------------------------------------------------------------- +# A2: D6 tenant column check for the store bound from 10xgraph.json +# --------------------------------------------------------------------------- + + +class _Store: + def __init__(self, tenant: dict | None = None) -> None: + self.columns = parse_columns(tenant) + self.tenant_column_names = tuple(tenant or ()) + + +def _graph(checkpointer, store=None): + return SimpleNamespace(_checkpointer=checkpointer, _store=store) + + +@pytest.fixture +def production(monkeypatch): + monkeypatch.setenv("MODE", "production") + + +@pytest.fixture +def development(monkeypatch): + monkeypatch.setenv("MODE", "development") + + +def test_store_key_mismatch_refuses_in_production(production): + from tenxgraph_api.src.app.loader import check_store_tenant_columns + + with pytest.raises(GraphError): + check_store_tenant_columns(_graph(_TenantCheckpointer()), InjectQ(), _Store()) + + +def test_store_key_mismatch_warns_in_development(development, caplog): + from tenxgraph_api.src.app.loader import check_store_tenant_columns + + with caplog.at_level(logging.WARNING, logger="tenxgraph.graph"): + check_store_tenant_columns(_graph(_TenantCheckpointer()), InjectQ(), _Store()) + assert "declares tenant columns" in caplog.text + + +def test_store_key_matching_columns_pass(production): + from tenxgraph_api.src.app.loader import check_store_tenant_columns + + check_store_tenant_columns(_graph(_TenantCheckpointer()), InjectQ(), _Store({"org_id": "text"})) + + +def test_store_key_opt_out(production): + from tenxgraph_api.src.app.loader import check_store_tenant_columns + + check_store_tenant_columns( + _graph(_TenantCheckpointer()), InjectQ(), _Store(), allow_mismatch=True + ) + + +def test_store_key_checked_against_the_container_checkpointer(production): + from tenxgraph_api.src.app.loader import check_store_tenant_columns + + container = InjectQ() + container.bind_instance(BaseCheckpointer, _TenantCheckpointer(), allow_concrete=True) + with pytest.raises(GraphError): + check_store_tenant_columns(_graph(InMemoryCheckpointer()), container, _Store()) + + +def test_store_compiled_into_the_graph_is_not_checked_twice(production): + """compile() already checked it, with the developer's allow_tenant_mismatch.""" + from tenxgraph_api.src.app.loader import check_store_tenant_columns + + store = _Store() + check_store_tenant_columns(_graph(_TenantCheckpointer(), store), InjectQ(), store) + + +def _config(store_path, mismatch=False): + from tenxgraph_api.src.app.core.config.graph_config import GraphConfig, RunsConfig + + config = MagicMock(spec=GraphConfig) + config.graph_path = "mod:graph" + config.auth_config.return_value = None + config.thread_name_generator = "none" + config.id_generator = None + config.authorization_path = None + config.store_path = store_path + config.store_allow_tenant_mismatch = mismatch + config.cache_store = None + config.checkpointer_path = None + config.remote_tools = [] + config.runs = RunsConfig() + return config + + +async def _attach(config, mode, monkeypatch): + from tenxgraph_api.src.app.core.config import settings as settings_mod + from tenxgraph_api.src.app.loader import attach_all_modules + + # Build (or reuse) the cached settings before MODE changes, so no production Settings + # object is created here and cached for other tests. + monkeypatch.setattr(settings_mod.get_settings(), "MODE", mode) + monkeypatch.setenv("MODE", mode) # core's tenant check reads the environment + graph = _graph(_TenantCheckpointer()) + with ( + patch("tenxgraph_api.src.app.loader.load_graph", return_value=graph), + patch("tenxgraph_api.src.app.loader.apply_id_generator"), + patch("tenxgraph_api.src.app.loader.load_store", return_value=_Store()), + patch("tenxgraph_api.src.app.core.config.media_settings.get_media_settings"), + ): + return await attach_all_modules(config, InjectQ()) + + +@pytest.mark.asyncio +async def test_production_boot_refuses_a_mismatched_store_key(monkeypatch): + with pytest.raises(GraphError): + await _attach(_config("graph.deps:store"), "production", monkeypatch) + + +@pytest.mark.asyncio +async def test_development_boot_warns_on_a_mismatched_store_key(monkeypatch, caplog): + with caplog.at_level(logging.WARNING, logger="tenxgraph.graph"): + await _attach(_config("graph.deps:store"), "development", monkeypatch) + assert "declares tenant columns" in caplog.text + + +@pytest.mark.asyncio +async def test_production_boot_with_the_opt_out(monkeypatch): + await _attach(_config("graph.deps:store", mismatch=True), "production", monkeypatch) + + +def test_graph_config_parses_the_opt_out(tmp_path): + from tenxgraph_api.src.app.core.config.graph_config import GraphConfig + + path = tmp_path / "10xgraph.json" + path.write_text(json.dumps({"agent": "g:app"})) + assert GraphConfig(str(path)).store_allow_tenant_mismatch is False + path.write_text(json.dumps({"agent": "g:app", "store_allow_tenant_mismatch": True})) + assert GraphConfig(str(path)).store_allow_tenant_mismatch is True + path.write_text(json.dumps({"agent": "g:app", "store_allow_tenant_mismatch": "maybe"})) + with pytest.raises(ValueError, match="store_allow_tenant_mismatch"): + _ = GraphConfig(str(path)).store_allow_tenant_mismatch + + +def test_config_editor_knows_the_opt_out(): + from tenxgraph_api.cli.config_editor.schema import KNOWN_TOP_LEVEL_KEYS, build_schema + from tenxgraph_api.cli.config_editor.validation import validate_config + + assert "store_allow_tenant_mismatch" in KNOWN_TOP_LEVEL_KEYS + paths = [ + field["path"] + for section in build_schema()["sections"] + for field in section.get("fields", []) + ] + assert ["store_allow_tenant_mismatch"] in paths + issues = validate_config({"agent": "g:app", "store_allow_tenant_mismatch": "yes"}) + assert any( + issue["path"] == "store_allow_tenant_mismatch" and issue["level"] == "error" + for issue in issues + ) + assert not [ + issue + for issue in validate_config({"agent": "g:app", "store_allow_tenant_mismatch": True}) + if issue["path"] == "store_allow_tenant_mismatch" + ] diff --git a/tests/unit_tests/test_thread_scope_resolver.py b/tests/unit_tests/test_thread_scope_resolver.py new file mode 100644 index 00000000..e7eea820 --- /dev/null +++ b/tests/unit_tests/test_thread_scope_resolver.py @@ -0,0 +1,283 @@ +"""ThreadOwnershipResolver caching thread scopes (user_id plus tenant values).""" + +from __future__ import annotations + +import json +import uuid +from datetime import UTC, datetime, timedelta, timezone + +import pytest +from tenxgraph.storage.columns import parse_columns + +from tenxgraph_api.src.app.core.auth.authorization import ( + OwnershipAuthorizationBackend, + scope_lookup, + supports_owner_lookup, +) +from tenxgraph_api.src.app.core.auth.ownership_resolver import ( + ThreadOwnershipResolver, + canonical_value, + to_scope, +) + + +class _ScopeLookup: + def __init__(self, scopes: dict[str, dict]): + self.scopes = scopes + self.calls = 0 + + async def __call__(self, thread_id: str): + self.calls += 1 + return self.scopes.get(str(thread_id)) + + +class _FakeCache: + def __init__(self): + self.store: dict[str, bytes] = {} + + async def get(self, key): + return self.store.get(key) + + async def set(self, key, value, ttl=None, mode="always"): + self.store[key] = value + return True + + async def delete(self, key): + self.store.pop(key, None) + + +ORG = uuid.UUID("6f1c0d1e-8a43-4c5e-9b0e-2f1d3c4b5a69") + + +def test_canonical_value_normalises_types(): + assert canonical_value(None) is None + assert canonical_value(True) == "true" + assert canonical_value(7) == "7" + assert canonical_value(ORG) == str(ORG) + later = datetime(2026, 1, 1, 5, 30, tzinfo=timezone(timedelta(hours=5, minutes=30))) + assert canonical_value(later) == canonical_value(datetime(2026, 1, 1, tzinfo=UTC)) + + +def test_to_scope_reads_owner_and_dict_results(): + assert to_scope(None) is None + assert to_scope("alice") == {"user_id": "alice"} + assert to_scope(42) == {"user_id": "42"} + assert to_scope({"user_id": "alice", "org_id": ORG, "team": None}) == { + "user_id": "alice", + "org_id": str(ORG), + "team": None, + } + # A scope with no owner is not a thread. + assert to_scope({"org_id": "x"}) is None + + +@pytest.mark.asyncio +async def test_scope_is_cached_in_l1_with_tenant_values(): + lookup = _ScopeLookup({"t1": {"user_id": "alice", "org_id": ORG}}) + r = ThreadOwnershipResolver(lookup, tenant_columns=("org_id",)) + + for _ in range(3): + assert await r.scope_of("t1", ("user_id", "org_id")) == { + "user_id": "alice", + "org_id": str(ORG), + } + assert lookup.calls == 1 + assert await r.owner_of("t1") == "alice" + + +@pytest.mark.asyncio +async def test_returned_scope_is_a_copy(): + r = ThreadOwnershipResolver(_ScopeLookup({"t1": {"user_id": "alice", "org_id": "a"}})) + scope = await r.scope_of("t1") + scope["org_id"] = "b" + assert (await r.scope_of("t1"))["org_id"] == "a" + + +@pytest.mark.asyncio +async def test_tenant_scopes_use_their_own_json_l2_key(): + cache = _FakeCache() + lookup = _ScopeLookup({"t:1": {"user_id": "alice", "org_id": "org-a"}}) + r = ThreadOwnershipResolver(lookup, cache=cache, tenant_columns=("org_id",)) + await r.scope_of("t:1", ("user_id", "org_id")) + + assert list(cache.store) == ["owner:scope:t%3A1"] + assert json.loads(cache.store["owner:scope:t%3A1"]) == {"org_id": "org-a", "user_id": "alice"} + + other_lookup = _ScopeLookup({}) + other = ThreadOwnershipResolver(other_lookup, cache=cache, tenant_columns=("org_id",)) + assert await other.scope_of("t:1", ("user_id", "org_id")) == { + "user_id": "alice", + "org_id": "org-a", + } + assert other_lookup.calls == 0 + + +@pytest.mark.asyncio +async def test_without_tenant_columns_l2_keeps_the_plain_owner_layout(): + cache = _FakeCache() + r = ThreadOwnershipResolver(_ScopeLookup({"t1": {"user_id": "alice"}}), cache=cache) + await r.scope_of("t1", ("user_id",)) + assert cache.store == {"owner:id:t1": b"alice"} + + +@pytest.mark.asyncio +async def test_a_plain_owner_entry_is_never_read_as_a_tenant_scope(): + """An entry an older release wrote (owner only) must not stand in for a full scope.""" + cache = _FakeCache() + cache.store["owner:id:t1"] = b"alice" + lookup = _ScopeLookup({"t1": {"user_id": "alice", "org_id": "org-a"}}) + r = ThreadOwnershipResolver(lookup, cache=cache, tenant_columns=("org_id",)) + + assert (await r.scope_of("t1", ("user_id", "org_id")))["org_id"] == "org-a" + assert lookup.calls == 1 + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "raw", + [b"not json", b"[1]", b'{"org_id": "a"}', b'{"user_id": "a", "org_id": 5}', b"\xff"], +) +async def test_malformed_l2_scope_entries_are_misses(raw): + cache = _FakeCache() + cache.store["owner:scope:t1"] = raw + lookup = _ScopeLookup({"t1": {"user_id": "alice", "org_id": "org-a"}}) + r = ThreadOwnershipResolver(lookup, cache=cache, tenant_columns=("org_id",)) + + assert await r.scope_of("t1", ("user_id", "org_id")) == { + "user_id": "alice", + "org_id": "org-a", + } + assert lookup.calls == 1 + + +@pytest.mark.asyncio +async def test_a_cached_scope_missing_a_compared_column_is_looked_up_again(): + lookup = _ScopeLookup({"t1": {"user_id": "alice"}}) + r = ThreadOwnershipResolver(lookup, tenant_columns=("org_id",)) + await r.scope_of("t1", ("user_id",)) + # The column was declared later: the cached entry cannot answer it. + lookup.scopes["t1"] = {"user_id": "alice", "org_id": "org-a"} + + assert (await r.scope_of("t1", ("user_id", "org_id")))["org_id"] == "org-a" + assert lookup.calls == 2 + await r.scope_of("t1", ("user_id", "org_id")) + assert lookup.calls == 2 + + +@pytest.mark.asyncio +async def test_a_null_tenant_value_is_cached_as_known(): + lookup = _ScopeLookup({"t1": {"user_id": "alice", "org_id": None}}) + r = ThreadOwnershipResolver(lookup, tenant_columns=("org_id",)) + for _ in range(2): + assert (await r.scope_of("t1", ("org_id",)))["org_id"] is None + assert lookup.calls == 1 + + +@pytest.mark.asyncio +async def test_evict_clears_both_l2_layouts(): + cache = _FakeCache() + cache.store["owner:id:t1"] = b"alice" + r = ThreadOwnershipResolver( + _ScopeLookup({"t1": {"user_id": "alice", "org_id": "a"}}), + cache=cache, + tenant_columns=("org_id",), + ) + await r.scope_of("t1", ("user_id", "org_id")) + await r.evict("t1", "alice") + assert list(cache.store) == ["owner:tomb:t1"] + + +@pytest.mark.asyncio +async def test_a_delete_by_someone_other_than_the_owner_blocks_everyone(): + """An org admin deleting a member's thread gets no head start on recreating the id.""" + r = ThreadOwnershipResolver(_ScopeLookup({"t1": {"user_id": "alice", "org_id": "a"}})) + await r.scope_of("t1") + await r.evict("t1", "carol") + assert await r.recently_deleted("t1", "carol") is True + assert await r.recently_deleted("t1", "alice") is True + + +# --------------------------------------------------------------------------- +# Backend: which lookup it uses, and matching +# --------------------------------------------------------------------------- + + +class _OwnerOnly: + async def aget_thread_owner(self, thread_id): + return "alice" + + +class _WithScope(_OwnerOnly): + tenant_column_names = ("org_id",) + + def __init__(self): + self.calls = 0 + + async def aget_thread_scope(self, thread_id): + self.calls += 1 + return {"user_id": "alice", "org_id": ORG} + + +def test_scope_lookup_prefers_aget_thread_scope(): + with_scope = _WithScope() + assert scope_lookup(with_scope) == with_scope.aget_thread_scope + owner_only = _OwnerOnly() + assert scope_lookup(owner_only) == owner_only.aget_thread_owner + + +def test_supports_owner_lookup_accepts_either_method(): + class _ScopeOnly: + async def aget_thread_scope(self, thread_id): + return None + + assert supports_owner_lookup(_ScopeOnly()) + assert supports_owner_lookup(_OwnerOnly()) + assert not supports_owner_lookup(object()) + + +@pytest.mark.asyncio +async def test_uuid_tenant_values_match_whatever_case_the_identity_uses(): + cp = _WithScope() + cp.columns = parse_columns({"org_id": "uuid"}, None) + backend = OwnershipAuthorizationBackend(checkpointer=cp) + upper = {"user_id": "alice", "org_id": str(ORG).upper()} + + assert await backend.authorize(upper, "checkpointer", "read", resource_id="t1") + other = {"user_id": "alice", "org_id": str(uuid.uuid4())} + assert not await backend.authorize(other, "checkpointer", "read", resource_id="t1") + bad = {"user_id": "alice", "org_id": "not-a-uuid"} + assert not await backend.authorize(bad, "checkpointer", "read", resource_id="t1") + assert cp.calls == 1 + + +@pytest.mark.asyncio +async def test_none_scope_reaches_any_existing_thread(): + class _Shared(OwnershipAuthorizationBackend): + def isolation_scope(self, user=None): + return "none" + + backend = _Shared(checkpointer=_WithScope()) + bob = {"user_id": "bob", "org_id": "elsewhere"} + assert await backend.authorize(bob, "checkpointer", "read", resource_id="t1") + + +@pytest.mark.asyncio +async def test_an_unknown_scope_string_denies(): + class _Typo(OwnershipAuthorizationBackend): + def isolation_scope(self, user=None): + return "owners" + + backend = _Typo(checkpointer=_WithScope()) + alice = {"user_id": "alice", "org_id": str(ORG)} + assert not await backend.authorize(alice, "checkpointer", "read", resource_id="t1") + + +@pytest.mark.asyncio +async def test_a_custom_resolver_with_only_owner_of_still_works(): + class _Resolver: + async def owner_of(self, thread_id): + return "alice" + + backend = OwnershipAuthorizationBackend(checkpointer=_OwnerOnly(), resolver=_Resolver()) + assert await backend.authorize({"user_id": "alice"}, "graph", "invoke", resource_id="t1") + assert not await backend.authorize({"user_id": "bob"}, "graph", "invoke", resource_id="t1") diff --git a/uv.lock b/uv.lock index 182e1816..ad958472 100644 --- a/uv.lock +++ b/uv.lock @@ -10,18 +10,14 @@ resolution-markers = [ [[package]] name = "10xgraph" -version = "1.0.0rc3" -source = { registry = "https://pypi.org/simple" } +version = "1.0.0rc4" +source = { git = "https://github.com/10xGraph/10xGraph?branch=main#f32758c2a62567106e57c7b9ba6ad2726c3e7e8d" } dependencies = [ { name = "injectq" }, { name = "pydantic" }, { name = "python-dotenv" }, { name = "pyyaml" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/93/97/37e955f09e3f1147944752f53e8d957f90912be3cd0b2899c09b2252f7b8/10xgraph-1.0.0rc3.tar.gz", hash = "sha256:36e7562b4391db133be57b7237c8988d9e991f7758b3a6341dde9d5b4821846c", size = 686432, upload-time = "2026-10-30T13:00:31.782Z" } -wheels = [ - { url = "https://files.pythonhosted.org/packages/46/9b/b0d455b1ad8e9e2dd9e25d6320663b178f30553e70ee36e165fd2303d28f/10xgraph-1.0.0rc3-py3-none-any.whl", hash = "sha256:609ea1f9f65d5fda866eab5f0cd5fca5e3d41e848f6741540dd54646aad5a843", size = 742737, upload-time = "2026-10-30T13:00:28.974Z" }, -] [package.optional-dependencies] images = [ @@ -30,7 +26,7 @@ images = [ [[package]] name = "10xgraph-api" -version = "1.0.0rc3" +version = "1.0.0rc4" source = { editable = "." } dependencies = [ { name = "10xgraph" }, @@ -109,8 +105,8 @@ dev = [ [package.metadata] requires-dist = [ - { name = "10xgraph", specifier = ">=1.0.0rc3,<2.0" }, - { name = "10xgraph", extras = ["images"], marker = "extra == 'images'", specifier = ">=1.0.0rc3,<2.0" }, + { name = "10xgraph", git = "https://github.com/10xGraph/10xGraph?branch=main" }, + { name = "10xgraph", extras = ["images"], marker = "extra == 'images'", git = "https://github.com/10xGraph/10xGraph?branch=main" }, { name = "ag-ui-protocol", marker = "extra == 'ag-ui'", specifier = ">=1.0,<2" }, { name = "fastapi", specifier = ">=0.116,<1.0" }, { name = "firebase-admin", marker = "extra == 'firebase'", specifier = ">=6.5.0" }, @@ -140,7 +136,7 @@ provides-extras = ["sentry", "firebase", "snowflakekit", "redis", "jwt", "media" [package.metadata.requires-dev] dev = [ - { name = "10xgraph", specifier = ">=1.0.0rc3" }, + { name = "10xgraph", git = "https://github.com/10xGraph/10xGraph?branch=main" }, { name = "ag-ui-protocol", specifier = ">=1.0,<2" }, { name = "gunicorn", specifier = ">=23,<27" }, { name = "httpx", specifier = "==0.27.0" },