objectstack-ai
diff --git a/‎.changeset/team-approver-org-screen.md‎
Lines changed: 30 additions & 0 deletions b/‎.changeset/team-approver-org-screen.md‎
Lines changed: 30 additions & 0 deletions
diff --git a/‎packages/plugins/plugin-approvals/src/approval-service.ts‎
Lines changed: 97 additions & 4 deletions b/‎packages/plugins/plugin-approvals/src/approval-service.ts‎
Lines changed: 97 additions & 4 deletions
diff --git a/‎packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts‎
Lines changed: 9 additions & 4 deletions b/‎packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts‎
Lines changed: 9 additions & 4 deletions
@@ -0,0 +1,30 @@
+---
+"@objectstack/plugin-approvals": patch
+---
+
+**Who loses access:** members of a team belonging to a *different* organization
+than the record being approved. Concretely — a request raised in `org_a` routed
+to a `team` approver whose `sys_team.organization_id` is `org_b` used to place
+every `sys_team_member` of that team into `pending_approvers`, giving them the
+approve/reject buttons on a record they are not a tenant of. They no longer
+enter the slate, and the step falls back to the dead `team:<id>` literal with
+the existing `#3807` "expanded to nobody" warning — the same shape a cross-org
+`position` approver has always produced (#10230).
+
+`team` was the last approver expansion that resolved people without asking
+which organization was asking; `department`, `position`, `org_membership_level`
+and (since #10153) `manager` all do. The screen reads the team's own
+`organization_id`, so it costs one row and a team that fails it never fans out.
+
+**Who does not lose access**, deliberately: a team stamped with the request's
+own organization; a team stamped with **no** organization (`organization_id:
+null` on a platform object means "owned by no organization" — what a seed
+writes, since a seed cannot know the id the runtime mints at boot); a team id
+with no `sys_team` row at all; and any request that carries no organization —
+all four leave routing exactly as it was, because the tenancy fact is absent
+rather than negative.
+
+⚠️ One externally observable accept→reject change beyond the routing itself:
+under the non-default `onEmptyApprovers: 'fail'` policy, a node whose *sole*
+approver was a cross-org team used to open a request and now throws
+`NO_APPROVERS`. Under the default (`admin_rescue`) the node still opens.
@@ -971,7 +971,12 @@ export class ApprovalService implements IApprovalService {
 
     try {
       if (type === 'team') {
-        const users = await this.expandTeamUsers(String(a.value));
+        // #10230: the request's OWN organization, not `directoryOrg`. They are
+        // provably equal on this branch (`team` is not org-scoped, so an
+        // `organization` declaration is refused above), and naming the request
+        // org says what the screen asserts: tenancy of the record being
+        // approved, never an ADR-0105 D9 retarget this type does not have.
+        const users = await this.expandTeamUsers(String(a.value), organizationId);
         if (users.length) return users;
       } else if (type === 'department' || type === 'business_unit' || type === 'bu') {
         const users = await bounded(await this.expandBusinessUnitUsers(String(a.value), directoryOrg));
@@ -1139,7 +1144,14 @@ export class ApprovalService implements IApprovalService {
       try {
         if (resolveAs === 'department') users = await this.expandBusinessUnitUsers(key, directoryOrg);
         else if (resolveAs === 'position') users = await this.expandPositionUsers(key, directoryOrg);
-        else if (resolveAs === 'team') users = await this.expandTeamUsers(key);
+        // #10230: `directoryOrg` and NOT the request org, the opposite of the
+        // static `team` branch — and deliberately so. `expression` IS org-scoped
+        // (APPROVER_ORG_SCOPED), so a declaration here resolves to a legitimately
+        // retargeted sibling organization and the team must belong to the
+        // directory actually being consulted. `filterApproversWhoCanRead` below
+        // then applies the D2 read screen to what comes back, exactly as it
+        // already does for the other `resolveAs` kinds.
+        else if (resolveAs === 'team') users = await this.expandTeamUsers(key, directoryOrg);
         else {
           throw new Error(
             `VALIDATION_FAILED: expression approver has unknown resolveAs '${resolveAs}' — `
@@ -1164,9 +1176,30 @@ export class ApprovalService implements IApprovalService {
     return { slots, raw };
   }
 
-  /** Flat team — `sys_team` is better-auth's collaboration grouping (no hierarchy). */
-  private async expandTeamUsers(teamId: string): Promise<string[]> {
+  /**
+   * Flat team — `sys_team` is better-auth's collaboration grouping (no hierarchy).
+   *
+   * Takes an organization for the reason every sibling expansion does
+   * ({@link expandBusinessUnitUsers}, {@link expandPositionUsers},
+   * {@link expandMembershipTierUsers}): an approver expansion answers "who, in
+   * THIS organization". Before #10230 this one did not ask, and it was the last
+   * expansion that did not — a `team` approver naming ANOTHER organization's
+   * team routed that organization's people an approval over a record they are
+   * not a tenant of.
+   *
+   * ⚠️ The screen is on the TEAM, not on its members, and that is the whole
+   * difference from the screen next door ({@link managerIsProvablyOutsideOrg},
+   * #10153). `sys_user` carries no tenancy fact at all, so a manager can only
+   * be placed by his `sys_member` rows; `sys_team` carries `organization_id`
+   * outright (`packages/platform-objects/src/identity/sys-team.object.ts`), so
+   * a team id transitively names exactly one organization and ONE row answers
+   * the question. Screening the MEMBERS instead would be both a wider read and
+   * a different assertion — it would rule on #7497 (does approver routing imply
+   * record read visibility?), which this card does not.
+   */
+  private async expandTeamUsers(teamId: string, organizationId?: string | null): Promise<string[]> {
     if (!teamId) return [];
+    if (await this.teamIsProvablyOutsideOrg(teamId, organizationId)) return [];
     let rows: any[] = [];
     try {
       rows = await this.engine.find('sys_team_member', {
@@ -1179,6 +1212,66 @@ export class ApprovalService implements IApprovalService {
     return Array.from(new Set((rows ?? []).map((r: any) => String(r.user_id ?? '')).filter(Boolean)));
   }
 
+  /**
+   * Is `teamId` PROVABLY a team of a DIFFERENT organization? (#10230)
+   *
+   * "Provably" carries the same posture the sibling screen states at length in
+   * {@link managerIsProvablyOutsideOrg}, for the same reasons:
+   *
+   *   - the team row carries an `organization_id` and it is not the request's
+   *       ⇒ the tenancy fact is present and NEGATIVE ⇒ screen it out;
+   *   - the row carries no `organization_id`, does not exist, or the read failed
+   *       ⇒ the tenancy fact is ABSENT ⇒ leave routing exactly as it was.
+   *
+   * The `organization_id = null` limb is not timidity — it is the reading
+   * {@link businessUnitOrgScope} settled on one screen below, for the identical
+   * shape: null on a platform object means "owned by no organization", which is
+   * what a seed writes because a seed cannot know the organization id the
+   * runtime mints at boot. Treating null as "not mine" would delete every
+   * seeded team approver at once — a larger behaviour change than the hole
+   * being closed. Measured, and not hypothetically: this package's own
+   * `team_ok` expansion fixture is exactly such a stack (it has
+   * `sys_team_member` rows, a request carrying an organization, and no
+   * `sys_team` row at all).
+   *
+   * Screening the TEAM before reading its members is also what keeps the cost
+   * at one row: a team that fails the screen never fans out.
+   */
+  private async teamIsProvablyOutsideOrg(
+    teamId: string,
+    organizationId?: string | null,
+  ): Promise<boolean> {
+    const requestOrg = organizationId ? String(organizationId) : '';
+    // No organization on the request ⇒ nothing to screen against, and no read.
+    // The ordinary single-organization / embedded stack costs nothing here.
+    if (!requestOrg) return false;
+    let rows: any[] = [];
+    try {
+      // No `as any` on this options bag — #4918's ratchet grandfathers this
+      // file for its EXISTING erasures only, and a NEW one must carry the
+      // declared type. `ApprovalEngine.find` already accepts it as written.
+      rows = await this.engine.find('sys_team', {
+        where: { id: teamId },
+        fields: ['id', 'organization_id'],
+        limit: 1,
+        context: SYSTEM_CTX,
+      });
+    } catch { return false; } // team unreadable — see the fail-open note above
+    const row: any = Array.isArray(rows) ? rows[0] : null;
+    const teamOrg = row?.organization_id ? String(row.organization_id) : '';
+    if (!teamOrg) return false;                  // no tenancy fact on this team
+    if (teamOrg === requestOrg) return false;    // it is this org's team — route as before
+    this.logger?.warn?.(
+      `[approvals] #10230: team '${teamId}' was dropped from the approver slate — `
+      + `'sys_team.organization_id' is '${teamOrg}', not the request's organization `
+      + `'${requestOrg}', so routing this approval to its members would put approval `
+      + `authority over the record outside its tenant. Point the approver at a team in `
+      + `this organization, or route this step with an approver type that names someone in it.`,
+      { teamId, teamOrganizationId: teamOrg, requestOrganizationId: requestOrg },
+    );
+    return true;
+  }
+
   /**
    * Tenant scope for a `sys_business_unit` read that may legitimately be
    * env-wide (#3807).
 
@@ -22,8 +22,9 @@
  *           no read to perform it).
  *   B/B2  — the sibling `position` expansion IS screened, and its screen is not
  *           reject-everything.
- *   W     — `team` is still NOT screened. #10230 owns that; this card did not
- *           touch it, and the pin says so out loud.
+ *   W     — `team` was still NOT screened when this file was written; #10230
+ *           closed that and INVERTED this pin. It stays here as the cross-file
+ *           statement that no unscreened expansion is left.
  *   C-a   — THE ACCEPT-TO-REJECT FLIP. Under `onEmptyApprovers: 'fail'` a node
  *           whose sole approver is a cross-org `manager` used to OPEN; it now
  *           throws `NO_APPROVERS`. That throw is PRE-EXISTING code and a bare
@@ -237,11 +238,15 @@ describe('#10153 manager approver org screen', () => {
     expect(err).toBeTruthy();
   });
 
-  it('W — `team` is STILL not org-screened (#10230 owns it; this card did not touch it)', async () => {
+  it('W — `team` IS org-screened now too (#10230 landed; the gap this pin held is closed)', async () => {
     engine._tables['sys_team'] = [{ id: 'team_b', name: 'B team', organization_id: 'org_b' }];
     engine._tables['sys_team_member'] = [{ id: 'tm1', team_id: 'team_b', user_id: 'u_team_b' }];
     const req = opened(await svc.openNodeRequest(input([{ type: 'team', value: 'team_b' }]), CTX_A));
     console.log('[PROBE W] org_a request, org_b team -> pending_approvers =', JSON.stringify(req.pending_approvers));
-    expect(req.pending_approvers).toEqual(['u_team_b']);
+    // Inverted by #10230, which this pin was written to hand off to. The two
+    // directions and the `null` / absent-row limbs live in that card's own file
+    // (`team-approver-org-screen.test.ts`); what stays HERE is the cross-file
+    // fact this file exists to keep true — the last unscreened expansion is gone.
+    expect(req.pending_approvers).toEqual(['team:team_b']);
   });
 });