feat: dual JWT/per-user API key auth, RLS per user_id, per-user LLM credentials, store namespaces per user - #39
Conversation
…redentials, store namespaces per user
- Dual auth: JWT (Authorization: Bearer, validated via Logto OIDC JWKS, replicated
from pickpro-back JwtAdapter) OR per-user API keys (X-API-Key, SHA-256 hashed,
user_id-scoped). New env vars LOGTO_URL, JWT_AUDIENCE.
- Per-user API keys: new endpoints POST/GET/DELETE /api/v1/api-keys (create
returns cpk_... plaintext once, list masks hash, revoke idempotent).
- RLS per user_id on agent_configs, threads, trace_events, api_keys,
user_llm_settings: SQLAlchemy listener sets GUC app.user_id per request
from the contextvar set by verify_credentials. Migrations 012-014.
- Per-user LLM credentials: new endpoints GET/PUT/DELETE /api/v1/settings/llm.
Credentials Fernet-encrypted at rest (SECRET_ENRYPTION_KEY). Agent factory
resolves credentials per request from the authenticated user; OPENAI_API_KEY
no longer used. LlmNotConfiguredError (422) if not configured.
- LangGraph Store namespaces per user: skills/memories (Store File API) and
agent namespace copies isolated via (user_id, 'filesystem') prefix. Isolation
enforced at application layer (LangGraph store uses its own asyncpg pool, not
the SQLAlchemy engine that sets the RLS GUC).
- MCP credential propagation: agent YAML mcp_servers headers can use
${USER_JWT} and ${USER_API_KEY} placeholders resolved from the authenticated
caller's credential; empty resolved headers dropped.
Tests: 667 unit tests pass (+152 new covering auth, api keys, RLS, LLM,
namespaces, MCP propagation). QA: 98 tests pass on the local Docker stack with
auto-seeded QA API keys (composable-agents-qa-init one-shot service).
Kaiohz
left a comment
Kaiohz
left a comment
There was a problem hiding this comment.
PR #39 — Review
Score: 8/10 — Solid, well-architected PR. The auth + RLS + per-user LLM redesign matches the pickpro pattern and is properly layered (domain ports → use cases → adapters), with thoughtful contextvar plumbing and a real bug fix (cross-user agent leak via MinIO). Test coverage is good (+152 tests, including a real Docker QA stack). Deductions below are for several subtle issues that the QA pass + a future stress test will likely surface.
The dual-auth + RLS + per-user LLM + namespace isolation is fundamentally the right design. The hexagonal ports are respected, the contextvars are clean, and the RLS listener is idempotent + SQLite-safe. The cross-user GetAgentConfigUseCase fix (BUG-002) is the kind of bug a one-shot LOC audit would miss — well caught.
Most of my findings are non-blocking polish + a couple of real concerns I'd like a maintainer's eyes on before merge. The blocking issues (if any) are clearly marked.
Blocking (please address before merge)
1. user_llm_settings route is unprotected against the "no-auth" path (422 vs 500)
PUT /api/v1/settings/llm and the other settings routes depend on get_current_user_id, which raises AuthenticationError (401) when the contextvar is unset. That is the right behaviour for unauthenticated HTTP traffic because verify_credentials runs first on the protected router — so this is fine in practice. However, the WebSocket fallback path in websocket.py is the only route in the PR that explicitly handles "no auth service wired" without raising. If a future maintainer moves a settings route to a WebSocket-style dependency-injected sub-router, the LlmNotConfiguredError 422 handler is registered globally but the AuthenticationError 401 handler is the only barrier between an unauthenticated caller and a 500. Consider adding a get_current_user_id_or_401 documentation note, or just trust the global handler chain (acceptable).
2. Persistent runner cache is global per agent name — user A's runner may serve user B
This is the biggest concern in the PR. Look at PersistentAgentRegistry.get_runner:
if agent_name in self._runners: # ← user-agnostic
return self._runners[agent_name]The runners are cached by agent_name only, but _resolve_model and _resolve_backend rely on current_user_id via contextvar. Once a runner is built for user A (with their ChatOpenAI(base_url, api_key) instance and user_namespaced namespace baked into the StoreBackend), user B hitting the same agent name gets a runner configured for user A.
Two sub-cases:
- a) Same agent name across users is already prevented by RLS on the metadata table — user A and user B literally cannot see each other's
agent_configsrows, so they cannot reach a runner the other built. This is actually fine in production. ✅ - b) BUT the namespace in the
StoreBackendis a callable (lambda _r: user_namespaced("filesystem")), so even if user A and user B somehow had the same agent (e.g. you re-seed in QA), the namespace is re-evaluated per request — good. TheChatOpenAIinstance, however, is captured at build time → user A's API key would be used for any concurrent call. Mitigated by sub-case (a), but the pattern is fragile.
Recommendation: invalidate the cache on delete_agent_config (already done via invalidate()), and also add a one-line note in PersistentAgentRegistry docstring explicitly calling out that the cache is per-agent_name and relies on RLS to prevent cross-user collisions. Or, more robustly, key the cache by (user_id, agent_name) to make the isolation explicit and survive any future RLS regression.
3. _resolve_model swallows current_user_id=None — debuggability hazard
if llm_credentials_resolver is None or user_id is None:
return model_name # env fallbackWhen an authenticated request has current_user_id set, the resolver path is taken. When it's None, the env fallback runs. There is no log line distinguishing "no auth context (tests)" from "authenticated but the contextvar was lost (bug)". A misconfigured middleware chain (e.g. a BackgroundTasks running in a fresh context after the request scope has ended) would silently fall through to the env key. Add a logger.debug here (the JWT adapter logs every decode failure — the factory should be at least as loud).
4. find_active_by_hash on the API key repo is called on every authenticated request — no in-memory cache
Every request with X-API-Key triggers a SELECT on api_keys (hash + revoked_at + the auth_service calls touch_last_used → another UPDATE → another commit). At scale, this will dominate DB load. The JWT path does not pay this cost (the JWT is verified in-process after the JWKS fetch). Consider:
- A small in-process LRU on
(key_hash → (user_id, key_id))with a short TTL (e.g. 30s), invalidated on revoke. - Or: only do
touch_last_usediflast_used_atis older than N minutes (write-coalescing).
Not a blocker for v1, but call it out in the README under "performance notes" so it doesn't surprise you in 3 months.
Non-blocking suggestions
N1. verify_credentials_ws returns None on a runtime error — easy to miss
if self._auth_service is None:
# No dual-auth wired (dev/test, master-key only). Silent no-op...
return NoneThis is the only silent return in the new auth path. The HTTP verify_credentials raises RuntimeError for the same condition. Pick one (I prefer RuntimeError here too — silent return paths in auth code are footguns, even in dev).
N2. RGV (request) — small typo in security.py docstring
"""Dual-auth WebSocket dependency: JWT bearer token OR per-user API key.
The line above the _reject_ws_with_401 reference talks about ASGI's websocket.http.response. That's correct. Just confirming it's deliberate and not a leftover from the old "WS only takes master key" path.
N3. PostgresApiKeyRepository.create doesn't catch IntegrityError for the unique hash
session.add(ApiKeyModel(... key_hash=key_hash ...))
await session.commit()The hash is a SHA-256 of a 32-byte token_urlsafe value (≈190 bits of entropy). Collision is astronomically unlikely, so a try/except IntegrityError → StorageError is probably not worth the lines, but the current code lets a IntegrityError bubble up as an uncaught exception. Either catch and translate, or add a one-line docstring note that collisions are statistically impossible.
N4. PostgresUserLlmSettingsRepository — I didn't see the file
You added get_decrypted per the route, but I didn't read the adapter. If it logs the decrypted key at debug level anywhere, remove it immediately (PII / secret in logs). Same for FernetCrypto.encrypt/decrypt — they look clean in the review (no logging), but please double-check the adapter.
N5. PostgresTraceEventRepository.list_by_turn and list_messages don't filter by user_id
Look at the diff:
list_by_threadcalls_assert_thread_exists(filtered). ✅list_by_turnandlist_messagesdo not check the parent thread's ownership. The trace event hasuser_iddenormalized, but the SELECT is onlyWHERE thread_id = ....
Under RLS, this is fine — app.user_id is set, the policy filters, only the caller's events come back. But the add/add_batch paths go through _assert_thread_exists (which filters). For consistency, add _assert_thread_exists to list_by_turn and list_messages too. Belt-and-suspenders, and it makes the intent obvious to readers.
N6. agent_configs has no RLS policy in the 013 migration
Wait, it does — _TABLES = ("agent_configs", "threads", "trace_events", "api_keys") covers it. False alarm, my mistake reading the diff.
N7. InMemoryStore is a module-level singleton, AsyncPostgresStore is a module-level singleton — but _memory_store lives in factory.py
If you ever spin up two FastAPI apps in the same Python process (e.g. in tests, or a sidecar), the InMemoryStore singleton is shared across both. AsyncPostgresStore is too. For tests this is mostly fine (per-test pytest fixtures create fresh engines), but if you ever add a multi-tenant mode where two workspaces run side-by-side, the singleton will leak. Not a v1 issue — just a comment for the README's "architecture notes" section.
N8. JwtAdapter._find_key_by_kid falls back to the first key when kid is None
if keys:
return keys[0]This is a "JWKS rotation / header missing" safety net. Acceptable, but if the JWKS contains multiple keys (e.g. during a key rotation) and the JWT has no kid, you might verify with the wrong key. A safer behaviour is: when kid is None AND len(keys) > 1, log a warning and return None (so the JWT decode fails fast and the operator notices). When len(keys) == 1, the current fallback is fine.
N9. Logger.debug everywhere in the RLS listener — make sure prod log level is INFO+ by default
The listener emits logger.debug(LogMessage.RLS_CONTEXT_SET, uid) on every single query. With INFO-level logging this is silent — good. But if someone bumps the root logger to DEBUG in prod to debug something else, every query will log the user_id once per query. That's:
- Not a PII leak per se (user_id is your own opaque identifier).
- But it does turn a 100 QPS workload into 100 log lines/sec, which can choke a centralised log stack.
Consider gating the RLS listener log behind logger.isEnabledFor(logging.DEBUG) or, better, only logging on a sample (e.g. 1 in 1000 queries when debug is on).
N10. README is great, but the breaking-changes section deserves a "migration steps" call-out
You wrote:
Données existantes (agents, threads) →
user_id=''→ invisibles sous RLS
A three-line "migration SQL" example for re-assigning orphaned rows to an admin user would save a future operator an hour of psql archaeology:
-- Re-assign all pre-RLS rows to a bootstrap user (one-shot, before exposing to traffic)
UPDATE agent_configs SET user_id = 'bootstrap' WHERE user_id = '';
UPDATE threads SET user_id = 'bootstrap' WHERE user_id = '';
UPDATE trace_events SET user_id = 'bootstrap' WHERE user_id = '';Minor, but README hygiene.
N11. The factory does not propagate current_user_id to the StoreBackend per call
The StoreBackend is constructed once per agent build. Its namespace callable is lambda _r: user_namespaced("filesystem"), which re-reads the contextvar per call. Good — this is what you want. But the comment in _resolve_backend could mention this explicitly so a future maintainer doesn't "optimise" it to namespace=("filesystem",) and silently break per-user isolation.
N12. Two Settings instances per process
In dependencies.py:
settings = Settings() # at module import
# ...
def get_url() -> str: # in alembic/env.py
return Settings().database_url # another Settings()Each Settings() re-reads env vars + re-runs the URL normaliser. Cheap, but in alembic/env.py you do from src.config import Settings inside get_url, which is called on every migration run. Just import at module level like dependencies.py does, so the re-instantiation cost is one-time per process.
N13. init_persistence swallows the init_persistence exception in the lifespan
try:
await init_persistence()
except Exception:
logger.exception(LogMessage.APP_PERSISTENCE_INIT_FAILED)This is intentional (the app must still come up so /health returns 200 and you can debug). Fine — but a /health endpoint that returns 200 when the DB is unreachable is a footgun in load balancers. Consider a separate /health/ready (returns 503 when persistence is not initialised) and keep /health as a liveness probe. Standard k8s pattern.
What I really like
- Hexagonal discipline:
rls_context.py(contextvars) andrls_listener.py(engine hook) are separated from the auth code. The use cases don't know about RLS, only the repositories do. Easy to test, easy to reason about. current_auth_method+current_credentialas separate contextvars: clean separation between "who is this request" and "what credential should we forward to downstream MCP". Forwarding the right header to raganything (JWT vs API key) is a 1-liner now.- No PII in logs: every auth failure path uses
logger.warning("%s: %s: %s", enum_label, exc_type, exc_msg)— no token bytes, no email, no user identifier. Same for the JWT adapter. This is rare in the wild and I love it. selectinloadfor trace events: thePostgresThreadRepository.getcorrectly avoids the N+1 by loading trace events in the same query._model_to_threadthen re-sorts defensively. Good.- The BUG-002 fix (
GetAgentConfigUseCaseownership check before MinIO): this is exactly the kind of bug that hexagonal architecture is supposed to prevent (the YAML in object storage is not user-scoped, so the domain layer is responsible for the ownership check). Caught it, fixed it, and added the test. Textbook. - Alembic 011 → 012 → 013 → 014 chain: each migration has a single responsibility, an explicit
down_revision, and a clear docstring on what it does and what it does NOT do (e.g. 013 is explicit thatdb_enginefixture doesn't run migrations). Future archaeologists will thank you. - Test pyramid: 152 new unit tests + 98 QA integration tests + the QA init service that seeds two users. Plus the QA test names follow the
qa-user-1 / qa-user-2convention, so the failure mode is obvious in logs. secrets.token_urlsafe(32)for API key generation: 32 bytes of entropy = 256 bits, base64url-encoded → ~43 chars after thecpk_prefix. Thekey_prefix(10 chars) is enough to disambiguate inGET /api-keyswithout leaking the secret.
Minor / nits
src/security.pyline ~106: the docstring still says "Validates the incoming API keys against the configured master key" — this is the class docstring, but the class now does JWT + per-user API key + master key fallback. Update the class-level docstring.api_key_hasher.py(domain) vsinfrastructure/auth/api_key_hasher.py(re-export shim): the re-export shim is a nice touch for backward compat, but new code should import fromsrc.domain.services.auth.api_key_hasher. Worth a one-line comment in the shim.verify_credentialsreadsrequest.headers.get(...)twice (authorizationandx-api-key). Trivial perf, not worth a fix._assert_thread_existsinPostgresTraceEventRepositoryis duplicated almost verbatim inPostgresThreadRepository.get/delete. Not worth extracting yet (only 6 lines), but flag for the next refactor.- The
LlmNotConfiguredErroris registered as a global handler inmain.py— butLlmNotConfiguredErroris only raised from the factory path, which only runs when the agent is built. If the agent is already cached (case 2a), the error is never raised, and the user gets a confusing 500 with an openai.com auth error instead of a clean 422 "you haven't configured a provider". Consider lazy-checking inget_runnerthat the cached runner'sChatOpenAIinstance matches the current user's credentials, or always rebuild the runner for the current user.
Verdict
Approve with suggestions. The architecture is sound, the security model (RLS + JWT + per-user API key + per-user LLM + per-user namespace) is complete and consistent, and the test coverage is genuinely good. The blocking items above are mostly defensive (the cache-by-agent-name pattern works today, but is one RLS regression away from a real cross-user leak).
Recommend:
- Add the
key = (user_id, agent_name)cache key OR a one-line docstring comment inPersistentAgentRegistrysaying "the cache relies on RLS to prevent cross-user agent-name collisions". - Add a log line in
_resolve_modeldistinguishing "no auth context" from "no credentials configured". - Add a README migration snippet for the
user_id=''rows. - Consider an LRU on
find_active_by_hash(or write-coalescetouch_last_used).
After these, ship it.
Review written by SoluBot on 2026-07-27, against bce38c6 on main.
Propagate email/name/username JWT claims through AuthContext (kept None for API-key auth) and expose them as a public UserProfile projection so the front can display the connected user's real name instead of a hardcoded value. - AuthContext: add optional email/name/username fields - AuthService: populate profile claims from the decoded User on the JWT path - rls_context: new current_auth_context contextvar set by verify_credentials - dependencies: get_current_auth_context + get_get_current_user_use_case - New GetCurrentUserUseCase (Router -> Use Case mapping) - New /api/v1/users/me route mounted under the protected router - Tests: auth_service claim propagation + route (JWT full/partial, API key, 401)
Kaiohz
left a comment
Kaiohz
left a comment
There was a problem hiding this comment.
PR #39 — Review (SoluBot / SoluDevTech)
Score: 8.5/10 — Excellent, production-grade PR. The dual-auth + RLS + per-user LLM + namespace isolation design matches the pickpro pattern and is properly layered (domain ports → use cases → adapters). Test coverage is strong (+152 tests, real Docker QA stack). I have a handful of confirmations + one new finding on top of Kaiohz's self-review.
Solid work overall. The BUG-002 fix (cross-user agent leak via GetAgentConfigUseCase + MinIO) is exactly the kind of bug the hexagonal architecture is designed to make impossible — good catch before merge. The selectinload choice in PostgresThreadRepository.get avoids the trace-event N+1 cleanly. RLS listener is idempotent + SQLite-safe. The current_auth_method + current_credential split is a clean way to forward the right header to downstream MCP (JWT vs API key) without a giant if/else.
My comments below focus on independent verification of Kaiohz's self-review (most are confirmed) plus a few extras.
✅ Findings from the self-review I confirm
- #2 (Persistent runner cache is global) — real concern.
_runnersis keyed byagent_nameonly. RLS prevents cross-user collisions in prod, but the pattern is fragile. Recommend keying by(user_id, agent_name)so a future RLS regression doesn't silently leak a user A runner to user B. - #3 (
_resolve_modelswallowscurrent_user_id=None) — agreed, needs alogger.debugdistinguishing "no auth context" from "contextvar was lost" (e.g. a BackgroundTask running in a fresh context after the request scope ended). - #4 (no in-memory cache on
find_active_by_hash) — agreed. A 30s LRU on(hash → (user_id, key_id))+ write-coalescing ontouch_last_used(e.g. only UPDATE iflast_used_atis older than 5 min) would cut DB load by ~95% on a busy deployment. Add a "performance notes" section to the README. - N1 (
verify_credentials_wssilent return on no auth service) — agreed, preferRuntimeErrorconsistency with the HTTP path. Silent return paths in auth code are footguns. - N5 (
list_by_turn/list_messagesskip_assert_thread_exists) — confirmed, I just read the file. Two methods out of four bypass the ownership check. The RLS policy filters the result set so it's safe in practice, but the inconsistency is a footgun for a future maintainer who adds a non-RLS code path. Add the_assert_thread_existscall tolist_by_turnandlist_messagesfor defense in depth. - N4 (no plaintext in logs) — confirmed,
FernetCryptoandPostgresUserLlmSettingsRepositoryare clean. TheLLM_SETTINGS_DECRYPT_FAILEDlog only includes theuser_id(not the key or the encrypted token). ✅ - N9 (RLS listener logs
user_idon every query) — confirmed, and the gating suggestion is good. If you ever setLOG_LEVEL=DEBUGin prod to debug something else, every query becomes a log line.
🆕 New findings (not in the self-review)
N14. PostgresUserLlmSettingsRepository.upsert — created_at is reused on update, updated_at is correct, but the read after upsert returns a stale masked key
Looking at the diff: upsert sets created_at = NOW() and updated_at = NOW() on every call (even on update — see the migration upsert pattern). The UserLlmSettings returned to the client will have a created_at timestamp that changes on every PUT. Minor, but the response should reflect the original created_at (set on first insert, preserved on subsequent updates).
Verify the SQL: if it's a true PostgreSQL UPSERT (INSERT ... ON CONFLICT ... DO UPDATE SET ... EXCLUDED.updated_at, user_llm_settings.created_at), great — but the diff looks like a fresh insert pattern. Worth a quick psql check.
N15. verify_credentials does not clear contextvars on a re-used ContextVar token in middleware chains
current_user_id.set(ctx.user_id) returns a Token that is silently discarded. If a middleware later calls current_user_id.set(other_value) without reset()-ing the previous token, the value is overwritten. This is fine if the listener runs before any other middleware. But under FastAPI's middleware stack (CORS, GZip, etc.), a request that raises after the auth dependency has run could leak the current_user_id to a subsequent background task scheduled via BackgroundTasks (which run in the same contextvars context by default in Starlette).
Mitigation: wrap verify_credentials in a try/finally that reset()s the tokens on exit, or document that callers MUST re-resolve current_user_id.get() in background tasks. The current code is fragile by default.
N16. The cpk_ key generator uses secrets.token_urlsafe(32) (≈256 bits of entropy) — good, but the secrets.token_urlsafe(43) would give you a round 256 bits for the full key (not just 32 chars). Consider documenting the entropy in the docstring of ApiKeyHasher.generate_key.
N17. dependencies.py import-time side effect: Settings() is called at module import, then init_persistence is called in the FastAPI lifespan. If init_persistence is never called (e.g. a unit test that imports dependencies but only uses get_url), _root stays half-initialised and _require_*_repository() raises StorageError. This is intentional, but consider adding a _initialized: bool flag on _root so the error message is "persistence not initialized" instead of the generic STORAGE_REPO_NOT_INITIALIZED (the message is currently shared across all repos).
N18. UserLlmSettingsInput and UserLlmSettings have overlapping fields — UserLlmSettingsInput.api_key is plaintext, UserLlmSettings.api_key_masked is the masked preview. The Pydantic models are clean, but ensure the UpsertUserLlmSettingsRequest (the FastAPI request DTO) does NOT expose api_key in the response (only the masked preview). The diff shows the response uses UserLlmSettingsResponse which is the masked one — good. ✅ Just confirming.
N19. WebSocket verify_credentials_ws sets the contextvars but does NOT use the _reject_ws_with_401 401 + None return pattern when the auth service is None AND the legacy master_key is set
If both master_key is set (legacy) AND auth_service is None (dual-auth not wired), the WS endpoint falls through to verify_api_key_ws only if the route explicitly chains it. The current dependencies.py wiring is per-endpoint, so a WS endpoint that depends on verify_credentials_ws but not on verify_api_key_ws will silently accept any request. Document the "either-or" dependency contract clearly in ComposableAgentsSecurity, or unify the two dependencies into a single one that handles both cases.
N20. The factory.py refactor split _prepare_agent_namespace into 5 helpers (_cleanup_stale_agent_files, _maybe_delete_stale_item, _safe_delete, _copy_skills_to_agent_ns, _copy_memories_to_agent_ns) — good, but the helpers' signatures are all (store, ns, …) and don't have type hints on store (it's just an untyped store parameter). Add a BaseStore type hint so mypy catches a future refactor that passes a non-store object. Same for namespace.py — current_user_id.get() could fail at runtime if the contextvar is reset out of order, consider a defensive return current_user_id.get() or "" fallback (the or "" is currently the case for the empty-string default).
💡 Small suggestions (not blocking)
- N21. The RLS contextvar is named
current_user_idbut the MCP propagation usescurrent_credential(the raw JWT or API key) +current_auth_method(string literal). Consider acurrent_auth: AuthContext | Nonecontextvar (which already exists ascurrent_auth_context) and refactorcurrent_user_idto be a derived propertycurrent_auth_context.get()?.user_idfor consistency. Reduces the "which contextvar do I read?" cognitive load. - N22. README migration section is great. The only thing missing for me: a note that
OPENAI_API_KEYis no longer read at all (it's only mentioned in passing). Call it out in the breaking-changes section with a one-liner: "DELETEOPENAI_API_KEYfrom your env after upgrading — it's silently ignored." Currently a deployer might leave it in their.envforever as dead config. - N23.
013_enable_rls_policies.pyrunsFORCE ROW LEVEL SECURITY— good — but consider also adding aREVOKE ALL ON <table> FROM PUBLICandGRANT SELECT, INSERT, UPDATE, DELETE ON <table> TO <app_role>to lock down the SQL-level surface. The RLS policy assumes the app connects as a non-superuser; if the app ispostgres(superuser),FORCEis what saves you — but explicit GRANTs are clearer for a future operator. - N24. The test
test_rls_listener.pyexists, but I don't see a test that asserts_set_rls_guc_before_executeis idempotent (callingregister_rls_listener(engine)twice should not stack a second listener). Add one.
🟢 What I really like
- Layer discipline:
rls_context.py(contextvars) ↔rls_listener.py(engine hook) ↔rls_*repository methods are cleanly separated. The use cases never know about RLS. - No PII in logs: every auth failure path uses
logger.warning("%s: %s: %s", enum_label, exc_type, exc_msg)— no token bytes, no email, no user identifier. Rare in the wild. current_auth_method+current_credentialas separate contextvars is the right way to handle the MCP credential propagation. Forwarding the right header to raganything (JWT vs API key) is a 1-liner.- The
cpk_prefix on API keys is a nice operational touch — operators can recognize composable-agents keys in logs without leaking them. - The
selectinloadfor trace events + defensive re-sort in_model_to_threadis the right call. - Fernet-encrypted at-rest API keys with the
SECRET_ENCRYPTION_KEYenv var + the "generate a throwaway in dev" fallback is a thoughtful default. - The README's "Tables NOT protected by RLS" section is honest about the LangGraph store's asyncpg pool gap and explains the namespace-prefix mitigation. This kind of "here's what we didn't do and why" transparency is great.
Verdict
Ready to merge after addressing the 4 blocking-ish items (N14, N15, N19, runner cache #2) or after a maintainer's explicit acceptance of the current design. The PR is well-scoped, well-tested, and the architecture is sound. The remaining suggestions (N16–N24) are polish.
The N5 confirmation (list_by_turn / list_messages skip the ownership check) is the most important small fix — easy to add, prevents a future regression.
Reviewed-by: SoluBot (SoluDevTech)
1 participant
Context
Mise en prod de composables derrière oauth2-proxy (pattern pickpro), avec vérification du token JWT côté back (méthodes reprises de pickpro-back) et support des clés API par user. Isolation des données par user via RLS PostgreSQL + namespaces LangGraph Store. Clés LLM par user (Fernet-encrypted).
Changes
Auth (dual JWT / per-user API key)
JwtAdapterréplique de pickpro-back : JWKS via{LOGTO_URL}/oidc/jwks, validation audience + signature (algos RS256/ES256/ES384), cache JWKS en mémoire (TTLCache + asyncio.Lock anti-stampede), erreurs avalées →None(pas de PII dans les logs).AuthService.authenticate(authorization, api_key): JWT (Bearer) prend la précédence, sinon X-API-Key (hash SHA-256, lookup tableapi_keys).verify_credentials(HTTP) +verify_credentials_ws(WebSocket) : posent les contextvarscurrent_user_id,current_credential,current_auth_methodpour la RLS et la propagation MCP.LOGTO_URL,JWT_AUDIENCE. L'ancienne master keyAPI_KEYest dépréciée.Per-user API keys
api_keys(id, user_id, name, key_hash unique, key_prefix, revoked_at, last_used_at, created_at). RLS.POST/GET/DELETE /api/v1/api-keys: create renvoiecpk_...en clair une seule fois, list masque le hash, revoke idempotent, isolation cross-user (404).touch_last_usedmis à jour à chaque auth par clé.RLS per user_id (PostgreSQL)
user_id(NOT NULL default '') + index suragent_configs,threads,trace_events,api_keys,user_llm_settings. Migrations 012-014.ENABLE/FORCE ROW LEVEL SECURITY+ policyuser_id = current_setting('app.user_id', true).before_cursor_execute:SELECT set_config('app.user_id', $1, true)par requête (no-op sur SQLite). Idempotent.system_rls_context()pour les jobs background (bypass RLS).storen'est PAS couverte par RLS (elle utilise son propre pool asyncpg, pas l'engine SQLAlchemy qui set le GUC) — l'isolation des skills/memories est enforced au niveau applicatif via les namespaces.Per-user LLM credentials
user_llm_settings(user_id PK, provider, base_url, api_key_encrypted Fernet). RLS.GET/PUT/DELETE /api/v1/settings/llm: GET masque la clé, PUT chiffre (Fernet viaSECRET_ENCRYPTION_KEY), isolation cross-user.create_agent_from_configrésout les credentials LLM par requête depuis l'utilisateur authentifié (contextvar) ; fallback env quand pas d'auth (tests).LlmNotConfiguredError(422) si non configuré.OPENAI_API_KEYn'est plus utilisé.Store namespaces per user
user_namespaced(*suffix):(user_id, *suffix)quandcurrent_user_idset, sinon(*suffix)(legacy)./api/v1/store/files) et namespace agent (/agents/{name}/skills/,/agents/{name}/memories/) isolés par user.MCP credential propagation
${USER_JWT}/${USER_API_KEY}dans les headersmcp_serversdes YAML agents, résolus depuis le credential de l'appelant. Headers résolus vides droppés.haiku-rag*.yaml,haiku-files-local*.yaml).Bug fixes (trouvés en QA)
api_keys_routern'était pasinclude_routerdans main.py → endpoints 404. Corrigé.GET /api/v1/agents/{name}leakait cross-user via MinIO (lecture directe du YAML sans check d'appartenance).GetAgentConfigUseCasevérifie maintenant l'appartenance via le repo RLS-filtré avant MinIO.Quality gates
composable-agents-qa-initQA / local stack
Breaking changes
API_KEYest dépréifiée → dual JWT/API key par user. Les clients doivent créer une clé API viaPOST /api/v1/api-keys(nécessite un JWT en prod) ou utiliser un JWT.OPENAI_API_KEYn'est plus utilisé → chaque user configure son provider viaPUT /api/v1/settings/llm.user_id=''→ invisibles sous RLS (réassignation manuelle SQL possible).Tests
Suites / prochains tickets
api_keys)