fix: audit remediation — SSE user scoping, FK transactional safety, UI error handling
- H4: scoped SSE import progress to exact userId match; non-HTTP events excluded from all subscribers - H2: moved PRAGMA foreign_key_check inside SQLite transaction before COMMIT; violations rollback preserving old tables - M1: removed dead axios-style error branch from extractErrorMessage (interceptor already unwraps) - M2: split handleSave try/catch — save errors vs reload errors shown distinctly - M3: added provider field validation before AI config test request - Added SSE scoping regression tests (import service + controller) - Added FK check failure rollback test (database-migrations.spec) - Updated controller spec expectations for userId parameter Co-authored-by: Code Review <branch-review>
This commit is contained in:
@@ -18,7 +18,7 @@ function mockRunner(overrides: {
|
||||
} = {}) {
|
||||
const release = jest.fn();
|
||||
const connect = jest.fn();
|
||||
const query = jest.fn();
|
||||
const query = jest.fn().mockResolvedValue([]);
|
||||
const getTables = jest.fn().mockResolvedValue(overrides.getTables ?? []);
|
||||
const getTable = jest.fn().mockResolvedValue(
|
||||
overrides.getTable ?? { name: 'ai_config', columns: [] },
|
||||
@@ -31,19 +31,21 @@ function mockRunner(overrides: {
|
||||
return { release, connect, query, getTables, getTable };
|
||||
}
|
||||
|
||||
function createDataSource(runner: ReturnType<typeof mockRunner>) {
|
||||
function createDataSource(runner: ReturnType<typeof mockRunner>, dbType: string = 'better-sqlite3') {
|
||||
return {
|
||||
options: { type: 'better-sqlite3' },
|
||||
options: { type: dbType },
|
||||
createQueryRunner: jest.fn().mockReturnValue(runner),
|
||||
transaction: jest.fn(),
|
||||
};
|
||||
}
|
||||
|
||||
// Type to reach the private ensureAiConfigTable for testing
|
||||
// Type to reach private migration methods for testing
|
||||
interface MigrationsPrivate {
|
||||
ensureAiConfigTable(): Promise<void>;
|
||||
backfillOrganizations(): Promise<void>;
|
||||
normalizeClassDates(): Promise<void>;
|
||||
ensureCourseAttendanceSchema(): Promise<void>;
|
||||
protectAttendanceHistory(): Promise<void>;
|
||||
}
|
||||
|
||||
describe('DatabaseMigrationsService — ensureAiConfigTable', () => {
|
||||
@@ -176,3 +178,251 @@ describe('DatabaseMigrationsService — bootstrap failure handling', () => {
|
||||
expect(normalize).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('DatabaseMigrationsService — course attendance schema', () => {
|
||||
it('adds schedule linkage columns to an existing attendance_records table', async () => {
|
||||
const runner = mockRunner({
|
||||
getTables: [
|
||||
{ name: 'attendance_records', columns: [{ name: 'id' }] },
|
||||
{ name: 'attendance_sessions', columns: [{ name: 'id' }] },
|
||||
],
|
||||
getTable: { name: 'attendance_records', columns: [{ name: 'id' }] },
|
||||
});
|
||||
await bootstrapCourseAttendance(runner);
|
||||
|
||||
await service.ensureCourseAttendanceSchema();
|
||||
|
||||
expect(runner.query).toHaveBeenCalledWith(
|
||||
expect.stringContaining('ALTER TABLE attendance_records ADD COLUMN schedule_id INTEGER'),
|
||||
);
|
||||
expect(runner.query).toHaveBeenCalledWith(
|
||||
expect.stringContaining('ALTER TABLE attendance_records ADD COLUMN attendance_session_id INTEGER'),
|
||||
);
|
||||
expect(runner.release).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('creates attendance_sessions with FK RESTRICT constraints when table is missing', async () => {
|
||||
const runner = mockRunner({
|
||||
getTables: [{ name: 'attendance_records', columns: [{ name: 'id' }] }],
|
||||
getTable: { name: 'attendance_records', columns: [{ name: 'id' }] },
|
||||
});
|
||||
await bootstrapCourseAttendance(runner);
|
||||
await service.ensureCourseAttendanceSchema();
|
||||
|
||||
const createSql: string = (runner.query as jest.Mock).mock.calls
|
||||
.map((c: unknown[]) => (typeof c[0] === 'string' ? c[0] : ''))
|
||||
.find((s: string) => s.includes('CREATE TABLE attendance_sessions')) ?? '';
|
||||
expect(createSql).toContain('FOREIGN KEY (schedule_id) REFERENCES class_schedule(id) ON DELETE RESTRICT');
|
||||
expect(createSql).toContain('FOREIGN KEY (class_id) REFERENCES classes(id) ON DELETE RESTRICT');
|
||||
});
|
||||
});
|
||||
|
||||
describe('DatabaseMigrationsService — protectAttendanceHistory', () => {
|
||||
let service: MigrationsPrivate & DatabaseMigrationsService;
|
||||
|
||||
async function bootstrap(runner: ReturnType<typeof mockRunner>, dbType: string = 'better-sqlite3') {
|
||||
const dataSource = createDataSource(runner, dbType);
|
||||
const module: TestingModule = await Test.createTestingModule({
|
||||
providers: [
|
||||
DatabaseMigrationsService,
|
||||
{ provide: getDataSourceToken(), useValue: dataSource },
|
||||
],
|
||||
}).compile();
|
||||
service = module.get(DatabaseMigrationsService) as DatabaseMigrationsService & MigrationsPrivate;
|
||||
}
|
||||
|
||||
it('skips when attendance_sessions table is absent', async () => {
|
||||
const runner = mockRunner({ getTables: [] });
|
||||
await bootstrap(runner);
|
||||
await service.protectAttendanceHistory();
|
||||
expect(runner.query).not.toHaveBeenCalled();
|
||||
expect(runner.release).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('SQLite: exits early when FKs already exist', async () => {
|
||||
const runner = mockRunner({
|
||||
getTables: [{ name: 'attendance_sessions', columns: [{ name: 'id' }] }],
|
||||
});
|
||||
runner.query.mockResolvedValueOnce([{ id: 0 }]); // PRAGMA foreign_key_list returns rows
|
||||
await bootstrap(runner);
|
||||
await service.protectAttendanceHistory();
|
||||
|
||||
// Should not run any TABLE creation (rebuild)
|
||||
const queries: string[] = (runner.query as jest.Mock).mock.calls
|
||||
.map((c: unknown[]) => (typeof c[0] === 'string' ? c[0] : ''));
|
||||
expect(queries.filter((q: string) => q.includes('CREATE TABLE'))).toHaveLength(0);
|
||||
expect(runner.release).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('SQLite: rebuilds table with FK constraints when FKs are absent', async () => {
|
||||
const runner = mockRunner({
|
||||
getTables: [{ name: 'attendance_sessions', columns: [{ name: 'id' }] }],
|
||||
});
|
||||
// PRAGMA foreign_key_list for attendance_sessions → empty
|
||||
runner.query.mockResolvedValueOnce([]);
|
||||
// PRAGMA foreign_key_list for attendance_records → also empty (no FK yet)
|
||||
runner.query.mockResolvedValueOnce([]);
|
||||
await bootstrap(runner);
|
||||
await service.protectAttendanceHistory();
|
||||
|
||||
const queries: string[] = (runner.query as jest.Mock).mock.calls
|
||||
.map((c: unknown[]) => (typeof c[0] === 'string' ? c[0] : ''));
|
||||
|
||||
// PRAGMA foreign_keys = OFF outside the transaction
|
||||
expect(queries.some((q: string) => q.includes('PRAGMA foreign_keys = OFF'))).toBe(true);
|
||||
expect(queries.some((q: string) => q.includes('CREATE TABLE attendance_sessions_new'))).toBe(true);
|
||||
expect(queries.some((q: string) =>
|
||||
q.includes('FOREIGN KEY (schedule_id) REFERENCES class_schedule(id) ON DELETE RESTRICT')
|
||||
)).toBe(true);
|
||||
expect(queries.some((q: string) =>
|
||||
q.includes('FOREIGN KEY (class_id) REFERENCES classes(id) ON DELETE RESTRICT')
|
||||
)).toBe(true);
|
||||
expect(queries.some((q: string) => q.includes('INSERT INTO attendance_sessions_new'))).toBe(true);
|
||||
expect(queries.some((q: string) => q.includes('DROP TABLE attendance_sessions'))).toBe(true);
|
||||
expect(queries.some((q: string) => q.includes('RENAME TO attendance_sessions'))).toBe(true);
|
||||
expect(queries.some((q: string) => q.includes('uq_attendance_session_schedule_date'))).toBe(true);
|
||||
// attendance_records rebuilt with FK
|
||||
expect(queries.some((q: string) => q.includes('CREATE TABLE attendance_records_new'))).toBe(true);
|
||||
expect(queries.some((q: string) => q.includes('INSERT INTO attendance_records_new'))).toBe(true);
|
||||
expect(queries.some((q: string) => q.includes('DROP TABLE attendance_records'))).toBe(true);
|
||||
expect(queries.some((q: string) => q.includes('uq_attendance_session_student'))).toBe(true);
|
||||
// PRAGMA foreign_keys restored to ON and foreign_key_check runs
|
||||
expect(queries.some((q: string) => q.includes('PRAGMA foreign_keys = ON'))).toBe(true);
|
||||
expect(queries.some((q: string) => q.includes('PRAGMA foreign_key_check'))).toBe(true);
|
||||
expect(runner.release).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('SQLite: rolls back transaction when foreign_key_check finds violations', async () => {
|
||||
const runner = mockRunner({
|
||||
getTables: [{ name: 'attendance_sessions', columns: [{ name: 'id' }] }],
|
||||
});
|
||||
// Use mockImplementation to match by SQL content, not call position
|
||||
runner.query.mockImplementation((sql: string) => {
|
||||
if (typeof sql === 'string' && sql.includes('PRAGMA foreign_key_list')) {
|
||||
return Promise.resolve([]); // FKs absent → trigger rebuild
|
||||
}
|
||||
if (typeof sql === 'string' && sql.includes('PRAGMA foreign_key_check')) {
|
||||
return Promise.resolve([
|
||||
{ table: 'attendance_sessions', rowid: 42, parent: 'class_schedule', fkid: 0 },
|
||||
]);
|
||||
}
|
||||
return Promise.resolve([]);
|
||||
});
|
||||
await bootstrap(runner);
|
||||
|
||||
await expect(service.protectAttendanceHistory()).rejects.toThrow(
|
||||
/外键一致性检查失败/,
|
||||
);
|
||||
|
||||
const queries: string[] = (runner.query as jest.Mock).mock.calls
|
||||
.map((c: unknown[]) => (typeof c[0] === 'string' ? c[0] : ''));
|
||||
|
||||
// The transaction should have been rolled back (ROLLBACK called)
|
||||
expect(queries.some((q: string) => q.includes('ROLLBACK'))).toBe(true);
|
||||
// COMMIT should NOT have been called
|
||||
expect(queries.some((q: string) => q.trim() === 'COMMIT')).toBe(false);
|
||||
// PRAGMA foreign_keys should still be restored
|
||||
expect(queries.some((q: string) => q.includes('PRAGMA foreign_keys = ON'))).toBe(true);
|
||||
expect(runner.release).toHaveBeenCalled();
|
||||
});
|
||||
it('MySQL: drops old FKs and recreates both schedule_id and class_id as RESTRICT', async () => {
|
||||
const runner = mockRunner({
|
||||
getTables: [{ name: 'attendance_sessions', columns: [{ name: 'id' }] }],
|
||||
});
|
||||
// Mock: override SELECT CONSTRAINT_NAME and REFERENTIAL_CONSTRAINTS queries
|
||||
runner.query.mockImplementation((sql: string, params?: string[]) => {
|
||||
if (typeof sql === 'string' && sql.includes('INFORMATION_SCHEMA.KEY_COLUMN_USAGE')) {
|
||||
if (params?.[0] === 'schedule_id') {
|
||||
return Promise.resolve([{ CONSTRAINT_NAME: 'fk_schedule_cascade' }]);
|
||||
}
|
||||
if (params?.[0] === 'class_id') {
|
||||
return Promise.resolve([{ CONSTRAINT_NAME: 'fk_class_cascade' }]);
|
||||
}
|
||||
}
|
||||
// REFERENTIAL_CONSTRAINTS check — constraint does not yet exist
|
||||
if (typeof sql === 'string' && sql.includes('INFORMATION_SCHEMA.REFERENTIAL_CONSTRAINTS')) {
|
||||
return Promise.resolve([]);
|
||||
}
|
||||
return Promise.resolve([]);
|
||||
});
|
||||
await bootstrap(runner, 'mysql');
|
||||
await service.protectAttendanceHistory();
|
||||
|
||||
const queries: string[] = (runner.query as jest.Mock).mock.calls
|
||||
.map((c: unknown[]) => (typeof c[0] === 'string' ? c[0] : ''));
|
||||
|
||||
// Drops old FKs
|
||||
expect(queries.some((q: string) => q.includes('DROP FOREIGN KEY `fk_schedule_cascade`'))).toBe(true);
|
||||
expect(queries.some((q: string) => q.includes('DROP FOREIGN KEY `fk_class_cascade`'))).toBe(true);
|
||||
// Checks REFERENTIAL_CONSTRAINTS before ADD
|
||||
expect(queries.some((q: string) =>
|
||||
q.includes('INFORMATION_SCHEMA.REFERENTIAL_CONSTRAINTS')
|
||||
)).toBe(true);
|
||||
// Creates new RESTRICT FKs
|
||||
expect(queries.some((q: string) =>
|
||||
q.includes('ADD CONSTRAINT fk_as_schedule_protect') && q.includes('ON DELETE RESTRICT')
|
||||
)).toBe(true);
|
||||
expect(queries.some((q: string) =>
|
||||
q.includes('ADD CONSTRAINT fk_as_class_protect') && q.includes('ON DELETE RESTRICT')
|
||||
)).toBe(true);
|
||||
expect(runner.release).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('MySQL: throws when ADD CONSTRAINT RESTRICT fails', async () => {
|
||||
const runner = mockRunner({
|
||||
getTables: [{ name: 'attendance_sessions', columns: [{ name: 'id' }] }],
|
||||
});
|
||||
const addError = new Error('Cannot add foreign key constraint');
|
||||
runner.query.mockImplementation((sql: string, params?: string[]) => {
|
||||
if (typeof sql === 'string' && sql.includes('INFORMATION_SCHEMA.KEY_COLUMN_USAGE')) {
|
||||
return Promise.resolve([]);
|
||||
}
|
||||
if (typeof sql === 'string' && sql.includes('INFORMATION_SCHEMA.REFERENTIAL_CONSTRAINTS')) {
|
||||
return Promise.resolve([]);
|
||||
}
|
||||
if (typeof sql === 'string' && sql.includes('ADD CONSTRAINT')) {
|
||||
return Promise.reject(addError);
|
||||
}
|
||||
return Promise.resolve([]);
|
||||
});
|
||||
await bootstrap(runner, 'mysql');
|
||||
await expect(service.protectAttendanceHistory()).rejects.toThrow('Cannot add foreign key constraint');
|
||||
expect(runner.release).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('MySQL: skips ADD when RESTRICT constraint already confirmed via information_schema', async () => {
|
||||
const runner = mockRunner({
|
||||
getTables: [{ name: 'attendance_sessions', columns: [{ name: 'id' }] }],
|
||||
});
|
||||
runner.query.mockImplementation((sql: string, params?: string[]) => {
|
||||
if (typeof sql === 'string' && sql.includes('INFORMATION_SCHEMA.KEY_COLUMN_USAGE')) {
|
||||
return Promise.resolve([]);
|
||||
}
|
||||
// REFERENTIAL_CONSTRAINTS confirms RESTRICT already present
|
||||
if (typeof sql === 'string' && sql.includes('INFORMATION_SCHEMA.REFERENTIAL_CONSTRAINTS')) {
|
||||
return Promise.resolve([{ DELETE_RULE: 'RESTRICT' }]);
|
||||
}
|
||||
return Promise.resolve([]);
|
||||
});
|
||||
await bootstrap(runner, 'mysql');
|
||||
await service.protectAttendanceHistory();
|
||||
|
||||
const queries: string[] = (runner.query as jest.Mock).mock.calls
|
||||
.map((c: unknown[]) => (typeof c[0] === 'string' ? c[0] : ''));
|
||||
|
||||
// No ADD CONSTRAINT calls
|
||||
expect(queries.filter((q: string) => q.includes('ADD CONSTRAINT')).length).toBe(0);
|
||||
expect(runner.release).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
async function bootstrapCourseAttendance(runner: ReturnType<typeof mockRunner>) {
|
||||
const dataSource = createDataSource(runner);
|
||||
const module: TestingModule = await Test.createTestingModule({
|
||||
providers: [
|
||||
DatabaseMigrationsService,
|
||||
{ provide: getDataSourceToken(), useValue: dataSource },
|
||||
],
|
||||
}).compile();
|
||||
service = module.get(DatabaseMigrationsService) as DatabaseMigrationsService & MigrationsPrivate;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user