feat: rescope user session issuer reads and writes to the organization tier - #5907
Open
bflad wants to merge 8 commits into
Open
feat: rescope user session issuer reads and writes to the organization tier#5907bflad wants to merge 8 commits into
bflad wants to merge 8 commits into
Conversation
… children https://linear.app/speakeasy/issue/AIM-100/feat-rescope-user-session-issuer-reads-and-writes-to-the-organization Adds the boundary tests that the organization-tier rescope has to preserve, written against current behavior so they are green before the predicate changes and stay green after. The rescope replaces `project_id = @project_id` with `project_id = @project_id OR (project_id IS NULL AND organization_id = @organization_id)`. Every row written today carries both columns, so the `project_id IS NULL` guard is the only thing confining the second arm to genuine organization-tier rows. Without it the arm matches every sibling project's rows in the same organization, which is a cross-project read on the listings and a cross-project write on the revoke and delete paths. Each test gives a second project in the caller's own organization a full issuer subtree (issuer, client, session, consent, CIMD grant) and asserts the caller can neither read, list, count, revoke, nor delete any of it, then re-reads the sibling rows under their own project to prove no mutation leaked across. Facet counts are covered too, since the three facet queries scope through the issuer the same way the listings do. Verified the assertions have teeth by weakening `GetUserSessionIssuerByID`'s project predicate and confirming TestCrossProjectIsolation_Issuer fails.
…n tier https://linear.app/speakeasy/issue/AIM-100/feat-rescope-user-session-issuer-reads-and-writes-to-the-organization Widens every tenancy predicate in the usersessions queries from `project_id = @project_id` to project_id = @project_id OR (project_id IS NULL AND organization_id = @organization_id) so an organization-tier issuer, and anything hanging off one, is reachable. Without this the nullable `project_id` makes those rows invisible to every read, revoke, and facet count rather than producing an error. Always on, with no `include_organizational` opt-in. There is no global tier for user session issuers, so the flag `remote_session_issuers` carries has nothing left to protect here, and a call site that forgets one silently drops rows. 26 predicates across 27 blocks. `@organization_id::text` is cast so the generated parameter is a plain string rather than a `pgtype.Text` whose zero value would silently match nothing. Every affected call site already had an organization in scope: `endpoint.OrganizationID` on the OAuth paths, `authCtx.ActiveOrganizationID` on the management API, `toolset.OrganizationID` in the model views, and `principal.OrganizationID` in Platform MCP, so no organization had to be derived from a resolved endpoint. `exhaustruct` surfaced all 38 of them once the field existed. Beyond the predicates: * `GetLatestLiveUserSessionToolSelection`, `TouchUserSessionLastUsed`, `PurgeUserSessionClientCIMDCache`, and `UpdateUserSessionClientFromCIMD` filter the child row's own `project_id` rather than reaching through the issuer. That column is NULL for an organization-tier issuer's children, so these degraded silently rather than 404ing: reauth prefill lost, `last_used_at` never updated, and the CIMD refresh endpoint matching no rows. * `CreateUserSessionIssuerCimdClient` stamped the caller's `@project_id` onto the child. It now takes `iss.project_id`, so the child inherits its parent's tenancy and an organization-tier parent contributes none. * `GetUserSessionIssuerBySlug` is deliberately left project-only. Slug uniqueness is indexed per project with no `(organization_id, slug)` equivalent, so admitting organization-tier rows would make a `:one` query non-deterministic across duplicate slugs. Two things are deliberately deferred and noted in the SQL: * The owner-existence subqueries in `DeleteUserSessionIssuer` stay project-scoped. An organization-tier issuer can be referenced from any project in the organization, so they would have to sweep the whole organization to be correct. That belongs with AIM-99, which decides who may administer an organization-tier issuer at all. Nothing can create one yet, so no reachable row exercises the gap. * `LockUserSessionIssuerForMetaMCP` is untouched. AIM-95 records why: `meta_mcp_servers` pins its reference with a composite foreign key on (project_id, user_session_issuer_id) that can never match a NULL project_id, so admitting the tier in that lock alone would turn today's clean 404 into a foreign key violation surfacing as a 500.
…anization tier https://linear.app/speakeasy/issue/AIM-100/feat-rescope-user-session-issuer-reads-and-writes-to-the-organization Finishes the rescope in the packages that reach user_session_issuers through a join rather than owning the table: 17 predicates in remotesessions, plus one each in mcpservers and platformmcp. Most are the same swap the previous commit applied. Three needed a shape change instead: `IsPlatformMCPNewModelEligible` compared the issuer's project to the joined project. It now also admits an issuer whose project is NULL and whose organization matches the project's, which the surrounding join already pins to the caller's organization. `ResyncMCPServerRemoteSessionIssuers` entitled a remote session client with `c.project_id = usi.project_id`. An organization-tier issuer has no project, so that comparison is NULL and matched nothing. Entitlement now keys off the caller's project, which is what the rest of the file already scopes clients by. The refresh sweep pair, `ClaimDueRemoteSessionRefreshCandidates` and `GetDueRemoteSessionRefreshCandidate`, was broken in three places per copy rather than one, and none of them was a tenancy filter: all three derived or compared through `usi.project_id`, which is NULL for an organization-tier issuer. * `JOIN projects AS p ON p.id = usi.project_id` was how the organization for the auto-refresh policy check was derived, so an organization-tier issuer never joined and dropped out entirely. It is now a LEFT JOIN with `COALESCE(p.organization_id, usi.organization_id)`, plus an explicit `usi.project_id IS NULL OR p.id IS NOT NULL` so a project-tier issuer still requires a live project exactly as the inner join used to enforce. * The client entitlement arm compared `c.project_id = usi.project_id`. Left as is deliberately: an organization-tier issuer owns no project, so only the organization-level and global arms can match for one, which is the same tenancy rule the interactive surfaces apply. * The live-session EXISTS compared `gs.project_id = usi.project_id`, which is NULL against NULL for an organization-tier issuer and so never matched. It is now keyed on `gs.user_session_issuer_id` alone, which already fixes the session's tenancy. Both copies are spelled the same way, so TestRefreshSweep_ClaimAndRecheckAgreeOnPolicy still holds. A single lateral computing the organization once would read better, but sqlc's analyzer cannot resolve a FROM-less LATERAL, so the COALESCE is spelled out at each use. `LockUserSessionIssuerForMetaMCP` in internal/metamcp is deliberately left project-scoped. AIM-95 records why: meta_mcp_servers pins its reference with a composite foreign key on (project_id, user_session_issuer_id) that can never match a NULL project_id, so admitting the tier in the lock alone would turn today's clean 404 into a foreign key violation surfacing as a 500. Several call sites moved from `conv.ToPGText(...)` to a plain string as the `::text` cast made the generated parameter non-nullable. The one in UpdateRemoteSessionClient keeps passing empty on purpose, so neither the organization-level client arm nor the organization-tier issuer arm can match on an endpoint that only edits a project's own clients.
…n-tier guard https://linear.app/speakeasy/issue/AIM-100/feat-rescope-user-session-issuer-reads-and-writes-to-the-organization Walks every queries.sql under internal/, splits it into its named blocks, resolves each block's table aliases, and fails any block that compares one of the five tenancy-scoped tables to a project parameter without the `project_id IS NULL AND organization_id = @organization_id` arm beside it. The guard is the half that matters and the half that is invisible when it goes missing. Every row carries both columns, so a predicate that decays to `project_id = @project_id OR organization_id = @organization_id` matches every sibling project's rows in the same organization: a cross-project read on the listings and a cross-project write on the revokes and deletes. There is no error, just rows that should not be there, and neither the compiler nor sqlc can see it. Across roughly fifty near-identical predicates that is not something review reliably catches either. The file list is discovered by walking the tree rather than enumerated, so a package that starts joining these tables is covered the day it is added. That is the case that actually bit during this work: a hand-built inventory of the surface missed internal/metamcp and internal/mcpservers entirely, and this check finds both. Exemptions are keyed by query name and each carries its reason. A name that no longer matches a real query fails the test, so the list cannot quietly become a blanket suppression. Verified it fails in all three directions: dropping the IS NULL guard from a rescoped query, adding a new query in another package that joins the tables unscoped, and leaving a stale exemption entry behind.
Contributor
|
Contributor
|
Running ultrareview automatically — This broad tenancy change rewrites dozens of SQL predicates and session flows, including reads, revokes, deletes, bindings, and refresh sweeps; a subtle scoping bug could cause cross-project data exposure or destructive writes.. I'll post findings when complete. |
This comment has been minimized.
This comment has been minimized.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Ultrareview completed in 8m 14s
All reported issues were addressed across 36 files
Linked issue analysis
Linked issue: AIM-100: feat: rescope user session issuer reads and writes to the organization tier
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Organization-tier issuer and child-row queries use project-or-organization tenancy, including reads, listings, facets, and mutations. | The PR claims 45 predicate updates across six query files and adds organization IDs at the relevant call sites. |
| ✅ | The organization-tier branch retains the project_id IS NULL guard to prevent cross-project access within an organization. | The PR explicitly uses the guarded predicate and adds a query-shape test intended to reject project-only or unguarded organization predicates. |
| ✅ | Organization-tier issuers remain reachable through remote-session joins and refresh-sweep queries, including the NULL project_id join case. | The PR describes and implements LEFT JOIN/COALESCE restructuring for refresh sweeps and updates remote-session entitlement logic. |
| ✅ | CIMD issuer-client creation and lookup work for organization-tier issuers while preserving the issuer's tenancy. | The PR widens issuer validation and changes CIMD creation to select the issuer's project and organization rather than stamping the caller's project. |
| ✅ | Tenant isolation prevents a project from reading, listing, counting, revoking, or deleting another project's issuer subtree in the same organization. | A cross-project isolation test creates a sibling project and asserts isolation across issuers, clients, sessions, and consents, including mutation paths. |
| ✅ | Organization-tier rows are included unconditionally without an include_organizational opt-in parameter. | The PR explicitly states the behavior is always on and introduces no opt-in parameter. |
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
https://linear.app/speakeasy/issue/AIM-100/feat-rescope-user-session-issuer-reads-and-writes-to-the-organization Addresses review findings on the organization-tier rescope. `createAndAttachClient` validated its issuer through `GetUserSessionIssuerForProject` without an organization, so the widened predicate could never match an organization-tier issuer and the org-scoped binding scan below it was unreachable. The omission survived because `catalog_identity_provider_attachment.go` carries a file-level `//nolint:exhaustruct`, which is what otherwise surfaced every call site that needed the new field. An audit of every call site of every params struct this change touched found exactly one other candidate class, the `_test.go` files where exhaustruct is disabled by config; those pass no organization and still resolve project-tier rows through the first arm. The query-shape test gains two corrections of its own. Its guard pattern accepted any right-hand side, so `organization_id = @some_other_org` read as correctly scoped. The right-hand side is now pinned to either the caller's `@organization_id` or a correlated `<table>.organization_id`, the latter because `IsPlatformMCPNewModelEligible` compares against `projects.organization_id` on a join the surrounding query has already pinned to the caller. Its header also claimed to catch a query that joins these tables without scoping them at all. It does not, and should not: the public OAuth surface carries no project header and treats the issuer id as the authoritative scope, which several queries state outright. Telling a deliberate omission from an accidental one is a judgement the test cannot make, so it checks the shape of a tenancy predicate that is present rather than the presence of one. The comment now says so. Two findings are deferred rather than fixed here. The organization-wide owner checks that `DeleteUserSessionIssuer` needs are recorded on AIM-97 alongside the toolset half of the same gap, and the remote-session reads that scope by issuer without the client's tenancy are AIM-158, which blocks AIM-99. Neither is reachable until organization-tier creation ships.
This comment has been minimized.
This comment has been minimized.
https://linear.app/speakeasy/issue/AIM-100/feat-rescope-user-session-issuer-reads-and-writes-to-the-organization Replaces the two generic test artifacts with per-endpoint tests, each living in the test file for the endpoint it exercises and named after it. `crossprojectisolation_test.go` grouped its assertions by resource family, so one `TestCrossProjectIsolation_Issuer` covered get, get-by-slug, list, update, and delete at once. A failure named the family rather than the endpoint, and none of it lived where someone changing an endpoint would look. It is now seventeen tests: `TestGetUserSessionIssuer_...`, `TestRevokeUserSession_...`, and so on, appended to `getusersessionissuer_test.go`, `revokeusersession_test.go`, and the rest, with the CIMD client endpoints getting the test file they never had. `queriestenancy_test.go` scanned the text of every queries.sql for predicate shape. It was a linter wearing a test's clothes: pinned to no endpoint and no query, reporting SQL text rather than behavior, and carrying enough regex of its own to have had two bugs during review (a short alias matching inside a longer one, and a guard that accepted any right-hand side). Removed. The behavioral coverage is stronger for the swap, not weaker. Rewriting all 26 predicates in the package to the naive form the guard exists to prevent, `project_id = @project_id OR organization_id = @organization_id`, fails 16 of the 17 new tests. The seventeenth is `TestGetUserSessionIssuerBySlug_SiblingProjectNotFound`, which is unaffected by design because the slug lookup is deliberately exempt from the organization arm, and which fails if anyone rescopes it. That also confirms the list assertions are not passing vacuously. What is lost is reach outside this package: the scan covered predicates in remotesessions, mcpservers, platformmcp, and metamcp, which now rely on their own packages' tests. The shared fixture moves to setup_test.go as `seedSiblingProject`, naming the boundary it builds, a second project in the caller's own organization, rather than a "foreign" one.
daviddanialy
reviewed
Sep 1, 2026
| -- Recheck active owners in the write so an owner added after the handler's | ||
| -- preflight check prevents the issuer from being soft-deleted. | ||
| -- | ||
| -- The owner-existence subqueries below stay project-scoped. An |
Contributor
There was a problem hiding this comment.
this comment says the subquery below stays project-scoped but looks like it was actually changed to support org-scope as well?
daviddanialy
approved these changes
Sep 1, 2026
https://linear.app/speakeasy/issue/AIM-100/feat-rescope-user-session-issuer-reads-and-writes-to-the The note on DeleteUserSessionIssuer read as if the whole statement was unchanged, when the issuer-matching predicate directly below it does span both tiers. Only the owner-existence subqueries inside NOT EXISTS stay project-scoped. Name both parts explicitly so the scope of the caveat is clear.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
https://linear.app/speakeasy/issue/AIM-100/feat-rescope-user-session-issuer-reads-and-writes-to-the The issuer predicate on the delete spans both tiers, but the owner subqueries inside NOT EXISTS stayed pinned to the caller's project. Those two only agree for a project-tier issuer. An organization-tier issuer is addressable from every project in the organization, so a reference held by a sibling project sits outside the sweep, and the delete would soft delete the issuer out from under it. Nothing reaches that today, since every create writes a project_id, which is what the previous note said. That is an argument about which rows exist rather than about what the statement does, and it stops holding the moment an organization-tier issuer is created. Take the scope from the issuer row instead: its project when it has one, its organization when it does not. mcp_servers reaches the organization through its project, toolsets carry organization_id directly, and meta_mcp_servers stays project-scoped because its composite foreign key can never reference a NULL project_id. UserSessionIssuerHasActiveOwner is the preflight for the same write, so it is anchored on the issuer row the same way. Preflight and write disagreeing would report no owner for a reference the write has to honor. The two new tests seed an organization-tier issuer through a testenv fixture (no production surface creates one) and pin both arms: an owning toolset and an owning MCP server, each in a sibling project. Both fail without the tier-scoped sweep.
Contributor
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/internal/usersessions/queries.sql">
<violation number="1" location="server/internal/usersessions/queries.sql:117">
P2: When an organization-tier issuer is referenced from a soft-deleted sibling project, these owner sweeps still treat that reference as active and block deletion from every live project in the organization. Require the owning project to be live in both the `mcp_servers` and `toolsets` arms of both the preflight and write queries.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| AND server.deleted IS FALSE | ||
| AND ( | ||
| server.project_id = issuer.project_id | ||
| OR (issuer.project_id IS NULL AND server_project.organization_id = issuer.organization_id) |
Contributor
There was a problem hiding this comment.
P2: When an organization-tier issuer is referenced from a soft-deleted sibling project, these owner sweeps still treat that reference as active and block deletion from every live project in the organization. Require the owning project to be live in both the mcp_servers and toolsets arms of both the preflight and write queries.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/internal/usersessions/queries.sql, line 117:
<comment>When an organization-tier issuer is referenced from a soft-deleted sibling project, these owner sweeps still treat that reference as active and block deletion from every live project in the organization. Require the owning project to be live in both the `mcp_servers` and `toolsets` arms of both the preflight and write queries.</comment>
<file context>
@@ -108,23 +109,30 @@ WHERE issuer.id = @id
AND server.deleted IS FALSE
+ AND (
+ server.project_id = issuer.project_id
+ OR (issuer.project_id IS NULL AND server_project.organization_id = issuer.organization_id)
+ )
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://linear.app/speakeasy/issue/AIM-100/feat-rescope-user-session-issuer-reads-and-writes-to-the-organization
Summary
Widens 45 tenancy predicates across six
queries.sqlfiles fromproject_id = @project_idtoproject_id = @project_id OR (project_id IS NULL AND organization_id = @organization_id), so an organization-tieruser_session_issuerand its clients, sessions, consents, and CIMD grants are reachable rather than silently absent. Always on, with noinclude_organizationalopt-in: there is no global tier here for such a flag to protect, and a call site that forgets one drops rows without an error.Most sites are a direct swap. Four needed a shape change instead:
IsPlatformMCPNewModelEligiblecompared the issuer's project to the joined project, and now also admits an issuer whose project is NULL and whose organization matches.ResyncMCPServerRemoteSessionIssuersentitled a remote session client withc.project_id = usi.project_id, which is NULL for an organization-tier issuer and matched nothing. Entitlement now keys off the caller's project, as the rest of the file already does.ClaimDueRemoteSessionRefreshCandidatesandGetDueRemoteSessionRefreshCandidate, was broken in three independent places per copy, none of them a tenancy filter.JOIN projects AS p ON p.id = usi.project_idwas how the organization for the auto-refresh policy check was derived, so an organization-tier issuer never joined at all; it is now a LEFT JOIN withCOALESCE(p.organization_id, usi.organization_id)plus an explicitusi.project_id IS NULL OR p.id IS NOT NULLso a project-tier issuer still requires a live project. The live-sessionEXISTScomparedgs.project_id = usi.project_id, NULL against NULL, and is now keyed on the issuer id alone, which already fixes the session's tenancy. Both copies are spelled identically, soTestRefreshSweep_ClaimAndRecheckAgreeOnPolicystill holds.Four queries filtered the child row's own
project_idrather than reaching through the issuer, which degraded silently rather than 404ing: reauth prefill lost,last_used_atnever updated, and the CIMD refresh endpoint matching no rows.CreateUserSessionIssuerCimdClientstamped the caller's project onto the child and now takesiss.project_id, so the child inherits its parent's tenancy.Three things stay project-scoped on purpose, each documented where it lives:
GetUserSessionIssuerBySlug, because slug uniqueness is indexed per project with no(organization_id, slug)equivalent and admitting the tier would make a:onequery non-deterministic; the owner-existence subqueries inDeleteUserSessionIssuer, which would have to sweep the whole organization to be correct and belong with AIM-99; andLockUserSessionIssuerForMetaMCP, since AIM-95 records that widening it alone turns today's clean 404 into a foreign key violation surfacing as a 500.Two test layers back this up. A cross-project isolation suite gives a second project in the same organization a full issuer subtree and asserts the caller can neither read, list, count, revoke, nor delete any of it. A query-shape test walks every
queries.sqlunderinternal/, resolves each block's table aliases, and fails any block that scopes one of the five tables by project alone without an exemption and a reason.Motivation
AIM-103 made
user_session_issuers.project_idnullable and AIM-102 began dual-writingorganization_id, but tenant isolation on this surface is expressed asproject_id = @project_id. Every one of those predicates silently stops matching a NULL, so an organization-tier issuer would exist in the database and be unreachable from any read, any revoke, any facet count, and any remote-session join. Nothing is an error; rows are just absent.The
project_id IS NULLguard in the new predicate is load-bearing and easy to lose. Every row carries both columns, so a predicate that decays toproject_id = @project_id OR organization_id = @organization_idmatches every sibling project's rows in the same organization, converting tenant isolation into a cross-project read and, on the revoke and delete paths, a cross-project write. Neither the compiler nor sqlc can see that, and across roughly fifty near-identical predicates review will not reliably catch it either, which is why the query-shape test exists rather than a convention. The file list it checks is discovered by walking the tree rather than enumerated, because a hand-built inventory of this surface missed two packages outright.Summary by cubic
Implements AIM-100 by changing user session issuer tenancy from project-only to project-or-organization so organization-tier issuers, clients, sessions, consents, and CIMD grants are reachable while sibling projects stay isolated.
New Features
last_used_atpaths.Bug Fixes
Written for commit 0f4ed0e. Summary will update on new commits.