Skip to content

Commit 67e6730

Browse files
tofikwesttofikwest
authored
Merge pull request #3317 from trycompai/tofik/cs-569-offboard-onboarding-loop-guard
fix(auth): stop offboarded users from looping into a spurious new org (CS-569)
2 parents 0def9d0 + ab7ba22 commit 67e6730

10 files changed

Lines changed: 512 additions & 40 deletions

File tree

‎apps/api/src/auth/auth.controller.spec.ts‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
const mockUserFindUnique = jest.fn();
2+
const mockMemberFindMany = jest.fn();
3+
const mockMemberCount = jest.fn();
4+
const mockInvitationFindFirst = jest.fn();
5+
6+
jest.mock('@db', () => ({
7+
db: {
8+
user: { findUnique: (...a: unknown[]) => mockUserFindUnique(...a) },
9+
member: {
10+
findMany: (...a: unknown[]) => mockMemberFindMany(...a),
11+
count: (...a: unknown[]) => mockMemberCount(...a),
12+
},
13+
invitation: { findFirst: (...a: unknown[]) => mockInvitationFindFirst(...a) },
14+
},
15+
}));
16+
17+
// Both guards import ./auth.server, which validates SECRET_KEY and pulls in
18+
// better-auth/redis at module load. Stub them so importing the controller is
19+
// hermetic — the test calls the method directly and never runs the guards.
20+
jest.mock('./hybrid-auth.guard', () => ({ HybridAuthGuard: class {} }));
21+
jest.mock('./permission.guard', () => ({
22+
PermissionGuard: class {},
23+
PERMISSIONS_KEY: 'permissions',
24+
}));
25+
26+
import { AuthController } from './auth.controller';
27+
import type { AuthContext } from './types';
28+
29+
const sessionContext = (): AuthContext => ({
30+
organizationId: 'org_1',
31+
authType: 'session',
32+
isApiKey: false,
33+
isPlatformAdmin: false,
34+
userRoles: null,
35+
userId: 'user_1',
36+
userEmail: 'user@example.com',
37+
});
38+
39+
describe('AuthController.getMe — hasInactiveMembership (CS-569)', () => {
40+
const controller = new AuthController();
41+
42+
beforeEach(() => {
43+
jest.clearAllMocks();
44+
mockUserFindUnique.mockResolvedValue({
45+
id: 'user_1',
46+
email: 'user@example.com',
47+
name: 'User',
48+
image: null,
49+
role: 'user',
50+
});
51+
mockInvitationFindFirst.mockResolvedValue(null);
52+
});
53+
54+
it('returns hasInactiveMembership: false for a genuinely new user (no memberships at all)', async () => {
55+
mockMemberFindMany.mockResolvedValue([]);
56+
mockMemberCount.mockResolvedValue(0);
57+
58+
const res = await controller.getMe(sessionContext());
59+
60+
expect(res.organizations).toEqual([]);
61+
expect(res.hasInactiveMembership).toBe(false);
62+
});
63+
64+
it('returns hasInactiveMembership: true for an offboarded user (no active org, only deactivated memberships)', async () => {
65+
mockMemberFindMany.mockResolvedValue([]); // no ACTIVE memberships
66+
mockMemberCount.mockResolvedValue(1); // one deactivated/inactive membership
67+
68+
const res = await controller.getMe(sessionContext());
69+
70+
expect(res.organizations).toEqual([]);
71+
expect(res.hasInactiveMembership).toBe(true);
72+
// The count that distinguishes "offboarded" from "new" must match
73+
// memberships that are deactivated OR no longer active.
74+
expect(mockMemberCount).toHaveBeenCalledWith({
75+
where: {
76+
userId: 'user_1',
77+
OR: [{ deactivated: true }, { isActive: false }],
78+
},
79+
});
80+
});
81+
82+
it('returns hasInactiveMembership: false for an active member', async () => {
83+
mockMemberFindMany.mockResolvedValue([
84+
{
85+
id: 'member_1',
86+
role: 'owner',
87+
organizationId: 'org_1',
88+
organization: {
89+
id: 'org_1',
90+
name: 'Org',
91+
logo: null,
92+
onboardingCompleted: true,
93+
hasAccess: true,
94+
createdAt: new Date(),
95+
},
96+
},
97+
]);
98+
mockMemberCount.mockResolvedValue(0);
99+
100+
const res = await controller.getMe(sessionContext());
101+
102+
expect(res.organizations).toHaveLength(1);
103+
expect(res.hasInactiveMembership).toBe(false);
104+
});
105+
});

‎apps/api/src/auth/auth.controller.ts‎

Lines changed: 48 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -35,44 +35,55 @@ export class AuthController {
3535
return { user: null, organizations: [], pendingInvitation: null };
3636
}
3737

38-
const [user, memberships, pendingInvitation] = await Promise.all([
39-
db.user.findUnique({
40-
where: { id: userId },
41-
select: {
42-
id: true,
43-
email: true,
44-
name: true,
45-
image: true,
46-
role: true,
47-
},
48-
}),
49-
db.member.findMany({
50-
where: { userId, isActive: true, deactivated: false },
51-
select: {
52-
id: true,
53-
role: true,
54-
organizationId: true,
55-
organization: {
56-
select: {
57-
id: true,
58-
name: true,
59-
logo: true,
60-
onboardingCompleted: true,
61-
hasAccess: true,
62-
createdAt: true,
38+
const [user, memberships, pendingInvitation, inactiveMembershipCount] =
39+
await Promise.all([
40+
db.user.findUnique({
41+
where: { id: userId },
42+
select: {
43+
id: true,
44+
email: true,
45+
name: true,
46+
image: true,
47+
role: true,
48+
},
49+
}),
50+
db.member.findMany({
51+
where: { userId, isActive: true, deactivated: false },
52+
select: {
53+
id: true,
54+
role: true,
55+
organizationId: true,
56+
organization: {
57+
select: {
58+
id: true,
59+
name: true,
60+
logo: true,
61+
onboardingCompleted: true,
62+
hasAccess: true,
63+
createdAt: true,
64+
},
6365
},
6466
},
65-
},
66-
orderBy: { createdAt: 'desc' },
67-
}),
68-
db.invitation.findFirst({
69-
where: {
70-
email: authContext.userEmail ?? '',
71-
status: 'pending',
72-
},
73-
select: { id: true },
74-
}),
75-
]);
67+
orderBy: { createdAt: 'desc' },
68+
}),
69+
db.invitation.findFirst({
70+
where: {
71+
email: authContext.userEmail ?? '',
72+
status: 'pending',
73+
},
74+
select: { id: true },
75+
}),
76+
// Count memberships that exist but are no longer active (deactivated
77+
// or removed). Lets the app tell a genuinely new user (no memberships
78+
// at all → onboarding) apart from an offboarded user whose access was
79+
// revoked (→ "access removed" instead of a spurious new org). CS-569.
80+
db.member.count({
81+
where: {
82+
userId,
83+
OR: [{ deactivated: true }, { isActive: false }],
84+
},
85+
}),
86+
]);
7687

7788
return {
7889
user,
@@ -82,6 +93,7 @@ export class AuthController {
8293
memberId: m.id,
8394
})),
8495
pendingInvitation,
96+
hasInactiveMembership: inactiveMembershipCount > 0,
8597
};
8698
}
8799

‎apps/app/src/app/(app)/setup/actions/create-organization-minimal.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,36 @@ export const createOrganizationMinimal = authActionClientWithoutOrg
4343
};
4444
}
4545

46+
// CS-569 backstop: never create an org for a user whose only memberships
47+
// are deactivated (0 active, >=1 inactive) — that user was offboarded and
48+
// the routing layer already sends them to /auth/access-removed. This
49+
// guards against a stale/replayed form POST slipping past that redirect
50+
// and spawning a spurious empty org. Genuinely new users (no memberships)
51+
// and users adding an additional org (have active memberships) pass.
52+
const [activeMembershipCount, inactiveMembershipCount] = await Promise.all([
53+
db.member.count({
54+
where: {
55+
userId: session.user.id,
56+
isActive: true,
57+
deactivated: false,
58+
},
59+
}),
60+
db.member.count({
61+
where: {
62+
userId: session.user.id,
63+
OR: [{ deactivated: true }, { isActive: false }],
64+
},
65+
}),
66+
]);
67+
68+
if (activeMembershipCount === 0 && inactiveMembershipCount > 0) {
69+
return {
70+
success: false,
71+
error:
72+
'Your access to this organization was removed. Contact your administrator to be re-invited.',
73+
};
74+
}
75+
4676
// Internal team accounts (verified @trycomp.ai) have access provisioned up front.
4777
const userEmail = session.user.email;
4878
const isVerifiedTryCompEmail =

‎apps/app/src/app/(app)/setup/route.test.ts‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
import { vi } from 'vitest';
2+
3+
vi.mock('@/utils/auth', async () => {
4+
const { mockAuth } = await import('@/test-utils/mocks/auth');
5+
return { auth: mockAuth };
6+
});
7+
8+
const { mockServerApiGet, mockCreateSetupSession } = vi.hoisted(() => ({
9+
mockServerApiGet: vi.fn(),
10+
mockCreateSetupSession: vi.fn(async () => ({ id: 'setup_1' })),
11+
}));
12+
vi.mock('@/lib/api-server', () => ({ serverApi: { get: mockServerApiGet } }));
13+
vi.mock('./lib/setup-session', () => ({ createSetupSession: mockCreateSetupSession }));
14+
15+
import { describe, it, expect, beforeEach } from 'vitest';
16+
import { NextRequest } from 'next/server';
17+
import { redirect } from 'next/navigation';
18+
import { GET } from './route';
19+
import { mockAuthApi, createMockSession, createMockUser } from '@/test-utils/mocks/auth';
20+
21+
const mockRedirect = vi.mocked(redirect);
22+
23+
interface MeData {
24+
organizations: unknown[];
25+
pendingInvitation: { id: string } | null;
26+
hasInactiveMembership?: boolean;
27+
}
28+
const mockMe = (data: MeData) => mockServerApiGet.mockResolvedValue({ data, status: 200 });
29+
const call = (url: string) => GET(new NextRequest(url));
30+
31+
describe('/setup route — CS-569 offboard guard vs invite precedence', () => {
32+
beforeEach(() => {
33+
vi.clearAllMocks();
34+
mockCreateSetupSession.mockResolvedValue({ id: 'setup_1' });
35+
mockRedirect.mockImplementation((url: string) => {
36+
throw new Error(`REDIRECT:${url}`);
37+
});
38+
mockAuthApi.getSession.mockResolvedValue({
39+
session: createMockSession(),
40+
user: createMockUser(),
41+
});
42+
});
43+
44+
it('redirects an offboarded user with no invite to access-removed', async () => {
45+
mockMe({ organizations: [], pendingInvitation: null, hasInactiveMembership: true });
46+
47+
await expect(call('http://localhost/setup')).rejects.toThrow('REDIRECT:/auth/access-removed');
48+
expect(mockCreateSetupSession).not.toHaveBeenCalled();
49+
});
50+
51+
it('does NOT block an offboarded user arriving via ?inviteCode= (invite handled downstream)', async () => {
52+
// Regression (cubic): the guard must not pre-empt the legacy invite entry.
53+
mockMe({ organizations: [], pendingInvitation: null, hasInactiveMembership: true });
54+
55+
await expect(call('http://localhost/setup?inviteCode=inv_abc')).rejects.toThrow(
56+
'REDIRECT:/setup/setup_1?inviteCode=inv_abc',
57+
);
58+
// Guard is skipped entirely when an invite code is present.
59+
expect(mockServerApiGet).not.toHaveBeenCalled();
60+
expect(mockRedirect).not.toHaveBeenCalledWith('/auth/access-removed');
61+
});
62+
63+
it('routes an offboarded user with a pending invitation to the invite, not access-removed', async () => {
64+
mockMe({
65+
organizations: [],
66+
pendingInvitation: { id: 'inv_xyz' },
67+
hasInactiveMembership: true,
68+
});
69+
70+
await expect(call('http://localhost/setup')).rejects.toThrow('REDIRECT:/invite/inv_xyz');
71+
expect(mockRedirect).not.toHaveBeenCalledWith('/auth/access-removed');
72+
});
73+
74+
it('lets a genuinely new user through to onboarding', async () => {
75+
mockMe({ organizations: [], pendingInvitation: null, hasInactiveMembership: false });
76+
77+
await expect(call('http://localhost/setup')).rejects.toThrow('REDIRECT:/setup/setup_1');
78+
expect(mockCreateSetupSession).toHaveBeenCalled();
79+
});
80+
});

‎apps/app/src/app/(app)/setup/route.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { serverApi } from '@/lib/api-server';
2+
import { resolveNoActiveOrgRedirect } from '@/lib/no-active-org-redirect';
13
import { auth } from '@/utils/auth';
24
import { headers } from 'next/headers';
35
import { redirect } from 'next/navigation';
@@ -17,6 +19,31 @@ export async function GET(request: NextRequest) {
1719
redirect(`/sign-in${queryString}`);
1820
}
1921

22+
// Invite flows take precedence over the CS-569 offboard guard: a raw
23+
// ?inviteCode= is turned into /invite/{code} downstream by /setup/[setupId],
24+
// so let it flow through untouched rather than pre-empting it here. Matches
25+
// the downstream truthy check (an empty ?inviteCode= is not an invite).
26+
const inviteCode = request.nextUrl.searchParams.get('inviteCode');
27+
if (!inviteCode) {
28+
// CS-569: guard the onboarding loop at the setup entry too (direct nav or
29+
// the create-additional intent). Reuse the same decision the landing page
30+
// uses so the two can't drift. A non-null target means the user is invited
31+
// or offboarded; `null` means a new user (or one with active orgs) who
32+
// should fall through to onboarding.
33+
const meRes = await serverApi.get<{
34+
organizations: unknown[];
35+
pendingInvitation: { id: string } | null;
36+
hasInactiveMembership?: boolean;
37+
}>('/v1/auth/me');
38+
const hasActiveOrg = (meRes.data?.organizations?.length ?? 0) > 0;
39+
if (!hasActiveOrg) {
40+
const target = resolveNoActiveOrgRedirect(meRes.data);
41+
if (target) {
42+
redirect(target);
43+
}
44+
}
45+
}
46+
2047
const setupSession = await createSetupSession(session.user.id);
2148
redirect(`/setup/${setupSession.id}${queryString}`);
2249
}

‎apps/app/src/app/(public)/auth/access-removed/page.tsx‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
import { SignOut } from '@/components/sign-out';
2+
3+
/**
4+
* Shown to a signed-in user who has no active organization membership but was
5+
* previously a member (all memberships deactivated/removed) — i.e. they were
6+
* offboarded. Without this, such a user was silently dropped into onboarding,
7+
* which spawned a spurious empty org and locked them into a loop (CS-569).
8+
*
9+
* The most common cause is a domain change: the old account keeps signing in
10+
* after being offboarded. The primary action is therefore "sign out" so they
11+
* can sign back in with their current account.
12+
*/
13+
export default function AccessRemovedPage() {
14+
return (
15+
<div className="flex min-h-dvh items-center justify-center p-4">
16+
<div className="w-full max-w-lg space-y-6 rounded-lg border p-8 text-center">
17+
<div className="space-y-2">
18+
<h1 className="text-2xl font-semibold tracking-tight">
19+
{'Your access was removed'}
20+
</h1>
21+
<p className="text-muted-foreground">
22+
{
23+
'This account is no longer a member of any organization. If your company recently changed email domains or offboarded this account, sign out and sign back in with your current account. Otherwise, contact your administrator to be re-invited.'
24+
}
25+
</p>
26+
</div>
27+
28+
<div className="flex justify-center">
29+
<SignOut asButton />
30+
</div>
31+
</div>
32+
</div>
33+
);
34+
}

0 commit comments

Comments
 (0)