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.

This commit is contained in:
dal 2025-07-10 07:45:04 -06:00
parent f22c03874c
commit 3ca54c7f28
No known key found for this signature in database
GPG Key ID: 16F4B0E1E9F61122
15 changed files with 65 additions and 41 deletions

View File

@ -99,7 +99,6 @@ describe('addApprovedDomainsHandler (integration)', () => {
const userWithoutOrg = await createUserWithoutOrganization(); const userWithoutOrg = await createUserWithoutOrganization();
const request = { domains: ['new.com'] }; const request = { domains: ['new.com'] };
await expect(addApprovedDomainsHandler(request, userWithoutOrg)).rejects.toThrow(HTTPException);
await expect(addApprovedDomainsHandler(request, userWithoutOrg)).rejects.toMatchObject({ await expect(addApprovedDomainsHandler(request, userWithoutOrg)).rejects.toMatchObject({
status: 403, status: 403,
message: 'User is not associated with an organization', 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.toThrow(HTTPException);
await expect(addApprovedDomainsHandler(request, testUser)).rejects.toMatchObject({ await expect(addApprovedDomainsHandler(request, testUser)).rejects.toMatchObject({
status: 403, status: 403,
message: 'Insufficient permissions to manage approved domains', message: 'Insufficient admin permissions',
}); });
}); });

View File

@ -10,8 +10,8 @@ export class DomainService {
// Filter out duplicates from new domains // Filter out duplicates from new domains
const uniqueNew = normalizedNew.filter((d) => !normalizedCurrent.includes(d)); const uniqueNew = normalizedNew.filter((d) => !normalizedCurrent.includes(d));
// Return original current domains plus unique new ones // Normalize all domains for consistency
return [...currentDomains, ...uniqueNew]; return [...normalizedCurrent, ...uniqueNew];
} }
filterDomains(currentDomains: string[], domainsToRemove: string[]): string[] { filterDomains(currentDomains: string[], domainsToRemove: string[]): string[] {

View File

@ -71,31 +71,41 @@ describe('getApprovedDomainsHandler (integration)', () => {
const roles = ['querier', 'data_admin', 'workspace_admin']; const roles = ['querier', 'data_admin', 'workspace_admin'];
for (const role of roles) { for (const role of roles) {
const roleUser = await createTestUserInDb(); let roleUser;
await createTestOrgMemberInDb(roleUser.id, testOrg.id, role); 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).toHaveLength(2);
expect(result.map((d) => d.domain)).toContain('example.com'); expect(result.map((d) => d.domain)).toContain('example.com');
expect(result.map((d) => d.domain)).toContain('test.io'); expect(result.map((d) => d.domain)).toContain('test.io');
} finally {
await cleanupTestUser(roleUser.id); if (roleUser) {
await cleanupTestUser(roleUser.id);
}
}
} }
}); });
}); });
describe('Error Cases', () => { describe('Error Cases', () => {
it('should return 403 for user without organization', async () => { it('should return 403 for user without organization', async () => {
const userWithoutOrg = await createUserWithoutOrganization(); let userWithoutOrg;
try {
await expect(getApprovedDomainsHandler(userWithoutOrg)).rejects.toThrow(HTTPException); userWithoutOrg = await createUserWithoutOrganization();
await expect(getApprovedDomainsHandler(userWithoutOrg)).rejects.toMatchObject({
status: 403, await expect(getApprovedDomainsHandler(userWithoutOrg)).rejects.toThrow(HTTPException);
message: 'User is not associated with an organization', await expect(getApprovedDomainsHandler(userWithoutOrg)).rejects.toMatchObject({
}); status: 403,
message: 'User is not associated with an organization',
await cleanupTestUser(userWithoutOrg.id); });
} finally {
if (userWithoutOrg) {
await cleanupTestUser(userWithoutOrg.id);
}
}
}); });
it('should return 404 for deleted organization', async () => { it('should return 404 for deleted organization', async () => {

View File

@ -66,6 +66,21 @@ describe('getApprovedDomainsHandler', () => {
expect(result).toEqual([]); 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 () => { it('should handle validation errors', async () => {
vi.mocked(securityUtils.validateUserOrganization).mockRejectedValue( vi.mocked(securityUtils.validateUserOrganization).mockRejectedValue(
new Error('User not in organization') new Error('User not in organization')

View File

@ -112,7 +112,6 @@ describe('getWorkspaceSettingsHandler (integration)', () => {
it('should return 403 for user without organization', async () => { it('should return 403 for user without organization', async () => {
const userWithoutOrg = await createUserWithoutOrganization(); const userWithoutOrg = await createUserWithoutOrganization();
await expect(getWorkspaceSettingsHandler(userWithoutOrg)).rejects.toThrow(HTTPException);
await expect(getWorkspaceSettingsHandler(userWithoutOrg)).rejects.toMatchObject({ await expect(getWorkspaceSettingsHandler(userWithoutOrg)).rejects.toMatchObject({
status: 403, status: 403,
message: 'User is not associated with an organization', message: 'User is not associated with an organization',
@ -165,6 +164,7 @@ describe('getWorkspaceSettingsHandler (integration)', () => {
.where(eq(organizations.id, testOrg.id)) .where(eq(organizations.id, testOrg.id))
.limit(1); .limit(1);
expect(orgAfter.length).toBe(1);
expect(orgAfter[0]?.restrictNewUserInvitations).toEqual( expect(orgAfter[0]?.restrictNewUserInvitations).toEqual(
originalOrg.restrictNewUserInvitations originalOrg.restrictNewUserInvitations
); );

View File

@ -47,7 +47,7 @@ describe('removeApprovedDomainsHandler (integration)', () => {
}); });
it('should handle non-existent domain removal gracefully', async () => { 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 request = { domains: ['notfound.com', 'alsonotfound.com'] };
const result = await removeApprovedDomainsHandler(request, testUser); 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.toThrow(HTTPException);
await expect(removeApprovedDomainsHandler(request, testUser)).rejects.toMatchObject({ await expect(removeApprovedDomainsHandler(request, testUser)).rejects.toMatchObject({
status: 403, status: 403,
message: 'Insufficient permissions to manage approved domains', message: 'Insufficient admin permissions',
}); });
// Verify no domains were removed // Verify no domains were removed
@ -166,7 +166,7 @@ describe('removeApprovedDomainsHandler (integration)', () => {
await createTestOrgMemberInDb(testUser.id, testOrg.id, 'workspace_admin'); await createTestOrgMemberInDb(testUser.id, testOrg.id, 'workspace_admin');
const originalUpdatedAt = testOrg.updatedAt; 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'] }; const request = { domains: ['remove1.com'] };
await removeApprovedDomainsHandler(request, testUser); await removeApprovedDomainsHandler(request, testUser);

View File

@ -126,7 +126,7 @@ describe('security-utils', () => {
roles.forEach((role) => { roles.forEach((role) => {
expect(() => checkAdminPermissions(role)).toThrow(HTTPException); expect(() => checkAdminPermissions(role)).toThrow(HTTPException);
expect(() => checkAdminPermissions(role)).toThrow( 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', () => { it('should reject null role', () => {
expect(() => checkAdminPermissions(null)).toThrow(HTTPException); expect(() => checkAdminPermissions(null)).toThrow(HTTPException);
expect(() => checkAdminPermissions(null)).toThrow( expect(() => checkAdminPermissions(null)).toThrow(
'Insufficient permissions to manage approved domains' 'Insufficient admin permissions'
); );
}); });

View File

@ -38,7 +38,7 @@ export async function fetchOrganization(organizationId: string) {
export function checkAdminPermissions(role: string | null): void { export function checkAdminPermissions(role: string | null): void {
if (role !== 'workspace_admin' && role !== 'data_admin') { if (role !== 'workspace_admin' && role !== 'data_admin') {
throw new HTTPException(403, { throw new HTTPException(403, {
message: 'Insufficient permissions to manage approved domains', message: 'Insufficient admin permissions',
}); });
} }
} }

View File

@ -31,7 +31,6 @@ export function createTestOrganization(overrides?: Partial<Organization>): Organ
} }
export function createTestOrgMember( export function createTestOrgMember(
userId: string,
organizationId: string, organizationId: string,
role = 'querier' role = 'querier'
): { organizationId: string; role: string } { ): { organizationId: string; role: string } {

View File

@ -2,7 +2,7 @@ import type { Organization, User } from '@buster/database';
import { db, usersToOrganizations } from '@buster/database'; import { db, usersToOrganizations } from '@buster/database';
import { eq } from 'drizzle-orm'; import { eq } from 'drizzle-orm';
import { HTTPException } from 'hono/http-exception'; import { HTTPException } from 'hono/http-exception';
import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { import {
cleanupTestOrganization, cleanupTestOrganization,
cleanupTestUser, cleanupTestUser,
@ -136,9 +136,6 @@ describe('updateWorkspaceSettingsHandler (integration)', () => {
const request = { restrict_new_user_invitations: true }; const request = { restrict_new_user_invitations: true };
await expect(updateWorkspaceSettingsHandler(request, roleUser)).rejects.toThrow(
HTTPException
);
await expect(updateWorkspaceSettingsHandler(request, roleUser)).rejects.toMatchObject({ await expect(updateWorkspaceSettingsHandler(request, roleUser)).rejects.toMatchObject({
status: 403, status: 403,
message: 'Only workspace admins can update workspace settings', message: 'Only workspace admins can update workspace settings',
@ -225,7 +222,7 @@ describe('updateWorkspaceSettingsHandler (integration)', () => {
await createTestOrgMemberInDb(testUser.id, testOrg.id, 'workspace_admin'); await createTestOrgMemberInDb(testUser.id, testOrg.id, 'workspace_admin');
const originalUpdatedAt = testOrg.updatedAt; 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 }; const request = { restrict_new_user_invitations: true };
await updateWorkspaceSettingsHandler(request, testUser); await updateWorkspaceSettingsHandler(request, testUser);

View File

@ -113,7 +113,7 @@ describe('updateWorkspaceSettingsHandler', () => {
}); });
it('should update database with new settings', async () => { it('should update database with new settings', async () => {
const request = { default_role: 'admin' }; const request = { default_role: 'data_admin' };
const mockDbChain = { const mockDbChain = {
update: vi.fn().mockReturnThis(), update: vi.fn().mockReturnThis(),
set: vi.fn().mockReturnThis(), set: vi.fn().mockReturnThis(),

View File

@ -72,7 +72,11 @@ export async function updateWorkspaceSettingsHandler(
} catch (error) { } catch (error) {
console.error('Error in updateWorkspaceSettingsHandler:', { console.error('Error in updateWorkspaceSettingsHandler:', {
userId: user.id, 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, error: error instanceof Error ? error.message : error,
}); });

View File

@ -47,7 +47,7 @@ describe('WorkspaceSettingsService', () => {
}); });
it('should handle all string values correctly', () => { 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) => { roles.forEach((role) => {
const settings = { const settings = {

View File

@ -26,17 +26,17 @@ export const WorkspaceRestrictions = React.memo(() => {
() => [ () => [
<EnableRestrictions <EnableRestrictions
key="enable-restrictions" key="enable-restrictions"
restrict_new_user_invitations={workspaceSettings?.restrict_new_user_invitations} restrict_new_user_invitations={workspaceSettings?.restrict_new_user_invitations ?? false}
updateWorkspaceSettings={updateWorkspaceSettings} updateWorkspaceSettings={updateWorkspaceSettings}
/>, />,
<DefaultRole <DefaultRole
key="default-role" key="default-role"
default_role={workspaceSettings?.default_role} default_role={workspaceSettings?.default_role ?? 'viewer' as OrganizationRole}
updateWorkspaceSettings={updateWorkspaceSettings} updateWorkspaceSettings={updateWorkspaceSettings}
/>, />,
<DefaultDatasets <DefaultDatasets
key="default-datasets" key="default-datasets"
default_datasets={workspaceSettings?.default_datasets} default_datasets={workspaceSettings?.default_datasets ?? []}
updateWorkspaceSettings={updateWorkspaceSettings} updateWorkspaceSettings={updateWorkspaceSettings}
/> />
], ],

View File

@ -28,7 +28,7 @@ export const RemoveApprovedDomainRequestSchema = z.preprocess(
return { ...rest, domains: [domainsArray] }; return { ...rest, domains: [domainsArray] };
} }
return rest; return { ...rest, domains: [] };
} }
return input; return input;
}, },