From 6ae774b99b1587e08dc498b056baaeeccd609fd4 Mon Sep 17 00:00:00 2001 From: Billy Dunn Date: Thu, 6 Aug 2026 11:54:15 -0500 Subject: [PATCH 1/2] fix(api): exclude disabled and invited users from PAM elevation approvers (#3174) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveElevationApprovers resolved both candidate sets — direct organization_users members and partner_users members whose org_access covers the org — without ever consulting users.status. Memberships are retained when an account is disabled or is still in 'invited', so those accounts kept counting as eligible approvers. Two consequences, both matching the AI-side twin (resolveIntentApprovers): the approver set is inflated with push targets that can never respond, and any logic keyed on the approver count is skewed by ghosts that look like real approvers. Both queries now innerJoin users and require status = 'active', which is the same shape the action-intent resolver uses. Tests assert the join target and the predicate that actually reach drizzle, not the ids the mock was primed to return — the mock resolves its rows regardless of the WHERE, so a test checking only returned ids could not fail if the gate were removed. Confirmed against the previous behaviour: deleting only the status predicate fails exactly the two new cases and leaves the other four passing. Claude-Session: https://claude.ai/code/session_01RUup17Z6KMH9jSkhBBhRJ1 --- apps/api/src/services/pamApprovers.test.ts | 86 +++++++++++++++++++--- apps/api/src/services/pamApprovers.ts | 18 ++++- 2 files changed, 91 insertions(+), 13 deletions(-) diff --git a/apps/api/src/services/pamApprovers.test.ts b/apps/api/src/services/pamApprovers.test.ts index 5fb505de1e..83b4ae3851 100644 --- a/apps/api/src/services/pamApprovers.test.ts +++ b/apps/api/src/services/pamApprovers.test.ts @@ -14,6 +14,7 @@ vi.mock('../db/schema', () => ({ rolePermissions: { roleId: 'role_id', permissionId: 'permission_id' }, permissions: { id: 'id', resource: 'resource', action: 'action' }, mobileDevices: { userId: 'user_id', status: 'status', notificationsEnabled: 'notifications_enabled' }, + users: { id: 'users.id', status: 'users.status' }, })); import { db } from '../db'; @@ -23,11 +24,29 @@ import { resolveElevationApprovers } from './pamApprovers'; * The resolver issues these selects in order: * 1. granting roles: select().from(rolePermissions).innerJoin(permissions).where() * 2. org partner: select().from(organizations).where().limit() - * 3. org members: select().from(organizationUsers).where() - * 4. partner members: select().from(partnerUsers).where() + * 3. org members: select().from(organizationUsers).innerJoin(users).where() + * 4. partner members: select().from(partnerUsers).innerJoin(users).where() * 5. mobile devices: select().from(mobileDevices).where() * (4 is skipped when the org has no partner; 5 is skipped when no candidates.) + * + * 3 and 4 gained their `users` innerJoin in #3174. `spies` exposes the + * arguments those two calls actually received so a test can assert on the real + * join target and predicate rather than on what the mock was told to return — + * the mock resolves its rows regardless of the WHERE, so a test that only + * checked returned ids could not fail if the status gate were deleted. */ +const spies = { + orgMembersInnerJoin: vi.fn(), + orgMembersWhere: vi.fn(), + partnerMembersInnerJoin: vi.fn(), + partnerMembersWhere: vi.fn(), +}; + +/** Serialize a drizzle condition so a test can look for a column/value in it. */ +function conditionText(cond: unknown): string { + return JSON.stringify(cond, (_k, v) => (typeof v === 'bigint' ? String(v) : v)) ?? ''; +} + function queueSelects(opts: { grantingRoles: Array<{ roleId: string }>; org: Array<{ partnerId: string | null }>; @@ -50,18 +69,18 @@ function queueSelects(opts: { }), } as any); - // 3. org members (where()) + // 3. org members (innerJoin().where()) + spies.orgMembersWhere.mockResolvedValue(opts.orgMembers); + spies.orgMembersInnerJoin.mockReturnValue({ where: spies.orgMembersWhere }); vi.mocked(db.select).mockReturnValueOnce({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue(opts.orgMembers), - }), + from: vi.fn().mockReturnValue({ innerJoin: spies.orgMembersInnerJoin }), } as any); - // 4. partner members (where()) + // 4. partner members (innerJoin().where()) + spies.partnerMembersWhere.mockResolvedValue(opts.partnerMembers); + spies.partnerMembersInnerJoin.mockReturnValue({ where: spies.partnerMembersWhere }); vi.mocked(db.select).mockReturnValueOnce({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockResolvedValue(opts.partnerMembers), - }), + from: vi.fn().mockReturnValue({ innerJoin: spies.partnerMembersInnerJoin }), } as any); // 5. mobile devices (where()) @@ -76,6 +95,7 @@ describe('resolveElevationApprovers', () => { beforeEach(() => { vi.clearAllMocks(); vi.mocked(db.select).mockReset(); + for (const s of Object.values(spies)) s.mockReset(); }); it('returns distinct userIds with an active mobile device (org + partner members)', async () => { @@ -129,6 +149,52 @@ describe('resolveElevationApprovers', () => { expect(db.select).toHaveBeenCalledTimes(1); }); + // #3174: memberships survive an account being disabled or left in 'invited', + // so both candidate queries must join `users` and require status='active'. + // These assert the join target and the predicate that reach drizzle, not the + // ids the mock was primed to return — the latter cannot fail if the gate goes. + it('gates direct org members on an active user account', async () => { + queueSelects({ + grantingRoles: [{ roleId: 'role-exec' }], + org: [{ partnerId: 'partner-1' }], + orgMembers: [{ userId: 'u-org' }], + partnerMembers: [], + mobile: [{ userId: 'u-org' }], + }); + + await resolveElevationApprovers('org-1'); + + expect(spies.orgMembersInnerJoin).toHaveBeenCalledTimes(1); + expect(spies.orgMembersInnerJoin.mock.calls[0]?.[0]).toEqual({ + id: 'users.id', + status: 'users.status', + }); + const where = conditionText(spies.orgMembersWhere.mock.calls[0]?.[0]); + expect(where).toContain('users.status'); + expect(where).toContain('active'); + }); + + it('gates partner members on an active user account', async () => { + queueSelects({ + grantingRoles: [{ roleId: 'role-exec' }], + org: [{ partnerId: 'partner-1' }], + orgMembers: [], + partnerMembers: [{ userId: 'u-all', orgAccess: 'all', orgIds: null }], + mobile: [{ userId: 'u-all' }], + }); + + await resolveElevationApprovers('org-1'); + + expect(spies.partnerMembersInnerJoin).toHaveBeenCalledTimes(1); + expect(spies.partnerMembersInnerJoin.mock.calls[0]?.[0]).toEqual({ + id: 'users.id', + status: 'users.status', + }); + const where = conditionText(spies.partnerMembersWhere.mock.calls[0]?.[0]); + expect(where).toContain('users.status'); + expect(where).toContain('active'); + }); + it('returns [] when eligible members have no active mobile device', async () => { queueSelects({ grantingRoles: [{ roleId: 'role-exec' }], diff --git a/apps/api/src/services/pamApprovers.ts b/apps/api/src/services/pamApprovers.ts index 7254b0f981..0838aa7247 100644 --- a/apps/api/src/services/pamApprovers.ts +++ b/apps/api/src/services/pamApprovers.ts @@ -3,8 +3,9 @@ * * Given an org, returns the distinct set of user ids who may approve a * uac_intercept elevation on their phone: a user is eligible iff - * 1. their role in (or covering) the org grants DEVICES_EXECUTE, AND - * 2. they have at least one active mobile device with notifications enabled + * 1. their account is active (users.status = 'active'), AND + * 2. their role in (or covering) the org grants DEVICES_EXECUTE, AND + * 3. they have at least one active mobile device with notifications enabled * (mobile_devices.status = 'active' AND notifications_enabled = true). * * Org membership mirrors how permissions.ts resolves access: @@ -26,6 +27,7 @@ import { rolePermissions, permissions, mobileDevices, + users, } from '../db/schema'; import { PERMISSIONS } from './permissions'; @@ -64,19 +66,27 @@ export async function resolveElevationApprovers(orgId: string): Promise(); - // 1. Direct org members holding a devices:execute role. + // 1. Direct org members holding a devices:execute role. Joined against + // `users` and gated on status='active' (#3174) so a disabled or still- + // invited account is never counted as an eligible approver: memberships + // are retained when an account is disabled, so without this the approver + // set is inflated with people who can never respond, and any logic keyed + // on the approver count is skewed by those ghosts. const orgMembers = await db .select({ userId: organizationUsers.userId }) .from(organizationUsers) + .innerJoin(users, eq(users.id, organizationUsers.userId)) .where( and( eq(organizationUsers.orgId, orgId), inArray(organizationUsers.roleId, grantingRoleIds), + eq(users.status, 'active'), ), ); for (const m of orgMembers) candidateUserIds.add(m.userId); // 2. Partner members of the org's partner whose org_access covers this org. + // Same `users` join + status='active' gate as above (#3174). if (org?.partnerId) { const partnerMembers = await db .select({ @@ -85,10 +95,12 @@ export async function resolveElevationApprovers(orgId: string): Promise Date: Thu, 6 Aug 2026 19:46:04 -0500 Subject: [PATCH 2/2] chore: retrigger ci after the 2026-08-06 github actions incident The original runs for this branch were queued during the Actions outage (incident opened 15:22Z) and stayed parked at `queued` for over three hours after the platform recovered, while every other queue drained. Re-running them repeatedly had no effect, because a rerun re-queues into the same stuck backlog rather than creating fresh work. No functional change: this commit is empty. The last real run on this branch was 42 jobs green with zero genuine job failures. Claude-Session: https://claude.ai/code/session_01RUup17Z6KMH9jSkhBBhRJ1