diff --git a/.changeset/fix-skipped-integration-tests-3321-3323.md b/.changeset/fix-skipped-integration-tests-3321-3323.md new file mode 100644 index 0000000000..72368e919f --- /dev/null +++ b/.changeset/fix-skipped-integration-tests-3321-3323.md @@ -0,0 +1,9 @@ +--- +--- + +Un-skip 10 integration tests across 4 files (#3321, #3322, #3323): + +- join-request-approval.test.ts: add @workos-inc/node class mock + WorkOS env var priming so handlers' new WorkOS() calls see test mocks; un-skip 5 tests +- personal-workspace-restrictions.test.ts: same fix; un-skip 4 tests +- self-service-delete.test.ts: update 404→403 assertion (membership check precedes existence check by design); fix active-subscription test to seed subscription_status='active' in DB rather than mocking stripe-client import +- admin-endpoints.test.ts: same DB-seed fix for active-subscription test diff --git a/server/tests/integration/admin-endpoints.test.ts b/server/tests/integration/admin-endpoints.test.ts index 4e20fad3c3..057d4c9344 100644 --- a/server/tests/integration/admin-endpoints.test.ts +++ b/server/tests/integration/admin-endpoints.test.ts @@ -360,29 +360,19 @@ describe('Admin Endpoints Integration Tests', () => { expect(afterResult.rows.length).toBe(0); }); - // Skipped: see #3289 — handler now reads subscription state from OrganizationDatabase.getSubscriptionInfo - // (DB-backed) instead of the stripe-client import the test mocks. Test needs to seed - // subscription_status='active' on the org row, not vi.mock the stripe call. - it.skip('should prevent deletion of organization with active subscription', async () => { - // Create org with active subscription + // OrganizationDatabase.getSubscriptionInfo reads subscription_status from the DB row; + // mocking the stripe-client import has no effect. Seed the column directly. + // The active-subscription guard applies to admin-initiated deletes too; force-deletion + // of a subscribed org requires a DB-level intervention (clear subscription_status). + it('should prevent deletion of organization with active subscription', async () => { const SUB_ORG_ID = 'org_delete_test_sub'; await pool.query( - `INSERT INTO organizations (workos_organization_id, name, stripe_customer_id, created_at, updated_at) - VALUES ($1, $2, $3, NOW(), NOW()) - ON CONFLICT (workos_organization_id) DO UPDATE SET name = $2, stripe_customer_id = $3`, - [SUB_ORG_ID, 'Subscribed Test Org', 'cus_sub_admin_test'] + `INSERT INTO organizations (workos_organization_id, name, subscription_status, created_at, updated_at) + VALUES ($1, $2, 'active', NOW(), NOW()) + ON CONFLICT (workos_organization_id) DO UPDATE SET name = $2, subscription_status = 'active'`, + [SUB_ORG_ID, 'Subscribed Test Org'] ); - // Mock getSubscriptionInfo to return active subscription - const { getSubscriptionInfo } = await import('../../src/billing/stripe-client.js'); - vi.mocked(getSubscriptionInfo).mockResolvedValueOnce({ - status: 'active', - product_id: 'prod_test', - product_name: 'Test Product', - current_period_end: Math.floor(Date.now() / 1000) + 86400, - cancel_at_period_end: false, - }); - const response = await request(app) .delete(`/api/admin/accounts/${SUB_ORG_ID}`) .send({ confirmation: 'Subscribed Test Org' }) @@ -399,7 +389,7 @@ describe('Admin Endpoints Integration Tests', () => { ); expect(checkResult.rows.length).toBe(1); - // Clean up + // SUB_ORG_ID is not covered by the inner afterEach; clean up inline. await pool.query('DELETE FROM organizations WHERE workos_organization_id = $1', [SUB_ORG_ID]); }); }); diff --git a/server/tests/integration/join-request-approval.test.ts b/server/tests/integration/join-request-approval.test.ts index 86fc9bb4f8..a974def9c6 100644 --- a/server/tests/integration/join-request-approval.test.ts +++ b/server/tests/integration/join-request-approval.test.ts @@ -1,31 +1,52 @@ import { describe, it, expect, beforeAll, afterAll, beforeEach, afterEach, vi } from 'vitest'; -// vi.hoisted ensures all of these are available inside vi.mock factories +// vi.hoisted ensures all of these are available inside vi.mock factories. +// Env vars must be set here so AUTH_ENABLED resolves true before organizations.ts loads +// and calls new WorkOS() — without them workos is null and every workos!. call throws. const { TEST_ADMIN_USER_ID, TEST_REQUESTER_USER_ID, TEST_ORG_ID, mockCreateOrganizationMembership, mockSendInvitation, -} = vi.hoisted(() => ({ - TEST_ADMIN_USER_ID: 'user_join_req_admin', - TEST_REQUESTER_USER_ID: 'user_join_req_requester', - TEST_ORG_ID: 'org_join_req_test', - mockCreateOrganizationMembership: vi.fn().mockResolvedValue({ id: 'om_test_new' }), - mockSendInvitation: vi.fn(), + listOrganizationMemberships, +} = vi.hoisted(() => { + process.env.WORKOS_API_KEY ||= 'sk_test_dummy_for_unit_tests'; + process.env.WORKOS_CLIENT_ID ||= 'client_test_dummy_for_unit_tests'; + process.env.WORKOS_COOKIE_PASSWORD ||= 'test-cookie-password-32chars-min-len-1234'; + return { + TEST_ADMIN_USER_ID: 'user_join_req_admin', + TEST_REQUESTER_USER_ID: 'user_join_req_requester', + TEST_ORG_ID: 'org_join_req_test', + mockCreateOrganizationMembership: vi.fn().mockResolvedValue({ id: 'om_test_new' }), + mockSendInvitation: vi.fn(), + listOrganizationMemberships: vi.fn(), + }; +}); + +// organizations.ts calls new WorkOS() directly; mocking @workos-inc/node intercepts that +// constructor so every new WorkOS() returns the same shared mock methods. +vi.mock('@workos-inc/node', () => ({ + WorkOS: class { + userManagement = { + listOrganizationMemberships, + createOrganizationMembership: mockCreateOrganizationMembership, + sendInvitation: mockSendInvitation, + getUser: vi.fn().mockResolvedValue({ id: TEST_ADMIN_USER_ID, email: 'admin@example.com' }), + }; + organizations = { + getOrganization: vi.fn().mockResolvedValue({ id: TEST_ORG_ID, name: 'Test Org' }), + }; + adminPortal = { + generateLink: vi.fn().mockResolvedValue({ link: 'https://test-portal.workos.com' }), + }; + }, })); vi.mock('../../src/auth/workos-client.js', () => ({ workos: { userManagement: { - listOrganizationMemberships: vi.fn().mockImplementation(({ userId, organizationId }) => { - if (userId === TEST_ADMIN_USER_ID && organizationId === TEST_ORG_ID) { - return Promise.resolve({ - data: [{ id: 'om_admin', userId: TEST_ADMIN_USER_ID, organizationId: TEST_ORG_ID, role: { slug: 'admin' }, status: 'active' }], - }); - } - return Promise.resolve({ data: [] }); - }), + listOrganizationMemberships, createOrganizationMembership: mockCreateOrganizationMembership, sendInvitation: mockSendInvitation, getUser: vi.fn().mockResolvedValue({ id: TEST_ADMIN_USER_ID, email: 'admin@example.com' }), @@ -105,6 +126,16 @@ describe('Join Request Approval', () => { beforeEach(async () => { vi.clearAllMocks(); mockCreateOrganizationMembership.mockResolvedValue({ id: 'om_test_new' }); + // Re-establish after clearAllMocks: handler calls workos!.userManagement.listOrganizationMemberships + // via the new WorkOS() instance; the mock must return admin membership for test user. + listOrganizationMemberships.mockImplementation(({ userId, organizationId }: { userId: string; organizationId: string }) => { + if (userId === TEST_ADMIN_USER_ID && organizationId === TEST_ORG_ID) { + return Promise.resolve({ + data: [{ id: 'om_admin', userId: TEST_ADMIN_USER_ID, organizationId: TEST_ORG_ID, role: { slug: 'admin' }, status: 'active' }], + }); + } + return Promise.resolve({ data: [] }); + }); await pool.query( `INSERT INTO organizations (workos_organization_id, name, is_personal, created_at, updated_at) @@ -126,12 +157,7 @@ describe('Join Request Approval', () => { await pool.query('DELETE FROM organization_join_requests WHERE workos_organization_id = $1', [TEST_ORG_ID]); }); - // Skipped: see #3289 — handler calls WorkOS userManagement.listOrganizationMemberships - // directly (organizations.ts:301) and the test isn't mocking @workos-inc/node, so the - // request hits WorkOS API with the test key and 401s → 500. Either add a per-file - // @workos-inc/node mock (member-by-email-policy.test.ts has the pattern) or refactor - // the route to push the membership check into a mockable helper. - it.skip('approves a join request by creating direct org membership, not sending an invitation', async () => { + it('approves a join request by creating direct org membership, not sending an invitation', async () => { const response = await request(app) .post(`/api/organizations/${TEST_ORG_ID}/join-requests/${joinRequestId}/approve`) .send({ role: 'member' }) @@ -148,7 +174,7 @@ describe('Join Request Approval', () => { expect(mockSendInvitation).not.toHaveBeenCalled(); }); - it.skip('marks the join request as approved after membership is created', async () => { + it('marks the join request as approved after membership is created', async () => { await request(app) .post(`/api/organizations/${TEST_ORG_ID}/join-requests/${joinRequestId}/approve`) .send({ role: 'member' }) @@ -161,7 +187,7 @@ describe('Join Request Approval', () => { expect(result.rows[0].status).toBe('approved'); }); - it.skip('returns 400 and clears stale pending row when user is already a member', async () => { + it('returns 400 and clears stale pending row when user is already a member', async () => { const alreadyMemberError: any = new Error('Already a member'); alreadyMemberError.code = 'organization_membership_already_exists'; mockCreateOrganizationMembership.mockRejectedValueOnce(alreadyMemberError); @@ -181,7 +207,7 @@ describe('Join Request Approval', () => { expect(result.rows[0].status).toBe('approved'); }); - it.skip('returns 409 and leaves pending row intact on cannot_reactivate error', async () => { + it('returns 409 and leaves pending row intact on cannot_reactivate error', async () => { // cannot_reactivate means a pending WorkOS invitation exists — the user is NOT yet // a member. The join request must stay pending for admin resolution. const reactivateError: any = new Error('Cannot reactivate'); @@ -203,7 +229,7 @@ describe('Join Request Approval', () => { expect(result.rows[0].status).toBe('pending'); }); - it.skip('returns 404 for a non-existent join request', async () => { + it('returns 404 for a non-existent join request', async () => { const response = await request(app) .post(`/api/organizations/${TEST_ORG_ID}/join-requests/00000000-0000-0000-0000-000000000000/approve`) .send({ role: 'member' }) diff --git a/server/tests/integration/personal-workspace-restrictions.test.ts b/server/tests/integration/personal-workspace-restrictions.test.ts index ae185df23a..039c857f9a 100644 --- a/server/tests/integration/personal-workspace-restrictions.test.ts +++ b/server/tests/integration/personal-workspace-restrictions.test.ts @@ -1,36 +1,59 @@ import { describe, it, expect, beforeAll, afterAll, beforeEach, afterEach, vi } from 'vitest'; -const TEST_USER_ID = 'user_personal_test'; -const TEST_PERSONAL_ORG_ID = 'org_personal_test'; -const TEST_TEAM_ORG_ID = 'org_team_test'; +// vi.hoisted ensures constants and mock fns are available inside vi.mock factories. +// Env vars must be set here so AUTH_ENABLED resolves true before organizations.ts loads +// and calls new WorkOS() — without them workos is null and every workos!. call throws. +const { + TEST_USER_ID, + TEST_PERSONAL_ORG_ID, + TEST_TEAM_ORG_ID, + listOrganizationMemberships, + sendInvitation, +} = vi.hoisted(() => { + process.env.WORKOS_API_KEY ||= 'sk_test_dummy_for_unit_tests'; + process.env.WORKOS_CLIENT_ID ||= 'client_test_dummy_for_unit_tests'; + process.env.WORKOS_COOKIE_PASSWORD ||= 'test-cookie-password-32chars-min-len-1234'; + return { + TEST_USER_ID: 'user_personal_test', + TEST_PERSONAL_ORG_ID: 'org_personal_test', + TEST_TEAM_ORG_ID: 'org_team_test', + listOrganizationMemberships: vi.fn(), + sendInvitation: vi.fn().mockResolvedValue({ id: 'inv_test' }), + }; +}); + +// organizations.ts calls new WorkOS() directly; mocking @workos-inc/node intercepts that +// constructor so every new WorkOS() returns the same shared mock methods. +vi.mock('@workos-inc/node', () => ({ + WorkOS: class { + userManagement = { + listOrganizationMemberships, + sendInvitation, + }; + organizations = { + getOrganization: vi.fn().mockImplementation((orgId: string) => Promise.resolve({ + id: orgId, + name: orgId === TEST_PERSONAL_ORG_ID ? 'Personal Workspace' : 'Team Workspace', + })), + }; + adminPortal = { + generateLink: vi.fn().mockResolvedValue({ link: 'https://test-portal.workos.com' }), + }; + }, +})); // Mock WorkOS client BEFORE any imports that use it vi.mock('../../src/auth/workos-client.js', () => ({ workos: { userManagement: { - listOrganizationMemberships: vi.fn().mockImplementation(({ userId, organizationId }) => { - if (organizationId === TEST_PERSONAL_ORG_ID || organizationId === TEST_TEAM_ORG_ID) { - return Promise.resolve({ - data: [{ - id: 'om_test', - userId: TEST_USER_ID, - organizationId: organizationId, - role: { slug: 'owner' }, - status: 'active' - }] - }); - } - return Promise.resolve({ data: [] }); - }), - sendInvitation: vi.fn().mockResolvedValue({ id: 'inv_test' }), + listOrganizationMemberships, + sendInvitation, }, organizations: { - getOrganization: vi.fn().mockImplementation((orgId) => { - return Promise.resolve({ - id: orgId, - name: orgId === TEST_PERSONAL_ORG_ID ? 'Personal Workspace' : 'Team Workspace', - }); - }), + getOrganization: vi.fn().mockImplementation((orgId: string) => Promise.resolve({ + id: orgId, + name: orgId === TEST_PERSONAL_ORG_ID ? 'Personal Workspace' : 'Team Workspace', + })), }, adminPortal: { generateLink: vi.fn().mockResolvedValue({ link: 'https://test-portal.workos.com' }), @@ -105,6 +128,18 @@ describe('Personal Workspace Restrictions', () => { }); beforeEach(async () => { + // Reset per-test: handler calls workos!.userManagement.listOrganizationMemberships via the new + // WorkOS() instance; return owner membership for test user in known org IDs. + // Note: the invitation test (team org) relies on community_only seat limit = 1 from DEFAULT_SEAT_LIMITS. + listOrganizationMemberships.mockReset().mockImplementation(({ organizationId }: { organizationId: string }) => { + if (organizationId === TEST_PERSONAL_ORG_ID || organizationId === TEST_TEAM_ORG_ID) { + return Promise.resolve({ + data: [{ id: 'om_test', userId: TEST_USER_ID, organizationId, role: { slug: 'owner' }, status: 'active' }], + }); + } + return Promise.resolve({ data: [] }); + }); + // Create fresh test organizations before each test await pool.query( `INSERT INTO organizations (workos_organization_id, name, is_personal, created_at, updated_at) @@ -122,16 +157,14 @@ describe('Personal Workspace Restrictions', () => { }); afterEach(async () => { - // Clean up test data + // Clean up test data; invitation_seat_types has no FK to organizations so must be deleted explicitly. + await pool.query('DELETE FROM invitation_seat_types WHERE workos_organization_id LIKE $1', ['org_team%']); await pool.query('DELETE FROM organizations WHERE workos_organization_id LIKE $1', ['org_personal%']); await pool.query('DELETE FROM organizations WHERE workos_organization_id LIKE $1', ['org_team%']); }); describe('POST /api/organizations/:orgId/invitations', () => { - // Skipped: see #3289 — handler hits WorkOS userManagement.* directly - // (organizations.ts) without a per-file @workos-inc/node mock; request hits - // real WorkOS with a test key and 401s into a 500. - it.skip('should reject invitations to personal workspaces', async () => { + it('should reject invitations to personal workspaces', async () => { const response = await request(app) .post(`/api/organizations/${TEST_PERSONAL_ORG_ID}/invitations`) .send({ email: 'test@example.com', role: 'member' }) @@ -141,7 +174,7 @@ describe('Personal Workspace Restrictions', () => { expect(response.body.message).toContain('Personal workspaces cannot have team members'); }); - it.skip('should allow invitations to team workspaces', async () => { + it('should allow invitations to team workspaces', async () => { const response = await request(app) .post(`/api/organizations/${TEST_TEAM_ORG_ID}/invitations`) .send({ email: 'test@example.com', role: 'member' }) @@ -152,7 +185,7 @@ describe('Personal Workspace Restrictions', () => { }); describe('POST /api/organizations/:orgId/domain-verification-link', () => { - it.skip('should reject domain verification for personal workspaces', async () => { + it('should reject domain verification for personal workspaces', async () => { const response = await request(app) .post(`/api/organizations/${TEST_PERSONAL_ORG_ID}/domain-verification-link`) .send() @@ -162,7 +195,7 @@ describe('Personal Workspace Restrictions', () => { expect(response.body.message).toContain('Personal workspaces cannot claim corporate domains'); }); - it.skip('should allow domain verification for team workspaces', async () => { + it('should allow domain verification for team workspaces', async () => { const response = await request(app) .post(`/api/organizations/${TEST_TEAM_ORG_ID}/domain-verification-link`) .send() diff --git a/server/tests/integration/self-service-delete.test.ts b/server/tests/integration/self-service-delete.test.ts index dec36aec3f..d5b51f8d5c 100644 --- a/server/tests/integration/self-service-delete.test.ts +++ b/server/tests/integration/self-service-delete.test.ts @@ -142,17 +142,15 @@ describe('Self-Service Delete Workspace', () => { }); describe('DELETE /api/organizations/:orgId', () => { - // Skipped: see #3289 — handler now returns 403 Access denied (rather - // than 404) when the user has no membership in the requested org. That's - // probably the right security behavior — don't enumerate orgs to outsiders — - // but the test was written against the older 404 surface. - it.skip('should return 404 for non-existent organization', async () => { + // Handler returns 403 Access denied for non-existent orgs: membership check runs + // before existence check, so callers cannot enumerate org IDs by probing deletions. + it('should return 403 when the requesting user has no membership in the org', async () => { const response = await request(app) .delete('/api/organizations/org_nonexistent') .send({ confirmation: 'Some Name' }) - .expect(404); + .expect(403); - expect(response.body.error).toBe('Organization not found'); + expect(response.body.error).toBe('Access denied'); }); it('should require confirmation to delete', async () => { @@ -289,20 +287,19 @@ describe('Self-Service Delete Workspace', () => { expect(response.body.error).toBe('Insufficient permissions'); }); - // Skipped: see #3289 — handler returns 500 on this path; either the - // active-subscription branch needs a fuller stripe-client mock or the - // route reads subscription status from somewhere this test doesn't seed. - it.skip('should prevent deletion of organization with active subscription', async () => { - // Create org with stripe customer + // OrganizationDatabase.getSubscriptionInfo reads subscription_status from the DB row; + // mocking the stripe-client import has no effect. Seed the column directly. + // No stripe_customer_id needed: without it, getSubscriptionInfo returns localInfo + // (built from subscription_status) without ever consulting Stripe. + it('should prevent deletion of organization with active subscription', async () => { const SUB_ORG_ID = 'org_self_delete_sub'; await pool.query( - `INSERT INTO organizations (workos_organization_id, name, stripe_customer_id, created_at, updated_at) - VALUES ($1, $2, $3, NOW(), NOW()) - ON CONFLICT (workos_organization_id) DO UPDATE SET name = $2, stripe_customer_id = $3`, - [SUB_ORG_ID, 'Subscribed Org', 'cus_sub_test'] + `INSERT INTO organizations (workos_organization_id, name, subscription_status, created_at, updated_at) + VALUES ($1, $2, 'active', NOW(), NOW()) + ON CONFLICT (workos_organization_id) DO UPDATE SET name = $2, subscription_status = 'active'`, + [SUB_ORG_ID, 'Subscribed Org'] ); - // Override the WorkOS membership mock so this test's user owns SUB_ORG_ID. workosMocks.listOrganizationMemberships.mockImplementation(({ organizationId }: { organizationId: string }) => { if (organizationId === SUB_ORG_ID) { return Promise.resolve({ @@ -318,16 +315,6 @@ describe('Self-Service Delete Workspace', () => { return Promise.resolve({ data: [] }); }); - // Mock getSubscriptionInfo to return active subscription - const { getSubscriptionInfo } = await import('../../src/billing/stripe-client.js'); - vi.mocked(getSubscriptionInfo).mockResolvedValueOnce({ - status: 'active', - product_id: 'prod_test', - product_name: 'Test Product', - current_period_end: Math.floor(Date.now() / 1000) + 86400, - cancel_at_period_end: false, - }); - const response = await request(app) .delete(`/api/organizations/${SUB_ORG_ID}`) .send({ confirmation: 'Subscribed Org' })