From 42d92b07b958f82c132e64e9ba6304f163b6a4ea Mon Sep 17 00:00:00 2001 From: Pramod Date: Sat, 24 Jan 2026 16:28:26 +0530 Subject: [PATCH 01/13] feat(identity): strict layer enforcement for SystemAdminController - Decouple SystemAdminController from Drizzle - Implement IUserProvider/ITenantProvider methods - Rewrite controller tests with strict mocking - Correct Jest coverage exclusions --- apps/api/package.json | 13 +- .../system-admin.controller.spec.ts | 680 +++++------------- .../system-admin/system-admin.controller.ts | 301 ++------ .../identity/users/users.controller.spec.ts | 4 +- .../identity/users/users.controller.ts | 5 +- docs/old/nexiom_tech_stack_and_repo_plan.md | 1 + .../task/01c_strict_layer_enforcement.md | 58 ++ .../src/adapters/drizzle-tenant.adapter.ts | 100 ++- .../src/adapters/drizzle-user.adapter.ts | 119 ++- .../interfaces/tenant-provider.interface.ts | 27 + .../src/interfaces/user-provider.interface.ts | 16 +- 11 files changed, 572 insertions(+), 752 deletions(-) create mode 100644 docs/refactor/modules/identity/task/01c_strict_layer_enforcement.md diff --git a/apps/api/package.json b/apps/api/package.json index 2c81e971..233cf442 100644 --- a/apps/api/package.json +++ b/apps/api/package.json @@ -95,14 +95,15 @@ }, "collectCoverageFrom": [ "**/*.(t|j)s", - "!main.ts", + "!src/main.ts", "!**/*.module.ts", "!**/*.abstract.ts", - "!test/**", - "!db/seed.ts", - "!db/truncate.ts", - "!db/verify.ts", - "!scripts/**" + "!src/test/**", + "!src/db/seed.ts", + "!src/db/truncate.ts", + "!src/db/verify.ts", + "!src/scripts/**", + "!test/**" ], "coverageDirectory": "./coverage", "testEnvironment": "node", diff --git a/apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts b/apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts index d476bb30..32592cc6 100644 --- a/apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts +++ b/apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts @@ -1,84 +1,60 @@ +import { Test, TestingModule } from '@nestjs/testing'; import { SystemAdminController } from './system-admin.controller'; - -import { NodePgDatabase } from 'drizzle-orm/node-postgres'; -import * as schema from '../../../db/schema'; import { BadRequestException, NotFoundException } from '@nestjs/common'; - -interface MockDb { - query: { - user: { findMany: jest.Mock; findFirst: jest.Mock }; - organization: { findFirst: jest.Mock; findMany: jest.Mock }; - member: { findFirst: jest.Mock }; - invitation: { findFirst: jest.Mock }; - }; - select: jest.Mock; - from: jest.Mock; - insert: jest.Mock; - update: jest.Mock; - delete: jest.Mock; - transaction: jest.Mock; - // Chain helpers - limit: jest.Mock; - offset: jest.Mock; - orderBy: jest.Mock; - groupBy: jest.Mock; - leftJoin: jest.Mock; - set: jest.Mock; - where: jest.Mock; - values: jest.Mock; - returning: jest.Mock; -} - -import { IAuthProvider } from '@nexiom/identity'; +import { + AUTH_PROVIDER, + USER_PROVIDER, + TENANT_PROVIDER, +} from '@nexiom/identity'; +import { SystemAdminGuard } from '../auth/system-admin.guard'; +import { PlatformGuard } from '../auth/platform.guard'; describe('SystemAdminController', () => { let controller: SystemAdminController; - let mockDb: MockDb; - let mockIdentityProvider: { - getSessionFromHeaders: jest.Mock; - createInvitation: jest.Mock; + const mockHeaders: Record = {}; + + const mockAuthProvider = { + getSessionFromHeaders: jest.fn(), + createInvitation: jest.fn(), }; - beforeEach(() => { + const mockUserProvider = { + findById: jest.fn(), + findByEmail: jest.fn(), + findAll: jest.fn(), + create: jest.fn(), + update: jest.fn(), + delete: jest.fn(), + count: jest.fn(), + }; + + const mockTenantProvider = { + findById: jest.fn(), + findBySlug: jest.fn(), + findAll: jest.fn(), + createTenant: jest.fn(), + update: jest.fn(), + delete: jest.fn(), + }; + + beforeEach(async () => { jest.clearAllMocks(); - mockIdentityProvider = { - getSessionFromHeaders: jest.fn(), - createInvitation: jest.fn(), - }; - - // Reset mocks with full structure - /* eslint-disable @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-return */ - mockDb = { - query: { - user: { findMany: jest.fn(), findFirst: jest.fn() }, - organization: { findFirst: jest.fn(), findMany: jest.fn() }, - member: { findFirst: jest.fn() }, - invitation: { findFirst: jest.fn() }, - }, - select: jest.fn().mockReturnThis(), - from: jest.fn().mockImplementation(() => Promise.resolve([{ count: 5 }])), - insert: jest.fn(), - update: jest.fn(), - delete: jest.fn(), - - transaction: jest.fn((cb) => cb(mockDb)), // Mock transaction execution - // Chain method definitions - limit: jest.fn().mockReturnThis(), - offset: jest.fn().mockReturnThis(), - orderBy: jest.fn().mockReturnThis(), - groupBy: jest.fn().mockReturnThis(), - leftJoin: jest.fn().mockReturnThis(), - set: jest.fn().mockReturnThis(), - where: jest.fn().mockReturnThis(), - values: jest.fn().mockReturnThis(), - returning: jest.fn(), - }; - - controller = new SystemAdminController( - mockDb as unknown as NodePgDatabase, - mockIdentityProvider as unknown as IAuthProvider, - ); + const module: TestingModule = await Test.createTestingModule({ + controllers: [SystemAdminController], + providers: [ + { provide: AUTH_PROVIDER, useValue: mockAuthProvider }, + { provide: USER_PROVIDER, useValue: mockUserProvider }, + { provide: TENANT_PROVIDER, useValue: mockTenantProvider }, + ], + }) + .overrideGuard(SystemAdminGuard) + .useValue({ canActivate: jest.fn(() => true) }) + .overrideGuard(PlatformGuard) + .useValue({ canActivate: jest.fn(() => true) }) + .compile(); + + controller = module.get(SystemAdminController); }); it('should be defined', () => { @@ -87,149 +63,58 @@ describe('SystemAdminController', () => { describe('listUsers', () => { it('should return paginated users and total count', async () => { - const mockUsers = [{ id: '1', name: 'User 1' }]; - mockDb.query.user.findMany.mockResolvedValue(mockUsers); + const mockResult = { data: [{ id: '1', name: 'User 1' }], total: 1 }; + mockUserProvider.findAll.mockResolvedValue(mockResult); const result = await controller.listUsers('1', '10'); - expect(result).toEqual({ - data: mockUsers, - total: 5, + expect(result).toEqual(mockResult); + expect(mockUserProvider.findAll).toHaveBeenCalledWith({ + page: 1, + limit: 10, }); - expect(mockDb.query.user.findMany).toHaveBeenCalledWith( - expect.objectContaining({ limit: 10, offset: 0 }), - ); - }); - - it('should handle invalid pagination params', async () => { - mockDb.query.user.findMany.mockResolvedValue([]); - - await controller.listUsers('bad', 'bad'); - - expect(mockDb.query.user.findMany).toHaveBeenCalledWith( - expect.objectContaining({ limit: 10, offset: 0 }), - ); }); it('should use default pagination parameters', async () => { - mockDb.query.user.findMany.mockResolvedValue([]); + mockUserProvider.findAll.mockResolvedValue({ data: [], total: 0 }); - // Testing default params - await controller.listUsers(); + await controller.listUsers(); // Defaults - expect(mockDb.query.user.findMany).toHaveBeenCalledWith( - expect.objectContaining({ limit: 10, offset: 0 }), - ); + expect(mockUserProvider.findAll).toHaveBeenCalledWith({ + page: 1, + limit: 10, + }); }); }); describe('listTenants', () => { - // Define strict interface for our chainable builder - interface MockQueryBuilder { - leftJoin: jest.Mock; - groupBy: jest.Mock; - limit: jest.Mock; - offset: jest.Mock; - orderBy: jest.Mock; - execute: () => Promise; - } - - const createMockBuilder = (result: unknown): MockQueryBuilder => ({ - leftJoin: jest.fn().mockReturnThis(), - groupBy: jest.fn().mockReturnThis(), - limit: jest.fn().mockReturnThis(), - offset: jest.fn().mockReturnThis(), - orderBy: jest.fn().mockReturnThis(), - execute: jest.fn().mockResolvedValue(result), - }); - - it('should return paginated tenants with aggregated user count', async () => { - // Mock db.select chain for tenants - const mockResult = [{ id: 't1', name: 'Tenant 1', userCount: 2 }]; - - // Call 1: Data - const mockQueryBuilder = createMockBuilder(mockResult); - mockDb.from.mockReturnValueOnce(mockQueryBuilder); - - // Call 2: Total Count - mockDb.from.mockReturnValueOnce(Promise.resolve([{ count: 5 }])); + it('should return paginated tenants', async () => { + const mockResult = { data: [{ id: 't1', name: 'Tenant 1' }], total: 1 }; + mockTenantProvider.findAll.mockResolvedValue(mockResult); const result = await controller.listTenants('1', '10'); - expect(result).toEqual({ - data: [{ id: 't1', name: 'Tenant 1', userCount: 2 }], - total: 5, + expect(result).toEqual(mockResult); + expect(mockTenantProvider.findAll).toHaveBeenCalledWith({ + page: 1, + limit: 10, }); - - // Verify specific builder usage - expect(mockQueryBuilder.limit).toHaveBeenCalledWith(10); - expect(mockQueryBuilder.offset).toHaveBeenCalledWith(0); - expect(mockQueryBuilder.execute).toHaveBeenCalled(); - }); - - it('should clamp pagination parameters', async () => { - const mockBuilder = createMockBuilder([]); - - mockDb.from.mockReturnValueOnce(mockBuilder); - mockDb.from.mockReturnValueOnce(Promise.resolve([{ count: 0 }])); - - // Request 1000 items (should clamp to 100) - await controller.listTenants('1', '1000'); - - expect(mockBuilder.limit).toHaveBeenCalledWith(100); - expect(mockBuilder.offset).toHaveBeenCalledWith(0); - expect(mockBuilder.execute).toHaveBeenCalled(); - }); - - it('should use default pagination parameters', async () => { - const mockBuilder = createMockBuilder([]); - mockDb.from.mockReturnValueOnce(mockBuilder); - mockDb.from.mockReturnValueOnce(Promise.resolve([{ count: 0 }])); - - // Testing default params - await controller.listTenants(); - - expect(mockBuilder.limit).toHaveBeenCalledWith(10); - expect(mockBuilder.offset).toHaveBeenCalledWith(0); - expect(mockBuilder.execute).toHaveBeenCalled(); }); }); describe('getTenant', () => { - const createMockBuilder = (result: unknown) => ({ - leftJoin: jest.fn().mockReturnThis(), - groupBy: jest.fn().mockReturnThis(), - limit: jest.fn().mockReturnThis(), - offset: jest.fn().mockReturnThis(), - orderBy: jest.fn().mockReturnThis(), - where: jest.fn().mockReturnThis(), // Added where - execute: jest.fn().mockResolvedValue(result), - }); - - it('should return a single tenant', async () => { - const mockResult = [{ id: 't1', name: 'Tenant 1' }]; - const mockQueryBuilder = createMockBuilder(mockResult); - - // Mock select().from() chain - const mockSelect = { - from: jest.fn().mockReturnValue(mockQueryBuilder), - }; - mockDb.select.mockReturnValueOnce(mockSelect); + it('should return a tenant by ID', async () => { + const mockTenant = { id: 't1', name: 'Tenant 1' }; + mockTenantProvider.findById.mockResolvedValue(mockTenant); const result = await controller.getTenant('t1'); - expect(result).toEqual(mockResult[0]); - expect(mockSelect.from).toHaveBeenCalledWith(schema.organization); - expect(mockQueryBuilder.where).toHaveBeenCalled(); - expect(mockQueryBuilder.execute).toHaveBeenCalled(); + expect(result).toEqual(mockTenant); + expect(mockTenantProvider.findById).toHaveBeenCalledWith('t1'); }); it('should throw NotFoundException if tenant not found', async () => { - const mockQueryBuilder = createMockBuilder([]); - const mockSelect = { - from: jest.fn().mockReturnValue(mockQueryBuilder), - }; - mockDb.select.mockReturnValueOnce(mockSelect); + mockTenantProvider.findById.mockResolvedValue(null); await expect(controller.getTenant('missing')).rejects.toThrow( NotFoundException, @@ -239,7 +124,7 @@ describe('SystemAdminController', () => { describe('createTenant', () => { it('should throw BadRequestException if slug exists', async () => { - mockDb.query.organization.findFirst.mockResolvedValue({ id: 'existing' }); + mockTenantProvider.findBySlug.mockResolvedValue({ id: 'existing' }); await expect( controller.createTenant({ @@ -250,13 +135,10 @@ describe('SystemAdminController', () => { ).rejects.toThrow(BadRequestException); }); - it('should create tenant if slug is unique', async () => { - mockDb.query.organization.findFirst.mockResolvedValue(null); - mockDb.insert = jest.fn().mockReturnValue({ - values: jest.fn().mockReturnValue({ - returning: jest.fn().mockResolvedValue([{ id: 'new', slug: 'test' }]), - }), - }); + it('should create tenant via provider if slug is unique', async () => { + mockTenantProvider.findBySlug.mockResolvedValue(null); + const mockCreated = { id: 'new', slug: 'test' }; + mockTenantProvider.createTenant.mockResolvedValue(mockCreated); const result = await controller.createTenant({ name: 'Test', @@ -264,319 +146,160 @@ describe('SystemAdminController', () => { logo: '', }); - expect(result).toEqual({ id: 'new', slug: 'test' }); + expect(result).toEqual(mockCreated); + expect(mockTenantProvider.createTenant).toHaveBeenCalledWith({ + name: 'Test', + slug: 'test', + logo: '', + }); }); }); describe('createUser', () => { it('should throw BadRequestException if email exists', async () => { - mockDb.query.user.findFirst.mockResolvedValue({ id: 'existing' }); + mockUserProvider.findByEmail.mockResolvedValue({ id: 'existing' }); await expect( controller.createUser({ name: 'Test', - email: 'test@example.com', + email: 'taken@example.com', systemRole: 'platform_user', }), ).rejects.toThrow(BadRequestException); }); - it('should create user if email is unique', async () => { - mockDb.query.user.findFirst.mockResolvedValue(null); - mockDb.insert = jest.fn().mockReturnValue({ - values: jest.fn().mockReturnValue({ - returning: jest.fn().mockResolvedValue([ - { - id: 'new', - email: 'test@example.com', - name: 'Test', - systemRole: 'platform_user', - }, - ]), - }), + it('should create user via provider', async () => { + mockUserProvider.findByEmail.mockResolvedValue(null); + const mockUser = { id: 'u1', email: 'new@example.com' }; // Initial return + mockUserProvider.create.mockResolvedValue(mockUser); + // Simulate subsequent fetch for systemRole update if needed + mockUserProvider.findById.mockResolvedValue({ + ...mockUser, + systemRole: 'platform_user', }); const result = await controller.createUser({ name: 'Test', - email: 'test@example.com', + email: 'new@example.com', systemRole: 'platform_user', }); - expect(result).toEqual({ - id: 'new', - email: 'test@example.com', - name: 'Test', + // Matches controller logic: if systemRole, fetches updated + expect(result).toEqual({ ...mockUser, systemRole: 'platform_user' }); + expect(mockUserProvider.create).toHaveBeenCalled(); + expect(mockUserProvider.update).toHaveBeenCalledWith('u1', { systemRole: 'platform_user', }); - expect(mockDb.insert).toHaveBeenCalledWith(schema.user); }); }); describe('updateTenant', () => { it('should throw NotFoundException if tenant not found', async () => { - mockDb.query.organization.findFirst.mockResolvedValue(null); + mockTenantProvider.findById.mockResolvedValue(null); await expect( controller.updateTenant('missing', { name: 'New' }), ).rejects.toThrow(NotFoundException); }); - it('should throw BadRequestException if new slug overlaps', async () => { - mockDb.query.organization.findFirst - .mockResolvedValueOnce({ id: 't1', slug: 'old' }) // Existing target - .mockResolvedValueOnce({ id: 't2', slug: 'taken' }); // Collision check + it('should throw BadRequestException if slug overlaps', async () => { + mockTenantProvider.findById.mockResolvedValue({ + id: 't1', + slug: 'old-slug', + }); + mockTenantProvider.findBySlug.mockResolvedValue({ + id: 't2', // diff ID + slug: 'taken', + }); await expect( controller.updateTenant('t1', { slug: 'taken' }), ).rejects.toThrow(BadRequestException); }); - it('should throw BadRequestException if payload is empty', async () => { - mockDb.query.organization.findFirst.mockResolvedValue({ id: 't1' }); - // Valid tenant, but empty update - await expect(controller.updateTenant('t1', {})).rejects.toThrow( - BadRequestException, - ); - }); + it('should update tenant via provider', async () => { + mockTenantProvider.findById.mockResolvedValue({ id: 't1' }); + // Slug check: returns null or same ID + mockTenantProvider.findBySlug.mockResolvedValue(null); + const mockUpdated = { id: 't1', name: 'New' }; + mockTenantProvider.update.mockResolvedValue(mockUpdated); - it('should update tenant successfully', async () => { - // 1. Find existing - mockDb.query.organization.findFirst - .mockResolvedValueOnce({ id: 't1', slug: 'old' }) // Existing - .mockResolvedValueOnce(null); // Collision check (slug change) - - // 2. Update - mockDb.update = jest.fn().mockReturnValue({ - set: jest.fn().mockReturnValue({ - where: jest.fn().mockReturnValue({ - returning: jest - .fn() - .mockResolvedValue([{ id: 't1', slug: 'new-slug', name: 'New' }]), - }), - }), - }); + const result = await controller.updateTenant('t1', { name: 'New' }); - const result = await controller.updateTenant('t1', { + expect(result).toEqual(mockUpdated); + expect(mockTenantProvider.update).toHaveBeenCalledWith('t1', { name: 'New', - slug: 'new-slug', }); - - expect(result).toEqual({ id: 't1', slug: 'new-slug', name: 'New' }); - // Verify update called with correct args - expect(mockDb.update).toHaveBeenCalled(); - }); - - it('should update specific fields (status only)', async () => { - mockDb.query.organization.findFirst.mockResolvedValue({ id: 't1' }); - - const mockReturning = jest - .fn() - .mockResolvedValue([{ id: 't1', status: 'suspended' }]); - const mockSet = jest.fn().mockReturnValue({ - where: jest.fn().mockReturnValue({ returning: mockReturning }), - }); - mockDb.update = jest.fn().mockReturnValue({ set: mockSet }); - - await controller.updateTenant('t1', { status: 'suspended' }); - - // Verify payload passed to set() - expect(mockSet).toHaveBeenCalledWith( - expect.objectContaining({ status: 'suspended' }), - ); - }); - - it('should update all fields including metadata', async () => { - mockDb.query.organization.findFirst - .mockResolvedValueOnce({ id: 't1', slug: 'old' }) - .mockResolvedValueOnce(null); - - const mockSet = jest.fn().mockReturnValue({ - where: jest.fn().mockReturnValue({ - returning: jest.fn().mockResolvedValue([{ id: 't1' }]), - }), - }); - mockDb.update = jest.fn().mockReturnValue({ set: mockSet }); - - const payload = { - name: 'Full Update', - slug: 'full-update', - logo: 'logo.png', - status: 'active' as const, - metadata: { key: 'value' }, - }; - - await controller.updateTenant('t1', payload); - - expect(mockSet).toHaveBeenCalledWith( - expect.objectContaining({ - name: 'Full Update', - slug: 'full-update', - logo: 'logo.png', - status: 'active', - metadata: JSON.stringify({ key: 'value' }), - }), - ); }); }); describe('deleteTenant', () => { it('should throw NotFoundException if tenant not found', async () => { - mockDb.query.organization.findFirst.mockResolvedValue(null); + mockTenantProvider.findById.mockResolvedValue(null); await expect(controller.deleteTenant('missing')).rejects.toThrow( NotFoundException, ); }); - it('should delete tenant successfully inside transaction', async () => { - // 1. Find existing - mockDb.query.organization.findFirst.mockResolvedValue({ id: 't1' }); - - // 2. Setup Deletes - // We expect 3 deletes: member, invitation, organization - // We'll mock the delete chain to return a 'where' mock - const mockWhere = jest.fn().mockResolvedValue({}); - mockDb.delete = jest.fn().mockReturnValue({ - where: mockWhere, - }); + it('should delete tenant via provider', async () => { + mockTenantProvider.findById.mockResolvedValue({ id: 't1' }); await controller.deleteTenant('t1'); - expect(mockDb.transaction).toHaveBeenCalled(); - - // Verify calls inside transaction - // Since it's inside transaction, we check the calls on mockDb (because our mock transaction calls cb(mockDb)) - // Call 1: Member - expect(mockDb.delete).toHaveBeenNthCalledWith(1, schema.member); - // Call 2: Invitation - expect(mockDb.delete).toHaveBeenNthCalledWith(2, schema.invitation); - // Call 3: Organization - expect(mockDb.delete).toHaveBeenNthCalledWith(3, schema.organization); - - // Verify 3 executions of 'where' - expect(mockWhere).toHaveBeenCalledTimes(3); + expect(mockTenantProvider.delete).toHaveBeenCalledWith('t1'); }); }); describe('updateUser', () => { it('should throw NotFoundException if user not found', async () => { - mockDb.query.user.findFirst.mockResolvedValue(null); + mockUserProvider.findById.mockResolvedValue(null); await expect( controller.updateUser('missing', { name: 'New' }), ).rejects.toThrow(NotFoundException); }); - it('should throw BadRequestException if payload is empty', async () => { - mockDb.query.user.findFirst.mockResolvedValue({ id: 'u1' }); - - await expect(controller.updateUser('u1', {})).rejects.toThrow( - BadRequestException, - ); - }); - - it('should update user successfully', async () => { - mockDb.query.user.findFirst.mockResolvedValue({ id: 'u1' }); - - mockDb.update = jest.fn().mockReturnValue({ - set: jest.fn().mockReturnValue({ - where: jest.fn().mockReturnValue({ - returning: jest - .fn() - .mockResolvedValue([ - { id: 'u1', name: 'New Name', systemRole: 'platform_admin' }, - ]), - }), - }), - }); - - const result = await controller.updateUser('u1', { - name: 'New Name', - systemRole: 'platform_admin', - }); - - expect(result).toEqual({ + it('should throw BadRequestException if email overlaps', async () => { + mockUserProvider.findById.mockResolvedValue({ id: 'u1', - name: 'New Name', - systemRole: 'platform_admin', + email: 'old@example.com', }); - expect(mockDb.update).toHaveBeenCalledWith(schema.user); - }); - - it('should update user specific fields (emailVerified)', async () => { - mockDb.query.user.findFirst.mockResolvedValue({ id: 'u1' }); - - const mockSet = jest.fn().mockReturnValue({ - where: jest.fn().mockReturnValue({ - returning: jest.fn().mockResolvedValue([{ id: 'u1' }]), - }), + mockUserProvider.findByEmail.mockResolvedValue({ + id: 'u2', // Diff ID + email: 'taken@example.com', }); - mockDb.update = jest.fn().mockReturnValue({ set: mockSet }); - - await controller.updateUser('u1', { emailVerified: true }); - - expect(mockSet).toHaveBeenCalledWith( - expect.objectContaining({ emailVerified: true }), - ); - }); - - it('should throw BadRequestException if email already taken (pre-check)', async () => { - // 1. Return payload user first - mockDb.query.user.findFirst - .mockResolvedValueOnce({ id: 'u1', email: 'old@example.com' }) - // 2. Return collision user next - .mockResolvedValueOnce({ id: 'u2', email: 'taken@example.com' }); await expect( controller.updateUser('u1', { email: 'taken@example.com' }), ).rejects.toThrow(BadRequestException); }); - it('should throw BadRequestException on race condition (duplicate key)', async () => { - // 1. Return payload user (user exists) - mockDb.query.user.findFirst.mockResolvedValueOnce({ - id: 'u1', - email: 'old@example.com', - }); + it('should update user via provider', async () => { + mockUserProvider.findById.mockResolvedValue({ id: 'u1' }); + mockUserProvider.update.mockResolvedValue({ id: 'u1', name: 'New' }); - // 2. Return null for uniqueness check (simulate pre-check pass) - mockDb.query.user.findFirst.mockResolvedValueOnce(null); - - // 3. Mock db update to throw unique constraint error (race condition hit) - mockDb.update = jest.fn().mockReturnValue({ - set: jest.fn().mockReturnValue({ - where: jest.fn().mockReturnValue({ - returning: jest - .fn() - .mockRejectedValue( - new Error('duplicate key value violates unique constraint'), - ), - }), - }), - }); + const result = await controller.updateUser('u1', { name: 'New' }); - await expect( - controller.updateUser('u1', { email: 'race@example.com' }), - ).rejects.toThrow(BadRequestException); + expect(result).toEqual({ id: 'u1', name: 'New' }); + expect(mockUserProvider.update).toHaveBeenCalledWith('u1', { + name: 'New', + }); }); }); describe('getUser', () => { - it('should return a single user', async () => { - const mockUser = { id: 'u1', name: 'User 1' }; - mockDb.query.user.findFirst.mockResolvedValue(mockUser); + it('should return user by ID', async () => { + const mockUser = { id: 'u1' }; + mockUserProvider.findById.mockResolvedValue(mockUser); const result = await controller.getUser('u1'); expect(result).toEqual(mockUser); - - expect(mockDb.query.user.findFirst).toHaveBeenCalledWith( - expect.objectContaining({ where: expect.anything() }), - ); }); it('should throw NotFoundException if user not found', async () => { - mockDb.query.user.findFirst.mockResolvedValue(null); + mockUserProvider.findById.mockResolvedValue(null); await expect(controller.getUser('missing')).rejects.toThrow( NotFoundException, @@ -586,127 +309,90 @@ describe('SystemAdminController', () => { describe('deleteUser', () => { it('should throw NotFoundException if user not found', async () => { - mockDb.query.user.findFirst.mockResolvedValue(null); + mockUserProvider.findById.mockResolvedValue(null); await expect(controller.deleteUser('missing')).rejects.toThrow( NotFoundException, ); }); - it('should delete user and dependencies transactionally', async () => { - mockDb.query.user.findFirst.mockResolvedValue({ - id: 'u1', - systemRole: 'platform_user', - }); // Normal user - - const mockWhere = jest.fn().mockResolvedValue({}); - - mockDb.delete = jest.fn().mockReturnValue({ - where: mockWhere, - }); - - await controller.deleteUser('u1'); - - expect(mockDb.transaction).toHaveBeenCalled(); - - // Verify deletion order inside transaction - // 1. Memberships - expect(mockDb.delete).toHaveBeenNthCalledWith(1, schema.member); - // 2. Invitations (Inviter) - expect(mockDb.delete).toHaveBeenNthCalledWith(2, schema.invitation); - // 3. Invitations (Recipient) - expect(mockDb.delete).toHaveBeenNthCalledWith(3, schema.invitation); - // 4. Sessions - expect(mockDb.delete).toHaveBeenNthCalledWith(4, schema.session); - // 5. Accounts - expect(mockDb.delete).toHaveBeenNthCalledWith(5, schema.account); - // 6. User - expect(mockDb.delete).toHaveBeenNthCalledWith(6, schema.user); - - expect(mockWhere).toHaveBeenCalledTimes(6); - }); - it('should prevent deleting the last platform admin', async () => { - mockDb.query.user.findFirst.mockResolvedValue({ + mockUserProvider.findById.mockResolvedValue({ id: 'admin1', systemRole: 'platform_admin', }); - - // Mock db.select().from().where() - const mockBuilder = { - where: jest.fn().mockResolvedValue([{ count: 0 }]), - }; - // mockDb.from returns the builder (because db.select() returns 'this', and 'this.from' is called) - mockDb.from.mockReturnValueOnce(mockBuilder); + // count returns 1 (only this user left) + mockUserProvider.count.mockResolvedValue(1); await expect(controller.deleteUser('admin1')).rejects.toThrow( BadRequestException, ); + expect(mockUserProvider.count).toHaveBeenCalledWith({ + systemRole: 'platform_admin', + }); }); it('should allow deleting platform admin if others exist', async () => { - mockDb.query.user.findFirst.mockResolvedValue({ + mockUserProvider.findById.mockResolvedValue({ id: 'admin1', systemRole: 'platform_admin', }); + // count returns 2 + mockUserProvider.count.mockResolvedValue(2); - // Mock db.select().from().where() - const mockBuilder = { - where: jest.fn().mockResolvedValue([{ count: 1 }]), - }; - mockDb.from.mockReturnValueOnce(mockBuilder); + await controller.deleteUser('admin1'); - const mockWhere = jest.fn().mockResolvedValue({}); - mockDb.delete = jest.fn().mockReturnValue({ where: mockWhere }); + expect(mockUserProvider.delete).toHaveBeenCalledWith('admin1'); + }); - await controller.deleteUser('admin1'); + it('should delete normal user', async () => { + mockUserProvider.findById.mockResolvedValue({ + id: 'u1', + systemRole: 'platform_user', + }); - expect(mockDb.transaction).toHaveBeenCalled(); + await controller.deleteUser('u1'); + + expect(mockUserProvider.delete).toHaveBeenCalledWith('u1'); + // Should not check admin count for normal user + expect(mockUserProvider.count).not.toHaveBeenCalled(); }); }); describe('inviteUser', () => { - const mockHeaders = {}; - it('should throw NotFoundException if user not found', async () => { - mockDb.query.user.findFirst.mockResolvedValue(null); + mockUserProvider.findById.mockResolvedValue(null); await expect( - controller.inviteUser('missing', mockHeaders as Record), + controller.inviteUser('missing', mockHeaders), ).rejects.toThrow(NotFoundException); }); - it('should throw BadRequestException if unauthorized (no session)', async () => { - mockDb.query.user.findFirst.mockResolvedValue({ id: 'u1' }); - mockIdentityProvider.getSessionFromHeaders.mockResolvedValue(null); + it('should throw BadRequestException if unauthorized', async () => { + mockUserProvider.findById.mockResolvedValue({ id: 'u1' }); + mockAuthProvider.getSessionFromHeaders.mockResolvedValue(null); - await expect( - controller.inviteUser('u1', mockHeaders as Record), - ).rejects.toThrow(BadRequestException); + await expect(controller.inviteUser('u1', mockHeaders)).rejects.toThrow( + BadRequestException, + ); }); - it('should create system invitation successfully', async () => { - const mockUser = { + it('should create system invite', async () => { + mockUserProvider.findById.mockResolvedValue({ id: 'u1', email: 'test@example.com', systemRole: 'platform_admin', - }; - const mockSession = { user: { id: 'admin1' } }; - - mockDb.query.user.findFirst.mockResolvedValue(mockUser); - mockIdentityProvider.getSessionFromHeaders.mockResolvedValue(mockSession); - mockIdentityProvider.createInvitation.mockResolvedValue({ id: 'inv1' }); + }); + mockAuthProvider.getSessionFromHeaders.mockResolvedValue({ + user: { id: 'admin1' }, + }); - const result = await controller.inviteUser( - 'u1', - mockHeaders as Record, - ); + await controller.inviteUser('u1', mockHeaders); - expect(result).toEqual({ success: true }); - expect(mockIdentityProvider.createInvitation).toHaveBeenCalledWith({ - email: mockUser.email, - role: mockUser.systemRole, - organizationId: null, + expect(mockAuthProvider.createInvitation).toHaveBeenCalledWith({ + email: 'test@example.com', + role: 'platform_admin', + organizationId: null, // System invite inviterId: 'admin1', }); }); diff --git a/apps/api/src/modules/identity/system-admin/system-admin.controller.ts b/apps/api/src/modules/identity/system-admin/system-admin.controller.ts index cf7a53a3..0b355772 100644 --- a/apps/api/src/modules/identity/system-admin/system-admin.controller.ts +++ b/apps/api/src/modules/identity/system-admin/system-admin.controller.ts @@ -13,13 +13,16 @@ import { NotFoundException, Headers as RequestHeaders, } from '@nestjs/common'; -import { AUTH_PROVIDER, IAuthProvider } from '@nexiom/identity'; +import { + AUTH_PROVIDER, + IAuthProvider, + USER_PROVIDER, + IUserProvider, + TENANT_PROVIDER, + ITenantProvider, +} from '@nexiom/identity'; import { SystemAdminGuard } from '../auth/system-admin.guard'; import { PlatformGuard } from '../auth/platform.guard'; -import { DRIZZLE_DB } from '../../../db/db.provider'; -import { NodePgDatabase } from 'drizzle-orm/node-postgres'; -import * as schema from '../../../db/schema'; -import { desc, count, eq, ne, and } from 'drizzle-orm'; import { CreateTenantValidation, UpdateTenantValidation, @@ -27,13 +30,13 @@ import { CreateUserValidation, CreateSystemInvitationValidation, } from './system-admin.validation'; -import { v4 as uuidv4 } from 'uuid'; @Controller('admin') export class SystemAdminController { constructor( - @Inject(DRIZZLE_DB) private readonly db: NodePgDatabase, @Inject(AUTH_PROVIDER) private readonly authProvider: IAuthProvider, + @Inject(USER_PROVIDER) private readonly userProvider: IUserProvider, + @Inject(TENANT_PROVIDER) private readonly tenantProvider: ITenantProvider, ) {} @Post('users/:id/invite') @@ -42,9 +45,7 @@ export class SystemAdminController { @Param('id') id: string, @RequestHeaders() headers: Record, ) { - const user = await this.db.query.user.findFirst({ - where: eq(schema.user.id, id), - }); + const user = await this.userProvider.findById(id); if (!user) { throw new NotFoundException('User not found'); @@ -64,7 +65,6 @@ export class SystemAdminController { email: user.email, role: user.systemRole || 'platform_user', organizationId: null, // System Invite - inviterId: session.user.id, }); @@ -90,7 +90,6 @@ export class SystemAdminController { email: body.email, role: body.role, // Zod handles default organizationId: null, // System invitation - inviterId: session.user.id, }); @@ -111,25 +110,17 @@ export class SystemAdminController { @UseGuards(SystemAdminGuard) async createTenant(@Body() input: CreateTenantValidation) { // Check if slug exists - const existing = await this.db.query.organization.findFirst({ - where: eq(schema.organization.slug, input.slug), - }); + const existing = await this.tenantProvider.findBySlug(input.slug); if (existing) { throw new BadRequestException('Slug is already taken by another tenant'); } - const [tenant] = await this.db - .insert(schema.organization) - .values({ - id: uuidv4(), - name: input.name, - slug: input.slug, - logo: input.logo, - createdAt: new Date(), - status: 'active', - }) - .returning(); + const tenant = await this.tenantProvider.createTenant({ + name: input.name, + slug: input.slug, + logo: input.logo, + }); return tenant; } @@ -138,26 +129,32 @@ export class SystemAdminController { @UseGuards(SystemAdminGuard) async createUser(@Body() input: CreateUserValidation) { // Check if email already exists - const existing = await this.db.query.user.findFirst({ - where: eq(schema.user.email, input.email), - }); + const existing = await this.userProvider.findByEmail(input.email); if (existing) { throw new BadRequestException('User with this email already exists'); } - const [user] = await this.db - .insert(schema.user) - .values({ - id: uuidv4(), - name: input.name, - email: input.email, - systemRole: input.systemRole || 'platform_user', - emailVerified: false, - createdAt: new Date(), - updatedAt: new Date(), - }) - .returning(); + // Adapt input to provider requirement (provider handles ID generation and timestamps) + const user = await this.userProvider.create({ + ...input, + // Default systemRole handled by provider or we pass explicitly? + // create signature: (input: CreateUserInput) -> email, password?, firstName?, lastName?, role? + // Does CreateUserInput support systemRole? + // Let's check CreateUserInput interface. + // It supports role, but not systemRole explicitly in interface file I saw? + // Wait, let's verify CreateUserInput. + }); + // Ah, DrizzleUserAdapter delegates to authProvider which delegates to BetterAuth. + // BetterAuth input usually has role. + // If 'systemRole' is not in CreateUserInput, we might need to update user AFTER create + // OR update CreateUserInput. + // For now, assume provider handles it or update immediately. + // Let's assume we update immediately if provider doesn't support generic fields in create. + if (input.systemRole) { + await this.userProvider.update(user.id, { systemRole: input.systemRole }); + return this.userProvider.findById(user.id); + } return user; } @@ -168,9 +165,7 @@ export class SystemAdminController { @Param('id') id: string, @Body() input: UpdateTenantValidation, ) { - const tenant = await this.db.query.organization.findFirst({ - where: eq(schema.organization.id, id), - }); + const tenant = await this.tenantProvider.findById(id); if (!tenant) { throw new NotFoundException('Tenant not found'); @@ -178,64 +173,33 @@ export class SystemAdminController { // Check slug uniqueness if changing if (input.slug && input.slug !== tenant.slug) { - const existing = await this.db.query.organization.findFirst({ - where: and( - eq(schema.organization.slug, input.slug), - ne(schema.organization.id, id), - ), - }); + const existing = await this.tenantProvider.findBySlug(input.slug); - if (existing) { + if (existing && existing.id !== id) { throw new BadRequestException( 'Slug is already taken by another tenant', ); } } - const updatePayload: Partial = {}; - if (input.name !== undefined) updatePayload.name = input.name; - if (input.slug !== undefined) updatePayload.slug = input.slug; - if (input.logo !== undefined) updatePayload.logo = input.logo; - if (input.status !== undefined) updatePayload.status = input.status; - if (input.metadata !== undefined) - updatePayload.metadata = JSON.stringify(input.metadata); - - if (Object.keys(updatePayload).length === 0) { + if (Object.keys(input).length === 0) { throw new BadRequestException('No fields to update'); } - const [updated] = await this.db - .update(schema.organization) - .set(updatePayload) - .where(eq(schema.organization.id, id)) - .returning(); - + const updated = await this.tenantProvider.update(id, input); return updated; } @Delete('tenants/:id') @UseGuards(SystemAdminGuard) async deleteTenant(@Param('id') id: string) { - const tenant = await this.db.query.organization.findFirst({ - where: eq(schema.organization.id, id), - }); + const tenant = await this.tenantProvider.findById(id); if (!tenant) { throw new NotFoundException('Tenant not found'); } - await this.db.transaction(async (tx) => { - // Hard delete dependent records and organization - await tx - .delete(schema.member) - .where(eq(schema.member.organizationId, id)); - await tx - .delete(schema.invitation) - .where(eq(schema.invitation.organizationId, id)); - await tx - .delete(schema.organization) - .where(eq(schema.organization.id, id)); - }); + await this.tenantProvider.delete(id); return { success: true }; } @@ -253,25 +217,13 @@ export class SystemAdminController { 1, Math.min(MAX_PAGE_SIZE, parseInt(pageSize) || 10), ); - const offset = (p - 1) * limit; - const users = await this.db.query.user.findMany({ + const result = await this.userProvider.findAll({ + page: p, limit, - offset, - orderBy: (users, { desc }) => [desc(users.createdAt)], - }); + }); // No tenantId -> Global list - // Total count for pagination - const totalResult = await this.db - .select({ count: count() }) - .from(schema.user); - const total = Number(totalResult[0]?.count || 0); - - return { - // Envelope - data: users, - total, - }; + return result; // Envelope { data, total } matches } @Patch('users/:id') @@ -280,9 +232,7 @@ export class SystemAdminController { @Param('id') id: string, @Body() input: UpdateUserValidation, ) { - const user = await this.db.query.user.findFirst({ - where: eq(schema.user.id, id), - }); + const user = await this.userProvider.findById(id); if (!user) { throw new NotFoundException('User not found'); @@ -290,54 +240,25 @@ export class SystemAdminController { // Check email uniqueness if changing if (input.email && input.email !== user.email) { - const existing = await this.db.query.user.findFirst({ - where: and(eq(schema.user.email, input.email), ne(schema.user.id, id)), - }); + const existing = await this.userProvider.findByEmail(input.email); - if (existing) { + if (existing && existing.id !== id) { throw new BadRequestException('Email already in use'); } } - const updatePayload: Partial = {}; - if (input.name !== undefined) updatePayload.name = input.name; - if (input.systemRole !== undefined) - updatePayload.systemRole = input.systemRole; - if (input.email !== undefined) updatePayload.email = input.email; - if (input.emailVerified !== undefined) - updatePayload.emailVerified = input.emailVerified; - - if (Object.keys(updatePayload).length === 0) { + if (Object.keys(input).length === 0) { throw new BadRequestException('No fields to update'); } - try { - const [updated] = await this.db - .update(schema.user) - .set(updatePayload) - .where(eq(schema.user.id, id)) - .returning(); - - return updated; - } catch (error) { - // Catch race conditions for unique constraints - if ( - error instanceof Error && - (error.message.includes('unique') || - error.message.includes('duplicate')) - ) { - throw new BadRequestException('Email already in use'); - } - throw error; - } + const updated = await this.userProvider.update(id, input); + return updated; } @Get('users/:id') @UseGuards(PlatformGuard) async getUser(@Param('id') id: string) { - const user = await this.db.query.user.findFirst({ - where: eq(schema.user.id, id), - }); + const user = await this.userProvider.findById(id); if (!user) { throw new NotFoundException('User not found'); @@ -349,9 +270,7 @@ export class SystemAdminController { @Delete('users/:id') @UseGuards(SystemAdminGuard) async deleteUser(@Param('id') id: string) { - const user = await this.db.query.user.findFirst({ - where: eq(schema.user.id, id), - }); + const user = await this.userProvider.findById(id); if (!user) { throw new NotFoundException('User not found'); @@ -359,48 +278,23 @@ export class SystemAdminController { // Safety: Prevent deleting the last platform admin if (user.systemRole === 'platform_admin') { - const adminCountResult = await this.db - .select({ count: count() }) - .from(schema.user) - .where( - and( - eq(schema.user.systemRole, 'platform_admin'), - ne(schema.user.id, id), - ), - ); + const adminCount = await this.userProvider.count({ + systemRole: 'platform_admin', + }); - const otherAdmins = Number(adminCountResult[0]?.count || 0); - if (otherAdmins === 0) { + // If this user is an admin, and count is 1, they are the last one. + // However, count() includes this user. + // Original logic: count where role=admin AND id != target. + // Provider logic: count where role=admin. + // So if count <= 1, we prevent delete. + if (adminCount <= 1) { throw new BadRequestException( 'Cannot delete the last Platform Administrator', ); } } - // Transactional cleanup - await this.db.transaction(async (tx) => { - // 1. Delete memberships - await tx.delete(schema.member).where(eq(schema.member.userId, id)); - - // 2. Delete invitations - // - Created by this user (Inviter) - await tx - .delete(schema.invitation) - .where(eq(schema.invitation.inviterId, id)); - - // - Sent TO this user's email (Recipient) - // Note: We use user.email here, which we fetched above. - await tx - .delete(schema.invitation) - .where(eq(schema.invitation.email, user.email)); - - // 3. Delete session/account/etc (BetterAuth handles this typically if cascading, but we do manual for safety) - await tx.delete(schema.session).where(eq(schema.session.userId, id)); - await tx.delete(schema.account).where(eq(schema.account.userId, id)); - - // 4. Delete user - await tx.delete(schema.user).where(eq(schema.user.id, id)); - }); + await this.userProvider.delete(id); return { success: true }; } @@ -417,64 +311,19 @@ export class SystemAdminController { 1, Math.min(MAX_PAGE_SIZE, parseInt(pageSize) || 10), ); - const offset = (p - 1) * limit; - - const tenantsData = await this.db - .select({ - id: schema.organization.id, - name: schema.organization.name, - slug: schema.organization.slug, - createdAt: schema.organization.createdAt, - logo: schema.organization.logo, - metadata: schema.organization.metadata, - updatedAt: schema.organization.updatedAt, - status: schema.organization.status, // Included status - userCount: count(schema.member.id), - }) - .from(schema.organization) - .leftJoin( - schema.member, - eq(schema.organization.id, schema.member.organizationId), - ) - .groupBy(schema.organization.id) - .limit(limit) - .offset(offset) - .orderBy(desc(schema.organization.createdAt)) - .execute(); - - // Total count of tenants - const totalResult = await this.db - .select({ count: count() }) - .from(schema.organization); - const total = Number(totalResult[0]?.count || 0); - - return { - data: tenantsData.map((t) => ({ - ...t, - userCount: Number(t.userCount), - })), - total, - }; + + const result = await this.tenantProvider.findAll({ + page: p, + limit, + }); + + return result; // Envelope { data, total } matches } @Get('tenants/:id') @UseGuards(PlatformGuard) async getTenant(@Param('id') id: string) { - const [tenant] = await this.db - .select({ - id: schema.organization.id, - name: schema.organization.name, - slug: schema.organization.slug, - createdAt: schema.organization.createdAt, - logo: schema.organization.logo, - metadata: schema.organization.metadata, - updatedAt: schema.organization.updatedAt, - status: schema.organization.status, - }) - .from(schema.organization) - .where(eq(schema.organization.id, id)) - .limit(1) - .execute(); + const tenant = await this.tenantProvider.findById(id); if (!tenant) { throw new NotFoundException('Tenant not found'); diff --git a/apps/api/src/modules/identity/users/users.controller.spec.ts b/apps/api/src/modules/identity/users/users.controller.spec.ts index 3da87bdd..17f9f11b 100644 --- a/apps/api/src/modules/identity/users/users.controller.spec.ts +++ b/apps/api/src/modules/identity/users/users.controller.spec.ts @@ -94,12 +94,12 @@ describe('UsersController', () => { } as unknown as Request & { user: { organizationId?: string } }; const users = [{ id: '1' }]; - mockUserProvider.findAll.mockResolvedValue(users); + mockUserProvider.findAll.mockResolvedValue({ data: users, total: 1 }); const result = await controller.findAll(req); expect(result).toEqual(users); - expect(mockUserProvider.findAll).toHaveBeenCalledWith(tenantId); + expect(mockUserProvider.findAll).toHaveBeenCalledWith({ tenantId }); }); }); diff --git a/apps/api/src/modules/identity/users/users.controller.ts b/apps/api/src/modules/identity/users/users.controller.ts index f505dcc2..8b0a37f7 100644 --- a/apps/api/src/modules/identity/users/users.controller.ts +++ b/apps/api/src/modules/identity/users/users.controller.ts @@ -43,7 +43,7 @@ export class UsersController { * @returns List of users. */ @Get() - findAll(@Req() req: Request & { user: { organizationId?: string } }) { + async findAll(@Req() req: Request & { user: { organizationId?: string } }) { // AuthGuard guarantees session is valid and populates user info // We use 'organizationId' (mapped in getSessionWithOrg) const tenantId = req.user?.organizationId; @@ -55,7 +55,8 @@ export class UsersController { return []; } - return this.userProvider.findAll(tenantId); + const result = await this.userProvider.findAll({ tenantId }); + return result.data; } /** diff --git a/docs/old/nexiom_tech_stack_and_repo_plan.md b/docs/old/nexiom_tech_stack_and_repo_plan.md index 4538408b..f052922a 100644 --- a/docs/old/nexiom_tech_stack_and_repo_plan.md +++ b/docs/old/nexiom_tech_stack_and_repo_plan.md @@ -38,6 +38,7 @@ The repository follows a standard **Turborepo** layout. We strictly separate **S │ │ │ │ ├── auth.controller.ts # Login / Session Endpoints │ │ │ │ ├── user.controller.ts # Profile / Invites Endpoints │ │ │ │ └── tenant.controller.ts # Org / Schema Endpoints +│ │ │ ├── /notification # Notification Controller │ │ │ ├── /billing # Billing Controller │ │ │ └── /engine # ⚙️ Integration API │ │ │ ├── /webhooks # Ingestion Endpoint diff --git a/docs/refactor/modules/identity/task/01c_strict_layer_enforcement.md b/docs/refactor/modules/identity/task/01c_strict_layer_enforcement.md new file mode 100644 index 00000000..0edb24de --- /dev/null +++ b/docs/refactor/modules/identity/task/01c_strict_layer_enforcement.md @@ -0,0 +1,58 @@ +# Task 01c: Strict Layer Enforcement + +**Priority:** CRITICAL +**Estimated Time:** 4 hours +**Assignee:** Coder +**Status:** Ready to Start + +--- + +## Objective + +Fix the architectural violations in the Identity module. Specifically, `SystemAdminController` is bypassing the adapter layer and accessing the database directly, leading to brittle tests and vendor lock-in. + +**Goal:** Ensure all Controllers interact ONLY with Interfaces (`IUserProvider`, `ITenantProvider`), never raw DB connections. + +--- + +## Directives + +> [!IMPORTANT] +> **Strict Rule:** No Controller in `apps/api` shall import `DRIZZLE_DB` or `NodePgDatabase`. + +## Implementation Steps + +### 1. Enhance Interfaces + +- [ ] **Modify `IUserProvider`**: Add methods to support Admin Console requirements. + - `findAll(options?: { page: number; limit: number; search?: string }): Promise<{ data: User[]; total: number }>` + - `count(): Promise` +- [ ] **Modify `ITenantProvider`**: Add methods for Admin listing. + - `findAll(options?: { page: number; limit: number }): Promise<{ data: Tenant[]; total: number }>` + +### 2. Implement Adapters + +- [ ] **Update `DrizzleUserAdapter`**: Implement the new `findAll` and `count` methods using Drizzle. +- [ ] **Update `DrizzleTenantAdapter`**: Implement the new listing logic. + +### 3. Refactor Controller + +- [ ] **Target:** `apps/api/src/modules/identity/system-admin/system-admin.controller.ts` +- [ ] **Action:** Remove `@Inject(DRIZZLE_DB) private readonly db`. +- [ ] **Action:** Inject `@Inject(USER_PROVIDER) private readonly userProvider`. +- [ ] **Action:** Inject `@Inject(TENANT_PROVIDER) private readonly tenantProvider`. +- [ ] **Action:** Replace all `this.db.query...` calls with `this.userProvider...` or `this.tenantProvider...` calls. + +### 4. Rewrite Tests + +- [ ] **Target:** `apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts` +- [ ] **Action:** Remove all `MockDb` and Drizzle-chain mocking. +- [ ] **Action:** Mock the `IUserProvider` and `ITenantProvider` interfaces. +- [ ] **Verification:** Tests should look like: `expect(userProvider.findAll).toHaveBeenCalledWith(...)`. + +--- + +## Verification Plan + +- [ ] **Search:** Grep for `DRIZZLE_DB` in `apps/api/src/modules/identity/`. Result should be empty (except for Module definition). +- [ ] **Test:** Run `pnpm test apps/api`. Tests must pass without mocking Drizzle internals. diff --git a/packages/identity/src/adapters/drizzle-tenant.adapter.ts b/packages/identity/src/adapters/drizzle-tenant.adapter.ts index babce727..752ec574 100644 --- a/packages/identity/src/adapters/drizzle-tenant.adapter.ts +++ b/packages/identity/src/adapters/drizzle-tenant.adapter.ts @@ -1,7 +1,11 @@ import { NodePgDatabase } from "drizzle-orm/node-postgres"; -import { eq } from "drizzle-orm"; +import { eq, count, ilike, desc, and } from "drizzle-orm"; import { v4 as uuidv4 } from "uuid"; -import { ITenantProvider, Tenant as TenantInterface } from "../interfaces"; +import { + ITenantProvider, + Tenant as TenantInterface, + UpdateTenantInput, +} from "../interfaces"; import * as schema from "../schema"; export class DrizzleTenantAdapter implements ITenantProvider { @@ -52,6 +56,58 @@ export class DrizzleTenantAdapter implements ITenantProvider { throw new Error("Failed to generate unique slug for tenant"); } + async createTenant(input: { + name: string; + slug: string; + logo?: string | null; + }): Promise { + const [org] = await this.db + .insert(schema.organization) + .values({ + id: uuidv4(), + name: input.name, + slug: input.slug, + logo: input.logo, + createdAt: new Date(), + status: "active", + }) + .returning(); + return this.mapTenant(org); + } + + async update(id: string, input: UpdateTenantInput): Promise { + const updatePayload: Partial = {}; + if (input.name !== undefined) updatePayload.name = input.name; + if (input.slug !== undefined) updatePayload.slug = input.slug; + if (input.logo !== undefined) updatePayload.logo = input.logo; + if (input.status !== undefined) updatePayload.status = input.status; + if (input.metadata !== undefined) + updatePayload.metadata = JSON.stringify(input.metadata); + + const [updated] = await this.db + .update(schema.organization) + .set({ ...updatePayload, updatedAt: new Date() }) + .where(eq(schema.organization.id, id)) + .returning(); + + if (!updated) throw new Error("Tenant not found"); + return this.mapTenant(updated); + } + + async delete(id: string): Promise { + await this.db.transaction(async (tx) => { + await tx + .delete(schema.member) + .where(eq(schema.member.organizationId, id)); + await tx + .delete(schema.invitation) + .where(eq(schema.invitation.organizationId, id)); + await tx + .delete(schema.organization) + .where(eq(schema.organization.id, id)); + }); + } + // ... private generateSlug(name: string): string { @@ -86,6 +142,39 @@ export class DrizzleTenantAdapter implements ITenantProvider { })); } + async findAll(options?: { + page?: number; + limit?: number; + search?: string; + }): Promise<{ data: TenantInterface[]; total: number }> { + const page = options?.page || 1; + const limit = options?.limit || 10; + const offset = (page - 1) * limit; + + const filters = []; + if (options?.search) { + filters.push(ilike(schema.organization.name, `%${options.search}%`)); + } + + const data = await this.db + .select() + .from(schema.organization) + .where(and(...filters)) + .limit(limit) + .offset(offset) + .orderBy(desc(schema.organization.createdAt)); + + const [countResult] = await this.db + .select({ count: count(schema.organization.id) }) + .from(schema.organization) + .where(and(...filters)); + + return { + data: data.map((d) => this.mapTenant(d)), + total: Number(countResult?.count || 0), + }; + } + async findById(id: string): Promise { const org = await this.db.query.organization.findFirst({ where: eq(schema.organization.id, id), @@ -93,6 +182,13 @@ export class DrizzleTenantAdapter implements ITenantProvider { return org ? this.mapTenant(org) : null; } + async findBySlug(slug: string): Promise { + const org = await this.db.query.organization.findFirst({ + where: eq(schema.organization.slug, slug), + }); + return org ? this.mapTenant(org) : null; + } + async updateStatus( id: string, status: TenantInterface["status"], diff --git a/packages/identity/src/adapters/drizzle-user.adapter.ts b/packages/identity/src/adapters/drizzle-user.adapter.ts index 9a9bebef..30b80caa 100644 --- a/packages/identity/src/adapters/drizzle-user.adapter.ts +++ b/packages/identity/src/adapters/drizzle-user.adapter.ts @@ -1,5 +1,5 @@ import { NodePgDatabase } from "drizzle-orm/node-postgres"; -import { eq } from "drizzle-orm"; +import { eq, and, ilike, count, desc } from "drizzle-orm"; import { IUserProvider, CreateUserInput, @@ -13,7 +13,7 @@ export class DrizzleUserAdapter implements IUserProvider { constructor( private readonly db: NodePgDatabase, private readonly authProvider: IAuthProvider, - ) {} + ) { } async create(input: CreateUserInput): Promise { // Delegate to AuthProvider to handle account creation (and password hashing) @@ -84,26 +84,113 @@ export class DrizzleUserAdapter implements IUserProvider { return user ? this.mapUser(user) : null; } - async findAll(tenantId?: string): Promise { - if (tenantId) { - const users = await this.db - .select({ - user: schema.user, - }) + async findAll(options?: { + page?: number; + limit?: number; + search?: string; + tenantId?: string; + }): Promise<{ data: UserInterface[]; total: number }> { + const page = options?.page || 1; + const limit = options?.limit || 10; + const offset = (page - 1) * limit; + + const filters = []; + if (options?.tenantId) { + filters.push(eq(schema.member.organizationId, options.tenantId)); + } + if (options?.search) { + filters.push( + ilike(schema.user.email, `%${options.search}%`), + // OR name search if needed, but keeping simple for now + ); + } + + // Base query logic + // If tenantId is present, we must join with member + let dataQuery; + + if (options?.tenantId) { + // Tenant-scoped + dataQuery = this.db + .select({ user: schema.user }) + .from(schema.user) + .innerJoin(schema.member, eq(schema.member.userId, schema.user.id)) + .where(and(...filters)) + .limit(limit) + .offset(offset) + .orderBy(desc(schema.user.createdAt)); + + // Optimized count for tenant scope + const [countResult] = await this.db + .select({ count: count(schema.user.id) }) .from(schema.user) .innerJoin(schema.member, eq(schema.member.userId, schema.user.id)) - .where(eq(schema.member.organizationId, tenantId)); + .where(and(...filters)); + + const users = await dataQuery; + return { + data: users.map((u) => this.mapUser(u.user)), + total: Number(countResult?.count || 0), + }; + } else { + // Global list (Admin) + const globalFilters = []; + if (options?.search) { + globalFilters.push(ilike(schema.user.email, `%${options.search}%`)); + } + + dataQuery = this.db + .select() + .from(schema.user) + .where(and(...globalFilters)) + .limit(limit) + .offset(offset) + .orderBy(desc(schema.user.createdAt)); + + const [countResult] = await this.db + .select({ count: count(schema.user.id) }) + .from(schema.user) + .where(and(...globalFilters)); - return users.map((u) => this.mapUser(u.user)); + const users = await dataQuery; + return { + data: users.map((u) => this.mapUser(u)), + total: Number(countResult?.count || 0), + }; + } + } + + async count(filters?: { + tenantId?: string; + search?: string; + systemRole?: string; + }): Promise { + const whereConditions = []; + + if (filters?.search) { + whereConditions.push(ilike(schema.user.email, `%${filters.search}%`)); + } + + if (filters?.systemRole) { + whereConditions.push(eq(schema.user.systemRole, filters.systemRole)); } - // Global list (careful with this in prod) - const users = await this.db - .select() + if (filters?.tenantId) { + whereConditions.push(eq(schema.member.organizationId, filters.tenantId)); + const [result] = await this.db + .select({ count: count(schema.user.id) }) + .from(schema.user) + .innerJoin(schema.member, eq(schema.member.userId, schema.user.id)) + .where(and(...whereConditions)); + return Number(result?.count || 0); + } + + const [result] = await this.db + .select({ count: count(schema.user.id) }) .from(schema.user) - .limit(100) - .orderBy(schema.user.createdAt); - return users.map((u) => this.mapUser(u)); + .where(and(...whereConditions)); + + return Number(result?.count || 0); } async forceVerifyEmail(userId: string): Promise { diff --git a/packages/identity/src/interfaces/tenant-provider.interface.ts b/packages/identity/src/interfaces/tenant-provider.interface.ts index 7e498d97..cccbefef 100644 --- a/packages/identity/src/interfaces/tenant-provider.interface.ts +++ b/packages/identity/src/interfaces/tenant-provider.interface.ts @@ -1,11 +1,38 @@ import { Tenant } from "./types"; +export interface UpdateTenantInput { + name?: string; + slug?: string; + logo?: string | null; + status?: Tenant["status"]; + metadata?: Record; +} + export interface ITenantProvider { + // User-scoped creation (auto-adds member) create(userId: string, name: string): Promise; + // Admin creation (pure tenant) + createTenant(input: { + name: string; + slug: string; + logo?: string | null; + }): Promise; + + update(id: string, input: UpdateTenantInput): Promise; + + delete(id: string): Promise; + findAllForUser(userId: string): Promise<(Tenant & { memberRole?: string })[]>; + findAll(options?: { + page?: number; + limit?: number; + search?: string; + }): Promise<{ data: Tenant[]; total: number }>; + findById(id: string): Promise; + findBySlug(slug: string): Promise; updateStatus(id: string, status: Tenant["status"]): Promise; diff --git a/packages/identity/src/interfaces/user-provider.interface.ts b/packages/identity/src/interfaces/user-provider.interface.ts index 4ad68f45..82524c0a 100644 --- a/packages/identity/src/interfaces/user-provider.interface.ts +++ b/packages/identity/src/interfaces/user-provider.interface.ts @@ -24,7 +24,21 @@ export interface IUserProvider { findByEmail(email: string): Promise; - findAll(tenantId?: string): Promise; + findAll(options?: { + page?: number; + limit?: number; + search?: string; + tenantId?: string; + }): Promise<{ data: User[]; total: number }>; + + // Kept for backward compatibility if needed, but the above covers it + // findAll(tenantId?: string): Promise; // Removed in favor of options forceVerifyEmail(userId: string): Promise; + + count(filters?: { + tenantId?: string; + search?: string; + systemRole?: string; + }): Promise; } From 62090c56c253315dc1734c499b36f60fecba8fac Mon Sep 17 00:00:00 2001 From: Pramod Date: Sat, 24 Jan 2026 16:32:25 +0530 Subject: [PATCH 02/13] style: format drizzle-user.adapter.ts --- packages/identity/src/adapters/drizzle-user.adapter.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/identity/src/adapters/drizzle-user.adapter.ts b/packages/identity/src/adapters/drizzle-user.adapter.ts index 30b80caa..8f47c8bf 100644 --- a/packages/identity/src/adapters/drizzle-user.adapter.ts +++ b/packages/identity/src/adapters/drizzle-user.adapter.ts @@ -13,7 +13,7 @@ export class DrizzleUserAdapter implements IUserProvider { constructor( private readonly db: NodePgDatabase, private readonly authProvider: IAuthProvider, - ) { } + ) {} async create(input: CreateUserInput): Promise { // Delegate to AuthProvider to handle account creation (and password hashing) From 737b437a8c33a1e0cf019a34c14ab0106797dacc Mon Sep 17 00:00:00 2001 From: Pramod Date: Sat, 24 Jan 2026 16:53:37 +0530 Subject: [PATCH 03/13] fix(identity): address CodeRabbit review feedback - SystemAdminController: Add createSystemInvitation tests, clarify deleteUser logic, add TODO - DrizzleTenantAdapter: Handle slug unique errors, check existence on delete - DrizzleUserAdapter: Consolidate findAll filters - UsersController: Return full pagination metadata --- .../system-admin.controller.spec.ts | 36 +++++++++++++++ .../system-admin/system-admin.controller.ts | 35 +++++---------- .../identity/users/users.controller.spec.ts | 2 +- .../identity/users/users.controller.ts | 2 +- .../src/adapters/drizzle-tenant.adapter.ts | 45 +++++++++++++------ .../src/adapters/drizzle-user.adapter.ts | 27 ++++------- 6 files changed, 89 insertions(+), 58 deletions(-) diff --git a/apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts b/apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts index 32592cc6..4a1a73ec 100644 --- a/apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts +++ b/apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts @@ -87,6 +87,42 @@ describe('SystemAdminController', () => { }); }); + describe('createSystemInvitation', () => { + it('should throw BadRequestException if unauthorized', async () => { + mockAuthProvider.getSessionFromHeaders.mockResolvedValue(null); + + await expect( + controller.createSystemInvitation( + { email: 'test@example.com', role: 'platform_admin' }, + mockHeaders, + ), + ).rejects.toThrow(BadRequestException); + expect(mockAuthProvider.createInvitation).not.toHaveBeenCalled(); + }); + + it('should create system invitation', async () => { + mockAuthProvider.getSessionFromHeaders.mockResolvedValue({ + user: { id: 'admin1' }, + }); + const mockInvitation = { id: 'inv1', email: 'test@example.com' }; + mockAuthProvider.createInvitation.mockResolvedValue(mockInvitation); + + const result = await controller.createSystemInvitation( + { email: 'test@example.com', role: 'platform_admin' }, + mockHeaders, + ); + + expect(result).toEqual(mockInvitation); + expect(mockAuthProvider.getSessionFromHeaders).toHaveBeenCalled(); + expect(mockAuthProvider.createInvitation).toHaveBeenCalledWith({ + email: 'test@example.com', + role: 'platform_admin', + organizationId: null, + inviterId: 'admin1', + }); + }); + }); + describe('listTenants', () => { it('should return paginated tenants', async () => { const mockResult = { data: [{ id: 't1', name: 'Tenant 1' }], total: 1 }; diff --git a/apps/api/src/modules/identity/system-admin/system-admin.controller.ts b/apps/api/src/modules/identity/system-admin/system-admin.controller.ts index 0b355772..f95edb2c 100644 --- a/apps/api/src/modules/identity/system-admin/system-admin.controller.ts +++ b/apps/api/src/modules/identity/system-admin/system-admin.controller.ts @@ -56,7 +56,7 @@ export class SystemAdminController { // Get current admin ID from session (via Headers -> BetterAuth) const session = await this.authProvider.getSessionFromHeaders(webHeaders); - if (!session || !session.user) { + if (!session?.user) { throw new BadRequestException('Unauthorized'); } @@ -82,7 +82,7 @@ export class SystemAdminController { // Get current admin ID from session const session = await this.authProvider.getSessionFromHeaders(webHeaders); - if (!session || !session.user) { + if (!session?.user) { throw new BadRequestException('Unauthorized'); } @@ -135,22 +135,11 @@ export class SystemAdminController { throw new BadRequestException('User with this email already exists'); } - // Adapt input to provider requirement (provider handles ID generation and timestamps) + // TODO: Add systemRole to CreateUserInput and Adapter so we can do this in one step const user = await this.userProvider.create({ ...input, - // Default systemRole handled by provider or we pass explicitly? - // create signature: (input: CreateUserInput) -> email, password?, firstName?, lastName?, role? - // Does CreateUserInput support systemRole? - // Let's check CreateUserInput interface. - // It supports role, but not systemRole explicitly in interface file I saw? - // Wait, let's verify CreateUserInput. }); - // Ah, DrizzleUserAdapter delegates to authProvider which delegates to BetterAuth. - // BetterAuth input usually has role. - // If 'systemRole' is not in CreateUserInput, we might need to update user AFTER create - // OR update CreateUserInput. - // For now, assume provider handles it or update immediately. - // Let's assume we update immediately if provider doesn't support generic fields in create. + if (input.systemRole) { await this.userProvider.update(user.id, { systemRole: input.systemRole }); return this.userProvider.findById(user.id); @@ -212,10 +201,10 @@ export class SystemAdminController { ) { const MAX_PAGE_SIZE = 100; // Basic pagination (Convert to Number safely) - const p = Math.max(1, parseInt(page) || 1); + const p = Math.max(1, Number.parseInt(page) || 1); const limit = Math.max( 1, - Math.min(MAX_PAGE_SIZE, parseInt(pageSize) || 10), + Math.min(MAX_PAGE_SIZE, Number.parseInt(pageSize) || 10), ); const result = await this.userProvider.findAll({ @@ -282,11 +271,9 @@ export class SystemAdminController { systemRole: 'platform_admin', }); - // If this user is an admin, and count is 1, they are the last one. - // However, count() includes this user. - // Original logic: count where role=admin AND id != target. - // Provider logic: count where role=admin. - // So if count <= 1, we prevent delete. + // The count includes the current target user. + // A count of 1 means this is the LAST admin left. + // Therefore, we must prevent deletion if count <= 1. if (adminCount <= 1) { throw new BadRequestException( 'Cannot delete the last Platform Administrator', @@ -306,10 +293,10 @@ export class SystemAdminController { @Query('pageSize') pageSize = '10', ) { const MAX_PAGE_SIZE = 100; - const p = Math.max(1, parseInt(page) || 1); + const p = Math.max(1, Number.parseInt(page) || 1); const limit = Math.max( 1, - Math.min(MAX_PAGE_SIZE, parseInt(pageSize) || 10), + Math.min(MAX_PAGE_SIZE, Number.parseInt(pageSize) || 10), ); const result = await this.tenantProvider.findAll({ diff --git a/apps/api/src/modules/identity/users/users.controller.spec.ts b/apps/api/src/modules/identity/users/users.controller.spec.ts index 17f9f11b..842158b3 100644 --- a/apps/api/src/modules/identity/users/users.controller.spec.ts +++ b/apps/api/src/modules/identity/users/users.controller.spec.ts @@ -97,7 +97,7 @@ describe('UsersController', () => { mockUserProvider.findAll.mockResolvedValue({ data: users, total: 1 }); const result = await controller.findAll(req); - expect(result).toEqual(users); + expect(result).toEqual({ data: users, total: 1 }); expect(mockUserProvider.findAll).toHaveBeenCalledWith({ tenantId }); }); diff --git a/apps/api/src/modules/identity/users/users.controller.ts b/apps/api/src/modules/identity/users/users.controller.ts index 8b0a37f7..e2ecc157 100644 --- a/apps/api/src/modules/identity/users/users.controller.ts +++ b/apps/api/src/modules/identity/users/users.controller.ts @@ -56,7 +56,7 @@ export class UsersController { } const result = await this.userProvider.findAll({ tenantId }); - return result.data; + return result; } /** diff --git a/packages/identity/src/adapters/drizzle-tenant.adapter.ts b/packages/identity/src/adapters/drizzle-tenant.adapter.ts index 752ec574..13443422 100644 --- a/packages/identity/src/adapters/drizzle-tenant.adapter.ts +++ b/packages/identity/src/adapters/drizzle-tenant.adapter.ts @@ -9,7 +9,7 @@ import { import * as schema from "../schema"; export class DrizzleTenantAdapter implements ITenantProvider { - constructor(private readonly db: NodePgDatabase) {} + constructor(private readonly db: NodePgDatabase) { } async create(userId: string, name: string): Promise { const orgId = uuidv4(); @@ -61,18 +61,26 @@ export class DrizzleTenantAdapter implements ITenantProvider { slug: string; logo?: string | null; }): Promise { - const [org] = await this.db - .insert(schema.organization) - .values({ - id: uuidv4(), - name: input.name, - slug: input.slug, - logo: input.logo, - createdAt: new Date(), - status: "active", - }) - .returning(); - return this.mapTenant(org); + try { + const [org] = await this.db + .insert(schema.organization) + .values({ + id: uuidv4(), + name: input.name, + slug: input.slug, + logo: input.logo, + createdAt: new Date(), + status: "active", + }) + .returning(); + return this.mapTenant(org); + } catch (error: any) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call + if (error.code === "23505" && error.detail?.includes("slug")) { + throw new Error("Tenant slug already exists"); + } + throw error; + } } async update(id: string, input: UpdateTenantInput): Promise { @@ -96,6 +104,17 @@ export class DrizzleTenantAdapter implements ITenantProvider { async delete(id: string): Promise { await this.db.transaction(async (tx) => { + // 1. Check existence first + const [existing] = await tx + .select({ id: schema.organization.id }) + .from(schema.organization) + .where(eq(schema.organization.id, id)); + + if (!existing) { + throw new Error("Tenant not found"); + } + + // 2. Proceed with deletes await tx .delete(schema.member) .where(eq(schema.member.organizationId, id)); diff --git a/packages/identity/src/adapters/drizzle-user.adapter.ts b/packages/identity/src/adapters/drizzle-user.adapter.ts index 8f47c8bf..72416e84 100644 --- a/packages/identity/src/adapters/drizzle-user.adapter.ts +++ b/packages/identity/src/adapters/drizzle-user.adapter.ts @@ -13,7 +13,7 @@ export class DrizzleUserAdapter implements IUserProvider { constructor( private readonly db: NodePgDatabase, private readonly authProvider: IAuthProvider, - ) {} + ) { } async create(input: CreateUserInput): Promise { // Delegate to AuthProvider to handle account creation (and password hashing) @@ -95,9 +95,6 @@ export class DrizzleUserAdapter implements IUserProvider { const offset = (page - 1) * limit; const filters = []; - if (options?.tenantId) { - filters.push(eq(schema.member.organizationId, options.tenantId)); - } if (options?.search) { filters.push( ilike(schema.user.email, `%${options.search}%`), @@ -105,13 +102,11 @@ export class DrizzleUserAdapter implements IUserProvider { ); } - // Base query logic - // If tenantId is present, we must join with member - let dataQuery; - if (options?.tenantId) { - // Tenant-scoped - dataQuery = this.db + // Tenant-scoped (requires Join) + filters.push(eq(schema.member.organizationId, options.tenantId)); + + const dataQuery = this.db .select({ user: schema.user }) .from(schema.user) .innerJoin(schema.member, eq(schema.member.userId, schema.user.id)) @@ -120,7 +115,6 @@ export class DrizzleUserAdapter implements IUserProvider { .offset(offset) .orderBy(desc(schema.user.createdAt)); - // Optimized count for tenant scope const [countResult] = await this.db .select({ count: count(schema.user.id) }) .from(schema.user) @@ -134,15 +128,10 @@ export class DrizzleUserAdapter implements IUserProvider { }; } else { // Global list (Admin) - const globalFilters = []; - if (options?.search) { - globalFilters.push(ilike(schema.user.email, `%${options.search}%`)); - } - - dataQuery = this.db + const dataQuery = this.db .select() .from(schema.user) - .where(and(...globalFilters)) + .where(and(...filters)) .limit(limit) .offset(offset) .orderBy(desc(schema.user.createdAt)); @@ -150,7 +139,7 @@ export class DrizzleUserAdapter implements IUserProvider { const [countResult] = await this.db .select({ count: count(schema.user.id) }) .from(schema.user) - .where(and(...globalFilters)); + .where(and(...filters)); const users = await dataQuery; return { From 75fce2b3b7bc231e02013029c55190b1a2650bd0 Mon Sep 17 00:00:00 2001 From: Pramod Date: Sat, 24 Jan 2026 16:57:06 +0530 Subject: [PATCH 04/13] style: format adapters --- packages/identity/src/adapters/drizzle-tenant.adapter.ts | 2 +- packages/identity/src/adapters/drizzle-user.adapter.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/identity/src/adapters/drizzle-tenant.adapter.ts b/packages/identity/src/adapters/drizzle-tenant.adapter.ts index 13443422..78807d5e 100644 --- a/packages/identity/src/adapters/drizzle-tenant.adapter.ts +++ b/packages/identity/src/adapters/drizzle-tenant.adapter.ts @@ -9,7 +9,7 @@ import { import * as schema from "../schema"; export class DrizzleTenantAdapter implements ITenantProvider { - constructor(private readonly db: NodePgDatabase) { } + constructor(private readonly db: NodePgDatabase) {} async create(userId: string, name: string): Promise { const orgId = uuidv4(); diff --git a/packages/identity/src/adapters/drizzle-user.adapter.ts b/packages/identity/src/adapters/drizzle-user.adapter.ts index 72416e84..3820d914 100644 --- a/packages/identity/src/adapters/drizzle-user.adapter.ts +++ b/packages/identity/src/adapters/drizzle-user.adapter.ts @@ -13,7 +13,7 @@ export class DrizzleUserAdapter implements IUserProvider { constructor( private readonly db: NodePgDatabase, private readonly authProvider: IAuthProvider, - ) { } + ) {} async create(input: CreateUserInput): Promise { // Delegate to AuthProvider to handle account creation (and password hashing) From 70e1090b5c28499b3f1e129101b351b2af670b24 Mon Sep 17 00:00:00 2001 From: Pramod Date: Sat, 24 Jan 2026 17:14:06 +0530 Subject: [PATCH 05/13] fix(identity): handle slug updates and safe filter construction - DrizzleTenantAdapter: Handle unique slug violation in update - DrizzleUserAdapter: Guard and(...filters) calls --- .../src/adapters/drizzle-tenant.adapter.ts | 24 ++++++++++++------- .../src/adapters/drizzle-user.adapter.ts | 16 +++++++------ 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/packages/identity/src/adapters/drizzle-tenant.adapter.ts b/packages/identity/src/adapters/drizzle-tenant.adapter.ts index 78807d5e..613cd5fc 100644 --- a/packages/identity/src/adapters/drizzle-tenant.adapter.ts +++ b/packages/identity/src/adapters/drizzle-tenant.adapter.ts @@ -9,7 +9,7 @@ import { import * as schema from "../schema"; export class DrizzleTenantAdapter implements ITenantProvider { - constructor(private readonly db: NodePgDatabase) {} + constructor(private readonly db: NodePgDatabase) { } async create(userId: string, name: string): Promise { const orgId = uuidv4(); @@ -92,14 +92,22 @@ export class DrizzleTenantAdapter implements ITenantProvider { if (input.metadata !== undefined) updatePayload.metadata = JSON.stringify(input.metadata); - const [updated] = await this.db - .update(schema.organization) - .set({ ...updatePayload, updatedAt: new Date() }) - .where(eq(schema.organization.id, id)) - .returning(); + try { + const [updated] = await this.db + .update(schema.organization) + .set({ ...updatePayload, updatedAt: new Date() }) + .where(eq(schema.organization.id, id)) + .returning(); - if (!updated) throw new Error("Tenant not found"); - return this.mapTenant(updated); + if (!updated) throw new Error("Tenant not found"); + return this.mapTenant(updated); + } catch (error: any) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call + if (error.code === "23505" && error.detail?.includes("slug")) { + throw new Error("Tenant slug already exists"); + } + throw error; + } } async delete(id: string): Promise { diff --git a/packages/identity/src/adapters/drizzle-user.adapter.ts b/packages/identity/src/adapters/drizzle-user.adapter.ts index 3820d914..327d6828 100644 --- a/packages/identity/src/adapters/drizzle-user.adapter.ts +++ b/packages/identity/src/adapters/drizzle-user.adapter.ts @@ -13,7 +13,7 @@ export class DrizzleUserAdapter implements IUserProvider { constructor( private readonly db: NodePgDatabase, private readonly authProvider: IAuthProvider, - ) {} + ) { } async create(input: CreateUserInput): Promise { // Delegate to AuthProvider to handle account creation (and password hashing) @@ -110,7 +110,7 @@ export class DrizzleUserAdapter implements IUserProvider { .select({ user: schema.user }) .from(schema.user) .innerJoin(schema.member, eq(schema.member.userId, schema.user.id)) - .where(and(...filters)) + .where(filters.length ? and(...filters) : undefined) .limit(limit) .offset(offset) .orderBy(desc(schema.user.createdAt)); @@ -119,7 +119,7 @@ export class DrizzleUserAdapter implements IUserProvider { .select({ count: count(schema.user.id) }) .from(schema.user) .innerJoin(schema.member, eq(schema.member.userId, schema.user.id)) - .where(and(...filters)); + .where(filters.length ? and(...filters) : undefined); const users = await dataQuery; return { @@ -131,7 +131,7 @@ export class DrizzleUserAdapter implements IUserProvider { const dataQuery = this.db .select() .from(schema.user) - .where(and(...filters)) + .where(filters.length ? and(...filters) : undefined) .limit(limit) .offset(offset) .orderBy(desc(schema.user.createdAt)); @@ -139,7 +139,7 @@ export class DrizzleUserAdapter implements IUserProvider { const [countResult] = await this.db .select({ count: count(schema.user.id) }) .from(schema.user) - .where(and(...filters)); + .where(filters.length ? and(...filters) : undefined); const users = await dataQuery; return { @@ -170,14 +170,16 @@ export class DrizzleUserAdapter implements IUserProvider { .select({ count: count(schema.user.id) }) .from(schema.user) .innerJoin(schema.member, eq(schema.member.userId, schema.user.id)) - .where(and(...whereConditions)); + .where( + whereConditions.length ? and(...whereConditions) : undefined, + ); return Number(result?.count || 0); } const [result] = await this.db .select({ count: count(schema.user.id) }) .from(schema.user) - .where(and(...whereConditions)); + .where(whereConditions.length ? and(...whereConditions) : undefined); return Number(result?.count || 0); } From 6ef3228027875de4a6703fb06f9c0a59c694bed6 Mon Sep 17 00:00:00 2001 From: Pramod Date: Sat, 24 Jan 2026 17:17:17 +0530 Subject: [PATCH 06/13] style: format adapters --- packages/identity/src/adapters/drizzle-tenant.adapter.ts | 2 +- packages/identity/src/adapters/drizzle-user.adapter.ts | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/identity/src/adapters/drizzle-tenant.adapter.ts b/packages/identity/src/adapters/drizzle-tenant.adapter.ts index 613cd5fc..770722e5 100644 --- a/packages/identity/src/adapters/drizzle-tenant.adapter.ts +++ b/packages/identity/src/adapters/drizzle-tenant.adapter.ts @@ -9,7 +9,7 @@ import { import * as schema from "../schema"; export class DrizzleTenantAdapter implements ITenantProvider { - constructor(private readonly db: NodePgDatabase) { } + constructor(private readonly db: NodePgDatabase) {} async create(userId: string, name: string): Promise { const orgId = uuidv4(); diff --git a/packages/identity/src/adapters/drizzle-user.adapter.ts b/packages/identity/src/adapters/drizzle-user.adapter.ts index 327d6828..d1f834ce 100644 --- a/packages/identity/src/adapters/drizzle-user.adapter.ts +++ b/packages/identity/src/adapters/drizzle-user.adapter.ts @@ -13,7 +13,7 @@ export class DrizzleUserAdapter implements IUserProvider { constructor( private readonly db: NodePgDatabase, private readonly authProvider: IAuthProvider, - ) { } + ) {} async create(input: CreateUserInput): Promise { // Delegate to AuthProvider to handle account creation (and password hashing) @@ -170,9 +170,7 @@ export class DrizzleUserAdapter implements IUserProvider { .select({ count: count(schema.user.id) }) .from(schema.user) .innerJoin(schema.member, eq(schema.member.userId, schema.user.id)) - .where( - whereConditions.length ? and(...whereConditions) : undefined, - ); + .where(whereConditions.length ? and(...whereConditions) : undefined); return Number(result?.count || 0); } From 184c8abebb445228176d9d68dca5fc4cb66dc092 Mon Sep 17 00:00:00 2001 From: Pramod Date: Sun, 25 Jan 2026 10:33:55 +0530 Subject: [PATCH 07/13] feat(identity): retire TenantsService and simplify system admin creation --- .../identity/auth/auth.controller.spec.ts | 13 +- .../modules/identity/auth/auth.controller.ts | 16 +- .../identity/auth/auth.service.spec.ts | 17 +- .../src/modules/identity/auth/auth.service.ts | 7 +- .../system-admin.controller.spec.ts | 20 +- .../system-admin/system-admin.controller.ts | 7 +- .../tenants/tenants.controller.spec.ts | 21 +- .../identity/tenants/tenants.controller.ts | 11 +- .../identity/tenants/tenants.module.ts | 8 +- .../identity/tenants/tenants.service.spec.ts | 187 --------------- .../identity/tenants/tenants.service.ts | 118 --------- .../modules/identity/users/users.module.ts | 3 +- docs/draft/identity_module_design_doc.md | 227 ++++++++++++++++++ .../task/01d_retire_legacy_tenants_service.md | 48 ++++ .../src/adapters/drizzle-tenant.adapter.ts | 4 +- .../src/adapters/drizzle-user.adapter.ts | 16 +- .../src/interfaces/user-provider.interface.ts | 1 + 17 files changed, 352 insertions(+), 372 deletions(-) delete mode 100644 apps/api/src/modules/identity/tenants/tenants.service.spec.ts delete mode 100644 apps/api/src/modules/identity/tenants/tenants.service.ts create mode 100644 docs/draft/identity_module_design_doc.md create mode 100644 docs/refactor/modules/identity/task/01d_retire_legacy_tenants_service.md diff --git a/apps/api/src/modules/identity/auth/auth.controller.spec.ts b/apps/api/src/modules/identity/auth/auth.controller.spec.ts index 8836250c..c8a00859 100644 --- a/apps/api/src/modules/identity/auth/auth.controller.spec.ts +++ b/apps/api/src/modules/identity/auth/auth.controller.spec.ts @@ -1,8 +1,7 @@ import { Test, TestingModule } from '@nestjs/testing'; import { AuthController } from './auth.controller'; import { AuthService } from './auth.service'; -import { USER_PROVIDER } from '@nexiom/identity'; -import { TenantsService } from '../tenants/tenants.service'; +import { USER_PROVIDER, TENANT_PROVIDER } from '@nexiom/identity'; import { InvitationsService } from '../invitations/invitations.service'; import { Request, Response } from 'express'; @@ -36,7 +35,7 @@ describe('AuthController', () => { forceVerifyEmail: jest.fn(), }; - const mockTenantsService = { + const mockTenantProvider = { provisionTenantForUser: jest.fn(), }; @@ -58,8 +57,8 @@ describe('AuthController', () => { useValue: mockUserProvider, }, { - provide: TenantsService, - useValue: mockTenantsService, + provide: TENANT_PROVIDER, + useValue: mockTenantProvider, }, { provide: InvitationsService, @@ -91,7 +90,7 @@ describe('AuthController', () => { }; mockAuthService.getSessionFromHeaders.mockResolvedValue(mockSessionData); - mockTenantsService.provisionTenantForUser.mockResolvedValue({ + mockTenantProvider.provisionTenantForUser.mockResolvedValue({ id: 'org-123', name: 'New Org', }); @@ -99,7 +98,7 @@ describe('AuthController', () => { const result = await controller.provisionTenant(mockRequest); expect(result).toBeDefined(); - expect(mockTenantsService.provisionTenantForUser).toHaveBeenCalledWith( + expect(mockTenantProvider.provisionTenantForUser).toHaveBeenCalledWith( 'user-123', ); }); diff --git a/apps/api/src/modules/identity/auth/auth.controller.ts b/apps/api/src/modules/identity/auth/auth.controller.ts index b97cafa8..d013fe19 100644 --- a/apps/api/src/modules/identity/auth/auth.controller.ts +++ b/apps/api/src/modules/identity/auth/auth.controller.ts @@ -8,10 +8,17 @@ import { UnauthorizedException, Inject, Logger, + BadRequestException, } from '@nestjs/common'; import { AuthService } from './auth.service'; -import { USER_PROVIDER, IUserProvider, Session, User } from '@nexiom/identity'; -import { TenantsService } from '../tenants/tenants.service'; +import { + USER_PROVIDER, + IUserProvider, + Session, + User, + TENANT_PROVIDER, + ITenantProvider, +} from '@nexiom/identity'; import { z } from 'zod'; import { createZodDto } from 'nestjs-zod'; import { Signup, CompleteInvite } from '../users/users.validation'; @@ -19,7 +26,6 @@ import { Response, Request } from 'express'; import { toNodeHandler } from 'better-auth/node'; import { InvitationsService } from '../invitations/invitations.service'; import { toWebHeaders } from '../../../shared/utils/headers.util'; -import { BadRequestException } from '@nestjs/common'; /** * Handles authentication-related operations such as user login. @@ -39,7 +45,7 @@ export class AuthController { constructor( private readonly authService: AuthService, @Inject(USER_PROVIDER) private readonly userProvider: IUserProvider, - private readonly tenantsService: TenantsService, + @Inject(TENANT_PROVIDER) private readonly tenantProvider: ITenantProvider, private readonly invitationsService: InvitationsService, ) {} @@ -83,7 +89,7 @@ export class AuthController { throw new UnauthorizedException('No Session Found'); } - return this.tenantsService.provisionTenantForUser(sessionData.user.id); + return this.tenantProvider.provisionTenantForUser(sessionData.user.id); } /** diff --git a/apps/api/src/modules/identity/auth/auth.service.spec.ts b/apps/api/src/modules/identity/auth/auth.service.spec.ts index b589be8e..e474aea3 100644 --- a/apps/api/src/modules/identity/auth/auth.service.spec.ts +++ b/apps/api/src/modules/identity/auth/auth.service.spec.ts @@ -1,7 +1,6 @@ import { Test, TestingModule } from '@nestjs/testing'; import { AuthService } from './auth.service'; -import { AUTH_PROVIDER } from '@nexiom/identity'; -import { TenantsService } from '../tenants/tenants.service'; +import { AUTH_PROVIDER, TENANT_PROVIDER } from '@nexiom/identity'; describe('AuthService', () => { let service: AuthService; @@ -15,7 +14,7 @@ describe('AuthService', () => { getHandler: jest.fn(), }; - const mockTenantsService = { + const mockTenantProvider = { findAllForUser: jest.fn(), }; @@ -28,8 +27,8 @@ describe('AuthService', () => { useValue: mockAuthProvider, }, { - provide: TenantsService, - useValue: mockTenantsService, + provide: TENANT_PROVIDER, + useValue: mockTenantProvider, }, ], }).compile(); @@ -89,12 +88,12 @@ describe('AuthService', () => { ]; mockAuthProvider.validateSession.mockResolvedValue(mockSession); - mockTenantsService.findAllForUser.mockResolvedValue(mockTenants); + mockTenantProvider.findAllForUser.mockResolvedValue(mockTenants); const result = await service.getEnrichedSession(token); expect(mockAuthProvider.validateSession).toHaveBeenCalledWith(token); - expect(mockTenantsService.findAllForUser).toHaveBeenCalledWith('u1'); + expect(mockTenantProvider.findAllForUser).toHaveBeenCalledWith('u1'); // Should pick 'org-new' because it is newer expect(result?.user.organizationId).toBe('org-new'); @@ -105,7 +104,7 @@ describe('AuthService', () => { it('should return enriched session without organizationId if no tenant', async () => { const token = 'valid-token'; const mockSession = { session: { id: 's1' }, user: { id: 'u1' } }; - mockTenantsService.findAllForUser.mockResolvedValue([]); + mockTenantProvider.findAllForUser.mockResolvedValue([]); mockAuthProvider.validateSession.mockResolvedValue(mockSession); @@ -153,7 +152,7 @@ describe('AuthService', () => { providers: [ AuthService, { provide: AUTH_PROVIDER, useValue: providerWithoutSetPassword }, - { provide: TenantsService, useValue: mockTenantsService }, + { provide: TENANT_PROVIDER, useValue: mockTenantProvider }, ], }).compile(); const localService = module.get(AuthService); diff --git a/apps/api/src/modules/identity/auth/auth.service.ts b/apps/api/src/modules/identity/auth/auth.service.ts index 76a04b3d..a980ce3c 100644 --- a/apps/api/src/modules/identity/auth/auth.service.ts +++ b/apps/api/src/modules/identity/auth/auth.service.ts @@ -7,14 +7,15 @@ import { Session, User, CreateUserInput, + TENANT_PROVIDER, + ITenantProvider, } from '@nexiom/identity'; -import { TenantsService } from '../tenants/tenants.service'; @Injectable() export class AuthService { constructor( @Inject(AUTH_PROVIDER) private readonly authProvider: IAuthProvider, - private readonly tenantsService: TenantsService, + @Inject(TENANT_PROVIDER) private readonly tenantProvider: ITenantProvider, ) {} async login(credentials: LoginCredentials): Promise { @@ -40,7 +41,7 @@ export class AuthService { const { session, user } = validSession; // Enrichment: Check if user has a tenant - const tenants = await this.tenantsService.findAllForUser(user.id); + const tenants = await this.tenantProvider.findAllForUser(user.id); // Deterministic selection: Sort by creation date (newest first) // using slice() to avoid mutating the original array const sortedTenants = tenants diff --git a/apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts b/apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts index 4a1a73ec..963a4703 100644 --- a/apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts +++ b/apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts @@ -206,13 +206,12 @@ describe('SystemAdminController', () => { it('should create user via provider', async () => { mockUserProvider.findByEmail.mockResolvedValue(null); - const mockUser = { id: 'u1', email: 'new@example.com' }; // Initial return - mockUserProvider.create.mockResolvedValue(mockUser); - // Simulate subsequent fetch for systemRole update if needed - mockUserProvider.findById.mockResolvedValue({ - ...mockUser, + const mockUser = { + id: 'u1', + email: 'new@example.com', systemRole: 'platform_user', - }); + }; + mockUserProvider.create.mockResolvedValue(mockUser); const result = await controller.createUser({ name: 'Test', @@ -220,12 +219,13 @@ describe('SystemAdminController', () => { systemRole: 'platform_user', }); - // Matches controller logic: if systemRole, fetches updated - expect(result).toEqual({ ...mockUser, systemRole: 'platform_user' }); - expect(mockUserProvider.create).toHaveBeenCalled(); - expect(mockUserProvider.update).toHaveBeenCalledWith('u1', { + expect(result).toEqual(mockUser); + expect(mockUserProvider.create).toHaveBeenCalledWith({ + name: 'Test', + email: 'new@example.com', systemRole: 'platform_user', }); + expect(mockUserProvider.update).not.toHaveBeenCalled(); }); }); diff --git a/apps/api/src/modules/identity/system-admin/system-admin.controller.ts b/apps/api/src/modules/identity/system-admin/system-admin.controller.ts index f95edb2c..e58432db 100644 --- a/apps/api/src/modules/identity/system-admin/system-admin.controller.ts +++ b/apps/api/src/modules/identity/system-admin/system-admin.controller.ts @@ -135,16 +135,11 @@ export class SystemAdminController { throw new BadRequestException('User with this email already exists'); } - // TODO: Add systemRole to CreateUserInput and Adapter so we can do this in one step + // Now uses single-step creation via Adapter logic const user = await this.userProvider.create({ ...input, }); - if (input.systemRole) { - await this.userProvider.update(user.id, { systemRole: input.systemRole }); - return this.userProvider.findById(user.id); - } - return user; } diff --git a/apps/api/src/modules/identity/tenants/tenants.controller.spec.ts b/apps/api/src/modules/identity/tenants/tenants.controller.spec.ts index 4d6c08ae..b6b7a621 100644 --- a/apps/api/src/modules/identity/tenants/tenants.controller.spec.ts +++ b/apps/api/src/modules/identity/tenants/tenants.controller.spec.ts @@ -1,15 +1,14 @@ import { Test, TestingModule } from '@nestjs/testing'; import { Request } from 'express'; import { TenantsController } from './tenants.controller'; -import { TenantsService } from './tenants.service'; +import { TENANT_PROVIDER, Tenant } from '@nexiom/identity'; import { AuthGuard } from '../auth/auth.guard'; -import { Organization } from './tenant.schema'; import { UpdateTenantStatus } from './tenants.validation'; describe('TenantsController', () => { let controller: TenantsController; - const mockTenantsService = { + const mockTenantProvider = { findAllForUser: jest.fn(), updateStatus: jest.fn(), }; @@ -23,8 +22,8 @@ describe('TenantsController', () => { controllers: [TenantsController], providers: [ { - provide: TenantsService, - useValue: mockTenantsService, + provide: TENANT_PROVIDER, + useValue: mockTenantProvider, }, ], }) @@ -43,7 +42,7 @@ describe('TenantsController', () => { describe('findAll', () => { it('should return an array of organizations for the user', async () => { - const result: Organization[] = [ + const result: Tenant[] = [ { id: '1', name: 'Test Org', @@ -51,17 +50,17 @@ describe('TenantsController', () => { logo: null, createdAt: new Date(), updatedAt: new Date(), - metadata: null, + metadata: undefined, status: 'active', }, ]; - mockTenantsService.findAllForUser.mockResolvedValue(result); + mockTenantProvider.findAllForUser.mockResolvedValue(result); const req = { user: { id: 'user-1' } } as unknown as Request & { user: { id: string }; }; expect(await controller.findAll(req)).toBe(result); - expect(mockTenantsService.findAllForUser).toHaveBeenCalledWith('user-1'); + expect(mockTenantProvider.findAllForUser).toHaveBeenCalledWith('user-1'); }); }); @@ -70,10 +69,10 @@ describe('TenantsController', () => { const id = '1'; const status: UpdateTenantStatus = { status: 'disabled' }; const result = { id, status: status.status }; - mockTenantsService.updateStatus.mockResolvedValue(result); + mockTenantProvider.updateStatus.mockResolvedValue(result); expect(await controller.updateStatus(id, status)).toBe(result); - expect(mockTenantsService.updateStatus).toHaveBeenCalledWith( + expect(mockTenantProvider.updateStatus).toHaveBeenCalledWith( id, status.status, ); diff --git a/apps/api/src/modules/identity/tenants/tenants.controller.ts b/apps/api/src/modules/identity/tenants/tenants.controller.ts index ce102210..89169b2b 100644 --- a/apps/api/src/modules/identity/tenants/tenants.controller.ts +++ b/apps/api/src/modules/identity/tenants/tenants.controller.ts @@ -6,24 +6,27 @@ import { Body, UseGuards, Req, + Inject, } from '@nestjs/common'; import { Request } from 'express'; -import { TenantsService } from './tenants.service'; import { AuthGuard } from '../auth/auth.guard'; import { UpdateTenantStatus } from './tenants.validation'; +import { TENANT_PROVIDER, ITenantProvider } from '@nexiom/identity'; @Controller('tenants') @UseGuards(AuthGuard) export class TenantsController { - constructor(private readonly tenantsService: TenantsService) {} + constructor( + @Inject(TENANT_PROVIDER) private readonly tenantProvider: ITenantProvider, + ) {} @Get() findAll(@Req() req: Request & { user: { id: string } }) { - return this.tenantsService.findAllForUser(req.user.id); + return this.tenantProvider.findAllForUser(req.user.id); } @Patch(':id') updateStatus(@Param('id') id: string, @Body() body: UpdateTenantStatus) { - return this.tenantsService.updateStatus(id, body.status); + return this.tenantProvider.updateStatus(id, body.status); } } diff --git a/apps/api/src/modules/identity/tenants/tenants.module.ts b/apps/api/src/modules/identity/tenants/tenants.module.ts index 83675caa..e8a51b8a 100644 --- a/apps/api/src/modules/identity/tenants/tenants.module.ts +++ b/apps/api/src/modules/identity/tenants/tenants.module.ts @@ -1,12 +1,10 @@ import { Module } from '@nestjs/common'; -import { TenantsService } from './tenants.service'; import { TenantsController } from './tenants.controller'; -import { DbModule } from '../../../db/db.module'; @Module({ - imports: [DbModule], + imports: [], controllers: [TenantsController], - providers: [TenantsService], - exports: [TenantsService], // Export for AuthModule to use + providers: [], + exports: [], }) export class TenantsModule {} diff --git a/apps/api/src/modules/identity/tenants/tenants.service.spec.ts b/apps/api/src/modules/identity/tenants/tenants.service.spec.ts deleted file mode 100644 index 1d49e976..00000000 --- a/apps/api/src/modules/identity/tenants/tenants.service.spec.ts +++ /dev/null @@ -1,187 +0,0 @@ -import { Test, TestingModule } from '@nestjs/testing'; -import { TenantsService } from './tenants.service'; -import { DRIZZLE_DB } from '../../../db/db.provider'; -import { DbOrganization as Organization } from '../../../db/schema'; - -const mockOrganizations: Organization[] = [ - { - id: '1', - name: 'Test Org 1', - slug: 'test-org-1', - logo: null, - createdAt: new Date(), - updatedAt: new Date(), - metadata: null, - status: 'active', - deletedAt: null, - }, - { - id: '2', - name: 'Test Org 2', - slug: 'test-org-2', - logo: null, - createdAt: new Date(), - updatedAt: new Date(), - metadata: null, - status: 'disabled', - deletedAt: null, - }, -]; - -// Define a type for our mock that satisfies the functionality we use -// We don't try to implement the full NodePgDatabase interface as it's too complex to mock fully manually -interface MockDrizzle { - select: jest.Mock; - from: jest.Mock; - where: jest.Mock; - limit: jest.Mock; - execute: jest.Mock; - insert: jest.Mock; - values: jest.Mock; - returning: jest.Mock; - transaction: jest.Mock; - update: jest.Mock; - set: jest.Mock; - innerJoin: jest.Mock; - - query: MockQuery; -} - -interface MockQuery { - user: { - findFirst: jest.Mock; - }; -} - -// Instantiate with type safety -const mockDb: MockDrizzle = { - select: jest.fn().mockReturnThis(), - from: jest.fn().mockReturnThis(), - where: jest.fn().mockReturnThis(), - limit: jest.fn().mockReturnThis(), - execute: jest.fn(), - insert: jest.fn().mockReturnThis(), - values: jest.fn().mockReturnThis(), - returning: jest.fn(), - transaction: jest.fn(), // Placeholder, implementation below to avoid circular ref - update: jest.fn().mockReturnThis(), - set: jest.fn().mockReturnThis(), - innerJoin: jest.fn().mockReturnThis(), - query: { - user: { - findFirst: jest.fn(), - }, - }, -}; - -// Implement circular transaction logic safely -mockDb.transaction.mockImplementation( - (cb: (tx: MockDrizzle) => Promise) => { - return cb(mockDb); - }, -); - -describe('TenantsService', () => { - let service: TenantsService; - - beforeEach(async () => { - const module: TestingModule = await Test.createTestingModule({ - providers: [ - TenantsService, - { - provide: DRIZZLE_DB, - useValue: mockDb, - }, - ], - }).compile(); - - service = module.get(TenantsService); - - jest.clearAllMocks(); - }); - - it('should be defined', () => { - expect(service).toBeDefined(); - }); - - describe('findAllForUser', () => { - it('should return tenants for the user', async () => { - // Mock the join query chain - // db.select().from().innerJoin().where().execute() - mockDb.execute.mockResolvedValue(mockOrganizations); - - const result = await service.findAllForUser('user-1'); - - expect(result).toEqual(mockOrganizations); - expect(mockDb.innerJoin).toHaveBeenCalled(); - expect(mockDb.where).toHaveBeenCalled(); - }); - }); - - describe('updateStatus', () => { - it('should update tenant status', async () => { - const updatedOrg: Organization = { - ...mockOrganizations[0], - status: 'disabled', - }; - mockDb.returning.mockResolvedValue([updatedOrg]); - - const result = await service.updateStatus('1', 'disabled'); - - expect(result).toEqual(updatedOrg); - expect(mockDb.update).toHaveBeenCalled(); - }); - - it('should throw error if tenant not found', async () => { - mockDb.returning.mockResolvedValue([]); - - await expect(service.updateStatus('999', 'disabled')).rejects.toThrow(); - }); - }); - - describe('createTenant', () => { - it('should create organization and member transactionally', async () => { - const newOrg: Organization = { ...mockOrganizations[0] }; - // specialized mocks for tx - mockDb.returning.mockResolvedValueOnce([newOrg]); // for org insert - // member insert doesn't return anything we check explicitly here, - // but the tx should return the org. - - const result = await service.createTenant('user-1', 'Test Corp'); - - expect(result).toEqual(newOrg); - expect(mockDb.transaction).toHaveBeenCalled(); - expect(mockDb.insert).toHaveBeenCalledTimes(2); // Org + Member - }); - }); - - describe('provisionTenantForUser', () => { - it('should provision a tenant with generated name', async () => { - const mockUser = { id: 'user-1', email: 'test@example.com' }; - const newOrg: Organization = { - ...mockOrganizations[0], - name: 'Organization X', - }; - - // Mock user lookup - mockDb.query.user.findFirst.mockResolvedValueOnce(mockUser); - - // Mock createTenant tx - mockDb.returning.mockResolvedValueOnce([newOrg]); // Org insert - - const result = await service.provisionTenantForUser('user-1'); - - expect(mockDb.query.user.findFirst).toHaveBeenCalled(); // User lookup - expect(mockDb.transaction).toHaveBeenCalled(); // createTenant - expect(result).toEqual(newOrg); - }); - - it('should throw if user not found', async () => { - mockDb.query.user.findFirst.mockResolvedValueOnce(null); // No user - - await expect( - service.provisionTenantForUser('user-999'), - ).rejects.toThrow(); - }); - }); -}); diff --git a/apps/api/src/modules/identity/tenants/tenants.service.ts b/apps/api/src/modules/identity/tenants/tenants.service.ts deleted file mode 100644 index 2e91ca15..00000000 --- a/apps/api/src/modules/identity/tenants/tenants.service.ts +++ /dev/null @@ -1,118 +0,0 @@ -import { Injectable, Inject } from '@nestjs/common'; -import { DRIZZLE_DB } from '../../../db/db.provider'; -import { NodePgDatabase } from 'drizzle-orm/node-postgres'; -import * as schema from '../../../db/schema'; -import { eq } from 'drizzle-orm'; -import { randomUUID } from 'crypto'; - -@Injectable() -export class TenantsService { - constructor( - @Inject(DRIZZLE_DB) private readonly db: NodePgDatabase, - ) {} - - async findAllForUser(userId: string) { - const query = this.db - .select({ - id: schema.organization.id, - name: schema.organization.name, - slug: schema.organization.slug, - logo: schema.organization.logo, - createdAt: schema.organization.createdAt, - metadata: schema.organization.metadata, - status: schema.organization.status, - memberRole: schema.member.role, // Optional: Return their role in that org - }) - .from(schema.organization) - .innerJoin( - schema.member, - eq(schema.member.organizationId, schema.organization.id), - ) - .where(eq(schema.member.userId, userId)); - - return query.execute(); - } - - async updateStatus(id: string, status: 'active' | 'disabled' | 'suspended') { - const result = await this.db - .update(schema.organization) - .set({ status }) - .where(eq(schema.organization.id, id)) - .returning(); - - if (!result[0]) { - throw new Error(`Organization with id ${id} not found`); - } - return result[0]; - } - - /** - * Domain Logic: Create a new Tenant (Organization) and assign the Creator as Admin. - */ - async createTenant(userId: string, name: string) { - const orgId = randomUUID(); - const slug = this.generateSlug(name); - - return await this.db.transaction(async (tx) => { - // 1. Create Organization - const [org] = await tx - .insert(schema.organization) - .values({ - id: orgId, - name: name, - slug: slug, - createdAt: new Date(), - status: 'active', - }) - .returning(); - - // 2. Add Member (Admin) - await tx.insert(schema.member).values({ - id: randomUUID(), - organizationId: orgId, - userId: userId, - role: 'admin', - createdAt: new Date(), - }); - - return org; - }); - } - - /** - * Orchestration Logic: Auto-provision a tenant for a given User ID. - * Derives company name from user metadata if possible, or generates a default. - */ - async provisionTenantForUser( - userId: string, - ): Promise { - // 1. Fetch User to get name/email for auto-naming (optional, but good UX) - const user = await this.db.query.user.findFirst({ - where: eq(schema.user.id, userId), - }); - - if (!user) { - throw new Error(`User with ID ${userId} not found`); - } - - // 2. Generate Company Name - // Logic: Try Company Name field? user doesn't have it standard. - // Use fallback: "Organization " - const randomSuffix = Math.random().toString(36).substring(7); - const companyName = `Organization ${randomSuffix}`; - - // 3. Create - return this.createTenant(userId, companyName); - } - - private generateSlug(name: string): string { - return ( - name - .toLowerCase() - .replace(/\s+/g, '-') - .replace(/[^a-z0-9-]/g, '') + - '-' + - randomUUID().slice(0, 4) - ); - } -} diff --git a/apps/api/src/modules/identity/users/users.module.ts b/apps/api/src/modules/identity/users/users.module.ts index 4f14fbc7..6ced066f 100644 --- a/apps/api/src/modules/identity/users/users.module.ts +++ b/apps/api/src/modules/identity/users/users.module.ts @@ -1,9 +1,8 @@ import { Module } from '@nestjs/common'; import { UsersController } from './users.controller'; -import { DbModule } from '../../../db/db.module'; @Module({ - imports: [DbModule], + imports: [], controllers: [UsersController], providers: [], }) diff --git a/docs/draft/identity_module_design_doc.md b/docs/draft/identity_module_design_doc.md new file mode 100644 index 00000000..7df93506 --- /dev/null +++ b/docs/draft/identity_module_design_doc.md @@ -0,0 +1,227 @@ +# Identity Module Architecture: The "Guardianship" Kernel + +**Status:** RFC (Request for Comments) +**Author:** Staff Architect +**Version:** 2.0 (Aligns with Nexiom Master Architecture) + +--- + +## 1. Architectural Context (C4 Model) + +The **Identity Module** (`@nexiom/identity`) is the "Security Kernel" of the Nexiom Platform. It is NOT just a user table; it is the **Authority** for: + +1. **Authentication:** Who are you? (Users/Machines) +2. **Multitenancy:** Which data silo do you own? (Tenant Resolution) +3. **Authorization:** What can you click? (CBAC: Capability-Based Access Control) +4. **Credental Management:** How do we talk to external apps? (Layer 5 Support) + +### 1.1 System Context Diagram (Level 1) + +```mermaid +C4Context + title System Context: Identity Module + + Person(user, "User", "System Admin or Tenant Member") + System_Ext(auth_provider, "Auth Provider", "BetterAuth / Clerk / Supabase") + + System_Boundary(nexiom, "Nexiom Platform") { + System(api, "API Monolith", "NestJS Backend") + System(identity, "Identity Kernel", "@nexiom/identity Package") + System(db, "Database", "Postgres (Public & Tenant Schemas)") + } + + Rel(user, api, "Uses", "HTTPS/JSON") + Rel(api, identity, "Delegates Auth To", "Interface Calls") + Rel(identity, auth_provider, "Verifies Tokens With", "HTTP/SDK") + Rel(identity, db, "Reads/Writes User Data", "SQL/ORM") +``` + +### 1.2 Container Diagram (Level 2) + +Shows how the Identity Package is embedded within the Monolith but logically isolated. + +```mermaid +C4Container + title Container Diagram: Identity Package Integration + + Container_Boundary(apps, "Applications") { + Container(api_layer, "API Layer", "NestJS Controllers", "Handles HTTP, Validation, Routing") + } + + Container_Boundary(libs, "Shared Libraries") { + Container(identity_pkg, "Identity Package", "@nexiom/identity", "Interfaces, Adapters, Guards") + } + + ContainerDb(db_users, "Public Schema", "Postgres", "Users, Tenants, Memberships") + + Rel(api_layer, identity_pkg, "Injects Interfaces", "Dependency Injection") + Rel(identity_pkg, db_users, "Manages", "Drizzle ORM") +``` + +--- + +## 2. The Identity & The 6-Layer Pipeline + +Identity is an **Orthogonal Concern**—it intersects the pipeline layers rather than being a step within them. + +### 2.1 Interaction Flow + +```mermaid +sequenceDiagram + autonumber + participant Gateway as Layer 1 (Gateway) + participant Worker as Layer 2-4 (Pipeline) + participant Delivery as Layer 5 (Delivery) + participant Identity as @nexiom/identity + participant DB as Public Schema + + Note over Gateway: 1. Ingestion + Gateway->>Identity: Validate Webhook Signature? + Identity-->>Gateway: OK (Shared Secret Check) + + Note over Worker: 2. Processing + Worker->>Identity: Resolve Tenant Context (envoy -> uuid) + Identity->>DB: SELECT id FROM tenants WHERE slug = 'envoy' + DB-->>Identity: tenant_uuid + Identity-->>Worker: Context { tenantId: '...' } + + Note over Delivery: 5. Execution + Delivery->>Identity: Get External App Credentials (Connection) + Identity->>DB: Decrypt OAuth Tokens + DB-->>Identity: Access Token + Identity-->>Delivery: Credentials +``` + +### 2.2 Layer Responsibilities + +| Pipeline Layer | Identity Responsibility | Interface Used | +| :--- | :--- | :--- | +| **L1: Gateway** | **Tenant Resolution** (Subdomain/Path) & **Signature Verification** (HMAC). | `ITenantProvider` | +| **L2: Replica** | **Context Injection** (Setting `AsyncLocalStorage`). | `ITenantProvider` | +| **L3: Norm** | N/A (Pure Logic) | N/A | +| **L4: Outbound**| N/A (Pure Logic) | N/A | +| **L5: Delivery** | **Credential Retrieval** (Decrypting OAuth tokens for destinations). | `ICredentialProvider` (Future) | +| **API (UI)** | **User Auth** (Session) & **RBAC** (Guards). | `IAuthProvider`, `IPermissionProvider` | + +--- + +## 3. Core Design Decisions (ADRs) + +### ADR-001: The Adapter Pattern + +* **Context:** We want to support "SaaS-in-a-Box" where users can bring their own Auth (Clerk, Auth0) or Database. +* **Decision:** All Identity logic must sit behind **Interfaces**. The API Layer NEVER imports the ORM directly for Identity tables. +* **Consequences:** + * (+) Zero vendor lock-in. + * (+) Easy to mock for TDD. + * (-) Slight boilerplate overhead (Interface + Adapter). + +### ADR-002: Capability-Based Access Control (CBAC) + +* **Context:** "Roles" (Admin, User) are too rigid for complex B2B apps. +* **Decision:** We perform checks on **Capabilities** (`create:tenant`, `read:report`), not Roles. +* **Consequences:** + * (+) Granular control. + * (+) Creating custom roles is just grouping capabilities. + +--- + +## 4. Source Code Structure (The Blueprint) + +### 4.1 Directory Map + +```text +packages/identity/ +├── src/ +│ ├── interfaces/ # 👈 THE LAW (Contracts) +│ │ ├── auth.provider.interface.ts +│ │ ├── user.provider.interface.ts +│ │ ├── tenant.provider.interface.ts +│ │ ├── permission.provider.interface.ts +│ │ +│ ├── adapters/ # 👈 THE WORKERS (Implementations) +│ │ ├── better-auth.adapter.ts +│ │ ├── drizzle-user.adapter.ts +│ │ ├── drizzle-tenant.adapter.ts +│ │ +│ ├── constants.ts # Dependency Injection Tokens +│ └── identity.module.ts # NestJS Dynamic Module +``` + +### 4.2 Key Interface Definitions + +#### `ITenantProvider` + +The "Landlord" of the system. + +```typescript +export interface ITenantProvider { + // Discovery + findBySlug(slug: string): Promise; + findById(id: string): Promise; + + // Resolution + resolveContext(request: Request): Promise; + + // Lifecycle + create(userId: string, name: string): Promise; + provisionTenantForUser(userId: string): Promise; // "Sign up and give me a workspace" +} +``` + +#### `IPermissionProvider` + +The "Bouncer" of the system. + +```typescript +export interface IPermissionProvider { + // The only method you actually need + can(user: User, action: string, resource: string): Promise; + + // Example usage: + // can(user, 'delete', 'production_db') +} +``` + +--- + +## 5. Flow Diagram: The "Strict Layer" Request + +How a `GET /admin/users` request travels through the refined architecture. + +```mermaid +sequenceDiagram + autonumber + participant Client + participant Controller as SystemAdminController + participant Guard as AuthGuard + participant UserProvider as DI Token (USER_PROVIDER) + participant Adapter as DrizzleUserAdapter + participant DB as Postgres + + Client->>Controller: GET /admin/users + + rect rgb(240, 248, 255) + Note right of Client: 1. Authentication Layer + Controller->>Guard: canActivate() + Guard->>Adapter: validateSession(token) + Adapter-->>Guard: User Session + end + + rect rgb(255, 240, 245) + Note right of Client: 2. Application Layer + Controller->>UserProvider: findAll({ tenantId: '...' }) + Note over Controller: Controller does NOT know about Drizzle/SQL + end + + rect rgb(240, 255, 240) + Note right of Client: 3. Infrastructure Layer + UserProvider->>Adapter: Adapter.findAll() + Adapter->>DB: db.select().from(users)... + DB-->>Adapter: Result Rows + Adapter-->>UserProvider: Mapped User[] + end + + UserProvider-->>Controller: User[] + Controller-->>Client: 200 OK +``` diff --git a/docs/refactor/modules/identity/task/01d_retire_legacy_tenants_service.md b/docs/refactor/modules/identity/task/01d_retire_legacy_tenants_service.md new file mode 100644 index 00000000..b3e4b95d --- /dev/null +++ b/docs/refactor/modules/identity/task/01d_retire_legacy_tenants_service.md @@ -0,0 +1,48 @@ +# Task 01d: Retire Legacy TenantsService + +**Priority:** HIGH +**Estimated Time:** 2 hours +**Assignee:** Coder +**Status:** Ready to Start + +--- + +## Objective + +The `TenantsService` class in `apps/api/src/modules/identity/tenants/tenants.service.ts` is a legacy artifact that violates the Strict Layer Enforcement by injecting `DRIZZLE_DB` directly. + +It duplicates logic (e.g., `provisionTenantForUser`) that is now correctly implemented in the `DrizzleTenantAdapter`. + +**Goal:** Remove `TenantsService` entirely and refactor its consumers to use `ITenantProvider`. + +--- + +## Implementation Steps + +### 1. Refactor Consumers + +- [ ] **Target:** `apps/api/src/modules/identity/tenants/tenants.controller.ts` + - Inject `ITenantProvider` (token: `TENANT_PROVIDER`) instead of `TenantsService`. + - Update methods to call provider methods directly. +- [ ] **Target:** `apps/api/src/modules/identity/auth/auth.service.ts` + - Inject `ITenantProvider` instead of `TenantsService`. + - Update `getEnrichedSession` to use `tenantProvider.findAllForUser`. + +### 2. Verify Adapter Capabilities + +- [ ] Ensure `DrizzleTenantAdapter` implements `provisionTenantForUser` correctly (it generates a slug and creates the org/member). +- [ ] Ensure `findAllForUser` is implemented. + +### 3. Cleanup + +- [ ] **Delete:** `apps/api/src/modules/identity/tenants/tenants.service.ts` +- [ ] **Delete:** `apps/api/src/modules/identity/tenants/tenants.service.spec.ts` +- [ ] **Remove Export:** Update `apps/api/src/modules/identity/tenants/tenants.module.ts` to stop exporting `TenantsService`. + +--- + +## Verification Plan + +- [ ] `grep -r "TenantsService" apps/api` should return 0 results (except maybe in migration files if any). +- [ ] `pnpm test apps/api` should pass. +- [ ] `grep -r "DRIZZLE_DB" apps/api/src/modules/identity` should return 0 results. diff --git a/packages/identity/src/adapters/drizzle-tenant.adapter.ts b/packages/identity/src/adapters/drizzle-tenant.adapter.ts index 770722e5..49575207 100644 --- a/packages/identity/src/adapters/drizzle-tenant.adapter.ts +++ b/packages/identity/src/adapters/drizzle-tenant.adapter.ts @@ -186,7 +186,7 @@ export class DrizzleTenantAdapter implements ITenantProvider { const data = await this.db .select() .from(schema.organization) - .where(and(...filters)) + .where(filters.length ? and(...filters) : undefined) .limit(limit) .offset(offset) .orderBy(desc(schema.organization.createdAt)); @@ -194,7 +194,7 @@ export class DrizzleTenantAdapter implements ITenantProvider { const [countResult] = await this.db .select({ count: count(schema.organization.id) }) .from(schema.organization) - .where(and(...filters)); + .where(filters.length ? and(...filters) : undefined); return { data: data.map((d) => this.mapTenant(d)), diff --git a/packages/identity/src/adapters/drizzle-user.adapter.ts b/packages/identity/src/adapters/drizzle-user.adapter.ts index d1f834ce..85a20823 100644 --- a/packages/identity/src/adapters/drizzle-user.adapter.ts +++ b/packages/identity/src/adapters/drizzle-user.adapter.ts @@ -17,7 +17,17 @@ export class DrizzleUserAdapter implements IUserProvider { async create(input: CreateUserInput): Promise { // Delegate to AuthProvider to handle account creation (and password hashing) - return await this.authProvider.createUser(input); + const user = await this.authProvider.createUser(input); + + // If systemRole is provided, we need to update the user record immediately + // because the Auth Provider might not support custom fields during creation + if (input.systemRole) { + await this.update(user.id, { systemRole: input.systemRole }); + const updated = await this.findById(user.id); + if (updated) return updated; + } + + return user; } async update(id: string, input: UpdateUserInput): Promise { @@ -90,8 +100,8 @@ export class DrizzleUserAdapter implements IUserProvider { search?: string; tenantId?: string; }): Promise<{ data: UserInterface[]; total: number }> { - const page = options?.page || 1; - const limit = options?.limit || 10; + const page = Math.max(1, Number(options?.page) || 1); + const limit = Math.max(1, Number(options?.limit) || 10); const offset = (page - 1) * limit; const filters = []; diff --git a/packages/identity/src/interfaces/user-provider.interface.ts b/packages/identity/src/interfaces/user-provider.interface.ts index 82524c0a..9b2c69bb 100644 --- a/packages/identity/src/interfaces/user-provider.interface.ts +++ b/packages/identity/src/interfaces/user-provider.interface.ts @@ -6,6 +6,7 @@ export interface CreateUserInput { firstName?: string; lastName?: string; role?: string; + systemRole?: string; companyName?: string; // Optional: for auto-provisioning } From c03f877fd7cb12bef9995ab8c532d3c9ae1d4914 Mon Sep 17 00:00:00 2001 From: Pramod Date: Sun, 25 Jan 2026 10:55:03 +0530 Subject: [PATCH 08/13] fix(identity): address CodeRabbit feedback on pagination and filters --- packages/identity/src/adapters/drizzle-tenant.adapter.ts | 4 ++-- packages/identity/src/adapters/drizzle-user.adapter.ts | 5 +++++ packages/identity/src/interfaces/user-provider.interface.ts | 1 + 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/identity/src/adapters/drizzle-tenant.adapter.ts b/packages/identity/src/adapters/drizzle-tenant.adapter.ts index 49575207..e69da4ba 100644 --- a/packages/identity/src/adapters/drizzle-tenant.adapter.ts +++ b/packages/identity/src/adapters/drizzle-tenant.adapter.ts @@ -174,8 +174,8 @@ export class DrizzleTenantAdapter implements ITenantProvider { limit?: number; search?: string; }): Promise<{ data: TenantInterface[]; total: number }> { - const page = options?.page || 1; - const limit = options?.limit || 10; + const page = Math.max(1, Number(options?.page) || 1); + const limit = Math.max(1, Number(options?.limit) || 10); const offset = (page - 1) * limit; const filters = []; diff --git a/packages/identity/src/adapters/drizzle-user.adapter.ts b/packages/identity/src/adapters/drizzle-user.adapter.ts index 85a20823..0499ec78 100644 --- a/packages/identity/src/adapters/drizzle-user.adapter.ts +++ b/packages/identity/src/adapters/drizzle-user.adapter.ts @@ -99,6 +99,7 @@ export class DrizzleUserAdapter implements IUserProvider { limit?: number; search?: string; tenantId?: string; + systemRole?: string; }): Promise<{ data: UserInterface[]; total: number }> { const page = Math.max(1, Number(options?.page) || 1); const limit = Math.max(1, Number(options?.limit) || 10); @@ -112,6 +113,10 @@ export class DrizzleUserAdapter implements IUserProvider { ); } + if (options?.systemRole) { + filters.push(eq(schema.user.systemRole, options.systemRole)); + } + if (options?.tenantId) { // Tenant-scoped (requires Join) filters.push(eq(schema.member.organizationId, options.tenantId)); diff --git a/packages/identity/src/interfaces/user-provider.interface.ts b/packages/identity/src/interfaces/user-provider.interface.ts index 9b2c69bb..804e3cde 100644 --- a/packages/identity/src/interfaces/user-provider.interface.ts +++ b/packages/identity/src/interfaces/user-provider.interface.ts @@ -30,6 +30,7 @@ export interface IUserProvider { limit?: number; search?: string; tenantId?: string; + systemRole?: string; }): Promise<{ data: User[]; total: number }>; // Kept for backward compatibility if needed, but the above covers it From 5014f86f5f9691f74eb8d60cc208754762edbe85 Mon Sep 17 00:00:00 2001 From: Pramod Date: Sun, 25 Jan 2026 12:14:46 +0530 Subject: [PATCH 09/13] refactor(identity): DRY DrizzleUserAdapter filter construction --- .../src/adapters/drizzle-user.adapter.ts | 47 ++++++++++--------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/packages/identity/src/adapters/drizzle-user.adapter.ts b/packages/identity/src/adapters/drizzle-user.adapter.ts index 0499ec78..06edb5b3 100644 --- a/packages/identity/src/adapters/drizzle-user.adapter.ts +++ b/packages/identity/src/adapters/drizzle-user.adapter.ts @@ -105,22 +105,10 @@ export class DrizzleUserAdapter implements IUserProvider { const limit = Math.max(1, Number(options?.limit) || 10); const offset = (page - 1) * limit; - const filters = []; - if (options?.search) { - filters.push( - ilike(schema.user.email, `%${options.search}%`), - // OR name search if needed, but keeping simple for now - ); - } - - if (options?.systemRole) { - filters.push(eq(schema.user.systemRole, options.systemRole)); - } + const filters = this.buildUserFilters(options); if (options?.tenantId) { // Tenant-scoped (requires Join) - filters.push(eq(schema.member.organizationId, options.tenantId)); - const dataQuery = this.db .select({ user: schema.user }) .from(schema.user) @@ -169,18 +157,9 @@ export class DrizzleUserAdapter implements IUserProvider { search?: string; systemRole?: string; }): Promise { - const whereConditions = []; - - if (filters?.search) { - whereConditions.push(ilike(schema.user.email, `%${filters.search}%`)); - } - - if (filters?.systemRole) { - whereConditions.push(eq(schema.user.systemRole, filters.systemRole)); - } + const whereConditions = this.buildUserFilters(filters); if (filters?.tenantId) { - whereConditions.push(eq(schema.member.organizationId, filters.tenantId)); const [result] = await this.db .select({ count: count(schema.user.id) }) .from(schema.user) @@ -197,6 +176,28 @@ export class DrizzleUserAdapter implements IUserProvider { return Number(result?.count || 0); } + private buildUserFilters(filters?: { + tenantId?: string; + search?: string; + systemRole?: string; + }) { + const whereConditions = []; + + if (filters?.search) { + whereConditions.push(ilike(schema.user.email, `%${filters.search}%`)); + } + + if (filters?.systemRole) { + whereConditions.push(eq(schema.user.systemRole, filters.systemRole)); + } + + if (filters?.tenantId) { + whereConditions.push(eq(schema.member.organizationId, filters.tenantId)); + } + + return whereConditions; + } + async forceVerifyEmail(userId: string): Promise { await this.db .update(schema.user) From 4bca9b3d3fc2c43283712f22352f49e24c4ec6e6 Mon Sep 17 00:00:00 2001 From: Pramod Date: Sun, 25 Jan 2026 12:38:24 +0530 Subject: [PATCH 10/13] refactor(identity): simplify createUser and final verification updates --- packages/identity/src/adapters/drizzle-user.adapter.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/identity/src/adapters/drizzle-user.adapter.ts b/packages/identity/src/adapters/drizzle-user.adapter.ts index 06edb5b3..3fabd6d8 100644 --- a/packages/identity/src/adapters/drizzle-user.adapter.ts +++ b/packages/identity/src/adapters/drizzle-user.adapter.ts @@ -22,9 +22,7 @@ export class DrizzleUserAdapter implements IUserProvider { // If systemRole is provided, we need to update the user record immediately // because the Auth Provider might not support custom fields during creation if (input.systemRole) { - await this.update(user.id, { systemRole: input.systemRole }); - const updated = await this.findById(user.id); - if (updated) return updated; + return this.update(user.id, { systemRole: input.systemRole }); } return user; From bdf29cd5bf89a92d3387839bf760615f1af72aa1 Mon Sep 17 00:00:00 2001 From: Pramod Date: Sun, 25 Jan 2026 14:41:21 +0530 Subject: [PATCH 11/13] refactor(identity): enhance atomicity and pagination safety --- .../identity/src/adapters/drizzle-tenant.adapter.ts | 4 ++-- .../identity/src/adapters/drizzle-user.adapter.ts | 12 +++++++++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/packages/identity/src/adapters/drizzle-tenant.adapter.ts b/packages/identity/src/adapters/drizzle-tenant.adapter.ts index e69da4ba..4fc94d22 100644 --- a/packages/identity/src/adapters/drizzle-tenant.adapter.ts +++ b/packages/identity/src/adapters/drizzle-tenant.adapter.ts @@ -174,8 +174,8 @@ export class DrizzleTenantAdapter implements ITenantProvider { limit?: number; search?: string; }): Promise<{ data: TenantInterface[]; total: number }> { - const page = Math.max(1, Number(options?.page) || 1); - const limit = Math.max(1, Number(options?.limit) || 10); + const page = Math.max(1, Math.floor(Number(options?.page) || 1)); + const limit = Math.max(1, Math.floor(Number(options?.limit) || 10)); const offset = (page - 1) * limit; const filters = []; diff --git a/packages/identity/src/adapters/drizzle-user.adapter.ts b/packages/identity/src/adapters/drizzle-user.adapter.ts index 3fabd6d8..1886b15f 100644 --- a/packages/identity/src/adapters/drizzle-user.adapter.ts +++ b/packages/identity/src/adapters/drizzle-user.adapter.ts @@ -22,7 +22,13 @@ export class DrizzleUserAdapter implements IUserProvider { // If systemRole is provided, we need to update the user record immediately // because the Auth Provider might not support custom fields during creation if (input.systemRole) { - return this.update(user.id, { systemRole: input.systemRole }); + try { + return await this.update(user.id, { systemRole: input.systemRole }); + } catch (error) { + // Compensating transaction: delete user if role update fails to maintain consistency + await this.delete(user.id); + throw error; + } } return user; @@ -99,8 +105,8 @@ export class DrizzleUserAdapter implements IUserProvider { tenantId?: string; systemRole?: string; }): Promise<{ data: UserInterface[]; total: number }> { - const page = Math.max(1, Number(options?.page) || 1); - const limit = Math.max(1, Number(options?.limit) || 10); + const page = Math.max(1, Math.floor(Number(options?.page) || 1)); + const limit = Math.max(1, Math.floor(Number(options?.limit) || 10)); const offset = (page - 1) * limit; const filters = this.buildUserFilters(options); From f95e25e31495a55c8f533208c8fe587d17519f12 Mon Sep 17 00:00:00 2001 From: Pramod Date: Sun, 25 Jan 2026 15:34:56 +0530 Subject: [PATCH 12/13] fix(identity): resolve compilation errors and lint issues in adapters --- .../src/adapters/drizzle-tenant.adapter.ts | 88 ++++++++++--------- .../src/adapters/drizzle-user.adapter.ts | 14 ++- 2 files changed, 58 insertions(+), 44 deletions(-) diff --git a/packages/identity/src/adapters/drizzle-tenant.adapter.ts b/packages/identity/src/adapters/drizzle-tenant.adapter.ts index 4fc94d22..b9a56141 100644 --- a/packages/identity/src/adapters/drizzle-tenant.adapter.ts +++ b/packages/identity/src/adapters/drizzle-tenant.adapter.ts @@ -61,77 +61,78 @@ export class DrizzleTenantAdapter implements ITenantProvider { slug: string; logo?: string | null; }): Promise { + const slug = input.slug.trim(); + if (!slug) { + throw new Error("Tenant slug is required"); + } + try { const [org] = await this.db .insert(schema.organization) .values({ id: uuidv4(), name: input.name, - slug: input.slug, + slug, logo: input.logo, createdAt: new Date(), status: "active", }) .returning(); + return this.mapTenant(org); } catch (error: any) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call - if (error.code === "23505" && error.detail?.includes("slug")) { - throw new Error("Tenant slug already exists"); + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + if (error.code === "23505") { + throw new Error("Tenant with this slug already exists"); } throw error; } } async update(id: string, input: UpdateTenantInput): Promise { - const updatePayload: Partial = {}; - if (input.name !== undefined) updatePayload.name = input.name; - if (input.slug !== undefined) updatePayload.slug = input.slug; - if (input.logo !== undefined) updatePayload.logo = input.logo; - if (input.status !== undefined) updatePayload.status = input.status; - if (input.metadata !== undefined) - updatePayload.metadata = JSON.stringify(input.metadata); + // If slug is updated, check uniqueness + if (input.slug) { + const existing = await this.findBySlug(input.slug); + if (existing && existing.id !== id) { + throw new Error("Tenant with this slug already exists"); + } + } - try { - const [updated] = await this.db - .update(schema.organization) - .set({ ...updatePayload, updatedAt: new Date() }) - .where(eq(schema.organization.id, id)) - .returning(); + const { metadata, ...rest } = input; - if (!updated) throw new Error("Tenant not found"); - return this.mapTenant(updated); - } catch (error: any) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call - if (error.code === "23505" && error.detail?.includes("slug")) { - throw new Error("Tenant slug already exists"); - } - throw error; + const [updated] = await this.db + .update(schema.organization) + .set({ + ...rest, + ...(metadata ? { metadata: JSON.stringify(metadata) } : {}), + updatedAt: new Date(), + }) + .where(eq(schema.organization.id, id)) + .returning(); + + if (!updated) { + throw new Error("Tenant not found"); } + + return this.mapTenant(updated); } async delete(id: string): Promise { await this.db.transaction(async (tx) => { - // 1. Check existence first - const [existing] = await tx - .select({ id: schema.organization.id }) - .from(schema.organization) - .where(eq(schema.organization.id, id)); - - if (!existing) { - throw new Error("Tenant not found"); - } - - // 2. Proceed with deletes await tx .delete(schema.member) .where(eq(schema.member.organizationId, id)); await tx .delete(schema.invitation) .where(eq(schema.invitation.organizationId, id)); - await tx + const [deleted] = await tx .delete(schema.organization) - .where(eq(schema.organization.id, id)); + .where(eq(schema.organization.id, id)) + .returning({ id: schema.organization.id }); + + if (!deleted) { + throw new Error("Tenant not found"); + } }); } @@ -183,21 +184,26 @@ export class DrizzleTenantAdapter implements ITenantProvider { filters.push(ilike(schema.organization.name, `%${options.search}%`)); } - const data = await this.db + const dataQuery = this.db .select() .from(schema.organization) .where(filters.length ? and(...filters) : undefined) .limit(limit) .offset(offset) - .orderBy(desc(schema.organization.createdAt)); + .orderBy( + desc(schema.organization.createdAt), + desc(schema.organization.id), + ); const [countResult] = await this.db .select({ count: count(schema.organization.id) }) .from(schema.organization) .where(filters.length ? and(...filters) : undefined); + const tenants = await dataQuery; + return { - data: data.map((d) => this.mapTenant(d)), + data: tenants.map((t) => this.mapTenant(t)), total: Number(countResult?.count || 0), }; } diff --git a/packages/identity/src/adapters/drizzle-user.adapter.ts b/packages/identity/src/adapters/drizzle-user.adapter.ts index 1886b15f..dd1a28fb 100644 --- a/packages/identity/src/adapters/drizzle-user.adapter.ts +++ b/packages/identity/src/adapters/drizzle-user.adapter.ts @@ -25,8 +25,12 @@ export class DrizzleUserAdapter implements IUserProvider { try { return await this.update(user.id, { systemRole: input.systemRole }); } catch (error) { - // Compensating transaction: delete user if role update fails to maintain consistency - await this.delete(user.id); + // Compensating transaction: best‑effort cleanup without masking the root cause + try { + await this.delete(user.id); + } catch (cleanupError) { + void cleanupError; // optional: log cleanupError + } throw error; } } @@ -106,7 +110,11 @@ export class DrizzleUserAdapter implements IUserProvider { systemRole?: string; }): Promise<{ data: UserInterface[]; total: number }> { const page = Math.max(1, Math.floor(Number(options?.page) || 1)); - const limit = Math.max(1, Math.floor(Number(options?.limit) || 10)); + const MAX_LIMIT = 100; + const limit = Math.min( + MAX_LIMIT, + Math.max(1, Math.floor(Number(options?.limit) || 10)), + ); const offset = (page - 1) * limit; const filters = this.buildUserFilters(options); From 2733da9f9f8f05f85670a9d518891a278ff0a1fd Mon Sep 17 00:00:00 2001 From: Pramod Date: Sun, 25 Jan 2026 17:44:03 +0530 Subject: [PATCH 13/13] fix(identity): resolve TOCTOU race condition and improve slug validation in tenant adapter --- .../src/adapters/drizzle-tenant.adapter.ts | 45 +++++++++++-------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/packages/identity/src/adapters/drizzle-tenant.adapter.ts b/packages/identity/src/adapters/drizzle-tenant.adapter.ts index b9a56141..92a21b9d 100644 --- a/packages/identity/src/adapters/drizzle-tenant.adapter.ts +++ b/packages/identity/src/adapters/drizzle-tenant.adapter.ts @@ -90,31 +90,40 @@ export class DrizzleTenantAdapter implements ITenantProvider { } async update(id: string, input: UpdateTenantInput): Promise { - // If slug is updated, check uniqueness - if (input.slug) { - const existing = await this.findBySlug(input.slug); - if (existing && existing.id !== id) { - throw new Error("Tenant with this slug already exists"); + // Validate slug if provided + if (input.slug !== undefined) { + const trimmedSlug = input.slug.trim(); + if (!trimmedSlug) { + throw new Error("Tenant slug cannot be empty"); } + input = { ...input, slug: trimmedSlug }; } const { metadata, ...rest } = input; - const [updated] = await this.db - .update(schema.organization) - .set({ - ...rest, - ...(metadata ? { metadata: JSON.stringify(metadata) } : {}), - updatedAt: new Date(), - }) - .where(eq(schema.organization.id, id)) - .returning(); + try { + const [updated] = await this.db + .update(schema.organization) + .set({ + ...rest, + ...(metadata ? { metadata: JSON.stringify(metadata) } : {}), + updatedAt: new Date(), + }) + .where(eq(schema.organization.id, id)) + .returning(); - if (!updated) { - throw new Error("Tenant not found"); - } + if (!updated) { + throw new Error("Tenant not found"); + } - return this.mapTenant(updated); + return this.mapTenant(updated); + } catch (error: any) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call + if (error.code === "23505" && error.detail?.includes("slug")) { + throw new Error("Tenant with this slug already exists"); + } + throw error; + } } async delete(id: string): Promise {