Repository navigation
chore: remove identity and API key management routes - #1361
henrypark133 wants to merge 9 commits into
Conversation
They move to cloud-platform. GET /v1/admin/platform/infra-summary and the encryption routes stay. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ement router AppState stays; only the router that used it goes. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Tests whose subject is a deleted route go. Tests of kept code that used a deleted route for setup or observation (API key validation and revocation, per-key spend limit, org deletion fail-closed, the org system prompt and allowed_models filter, admin token validation and audit) are seeded by SQL and probe kept encryption routes instead. The retired-route table lists the deleted routes. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
User erasure, contact email, UserService, the invitation repository and sender, and the organization service and repository methods behind the deleted routes. The organization service keeps what AuthService, the workspace service and the responses service call. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Session logout/access-token validation, inactive-user guards, API key expiry, and the route-guard table now run without the deleted identity routes. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
henrypark133
left a comment
There was a problem hiding this comment.
Verdict: approve-with-fixes (comment only; no blockers found). Fixes are docs and comment cleanups.
Reviewed origin/chore/remove-catalog-writes...HEAD (65 files, about -26.6k lines). cargo check --workspace --tests --offline passes with no warnings. I did not run the e2e suite because it needs a database, so test behaviour is unverified.
Route table comparison (verified)
I extracted 49 removed method+path pairs from the deleted #[utoipa::path] blocks and compared them with cp-compat-job/crates/api/tests/fixtures/routes.snapshot. Every removed route is registered by cloud-platform. None is missing. That includes the admin access-token routes (AdminSession), erasure (AdminSession / Admin), invitation deliveries, PUT /v1/admin/organizations/{org_id}/members/{user_id}, and POST /v1/users/me/access-tokens (RefreshSession).
- Caveat: this matches utoipa annotations, not the router calls. The router calls in
api.rsandlib.rsline up with the same list. - The snapshot lists three identity-adjacent routes that this PR did not remove:
GET /v1/admin/users,GET /v1/admin/organizations/{org_id}/membersandGET /v1/workspaces/{id}/api-keys/{id}/usage/history. An earlier PR in the stack removed them, andadmin_retired_routes.rscovers them.
What stays (checked in code)
- API key validation and cache:
ApiKeyRepositoryand the auth middleware are untouched. The only change is that the key create, revoke and spend-limit writes are gone. The newseed_revoke_api_keyandseed_api_key_spend_limithelpers write the DB directly, sotest_deleted_api_key_cannot_be_usedno longer exercises revoke-then-cache behaviour. That is acceptable, because the route is gone and the cache is not changed here. - Org and workspace lookups in inference:
OrganizationRepositorykeepsget_by_id,get_by_name,get_member,update,remove_memberandcreate.OrganizationServiceTraitkeepscreate_organization,is_member,get_user_roleandget_system_prompt.WorkspaceServiceis unchanged. The compile passing shows that no retained caller used a removed method. allowed_models: the read path (Organization::allowed_models,organization_model_filter) is unchanged. The write handler'sinvalidate_organization_model_filtercall was removed with the route. Writes now come from cloud-platform, so cache invalidation after a settings write becomes its concern (see question 1).- Sessions and login until E4:
init_auth_servicesstill buildsAuthService,UserRepositoryand the session and OAuth repositories. The org service is now constructed withnew(repo). Login only needscreate_organization.session_logout.rsand the newinactive_user_guards.rscover this, but I did not run them. - User reads for
adm_tokens:authenticate_admin_access_tokenusesAdminAccessTokenRepository::validate, which is unchanged, andAuthState's user repository is unchanged.UserService::get_useris gone, but nothing on this path used it. TheUserServicewiring was removed. - Usage attribution and staking preflight: the usage middleware and
staking_farmservices are untouched.ErasureBlocker::StakingActiveanddelete_if_no_staking_farm_sourcewere part of the deleted routes and have no remaining caller.
Findings
- should-fix, verified. Stale docs describe removed routes.
docs/local-development.md(lines about 149, 201, 218, 330-333, 471) tells developers to get an API key throughGET /v1/users/meandPOST /v1/workspaces/{id}/api-keys. Those routes return 404 here, so the local-dev flow in that doc no longer works.docs/architecture/c4-diagrams.md(lines about 36, 410-422, 639-672, 1353-1357) also lists them. Owner: PR author. Update the docs, or point to theseed_*or cloud-platform route. - nit, verified. Stale comments name a removed route.
crates/services/src/organization/ports.rs:52("validated write path isPATCH /v1/organizations/{org_id}/settings") and the header ofcrates/services/src/organization_model_filter.rs:4still point at it. The remark about a "full settingsPUT" is also stale. Reword them to say cloud-platform owns the write path. - nit, plausible.
AdminAudit::record_organizationandrecord_pageare now#[cfg_attr(not(test), allow(dead_code))]. Fine for a stack, but track their removal.InvitationEmailConfigand itsfrom_envare probably unused in runtime code now (config/src/types.rs:113). Those env vars still parse and have no consumer. I did not check whether anything else reads them. - question. When cloud-platform writes
allowed_modelsstraight toorganizations.settings, how does the inference-side filter cache (invalidate_organization_filter) get invalidated? Before this PR the API route did it in-process. The model-resolve cache TTL bounds the delay, but this deserves a rollout note, since the PR removes the only in-process invalidation caller for this path. Owner: rollout (Harbor) and the cloud-platform owner. - question (rollout note for the orchestrator). This removal is only safe once cloud-platform serves these routes at the edge. The snapshot says it does. The same holds for the dependent invitation email and contact-email verification (daily-cap table) behaviour: after this PR cloud-api no longer sends those emails, so the sender must live in cloud-platform. Confirm before deploy.
Not run
Security, performance and data lanes were not run separately. The e2e and nextest suites were not run. The removal of the contact_email::serial_ nextest override is consistent with the deleted tests.
Minimum fix set: finding 1, optionally 2. Findings 4 and 5 are confirmations rather than code changes.
Review · Status🟩 CompletedIronLoop completed the review and posted it to GitHub. ResultRun detailsAutomatic trigger · attempt 1 of 3 · completed in 18m 46s |
…ale docs - docs: local-development and C4 diagrams no longer describe removed organization/workspace/API key/user routes - comments: stop naming the removed settings PATCH route - remove AdminAudit record_organization/record_page (and the page fields and paginated-list e2e guard), InvitationEmailConfig (and its env vars), and the uncalled WorkspaceServiceTrait write methods with their permission helpers Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Review · Summary
🟢 No actionable findings
Review complete: no additional actionable issues found in this change.
Validation
- ✅ Static review — Reviewed the route removal, retained callers, service wiring, and focused regression coverage.
- ✅ Workspace compilation — The workspace and its test targets compiled successfully.
Review details
- Run:
2a9b032f-1fa3-40e4-b436-22e5b3b5be0b - Attempts: 1
lloydmak99
left a comment
There was a problem hiding this comment.
This PR cleanly removes the identity and API-key management routes along with their now-dead service wiring. Router definitions, call sites, and state structs (AppState, AdminAppState, DomainServices) were updated in lockstep, and retained paths — inference, API-key validation, org/workspace lookups, login/sessions, encryption, and admin infra-summary — remain wired.
No blocking issues found. A couple of optional, non-blocking notes:
OrganizationServiceImpl::new(repo)now only stores the repository;create_organization(used by login) relies solely onrepository.create(), so the slimmed constructor is safe.- The
fallback_enablede2e coverage gap is author-acknowledged and out of scope here.
This is a draft gated on the upstream client move; the main risk (a removed route not served by cloud-platform) was verified — all 49 removed routes are served upstream.
Checks: cargo fmt --all -- --check passed; git diff --check passed; GitHub lint/unit/integration/E2E/security-audit checks passed. cargo check --workspace --tests --offline could not run (no cc linker in the review environment), compensated by grep-based dangling-reference checks across production and test trees — all clean.
|
Superseded by #1413, rebuilt on current main. |
Do not merge. Stacked on #1360. Prepared ahead of the client move; it needs a longer soak than the PRs below it, because a remaining caller here would break users.
Removes organization, member, workspace, user, invitation and API key management routes, admin member, invitation, erasure and access-token routes, and the identity services and repositories nothing else uses. cloud-platform registers these routes.
Kept: API key validation and its cache, organization and workspace lookups on the inference path,
allowed_modelsenforcement,get_user_by_id,adm_token validation, login and sessions (a later PR removes those), and encryption routes.Kept-behaviour tests ported with SQL or mock seeding: session logout, inactive-user guards, API key expiry. Known gap: the
fallback_enabledorganization setting is read on the inference path and has no end-to-end test; it never had one apart from the deleted settings route.Checks run: fmt, clippy with all targets,
cargo test -p api --lib, local e2e 671 passed, database and services under nextest.test_spki_fingerprint_verifierfailed on a live network request in one run.🤖 Generated with Claude Code