From 166373aa85ca09e8a8448fc1c85325559e641c4d Mon Sep 17 00:00:00 2001 From: Hendrik Date: Thu, 26 Feb 2026 17:29:49 +0100 Subject: [PATCH] fixed error and failing unit tests (#203) --- src/hooks.server.test.ts | 5 +- .../services/__tests__/staff-service.test.ts | 125 ++++++++++++------ .../services/__tests__/user-service.test.ts | 55 ++++++++ src/lib/server/services/staff-service.ts | 82 +++++------- .../server/services/tenant-admin-service.ts | 4 +- src/lib/server/services/user-service.ts | 8 +- .../(pages)/dashboard/staff/+page.server.ts | 2 +- .../confirm/__tests__/confirm-api.test.ts | 2 +- .../tenants/[id]/staff/[staffId]/+server.ts | 15 +-- src/server-hooks/rateLimitHandle.ts | 2 +- 10 files changed, 191 insertions(+), 109 deletions(-) diff --git a/src/hooks.server.test.ts b/src/hooks.server.test.ts index c5c3e92..7b3c71e 100644 --- a/src/hooks.server.test.ts +++ b/src/hooks.server.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { handle } from "./hooks.server"; import { mockCookies } from "$lib/tests/const"; +import { RATE_LIMIT_MAX_REQUESTS } from "./server-hooks/rateLimitHandle"; // Mock the sequence function to avoid the request store issue vi.mock("@sveltejs/kit/hooks", () => ({ @@ -116,7 +117,7 @@ describe("hooks.server", () => { const event = createEvent("192.168.1.101"); // Unique IP for this test // Make multiple requests to exceed rate limit - for (let i = 0; i < 10; i++) { + for (let i = 0; i < RATE_LIMIT_MAX_REQUESTS; i++) { await handle({ event, resolve: mockResolve }); } @@ -131,7 +132,7 @@ describe("hooks.server", () => { const event = createEvent("192.168.1.102"); // Unique IP for this test // Exceed rate limit - for (let i = 0; i < 11; i++) { + for (let i = 0; i < RATE_LIMIT_MAX_REQUESTS + 1; i++) { await handle({ event, resolve: mockResolve }); } diff --git a/src/lib/server/services/__tests__/staff-service.test.ts b/src/lib/server/services/__tests__/staff-service.test.ts index 36028ba..a74de7b 100644 --- a/src/lib/server/services/__tests__/staff-service.test.ts +++ b/src/lib/server/services/__tests__/staff-service.test.ts @@ -148,20 +148,39 @@ describe("StaffService", () => { }); describe("deleteStaffMember", () => { - it("should prevent deletion of last staff member", async () => { + it("should propagate validation errors from UserService", async () => { const { centralDb } = await import("../../db"); + const { UserService } = await import("../user-service"); - // Mock the non-global admin staff check - only one staff member exists - const mockSelectBuilder = { - from: vi.fn().mockReturnThis(), - where: vi.fn().mockResolvedValue([ - { - id: "staff-123", // Only one staff member - }, - ]), - }; + vi.mocked(UserService.deleteUser).mockRejectedValue( + new ValidationError("Cannot delete the last STAFF user for this tenant"), + ); - vi.mocked(centralDb.select).mockReturnValue(mockSelectBuilder as any); + const mockTransaction = vi.fn().mockImplementation(async (callback) => { + const tx = { + select: vi.fn().mockReturnValueOnce({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockReturnValue({ + limit: vi.fn().mockResolvedValue([ + { + id: "staff-123", + email: "staff@example.com", + name: "Staff Member", + role: "STAFF", + tenantId: "tenant-123", + }, + ]), + }), + }), + }), + delete: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue({ count: 1 }), + }), + }; + return await callback(tx); + }); + + vi.mocked(centralDb.transaction).mockImplementation(mockTransaction); await expect(StaffService.deleteStaffMember("tenant-123", "staff-123")).rejects.toThrow( ValidationError, @@ -172,21 +191,6 @@ describe("StaffService", () => { const { centralDb, getTenantDb } = await import("../../db"); const { UserService } = await import("../user-service"); - // Mock the non-global admin staff check - multiple staff members exist - const mockSelectBuilder = { - from: vi.fn().mockReturnThis(), - where: vi.fn().mockResolvedValue([ - { - id: "staff-123", - }, - { - id: "staff-456", // Multiple staff members - }, - ]), - }; - - vi.mocked(centralDb.select).mockReturnValue(mockSelectBuilder as any); - // Mock UserService.deleteUser - wichtig: das muss vor der transaction() mock sein const mockUserDeletionResult = { success: true, @@ -253,27 +257,24 @@ describe("StaffService", () => { it("should throw NotFoundError when staff member not found", async () => { const { centralDb } = await import("../../db"); - // Mock the pre-transaction select queries - const mockSelectBuilder = { - from: vi.fn().mockReturnThis(), - where: vi.fn().mockResolvedValue([ - { - id: "staff-456", // Different ID to simulate multiple staff members - }, - ]), - }; - - vi.mocked(centralDb.select).mockReturnValue(mockSelectBuilder as any); - const mockTransaction = vi.fn().mockImplementation(async (callback) => { const tx = { - select: vi.fn().mockReturnValue({ - from: vi.fn().mockReturnValue({ - where: vi.fn().mockReturnValue({ - limit: vi.fn().mockResolvedValue([]), // No user found + select: vi + .fn() + .mockReturnValueOnce({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockReturnValue({ + limit: vi.fn().mockResolvedValue([]), // No user found + }), + }), + }) + .mockReturnValueOnce({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockReturnValue({ + limit: vi.fn().mockResolvedValue([]), // No invite found + }), }), }), - }), }; return await callback(tx); }); @@ -284,6 +285,44 @@ describe("StaffService", () => { NotFoundError, ); }); + + it("should delete invited staff member when no user exists", async () => { + const { centralDb } = await import("../../db"); + + const mockTransaction = vi.fn().mockImplementation(async (callback) => { + const tx = { + select: vi + .fn() + .mockReturnValueOnce({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockReturnValue({ + limit: vi.fn().mockResolvedValue([]), // No user found + }), + }), + }) + .mockReturnValueOnce({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockReturnValue({ + limit: vi.fn().mockResolvedValue([{ id: "staff-123" }]), // Invite found + }), + }), + }), + delete: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue({ count: 1 }), + }), + }; + return await callback(tx); + }); + + vi.mocked(centralDb.transaction).mockImplementation(mockTransaction); + + const result = await StaffService.deleteStaffMember("tenant-123", "staff-123"); + + expect(result.success).toBe(true); + expect(result.deletedUser.id).toBe("staff-123"); + expect(result.deletedPasskeysCount).toBe(0); + expect(result.deletedKeySharesCount).toBe(0); + }); }); describe("getStaffPublicKey", () => { diff --git a/src/lib/server/services/__tests__/user-service.test.ts b/src/lib/server/services/__tests__/user-service.test.ts index a863328..70aee52 100644 --- a/src/lib/server/services/__tests__/user-service.test.ts +++ b/src/lib/server/services/__tests__/user-service.test.ts @@ -370,6 +370,8 @@ describe("UserService", () => { name: "Last Admin", role: "TENANT_ADMIN", tenantId: "tenant-123", + isActive: true, + confirmationState: "ACCESS_GRANTED", }, ]), }), @@ -412,6 +414,8 @@ describe("UserService", () => { name: "Deleted Admin", role: "TENANT_ADMIN", tenantId: "tenant-123", + isActive: true, + confirmationState: "ACCESS_GRANTED", }, ]), }), @@ -445,6 +449,57 @@ describe("UserService", () => { expect(result.deletedPasskeysCount).toBe(1); expect(result.tenantId).toBe("tenant-123"); }); + + it("should allow deleting non-ACCESS_GRANTED staff", async () => { + const userId = "018f-a1b2-c3d4-e5f6-789abcdef099"; + const mockDeletedUser = { + id: userId, + name: "Pending Staff", + email: "pending@example.com", + role: "STAFF", + }; + + const mockTransaction = vi.fn().mockImplementation(async (callback) => { + const tx = { + select: vi.fn().mockReturnValueOnce({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockReturnValue({ + limit: vi.fn().mockResolvedValue([ + { + id: userId, + email: "pending@example.com", + name: "Pending Staff", + role: "STAFF", + tenantId: "tenant-123", + isActive: true, + confirmationState: "CONFIRMED", + }, + ]), + }), + }), + }), + delete: vi + .fn() + .mockReturnValueOnce({ + where: vi.fn().mockResolvedValue({ count: 0 }), + }) + .mockReturnValueOnce({ + where: vi.fn().mockReturnValue({ + returning: vi.fn().mockResolvedValue([mockDeletedUser]), + }), + }), + }; + return await callback(tx); + }); + + mockCentralDb.transaction.mockImplementation(mockTransaction); + + const result = await UserService.deleteUser(userId); + + expect(result.success).toBe(true); + expect(result.deletedUser).toEqual(mockDeletedUser); + expect(result.deletedPasskeysCount).toBe(0); + }); }); describe("addPasskey", () => { diff --git a/src/lib/server/services/staff-service.ts b/src/lib/server/services/staff-service.ts index c67e6ee..970a85e 100644 --- a/src/lib/server/services/staff-service.ts +++ b/src/lib/server/services/staff-service.ts @@ -185,7 +185,6 @@ export class StaffService { tenantId: string, staffId: string, currentUserId?: string, - confirmationState?: "INVITED" | "CONFIRMED" | "ACCESS_GRANTED", ): Promise { logger.debug("Deleting staff member", { tenantId, staffId, currentUserId }); @@ -194,58 +193,40 @@ export class StaffService { throw new ValidationError("You cannot delete your own account"); } - // Check if this is the only non-global admin staff member - const nonGlobalAdminStaff = await centralDb - .select({ id: user.id }) - .from(user) - .where( - and( - eq(user.tenantId, tenantId), - eq(user.isActive, true), - or(eq(user.role, "TENANT_ADMIN"), eq(user.role, "STAFF")), - eq(user.confirmationState, "ACCESS_GRANTED"), // prevents us from deleting the last staff member with appointment access - ), - ); - - if (nonGlobalAdminStaff.length === 1 && nonGlobalAdminStaff[0].id === staffId) { - throw new ValidationError("Cannot delete the last active non-global admin staff member"); - } - try { // Use transaction to ensure all related data is deleted consistently const result = await centralDb.transaction(async (tx) => { - // First, verify the user exists and belongs to this tenant - if (confirmationState !== "INVITED") { - const userToDelete = await tx - .select({ - id: user.id, - email: user.email, - name: user.name, - role: user.role, - tenantId: user.tenantId, - }) - .from(user) - .where(and(eq(user.id, staffId), eq(user.tenantId, tenantId))) - .limit(1); + const userToDelete = await tx + .select({ + id: user.id, + email: user.email, + name: user.name, + role: user.role, + tenantId: user.tenantId, + }) + .from(user) + .where(and(eq(user.id, staffId), eq(user.tenantId, tenantId))) + .limit(1); - if (userToDelete.length === 0) { - throw new NotFoundError("Staff member not found in this tenant"); - } - - // Use UserService to delete central database data (user + passkeys) first - // This ensures all validation logic is applied before deleting tenant data - const userDeletionResult = await UserService.deleteUser(staffId, tx); - - // Remove old invites of user if any exist + if (userToDelete.length > 0) { + // Remove invites referencing this user before deleting user to avoid FK violations on createdUserId const deletedInvites = await tx .delete(userInvite) - .where(eq(userInvite.email, userToDelete[0].email)); + .where( + or( + eq(userInvite.createdUserId, staffId), + eq(userInvite.email, userToDelete[0].email), + ), + ); logger.debug("Deleted user invites", { staffId, tenantId, deletedCount: deletedInvites.count || 0, }); + // Use UserService to delete central database data (user + passkeys) + const userDeletionResult = await UserService.deleteUser(staffId, tx); + // Delete tenant-specific data (client tunnel key shares) after user deletion succeeds const tenantDb = await getTenantDb(tenantId); const keyShareDeletionResult = await tenantDb @@ -262,7 +243,6 @@ export class StaffService { deletedKeySharesCount, }); - // Combine results for staff-specific response format const staffDeletionResult: StaffDeletionResult = { success: userDeletionResult.success, deletedUser: userDeletionResult.deletedUser, @@ -279,7 +259,15 @@ export class StaffService { }); return staffDeletionResult; - } else { + } + + const inviteToDelete = await tx + .select({ id: userInvite.id }) + .from(userInvite) + .where(and(eq(userInvite.id, staffId), eq(userInvite.tenantId, tenantId))) + .limit(1); + + if (inviteToDelete.length > 0) { const deletedInvites = await tx.delete(userInvite).where(eq(userInvite.id, staffId)); logger.debug("Deleted user invites", { staffId, @@ -299,17 +287,19 @@ export class StaffService { deletedKeySharesCount: 0, }; } + + throw new NotFoundError("Staff member not found in this tenant"); }); return result; } catch (error) { - if (error instanceof ValidationError || error instanceof NotFoundError) { - throw error; - } logger.error("Failed to delete staff member", { tenantId, staffId, error: String(error), }); + if (error instanceof ValidationError || error instanceof NotFoundError) { + throw error; + } throw new InternalError("Failed to delete staff member"); } } diff --git a/src/lib/server/services/tenant-admin-service.ts b/src/lib/server/services/tenant-admin-service.ts index d3c0042..9dad0c3 100644 --- a/src/lib/server/services/tenant-admin-service.ts +++ b/src/lib/server/services/tenant-admin-service.ts @@ -190,7 +190,9 @@ export class TenantAdminService { // Do not log logo to not spam logs data: { ...tenant.#tenant, - databaseUrl: redactDbUrl(tenant.#tenant.databaseUrl), + databaseUrl: tenant.#tenant.databaseUrl + ? redactDbUrl(tenant.#tenant.databaseUrl) + : "", logo: tenant.#tenant.logo ? "removed-from-log-but-set" : null, }, }); diff --git a/src/lib/server/services/user-service.ts b/src/lib/server/services/user-service.ts index 508e4cf..42e9957 100644 --- a/src/lib/server/services/user-service.ts +++ b/src/lib/server/services/user-service.ts @@ -578,6 +578,8 @@ export class UserService { name: centralSchema.user.name, role: centralSchema.user.role, tenantId: centralSchema.user.tenantId, + isActive: centralSchema.user.isActive, + confirmationState: centralSchema.user.confirmationState, }) .from(centralSchema.user) .where(eq(centralSchema.user.id, userId)) @@ -591,7 +593,11 @@ export class UserService { const user = userToDelete[0]; // We cannot delete the last user with access to the tenant's appointments - if (user.role === "TENANT_ADMIN" || user.role === "STAFF") { + if ( + (user.role === "TENANT_ADMIN" || user.role === "STAFF") && + user.isActive === true && + user.confirmationState === "ACCESS_GRANTED" + ) { const usersCount = await tx .select({ count: count() }) .from(centralSchema.user) diff --git a/src/routes/(pages)/dashboard/staff/+page.server.ts b/src/routes/(pages)/dashboard/staff/+page.server.ts index 76be18d..1b82089 100644 --- a/src/routes/(pages)/dashboard/staff/+page.server.ts +++ b/src/routes/(pages)/dashboard/staff/+page.server.ts @@ -162,7 +162,7 @@ export const actions: Actions = { } const resp = await event.fetch( - `/api/tenants/${event.locals.user?.tenantId}/staff/${form.data.id}?confirmationState=${form.data.confirmationState}`, + `/api/tenants/${event.locals.user?.tenantId}/staff/${form.data.id}`, { method: "DELETE", headers: { diff --git a/src/routes/api/tenants/[id]/appointments/[appointmentId]/confirm/__tests__/confirm-api.test.ts b/src/routes/api/tenants/[id]/appointments/[appointmentId]/confirm/__tests__/confirm-api.test.ts index 7008ccd..948cab6 100644 --- a/src/routes/api/tenants/[id]/appointments/[appointmentId]/confirm/__tests__/confirm-api.test.ts +++ b/src/routes/api/tenants/[id]/appointments/[appointmentId]/confirm/__tests__/confirm-api.test.ts @@ -78,7 +78,7 @@ describe("Appointment Confirm API Route", () => { expect(mockAppointmentService.confirmAppointment).toHaveBeenCalledWith( mockAppointmentId, undefined, - "de", + "en", ); }); diff --git a/src/routes/api/tenants/[id]/staff/[staffId]/+server.ts b/src/routes/api/tenants/[id]/staff/[staffId]/+server.ts index 2ae2da7..62de6eb 100644 --- a/src/routes/api/tenants/[id]/staff/[staffId]/+server.ts +++ b/src/routes/api/tenants/[id]/staff/[staffId]/+server.ts @@ -243,16 +243,10 @@ registerOpenAPIRoute("/tenants/{id}/staff/{staffId}", "PUT", { }, }); -export const DELETE: RequestHandler = async ({ request, params, locals }) => { +export const DELETE: RequestHandler = async ({ params, locals }) => { const log = logger.setContext("API"); const tenantId = params.id; const staffId = params.staffId; - const { searchParams } = new URL(request.url); - const confirmationState = searchParams.get("confirmationState") as - | "INVITED" - | "CONFIRMED" - | "ACCESS_GRANTED" - | undefined; if (!tenantId) { throw new ValidationError(ERRORS.TENANTS.NO_TENANT_ID); @@ -266,12 +260,7 @@ export const DELETE: RequestHandler = async ({ request, params, locals }) => { checkPermission(locals, tenantId, true); try { - const result = await StaffService.deleteStaffMember( - tenantId, - staffId, - locals.user?.id, - confirmationState, - ); + const result = await StaffService.deleteStaffMember(tenantId, staffId, locals.user?.id); return json(result); } catch (error) { diff --git a/src/server-hooks/rateLimitHandle.ts b/src/server-hooks/rateLimitHandle.ts index 7a6e78c..c18cb3b 100644 --- a/src/server-hooks/rateLimitHandle.ts +++ b/src/server-hooks/rateLimitHandle.ts @@ -6,7 +6,7 @@ const rateLimitStore = new Map(); /** Rate limiting window duration in milliseconds */ const RATE_LIMIT_WINDOW = 2000; // ms /** Maximum requests allowed per rate limiting window */ -const RATE_LIMIT_MAX_REQUESTS = 20; +export const RATE_LIMIT_MAX_REQUESTS = 20; /** * Extracts the client IP address from request headers