From 3ca54c7f282268c324378a1931aee3c199e0bf07 Mon Sep 17 00:00:00 2001 From: dal Date: Thu, 10 Jul 2025 07:45:04 -0600 Subject: [PATCH] Refactor integration tests for domain management and workspace settings. Update error messages for permission checks to be more specific. Enhance domain handling logic for consistency and improve test cleanup procedures. --- .../security/add-approved-domains.int.test.ts | 3 +- .../src/api/v2/security/domain-service.ts | 4 +- .../security/get-approved-domains.int.test.ts | 44 ++++++++++++------- .../v2/security/get-approved-domains.test.ts | 15 +++++++ .../get-workspace-settings.int.test.ts | 2 +- .../remove-approved-domains.int.test.ts | 6 +-- .../api/v2/security/security-utils.test.ts | 4 +- .../src/api/v2/security/security-utils.ts | 2 +- .../src/api/v2/security/test-fixtures.ts | 1 - .../update-workspace-settings.int.test.ts | 7 +-- .../update-workspace-settings.test.ts | 2 +- .../v2/security/update-workspace-settings.ts | 6 ++- .../workspace-settings-service.test.ts | 2 +- .../security/WorkspaceRestrictions.tsx | 6 +-- .../server-shared/src/security/requests.ts | 2 +- 15 files changed, 65 insertions(+), 41 deletions(-) diff --git a/apps/server/src/api/v2/security/add-approved-domains.int.test.ts b/apps/server/src/api/v2/security/add-approved-domains.int.test.ts index f072f3e73..f694477b5 100644 --- a/apps/server/src/api/v2/security/add-approved-domains.int.test.ts +++ b/apps/server/src/api/v2/security/add-approved-domains.int.test.ts @@ -99,7 +99,6 @@ describe('addApprovedDomainsHandler (integration)', () => { const userWithoutOrg = await createUserWithoutOrganization(); const request = { domains: ['new.com'] }; - await expect(addApprovedDomainsHandler(request, userWithoutOrg)).rejects.toThrow(HTTPException); await expect(addApprovedDomainsHandler(request, userWithoutOrg)).rejects.toMatchObject({ status: 403, message: 'User is not associated with an organization', @@ -117,7 +116,7 @@ describe('addApprovedDomainsHandler (integration)', () => { await expect(addApprovedDomainsHandler(request, testUser)).rejects.toThrow(HTTPException); await expect(addApprovedDomainsHandler(request, testUser)).rejects.toMatchObject({ status: 403, - message: 'Insufficient permissions to manage approved domains', + message: 'Insufficient admin permissions', }); }); diff --git a/apps/server/src/api/v2/security/domain-service.ts b/apps/server/src/api/v2/security/domain-service.ts index cf0bd6cf8..bde03ad68 100644 --- a/apps/server/src/api/v2/security/domain-service.ts +++ b/apps/server/src/api/v2/security/domain-service.ts @@ -10,8 +10,8 @@ export class DomainService { // Filter out duplicates from new domains const uniqueNew = normalizedNew.filter((d) => !normalizedCurrent.includes(d)); - // Return original current domains plus unique new ones - return [...currentDomains, ...uniqueNew]; + // Normalize all domains for consistency + return [...normalizedCurrent, ...uniqueNew]; } filterDomains(currentDomains: string[], domainsToRemove: string[]): string[] { diff --git a/apps/server/src/api/v2/security/get-approved-domains.int.test.ts b/apps/server/src/api/v2/security/get-approved-domains.int.test.ts index ad98abad1..7babf610a 100644 --- a/apps/server/src/api/v2/security/get-approved-domains.int.test.ts +++ b/apps/server/src/api/v2/security/get-approved-domains.int.test.ts @@ -71,31 +71,41 @@ describe('getApprovedDomainsHandler (integration)', () => { const roles = ['querier', 'data_admin', 'workspace_admin']; for (const role of roles) { - const roleUser = await createTestUserInDb(); - await createTestOrgMemberInDb(roleUser.id, testOrg.id, role); + let roleUser; + try { + roleUser = await createTestUserInDb(); + await createTestOrgMemberInDb(roleUser.id, testOrg.id, role); - const result = await getApprovedDomainsHandler(roleUser); + const result = await getApprovedDomainsHandler(roleUser); - expect(result).toHaveLength(2); - expect(result.map((d) => d.domain)).toContain('example.com'); - expect(result.map((d) => d.domain)).toContain('test.io'); - - await cleanupTestUser(roleUser.id); + expect(result).toHaveLength(2); + expect(result.map((d) => d.domain)).toContain('example.com'); + expect(result.map((d) => d.domain)).toContain('test.io'); + } finally { + if (roleUser) { + await cleanupTestUser(roleUser.id); + } + } } }); }); describe('Error Cases', () => { it('should return 403 for user without organization', async () => { - const userWithoutOrg = await createUserWithoutOrganization(); - - await expect(getApprovedDomainsHandler(userWithoutOrg)).rejects.toThrow(HTTPException); - await expect(getApprovedDomainsHandler(userWithoutOrg)).rejects.toMatchObject({ - status: 403, - message: 'User is not associated with an organization', - }); - - await cleanupTestUser(userWithoutOrg.id); + let userWithoutOrg; + try { + userWithoutOrg = await createUserWithoutOrganization(); + + await expect(getApprovedDomainsHandler(userWithoutOrg)).rejects.toThrow(HTTPException); + await expect(getApprovedDomainsHandler(userWithoutOrg)).rejects.toMatchObject({ + status: 403, + message: 'User is not associated with an organization', + }); + } finally { + if (userWithoutOrg) { + await cleanupTestUser(userWithoutOrg.id); + } + } }); it('should return 404 for deleted organization', async () => { diff --git a/apps/server/src/api/v2/security/get-approved-domains.test.ts b/apps/server/src/api/v2/security/get-approved-domains.test.ts index 852d570c1..9540ea360 100644 --- a/apps/server/src/api/v2/security/get-approved-domains.test.ts +++ b/apps/server/src/api/v2/security/get-approved-domains.test.ts @@ -66,6 +66,21 @@ describe('getApprovedDomainsHandler', () => { expect(result).toEqual([]); }); + it('should return empty array for org with empty domains array', async () => { + const orgWithEmptyDomains = { ...mockOrg, domains: [] }; + vi.mocked(securityUtils.fetchOrganization).mockResolvedValue(orgWithEmptyDomains); + + vi.mocked(DomainService.prototype.formatDomainsResponse).mockReturnValue([]); + + const result = await getApprovedDomainsHandler(mockUser); + + expect(DomainService.prototype.formatDomainsResponse).toHaveBeenCalledWith( + [], + mockOrg.createdAt + ); + expect(result).toEqual([]); + }); + it('should handle validation errors', async () => { vi.mocked(securityUtils.validateUserOrganization).mockRejectedValue( new Error('User not in organization') diff --git a/apps/server/src/api/v2/security/get-workspace-settings.int.test.ts b/apps/server/src/api/v2/security/get-workspace-settings.int.test.ts index 8b8dee696..25ab81f09 100644 --- a/apps/server/src/api/v2/security/get-workspace-settings.int.test.ts +++ b/apps/server/src/api/v2/security/get-workspace-settings.int.test.ts @@ -112,7 +112,6 @@ describe('getWorkspaceSettingsHandler (integration)', () => { it('should return 403 for user without organization', async () => { const userWithoutOrg = await createUserWithoutOrganization(); - await expect(getWorkspaceSettingsHandler(userWithoutOrg)).rejects.toThrow(HTTPException); await expect(getWorkspaceSettingsHandler(userWithoutOrg)).rejects.toMatchObject({ status: 403, message: 'User is not associated with an organization', @@ -165,6 +164,7 @@ describe('getWorkspaceSettingsHandler (integration)', () => { .where(eq(organizations.id, testOrg.id)) .limit(1); + expect(orgAfter.length).toBe(1); expect(orgAfter[0]?.restrictNewUserInvitations).toEqual( originalOrg.restrictNewUserInvitations ); diff --git a/apps/server/src/api/v2/security/remove-approved-domains.int.test.ts b/apps/server/src/api/v2/security/remove-approved-domains.int.test.ts index ec62502cb..f24ae80c4 100644 --- a/apps/server/src/api/v2/security/remove-approved-domains.int.test.ts +++ b/apps/server/src/api/v2/security/remove-approved-domains.int.test.ts @@ -47,7 +47,7 @@ describe('removeApprovedDomainsHandler (integration)', () => { }); it('should handle non-existent domain removal gracefully', async () => { - await createTestOrgMemberInDb(testUser.id, testOrg.id, 'data_admin'); + await createTestOrgMemberInDb(testUser.id, testOrg.id, 'workspace_admin'); const request = { domains: ['notfound.com', 'alsonotfound.com'] }; const result = await removeApprovedDomainsHandler(request, testUser); @@ -109,7 +109,7 @@ describe('removeApprovedDomainsHandler (integration)', () => { await expect(removeApprovedDomainsHandler(request, testUser)).rejects.toThrow(HTTPException); await expect(removeApprovedDomainsHandler(request, testUser)).rejects.toMatchObject({ status: 403, - message: 'Insufficient permissions to manage approved domains', + message: 'Insufficient admin permissions', }); // Verify no domains were removed @@ -166,7 +166,7 @@ describe('removeApprovedDomainsHandler (integration)', () => { await createTestOrgMemberInDb(testUser.id, testOrg.id, 'workspace_admin'); const originalUpdatedAt = testOrg.updatedAt; - await new Promise((resolve) => setTimeout(resolve, 10)); // Ensure time difference + await new Promise((resolve) => setTimeout(resolve, 100)); // Ensure time difference const request = { domains: ['remove1.com'] }; await removeApprovedDomainsHandler(request, testUser); diff --git a/apps/server/src/api/v2/security/security-utils.test.ts b/apps/server/src/api/v2/security/security-utils.test.ts index 74b1b0111..b372163c5 100644 --- a/apps/server/src/api/v2/security/security-utils.test.ts +++ b/apps/server/src/api/v2/security/security-utils.test.ts @@ -126,7 +126,7 @@ describe('security-utils', () => { roles.forEach((role) => { expect(() => checkAdminPermissions(role)).toThrow(HTTPException); expect(() => checkAdminPermissions(role)).toThrow( - 'Insufficient permissions to manage approved domains' + 'Insufficient admin permissions' ); }); }); @@ -134,7 +134,7 @@ describe('security-utils', () => { it('should reject null role', () => { expect(() => checkAdminPermissions(null)).toThrow(HTTPException); expect(() => checkAdminPermissions(null)).toThrow( - 'Insufficient permissions to manage approved domains' + 'Insufficient admin permissions' ); }); diff --git a/apps/server/src/api/v2/security/security-utils.ts b/apps/server/src/api/v2/security/security-utils.ts index 32eecde23..84de70cb1 100644 --- a/apps/server/src/api/v2/security/security-utils.ts +++ b/apps/server/src/api/v2/security/security-utils.ts @@ -38,7 +38,7 @@ export async function fetchOrganization(organizationId: string) { export function checkAdminPermissions(role: string | null): void { if (role !== 'workspace_admin' && role !== 'data_admin') { throw new HTTPException(403, { - message: 'Insufficient permissions to manage approved domains', + message: 'Insufficient admin permissions', }); } } diff --git a/apps/server/src/api/v2/security/test-fixtures.ts b/apps/server/src/api/v2/security/test-fixtures.ts index f8eea8b50..2edaf6bed 100644 --- a/apps/server/src/api/v2/security/test-fixtures.ts +++ b/apps/server/src/api/v2/security/test-fixtures.ts @@ -31,7 +31,6 @@ export function createTestOrganization(overrides?: Partial): Organ } export function createTestOrgMember( - userId: string, organizationId: string, role = 'querier' ): { organizationId: string; role: string } { diff --git a/apps/server/src/api/v2/security/update-workspace-settings.int.test.ts b/apps/server/src/api/v2/security/update-workspace-settings.int.test.ts index 2beec3165..20a8b740a 100644 --- a/apps/server/src/api/v2/security/update-workspace-settings.int.test.ts +++ b/apps/server/src/api/v2/security/update-workspace-settings.int.test.ts @@ -2,7 +2,7 @@ import type { Organization, User } from '@buster/database'; import { db, usersToOrganizations } from '@buster/database'; import { eq } from 'drizzle-orm'; import { HTTPException } from 'hono/http-exception'; -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { cleanupTestOrganization, cleanupTestUser, @@ -136,9 +136,6 @@ describe('updateWorkspaceSettingsHandler (integration)', () => { const request = { restrict_new_user_invitations: true }; - await expect(updateWorkspaceSettingsHandler(request, roleUser)).rejects.toThrow( - HTTPException - ); await expect(updateWorkspaceSettingsHandler(request, roleUser)).rejects.toMatchObject({ status: 403, message: 'Only workspace admins can update workspace settings', @@ -225,7 +222,7 @@ describe('updateWorkspaceSettingsHandler (integration)', () => { await createTestOrgMemberInDb(testUser.id, testOrg.id, 'workspace_admin'); const originalUpdatedAt = testOrg.updatedAt; - await new Promise((resolve) => setTimeout(resolve, 10)); // Ensure time difference + await new Promise((resolve) => setTimeout(resolve, 100)); // Ensure time difference const request = { restrict_new_user_invitations: true }; await updateWorkspaceSettingsHandler(request, testUser); diff --git a/apps/server/src/api/v2/security/update-workspace-settings.test.ts b/apps/server/src/api/v2/security/update-workspace-settings.test.ts index 883566fd8..712082cb2 100644 --- a/apps/server/src/api/v2/security/update-workspace-settings.test.ts +++ b/apps/server/src/api/v2/security/update-workspace-settings.test.ts @@ -113,7 +113,7 @@ describe('updateWorkspaceSettingsHandler', () => { }); it('should update database with new settings', async () => { - const request = { default_role: 'admin' }; + const request = { default_role: 'data_admin' }; const mockDbChain = { update: vi.fn().mockReturnThis(), set: vi.fn().mockReturnThis(), diff --git a/apps/server/src/api/v2/security/update-workspace-settings.ts b/apps/server/src/api/v2/security/update-workspace-settings.ts index fc74c3b39..1c485307f 100644 --- a/apps/server/src/api/v2/security/update-workspace-settings.ts +++ b/apps/server/src/api/v2/security/update-workspace-settings.ts @@ -72,7 +72,11 @@ export async function updateWorkspaceSettingsHandler( } catch (error) { console.error('Error in updateWorkspaceSettingsHandler:', { userId: user.id, - request, + requestFields: { + hasRestrictNewUserInvitations: request.restrict_new_user_invitations !== undefined, + hasDefaultRole: request.default_role !== undefined, + hasDefaultDatasets: request.default_datasets_ids !== undefined, + }, error: error instanceof Error ? error.message : error, }); diff --git a/apps/server/src/api/v2/security/workspace-settings-service.test.ts b/apps/server/src/api/v2/security/workspace-settings-service.test.ts index aeded9976..947cb17cd 100644 --- a/apps/server/src/api/v2/security/workspace-settings-service.test.ts +++ b/apps/server/src/api/v2/security/workspace-settings-service.test.ts @@ -47,7 +47,7 @@ describe('WorkspaceSettingsService', () => { }); it('should handle all string values correctly', () => { - const roles = ['member', 'admin', 'workspace_admin', 'data_admin']; + const roles = ['workspace_admin', 'data_admin', 'querier', 'restricted_querier', 'viewer', 'none']; roles.forEach((role) => { const settings = { diff --git a/apps/web/src/components/features/security/WorkspaceRestrictions.tsx b/apps/web/src/components/features/security/WorkspaceRestrictions.tsx index 0f02de255..e689c24ed 100644 --- a/apps/web/src/components/features/security/WorkspaceRestrictions.tsx +++ b/apps/web/src/components/features/security/WorkspaceRestrictions.tsx @@ -26,17 +26,17 @@ export const WorkspaceRestrictions = React.memo(() => { () => [ , , ], diff --git a/packages/server-shared/src/security/requests.ts b/packages/server-shared/src/security/requests.ts index 7bc42dea5..0103bfaab 100644 --- a/packages/server-shared/src/security/requests.ts +++ b/packages/server-shared/src/security/requests.ts @@ -28,7 +28,7 @@ export const RemoveApprovedDomainRequestSchema = z.preprocess( return { ...rest, domains: [domainsArray] }; } - return rest; + return { ...rest, domains: [] }; } return input; },