feat(auth): connect Memoria identity and scoped memory - #704
Conversation
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Conclusion: Request changes
The direction is valuable: a scoped key is safer than a Server master key, the CLI does not persist the Memoria secret, and read/write checks fail closed. However, this head introduces a parallel authentication and credential lifecycle and does not yet preserve Astra identity, consent, or deployment boundaries.
1. P1 — external identity is not namespaced by issuer and bypasses the canonical provider identity path
login_verified_identity derives the Astra account from only memoria_user_id, and the new table also keys only that subject. If MEMORIA_BASE_URL moves to another self-hosted instance, the same subject value is mapped to the existing Astra account. That is an account-confusion or takeover boundary: an external principal is (issuer, subject), never just subject.
The repository already has a provider-scoped external identity path using provider_id + external_subject, but this PR adds a Memoria-only table and emits tokens with origin internal, losing authentication provenance. Extend the canonical external identity/session lifecycle, or extract a generic verified-provider path keyed by a stable issuer/provider identifier. Do not add a second identity state machine.
2. P1 — one verified key can be sent to a different Memoria endpoint
/auth/memoria verifies against state.memoria_base_url, but the Server loop, background extraction, and session-end governance independently re-read MemoriaSettings::from_env(). build_server_state(settings) and AppState::with_memoria_config are explicit composition boundaries, so an injected setting or override can verify the secret against endpoint A and later decrypt and transmit it to endpoint B.
Carry one application-scoped Memoria provider identity/configuration and one user-credential resolver through every proxy, prompt recall, extraction, compaction, and governance path. A generic with_pool method must not implicitly select an external transport.
3. P1 — login and credential lifecycle is neither atomic nor revocable
The handler commits an Astra account and refresh session first, then writes the encrypted connection key in a separate transaction. A failure in the second phase returns login failure after durable session creation. Concurrent first logins can also race on account creation, and concurrent key replacements can leave more than one active row because auth_tokens has no unique active binding generation.
There is also no unlink/disconnect endpoint. astra logout revokes only one Astra refresh token, while the active encrypted Memoria bearer remains retained indefinitely. The schema inventory says removal happens through an explicit unlink or account-deletion workflow, but this PR implements neither. Put identity, session, and credential binding behind one service-owned transaction/state machine; add idempotent link, relink, revoke/unlink, and account-deletion behavior with an explicit retention contract.
4. P1 — access modes are checked too late in the runtime lifecycle
with_pool installs a Memoria port for every database-backed run. Accounts with no binding therefore attempt optional recall and report Unavailable instead of NotAttempted. For a read-only binding, background extraction can load memory, resolve an LLM selector, generate an update, and only then fail when store calls client(true). This creates repeated cost and error noise for work that consent already made inadmissible.
Resolve a typed per-user binding generation and access mode at the operation admission boundary. No binding or none should be a normal disabled state; read-only must not schedule write-oriented extraction, reflection, consolidation, or cleanup.
5. P1 — the default CLI journey breaks local and self-hosted Astra
No-argument astra login now always opens https://thememoria.ai, replacing the existing interactive username/password flow. The all-in-one stack points Astra Server at the local http://memoria:8100 and its documentation tells users to run no-argument astra login; a cloud website key cannot authenticate against that local issuer. The CLI-selected MEMORIA_WEB_URL is not discovered from or bound to the Astra Server configuration.
Preserve the existing login journey or present an explicit provider choice discovered from the target Server. The Server must own the trusted issuer and authorization URL. Also reject plaintext remote MEMORIA_WEB_URL values, and test the callback launcher on Windows: cmd /C start receives a URL containing & separators.
Required verification and documentation
The new tests cover a scope classifier, one callback parser happy path, and client-side secret persistence. They do not exercise the public auth route, a real identity/credential transaction, issuer changes, concurrent login/relink, partial failure, revocation/unlink, no-credential turns, read-only extraction, or callback rejection/platform behavior. Add focused DB and HTTP contract tests for these unhappy paths and an Astra-to-Memoria contract test pinned to the scoped-key API.
This PR changes a public endpoint, the default CLI command, configuration, credential retention, and the memory runtime contract but updates no public or design documentation. Update the CLI/API/configuration/deployment and memory/auth contracts in the same change.
Current CI is green and the merge tree is clean, but those facts do not cover the ownership and lifecycle failures above. The branch is behind current main; rebase after the design is corrected.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of 2b5e47a: REQUEST_CHANGES.
The revision materially improves the previous design: issuer-scoped canonical identity mapping, atomic credential/session persistence, account-serialized disconnect/refresh, a composition-owned memory resolver, early memory-consent admission, and Server-discovered CLI login are now present. The BYOK transport also explicitly pins validated public DNS answers and separates operator proxy configuration from user endpoint input.
Three remaining findings are detailed inline:
- P1: background memory inference bypasses the owner-scoped deployment-model eligibility applied to primary inference.
- P2: passwordless Memoria accounts have no working path to the existing purpose-bound reauthentication proofs required for device/session recovery.
- P2: the new real-DB/live contracts fail before their behavioral assertions in the repository's online lanes.
Validation:
- Executed the schema suite on this head: 47 tests passed. git diff --check passed.
- Inspected current GitHub checks and downloaded the failed online-job logs. Static Checks and the offline runtime/CLI/services jobs passed. Both online lanes failed. In addition to the new fixture failures described inline, the integration lane fails system_matrix_http_e2e::e2e_matrix_models at journey_models_matrix.rs:88 because the expected single access now includes Cloud BYOK; update that expectation while retaining default/pagination assertions for the intended product behavior.
- Local Cargo/Rust and a disposable MatrixOne runtime are unavailable, so I did not rerun Rust, database, provider, OAuth, or Windows browser tests. The two implementation findings are source-traced; the reported CI failures are directly observed in this head's job logs. Earlier PR-body test results are not treated as current-head validation.
Before approval, cover the full user journey after login and the actual background inference route, not only credential CRUD or a mock memory port's access boolean.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of 7a04fef: REQUEST_CHANGES.
This is materially better than 2b5e47a. The previous background deployment-key bypass is addressed at candidate resolution and by canonical owner-scoped revalidation before each background provider call. Hosted users take the deterministic/degraded memory path instead of silently spending deployment credentials. The fresh Memoria step-up now feeds the existing purpose-bound proof mechanism, and the Web/SDK consumer discovers the method and clears one-time evidence after an attempt. The database-name contract and explicit opt-in external-test feature address the previous CI provisioning mismatch.
Two remaining defects are detailed inline:
- P2: disconnect/reconnect with the same upstream key resurrects unexpired step-up authorization.
- P2: the mandatory personal-model probe rejects valid OpenAI o-series configurations because its wire request differs from normal inference.
Validation on this head:
- Locally executed
python3 -m unittest discover -s scripts/ci -p 'test_*.py': 60 passed.git diff --check e4dbe0d1...HEADpassed. - Inspected live GitHub checks. SDK and Web typecheck/test/build jobs passed, as did the offline Rust test jobs. Downloaded the turn-core/services/plan job log: 5,981 tests passed, with explicit passing results for the new TCP-only DNS unhappy-path test and binding-hash test. These are CI-executed tests, not local Rust executions.
- Both online lanes were still running at submission; the new real-DB reauthentication and background-inference contracts are therefore not claimed as passing here. The previous failures are not assumed to remain.
- Local Cargo/Rust and MatrixOne are unavailable. The findings are source-traced; the probe incompatibility is additionally verified against the current official OpenAI API reference linked inline. No live paid-provider request or Rust reproduction was executed.
- The GitHub connector returned 404 for the referenced memoria-website PR #216, so I could not independently inspect the email/attestation issuer implementation. The Astra test uses a deterministic attester; it does not validate that external email flow or deployment.
Please extend the real public-route tests through disconnect/relogin and the real probe path through provider-specific request validation. These test boundaries matter more than additional happy-path credential CRUD assertions.
Bind step-up proofs to an Astra-owned connection lifecycle and invalidate pending proofs atomically on disconnect. Share output-token wire rules between inference and provider probes while preserving native Anthropic parameters. Add same-key reconnect and in-flight reauthentication HTTP regressions, strict provider wire fixtures, and isolated database credential-rotation coverage. Document lifecycle compatibility and the opt-in probe test command.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact commit e8007db9bc37449e4a0fe5dac46b27239034b7a5: REQUEST_CHANGES.
The previous same-key reconnect finding is addressed: the binding now carries an Astra-owned lifecycle nonce, disconnect deletes pending proofs transactionally, and the second binding validation detects a reconnect while website attestation is in flight. Routine login preserves an uninterrupted binding. The extended public-route regression covers old device/takeover proofs, in-flight issuance, and successful fresh verification after reconnect.
The OpenAI o-series probe now uses the same output-limit serialization as inference. However, the shared serializer also applies OpenAI's field to DeepSeek, and the new provider fixtures encode that same incorrect assumption. One P2 provider-contract finding is detailed inline.
Beyond the previous findings, I traced credential/proof ownership, model create/rotation/probe/default paths, background inference revalidation, and the custom-endpoint transport lifetime. I have not confirmed another blocking defect.
Validation on this head:
- Locally executed the schema suite: 47 passed; CI-script suite: 60 passed;
git diff --check e4dbe0d1...HEADpassed. - Live PR Title, Static Checks and Test Suite workflows all succeeded. Both online lanes now succeeded, rather than remaining pending as in the previous review.
- Downloaded the services/turn-core/plan job log: 5,982 tests passed, including the new connectivity-probe test. That passing mock does not establish the real DeepSeek wire contract.
- Downloaded the online integration job log: 270 tests passed, including
memoria_step_up_authorizes_real_device_and_takeover_routes_without_passwordswith the reconnect changes. These are CI executions, not local Rust executions. - Local Cargo/Rust and a disposable MatrixOne service are unavailable. The separately feature-gated/ignored
user_model_probe_db_itis not claimed as executed. No live paid-provider call, external website email/OAuth flow, or Windows browser launch was performed. The remaining finding is based on the actual request-construction path and current official provider documentation, not an observed live-provider rejection.
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head. No remaining P0/P1 findings. The previous concerns around background model eligibility, re-authentication lifecycle, BYOK probing, and CI coverage are addressed. All required checks are green. Approving.
Use max_tokens for the native DeepSeek provider in the shared inference/probe serializer while retaining OpenAI max_completion_tokens and Anthropic max_tokens. Keep existing thinking-budget policy and generic compatible routing unchanged. Correct strict DeepSeek probe fixtures and independently specify provider wire expectations. Cover streaming/nonstreaming inference, existing thinking budgets, model creation, credential rotation, and invalid-key/model rejection without overwriting persisted credentials. Validation: 1 core wire test, 285 LLM client tests, 116 model-service tests, and 1 isolated provider-probe DB test passed. Core/services/runtime library clippy with warnings denied, formatting, and diff checks passed. No live paid-provider request was made.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-review of exact commit 15670a2e03c32224565770df2b41bfb7c7b64c18: APPROVE for this PR considered independently.
The previous DeepSeek finding is fixed: the shared serializer retains max_tokens for native DeepSeek/Anthropic and uses max_completion_tokens for OpenAI. The probe fixtures now enforce independent provider expectations, and the real inference request-builder tests cover both streaming modes. I reviewed the five-file delta from e8007db and its surrounding probe/request-construction paths, building on the prior full review. I have not confirmed another blocking defect.
Integration note after comparing #712 at f1058fb: these are different execution placements with overlapping product surfaces. #704 uploads an explicitly entrusted personal credential to Server and supports execution without an attached Runner; #712 keeps credentials/provider I/O on a selected Runner. Both change astra model add, model admission, background inference, and the model-access design. In particular, #712's design excludes arbitrary user-supplied Cloud Server inference URLs, whereas this PR deliberately supports policy-validated public HTTPS endpoints. Resolve that product-policy decision explicitly before combining the branches. Preserve one Offering/admission/usage authority, distinguish Cloud versus Runner setup before collecting a secret, and never silently transfer credentials or fall back across placements. This is an integration recommendation, not evidence that the branches have already been combined incorrectly.
Validation: current PR Title, Static Checks and Test Suite workflows succeeded, including both online lanes. I downloaded the current runtime job log (101929659483): 5,585 runtime tests passed, including the new provider-contract request-builder test and three DeepSeek output-limit regressions. The current services/turn-core/plan log (101929659638) reports 5,982 passed, including the corrected connectivity-probe test. These are CI-executed tests, not local executions. No local Rust toolchain or disposable MatrixOne is available; the separately opt-in/ignored provider DB contract, live paid-provider calls, external email/OAuth flow, and combined #704+#712 behavior were not executed in this review.
Merge Queue Status
This pull request spent 12 seconds in the queue, including 1 second running CI. Required conditions to merge
|
## What type of PR is this? - [x] feat (new feature) - [x] fix (bug fix) - [ ] docs (documentation) - [ ] style (formatting, no code change) - [ ] refactor (code change that neither fixes a bug nor adds a feature) - [ ] perf (performance improvement) - [x] test (adding or updating tests) - [ ] chore (maintenance, tooling) - [ ] build / ci (build or CI changes) ## Which issue(s) this PR fixes Related: #245 This reference intentionally does not automatically close the cross-repository issue. ## What this PR does / why we need it Provide reusable, least-privilege credentials for Astra account sign-in and optional memory sharing. - Add API-key scopes and stable `GET /auth/whoami` identity metadata. - Enforce identity/read/write/key-management scopes across REST and MCP. - Prevent identity-only/read-only/read-write connection keys from administering groups, managing keys, or reaching unclassified authenticated routes. - Preserve scopes during rotation and maintain explicit compatibility for legacy/default keys. - Reject unauthorized memory access before per-user memory provisioning. - Make whoami verify the current database grant rather than trust a warm replica-local cache, so login/refresh observes revocation made through another API replica. - Decouple MCP authorization from metrics invalidation. Explicitly require `memory:write` for `memory_apply`, `memory_rebuild_index` and `memory_tune_params`. - Classify exact dispatch names rather than sanitized tracking labels; reject unclassified tools by default, including with master credentials. - Apply the same admission to requests and notifications, with real HTTP/database regression coverage. ## Verification ### Latest MCP review-fix update (58744ff) - `cargo test --locked -p memoria-api --lib`: 93 passed. - `cargo test --locked -p memoria-api --test api_e2e test_scoped_ -- --nocapture`: 3 passed. - `cargo test --locked -p memoria-api --test api_e2e test_mcp_ -- --nocapture`: 16 passed. - HTTP tests used isolated local MatrixOne databases. The new regression checks all three missed write tools, notification handling, write-authorized positive controls, readable tools, identity-only denial and unknown/malformed names. - Persisted-state assertions confirm denied requests/notifications do not tune retrieval parameters, while the same write-authorized operation does. - Targeted Rust formatting and `git diff --check`: passed. - No user test data was cleared or local Memoria service replaced. ### Previously recorded verification Local validation before push: - Memoria API compile checks and targeted auth/MCP scope unit tests passed. - Real-database scoped-key API tests passed, including group-administration denial and uncached whoami revocation. - Seven legacy API-key regression tests passed. - Astra-to-real-Memoria API v1 contract passed for identity-only, read-only, read-write, master-key rejection, stable account identity and revocation. - Latest upstream main was merged; its intervening changes have no net source-tree delta relative to the tested Memoria implementation. - `git diff --check` passed. Production OAuth and GitOps deployment are not covered by these API tests. ## Cross-repository dependencies - Astra login, scoped credential lifecycle and user models: matrixorigin/Astra#704 - Website follow-up (login separated from memory sharing and local testing): matrixorigin/memoria-website#216 - Initial website integration, already merged: matrixorigin/memoria-website#213 No MatrixOne source changes are included.
3 participants
Summary
Implement hosted BYOK model configuration and scoped Memoria account sign-in, including the fixes requested in the review of this PR.
Add authenticated owner-scoped
/me/modelsAPIs and CLI model configuration, default selection and probing.Support OpenAI, Anthropic, DeepSeek and OpenAI-compatible endpoints through the existing provider/runtime path.
Protect user-supplied model endpoints with public-address admission, DNS/transport validation and explicit server-owned proxy configuration.
Resolve Memoria identities through the canonical provider/subject mapping, preserving issuer provenance instead of issuing an internal identity.
Commit the identity, encrypted connection credential and refresh session atomically; serialize concurrent binding changes and add disconnect/retention behavior.
Carry one application-scoped provider and credential resolver through proxy, recall, extraction, compaction and governance.
Treat missing/none memory access as disabled and deny write-oriented extraction before memory reads or LLM work for read-only users.
Discover the login website from the target Server, preserve self-hosted password login, and harden loopback callback parsing and Windows browser-command construction.
Add fresh Memoria account reauthentication for device trust, reenrollment and forced Work takeover, bound to issuer, subject, credential generation and purpose; update the SDK/Web consumer.
Revalidate background memory inference through canonical model admission on every provider attempt, avoiding stale credentials and deployment-key fallback for hosted users.
Add explicit TCP-only BYOK DNS configuration without UDP fallback; keep public-address validation and test synthetic DNS answers.
Isolate database fixtures and gate external Memoria/provider contracts behind an explicit opt-in feature so ordinary fork CI does not require unprovisioned services.
Related issue
Related: matrixorigin/memoria#245
Cross-repository dependencies:
These references intentionally do not automatically close the cross-repository issue.
Change type
User and compatibility impact
MEMORIA_WEB_URLis a Server setting exposed throughGET /auth/methods; the CLI no longer chooses its own website. Remote browser login URLs require HTTPS.MEMORIA_ISSUERgives the integration a stable identity namespace. Existing pre-issuer mappings require an explicitMEMORIA_LEGACY_ISSUERassertion and fresh login; migration preserves the Astra account, models and Work/history.Architecture and complexity delta
Verification
Latest review-fix update (7a04fef)
cargo fmt --all -- --check: passed.npm test -- src/__tests__/work-contract.test.ts: 60 passed.npm test -- __tests__/components/app/work-turn-composer.test.tsx __tests__/lib/work-criteria-actions.test.ts: 38 passed.git diff --check: passed.CARGO_INCREMENTAL=0 CARGO_PROFILE_DEV_DEBUG=0 CARGO_PROFILE_TEST_DEBUG=0 cargo test --locked -p astra-services --lib byok_endpoint, but the localquotedependency build-script artifact could not execute (exit 126;fileidentifies it as data). No Rust test ran in this attempt. This is not reported as a passing Rust gate; CI and a clean/local-cache Rust rerun remain required.Previously recorded verification
After merging the current upstream main and preserving the existing remote PR history:
cargo check --locked -p astra-runtime -p astra-cliwithCARGO_INCREMENTAL=0: passed.cargo fmt --all -- --check: passed.python3 -m unittest discover -s scripts/schema -p 'test_*.py': 47 passed.git diff --check github/main...HEAD: passed.Earlier local validation of the feature/review-fix implementation, before this final upstream synchronization:
Live GitHub/Google OAuth callbacks and actual Windows browser launch have not been exercised. The Windows test validates command construction, not an actual Windows browser. Full post-merge test/CI validation remains required; the earlier test results are not presented as results for the newly synchronized head.
Final checklist