Skip to content

Commit b347f5b

Browse files
Yerazeclaude
andauthored
test(routes): integration-grade route-test harness with real auth middleware (#3962) (#3986)
* feat(tests): add createRouteTestApp() integration-grade route test harness Introduces src/server/test-helpers/routeTestApp.ts — a real-middleware harness for route tests that replaces the monkey-patch pattern (vi.mock('../../services/database.js', ...)). Key design decisions: - Uses the live DatabaseService singleton (already :memory: + migrations) so authMiddleware sees the same data as the harness — a second createTestDb() connection would be invisible to the middleware. - Real express-session (MemoryStore) + real optionalAuth() + real requirePermission() — the full auth/permission chain executes. - Programmatic login via POST /__test__/login (no bcrypt) keeps tests fast. - Per-source grants via db.auth.createPermission with sourceId so real checkPermissionAsync enforces isolation. - Unique per-call usernames (rt-admin-N, rt-limited-N) prevent collisions in the file-persistent singleton DB. - cleanup() removes permissions + sources in afterEach. Part of remediation epic #3962, Phase 1 Task 1.3 WP-A. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AphLfgUJWaFNAgU5UXdJ4n * test(routes): convert sourceRoutes.permissions to real-middleware harness (WP-A) Removes the vi.mock('../../services/database.js') block and the hand-rolled checkPermissionAsync lambda that re-implemented per-source scoping logic in the test itself. Replaces with createRouteTestApp() so the real checkPermissionAsync + permissions.sourceId SQL enforces isolation. Changes: - Delete vi.mock of services/database.js and mockDb alias - Replace createApp() + hand-rolled session middleware with harness - beforeEach: seed real permission rows via harness.grant() - afterEach: harness.cleanup() removes perms + sources - Keep vi.mock of sourceManagerRegistry + meshtasticManager (non-DB, correct) New assertions added: - admin bypass: loginAs(admin) → 200 on both sources without explicit grants - anonymous fallback: loginAs(null) → 403 (real findUserByUsernameAsync path) - source isolation proven: limited user granted on sourceA gets 200/200 on A and 403/403 on B, driven purely by real SQL rows (no mock lambda) - channel filtering regression: same node seeded on both sources; user with channel_0:viewOnMap only on sourceA sees node on A (len≥1), filtered on B (len=0), driven by real getUserPermissionSetAsync per-source scoping Fixes the gap where the previous mock passed while the real SQL was broken (issue #3745 root cause). Part of remediation epic #3962, Phase 1 Task 1.3 WP-A. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AphLfgUJWaFNAgU5UXdJ4n * docs: document createRouteTestApp() as standard for new route tests Adds a 'Route Test Harness' subsection to the 'Test Mocking Pattern' section in CLAUDE.md explaining when and why to use the harness vs the legacy monkey-patch pattern, and pointing to sourceRoutes.permissions.test.ts as the canonical template. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AphLfgUJWaFNAgU5UXdJ4n * fix(tests): poll for anonymous user after waitForReady() in routeTestApp DatabaseService.ensureAnonymousUser() is a fire-and-forget async task launched from the constructor; readyResolve() fires synchronously before it completes. Under vitest singleFork+isolate a new module instance (and therefore a new :memory: SQLite) is created for each file, so the constructor re-runs and the first beforeEach may reach getUserByUsername before ensureAnonymousUser commits the row. Add a 2-second polling loop (25 ms intervals) after waitForReady() so the harness waits for the async seeding to land before asserting the anonymous row exists. Resolves the intermittent race that caused all channelDatabaseRoutes + waypoints tests to fail in the combined 3-file run. * fix(routes): use ALL_SOURCES sentinel when sourceId missing in v1 messages withSourceScope() now throws when given undefined (introduced in commit 64081b6 — "withSourceScope fails closed"). getMessages() and getMessagesByChannel() did not apply the ?? ALL_SOURCES guard that getMessagesAfterTimestamp() already carried, so a GET /v1/messages without a sourceId query-param produced an unhandled 500. Add the ?? ALL_SOURCES fallback to both call sites; the comment "intentional cross-source when sourceId omitted" matches the pattern already established by the existing guard on the third path. Exposed by the real-harness conversion of messages.permissions.test.ts. * test(routes): convert v1 messages permissions to real-middleware harness (WP-B) Remove vi.mock('../../services/database.js') and replace with createRouteTestApp(useOptionalAuth: false) + req.user injection (mirrors the requireAPIToken pattern used in production). vi.spyOn passthroughs on checkPermissionAsync and getUserPermissionSetAsync record call arguments while the real SQL logic runs — proving both the routing path taken (per-source vs global) and the actual permission enforcement (no hand-rolled lambda). Source-isolation assertion: limited user holds messages:read + channel_0:read on rt-source-a only. GET ?sourceId=rt-source-b returns 200 with count=0 because every getAccessibleChannels call returns false from real checkPermissionAsync — previously a () => sourceId==='rt-source-a' mock would have hidden any regression (issue #3745). Also revealed and fixed a production bug: getMessages() and getMessagesByChannel() were missing the ?? ALL_SOURCES sentinel, causing a 500 on requests without a sourceId param (see companion fix commit). * test(routes): convert channelDatabaseRoutes permissions to real-middleware harness (WP-B) Remove vi.mock('../../services/database.js') and vi.mock('../auth/authMiddleware.js') blocks. Real requireAuth() reads session.userId set by harness.loginAs(); real checkPermissionAsync enforces the permission gate against live DB rows. Non-DB service mocks kept: channelDecryptionService and retroactiveDecryptionService avoid file/network I/O in tests. Data repo methods (getAllAsync, getByIdAsync, etc.) are spied on per-test to control returned rows while auth still runs against the real DB. Source-isolation assertion: channel_database is a global-by-design resource (no sourceId column). The isolation proof here is therefore grant/no-grant: user WITH global channel_database:read gets 200; user WITHOUT gets 403 from real checkPermissionAsync (no NULL-sourceId row → false). Previously mockResolvedValue(false) could mask an implementation regression. 11 tests across 4 describe blocks covering read/write permission filtering, per-entry PSK masking, admin bypass, and the isolated source-isolation pair. * test(routes): convert waypoints to real-middleware harness (WP-B) Remove vi.mock('../../services/database.js'). Real requirePermission() calls checkPermissionAsync against live DB rows seeded per-test. Non-DB mocks kept: sourceManagerRegistry (avoids TCP) and waypointService (avoids file I/O). databaseService.waypoints.getAsync is spied on in DELETE tests where no rows are seeded. Source-isolation assertion: limited user holds waypoints:read only on rt-source-a. GET rt-source-a → 200; GET rt-source-b → 403 from real checkPermissionAsync (no row for source-b). Write-success tests use harness.admin (real admin bypass in requirePermission) rather than stacking a second grant() call on the same limited user, which would violate the UNIQUE constraint on (userId, resource, sourceId) and raise a DB error. Unauthenticated test updated from expected 401 to 403: the real anonymous user exists in the DB, so findUserByUsernameAsync('anonymous') succeeds and requirePermission falls through to a false check (no grant), returning 403 rather than the mock-induced 401 of the old test. 16 tests across 4 describe blocks covering GET/POST/PATCH/DELETE with permission, admin-bypass, source-isolation, and input-validation scenarios. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent b4bf3eb commit b347f5b

7 files changed

Lines changed: 824 additions & 414 deletions

File tree

CLAUDE.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,23 @@ Mock async methods used by `authMiddleware`:
9090
```
9191
For per-source permission tests, mock `getUserPermissionSetAsync(userId, sourceId)` to return source-specific resource maps.
9292

93+
### Route Test Harness (preferred for new/changed route tests)
94+
For route tests that exercise `requirePermission` / `optionalAuth`, use `createRouteTestApp()` (`src/server/test-helpers/routeTestApp.ts`) instead of monkey-patching `checkPermissionAsync` or mocking the whole `services/database.js` module. The harness wires real express-session (MemoryStore) + real auth middleware against the singleton's `:memory:` SQLite DB, and seeds per-test permission rows so that real SQL enforces isolation.
95+
96+
```typescript
97+
let harness: RouteTestHarness;
98+
beforeEach(async () => {
99+
harness = await createRouteTestApp({ mount: app => app.use('/', myRouter) });
100+
await harness.grant(harness.limited.id, 'nodes', 'read', harness.sourceA);
101+
});
102+
afterEach(() => harness.cleanup());
103+
```
104+
105+
- **New or changed route tests MUST use the harness.** Legacy tests using `vi.mock('../../services/database.js', ...)` convert opportunistically when the file is touched.
106+
- The monkey-patch pattern (`vi.mock(...)` + fake `checkPermissionAsync` lambda) is deprecated for route tests. A fake that re-implements the logic under test cannot catch regressions in that logic.
107+
- See `src/server/routes/sourceRoutes.permissions.test.ts` as the canonical template.
108+
- Non-DB mocks (`sourceManagerRegistry`, `meshtasticManager`, service mocks) are still correct and must remain.
109+
93110
### Raw SQL Ban
94111
- `src/services/database.ts` is raw-SQL-free. All domain queries live in `src/db/repositories/*`.
95112
- ESLint rule (`no-restricted-syntax` in `eslint.config.mjs`) forbids `this.db.prepare`, `this.db.exec`, `postgresPool.query`, `mysqlPool.query` outside `src/server/migrations/**` and test files.
Lines changed: 129 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,31 @@
11
/**
2-
* Legacy session-mount channel-database — permission model tests (PR-B mirror)
2+
* Legacy session-mount channel-database — permission model tests
33
*
4-
* Parity coverage for the `/api/channel-database` mount. Both v1 (Bearer-token)
5-
* and legacy (browser-session) mounts share `_channelDatabaseHandlers.ts`, so
6-
* the same permission logic must hold here.
4+
* Converted from the monkey-patch pattern (vi.mock('../../services/database.js')
5+
* + vi.mock('../auth/authMiddleware.js')) to the real-middleware harness
6+
* (createRouteTestApp). The harness uses the live DatabaseService singleton with
7+
* real session + requireAuth + real checkPermissionAsync, seeding per-test
8+
* permission rows instead of hand-rolling fake implementations.
79
*
8-
* The `requireAuth()` middleware is mocked to a passthrough — these tests
9-
* exercise handler logic, not auth-token validation.
10+
* Permission model (global resource — channel_database is NOT source-scoped):
11+
* channel_database:read → list / get (PSK masked) + retroactive-decrypt progress
12+
* channel_database:write → create / update / delete / reorder + ACL management
13+
*
14+
* Source-isolation note: channel_database is a global-by-design resource
15+
* (no sourceId column on its permissions). The isolation assertion below
16+
* therefore proves that the REAL checkPermissionAsync enforces a global
17+
* grant/no-grant boundary rather than a per-source row boundary.
18+
* For per-source resource isolation see sourceRoutes.permissions.test.ts.
19+
*
20+
* Non-DB service mocks stay: channelDecryptionService and
21+
* retroactiveDecryptionService would attempt real file/network I/O.
22+
*
23+
* See src/server/test-helpers/routeTestApp.ts for the design rationale.
1024
*/
1125

12-
import { describe, it, expect, beforeEach, vi } from 'vitest';
13-
import express, { Express } from 'express';
14-
import request from 'supertest';
15-
16-
vi.mock('../auth/authMiddleware.js', () => ({
17-
requireAuth: () => (_req: any, _res: any, next: any) => next(),
18-
}));
19-
20-
vi.mock('../../services/database.js', () => ({
21-
default: {
22-
channelDatabase: {
23-
getAllAsync: vi.fn(),
24-
getByIdAsync: vi.fn(),
25-
getPermissionsForUserAsync: vi.fn(),
26-
getPermissionAsync: vi.fn(),
27-
getPermissionsForChannelAsync: vi.fn(),
28-
createAsync: vi.fn(),
29-
updateAsync: vi.fn(),
30-
deleteAsync: vi.fn(),
31-
reorderAsync: vi.fn(),
32-
setPermissionAsync: vi.fn(),
33-
deletePermissionAsync: vi.fn(),
34-
},
35-
findUserByIdAsync: vi.fn(),
36-
checkPermissionAsync: vi.fn(),
37-
getDistinctEncryptedPacketSourceIdsAsync: vi.fn(),
38-
drizzleDbType: 'sqlite',
39-
},
40-
}));
26+
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
4127

28+
// Non-DB service mocks: keep these to avoid real I/O in tests.
4229
vi.mock('../services/channelDecryptionService.js', () => ({
4330
channelDecryptionService: { invalidateCache: vi.fn() },
4431
}));
@@ -53,13 +40,9 @@ vi.mock('../services/retroactiveDecryptionService.js', () => ({
5340

5441
import channelDatabaseRoutes from './channelDatabaseRoutes.js';
5542
import databaseService from '../../services/database.js';
43+
import { createRouteTestApp, type RouteTestHarness } from '../test-helpers/routeTestApp.js';
5644

57-
const mockDb = databaseService as any;
58-
59-
const adminUser = { id: 1, username: 'admin', isAdmin: true, isActive: true };
60-
const writerUser = { id: 50, username: 'writer', isAdmin: false, isActive: true };
61-
const readerUser = { id: 60, username: 'reader', isAdmin: false, isActive: true };
62-
const noPermsUser = { id: 70, username: 'nobody', isAdmin: false, isActive: true };
45+
// ── shared test fixtures ──────────────────────────────────────────────────────
6346

6447
const fakePsk = Buffer.alloc(16, 0xab).toString('base64');
6548
const channel1 = {
@@ -78,109 +61,162 @@ const channel1 = {
7861
updatedAt: 1000,
7962
};
8063

81-
const createApp = (user: any): Express => {
82-
const app = express();
83-
app.use(express.json());
84-
app.use((req: any, _res, next) => {
85-
req.user = user;
86-
next();
64+
// ── harness wiring ────────────────────────────────────────────────────────────
65+
66+
let harness: RouteTestHarness;
67+
68+
beforeEach(async () => {
69+
harness = await createRouteTestApp({
70+
// channelDatabaseRoutes apply requireAuth() internally; the harness mounts
71+
// optionalAuth() before the router so the session is available, and
72+
// requireAuth() reads req.session.userId set by loginAs().
73+
mount: (app) => app.use('/api/channel-database', channelDatabaseRoutes),
8774
});
88-
app.use('/api/channel-database', channelDatabaseRoutes);
89-
return app;
90-
};
9175

92-
beforeEach(() => {
93-
vi.clearAllMocks();
94-
mockDb.channelDatabase.getAllAsync.mockResolvedValue([channel1]);
95-
mockDb.channelDatabase.getByIdAsync.mockResolvedValue(channel1);
96-
mockDb.channelDatabase.getPermissionsForUserAsync.mockResolvedValue([]);
97-
mockDb.channelDatabase.getPermissionAsync.mockResolvedValue(null);
98-
mockDb.checkPermissionAsync.mockResolvedValue(false);
76+
// Spy on channelDatabase repo data methods so tests control returned rows
77+
// while real checkPermissionAsync enforces access gates.
78+
vi.spyOn(databaseService.channelDatabase, 'getAllAsync').mockResolvedValue([channel1 as any]);
79+
vi.spyOn(databaseService.channelDatabase, 'getByIdAsync').mockResolvedValue(channel1 as any);
80+
vi.spyOn(databaseService.channelDatabase, 'getPermissionsForUserAsync').mockResolvedValue([]);
81+
vi.spyOn(databaseService.channelDatabase, 'getPermissionAsync').mockResolvedValue(null);
82+
vi.spyOn(databaseService.channelDatabase, 'createAsync').mockResolvedValue(42);
83+
84+
// No channel_database permission grants by default — tests add what they need.
9985
});
10086

87+
afterEach(async () => {
88+
vi.restoreAllMocks();
89+
await harness.cleanup();
90+
});
91+
92+
// ── describe 1: permission isolation (global-resource proof) ─────────────────
93+
//
94+
// channel_database is global — no sourceId scoping on the resource itself.
95+
// The assertions below prove the real checkPermissionAsync enforces the
96+
// grant/no-grant boundary correctly.
97+
98+
describe('permission isolation — global resource (real checkPermissionAsync)', () => {
99+
it('user WITH global channel_database:read grant can list channels (200)', async () => {
100+
// Global grant (no sourceId arg → sourceId=NULL row)
101+
await harness.grant(harness.limited.id, 'channel_database', 'read');
102+
// Per-entry permission: canRead=true for channel 1
103+
vi.spyOn(databaseService.channelDatabase, 'getPermissionsForUserAsync').mockResolvedValue([
104+
{ userId: harness.limited.id, channelDatabaseId: 1, canViewOnMap: false, canRead: true } as any,
105+
]);
106+
107+
const agent = await harness.loginAs(harness.limited);
108+
const res = await agent.get('/api/channel-database');
109+
expect(res.status).toBe(200);
110+
});
111+
112+
it('user WITHOUT channel_database:read grant is denied (403 — real checkPermissionAsync)', async () => {
113+
// No grant seeded — real checkPermissionAsync finds no row → false → 403.
114+
// Previously a `mockResolvedValue(false)` hid any real implementation bug.
115+
const agent = await harness.loginAs(harness.limited);
116+
const res = await agent.get('/api/channel-database');
117+
expect(res.status).toBe(403);
118+
});
119+
});
120+
121+
// ── describe 2: GET / permission filtering ───────────────────────────────────
122+
101123
describe('Legacy mount — GET / permission filtering', () => {
102124
it('admin: returns channels with full PSK', async () => {
103-
const res = await request(createApp(adminUser)).get('/api/channel-database');
125+
const agent = await harness.loginAs(harness.admin);
126+
const res = await agent.get('/api/channel-database');
104127
expect(res.status).toBe(200);
105128
expect(res.body.data[0].psk).toBeDefined();
106129
});
107130

108131
it('non-admin with :read + per-entry canRead: returns entry with masked PSK', async () => {
109-
mockDb.checkPermissionAsync.mockImplementation(
110-
async (_uid: number, resource: string, action: string) =>
111-
resource === 'channel_database' && action === 'read'
112-
);
113-
mockDb.channelDatabase.getPermissionsForUserAsync.mockResolvedValue([
114-
{ userId: 60, channelDatabaseId: 1, canViewOnMap: false, canRead: true },
132+
await harness.grant(harness.limited.id, 'channel_database', 'read');
133+
vi.spyOn(databaseService.channelDatabase, 'getPermissionsForUserAsync').mockResolvedValue([
134+
{ userId: harness.limited.id, channelDatabaseId: 1, canViewOnMap: false, canRead: true } as any,
115135
]);
116-
const res = await request(createApp(readerUser)).get('/api/channel-database');
136+
137+
const agent = await harness.loginAs(harness.limited);
138+
const res = await agent.get('/api/channel-database');
117139
expect(res.status).toBe(200);
118140
expect(res.body.count).toBe(1);
119141
expect(res.body.data[0].psk).toBeUndefined();
120142
expect(res.body.data[0].pskPreview).toMatch(/\.\.\.$/);
121143
});
122144

123145
it('non-admin without :read: returns 403', async () => {
124-
const res = await request(createApp(noPermsUser)).get('/api/channel-database');
146+
// No grants for limited — real checkPermissionAsync returns false.
147+
const agent = await harness.loginAs(harness.limited);
148+
const res = await agent.get('/api/channel-database');
125149
expect(res.status).toBe(403);
126150
});
127151
});
128152

153+
// ── describe 3: GET /:id permission filtering ─────────────────────────────────
154+
129155
describe('Legacy mount — GET /:id permission filtering', () => {
130156
it('non-admin with :read + per-entry canRead: returns channel with masked PSK', async () => {
131-
mockDb.checkPermissionAsync.mockImplementation(
132-
async (_uid: number, resource: string, action: string) =>
133-
resource === 'channel_database' && action === 'read'
157+
await harness.grant(harness.limited.id, 'channel_database', 'read');
158+
vi.spyOn(databaseService.channelDatabase, 'getPermissionAsync').mockResolvedValue(
159+
{ userId: harness.limited.id, channelDatabaseId: 1, canViewOnMap: false, canRead: true } as any
134160
);
135-
mockDb.channelDatabase.getPermissionAsync.mockResolvedValue({
136-
userId: 60,
137-
channelDatabaseId: 1,
138-
canViewOnMap: false,
139-
canRead: true,
140-
});
141-
const res = await request(createApp(readerUser)).get('/api/channel-database/1');
161+
162+
const agent = await harness.loginAs(harness.limited);
163+
const res = await agent.get('/api/channel-database/1');
142164
expect(res.status).toBe(200);
143165
expect(res.body.data.psk).toBeUndefined();
144166
expect(res.body.data.pskPreview).toMatch(/\.\.\.$/);
145167
});
146168

147169
it('non-admin with :read but no per-entry row: returns 404', async () => {
148-
mockDb.checkPermissionAsync.mockImplementation(
149-
async (_uid: number, resource: string, action: string) =>
150-
resource === 'channel_database' && action === 'read'
151-
);
152-
const res = await request(createApp(readerUser)).get('/api/channel-database/1');
170+
await harness.grant(harness.limited.id, 'channel_database', 'read');
171+
// getPermissionAsync already mocked to null (beforeEach default)
172+
173+
const agent = await harness.loginAs(harness.limited);
174+
const res = await agent.get('/api/channel-database/1');
153175
expect(res.status).toBe(404);
154176
});
155177

156178
it('non-admin without :read: returns 403', async () => {
157-
const res = await request(createApp(noPermsUser)).get('/api/channel-database/1');
179+
const agent = await harness.loginAs(harness.limited);
180+
const res = await agent.get('/api/channel-database/1');
158181
expect(res.status).toBe(403);
159182
});
160183
});
161184

185+
// ── describe 4: Write endpoints require channel_database:write ────────────────
186+
162187
describe('Legacy mount — Write endpoints require channel_database:write', () => {
163188
it('POST / → 403 for :read-only user', async () => {
164-
mockDb.checkPermissionAsync.mockImplementation(
165-
async (_uid: number, resource: string, action: string) =>
166-
resource === 'channel_database' && action === 'read'
167-
);
168-
const res = await request(createApp(readerUser))
189+
await harness.grant(harness.limited.id, 'channel_database', 'read');
190+
191+
const agent = await harness.loginAs(harness.limited);
192+
const res = await agent
169193
.post('/api/channel-database')
170194
.send({ name: 'X', psk: fakePsk });
171195
expect(res.status).toBe(403);
172196
});
173197

174198
it('POST / → 201 for non-admin with :write', async () => {
175-
mockDb.checkPermissionAsync.mockImplementation(
176-
async (_uid: number, resource: string, action: string) =>
177-
resource === 'channel_database' && action === 'write'
199+
await harness.grant(harness.limited.id, 'channel_database', 'write');
200+
vi.spyOn(databaseService.channelDatabase, 'getByIdAsync').mockResolvedValue(
201+
{ ...channel1, id: 42 } as any
178202
);
179-
mockDb.channelDatabase.createAsync.mockResolvedValue(42);
180-
mockDb.channelDatabase.getByIdAsync.mockResolvedValue({ ...channel1, id: 42 });
181-
const res = await request(createApp(writerUser))
203+
204+
const agent = await harness.loginAs(harness.limited);
205+
const res = await agent
182206
.post('/api/channel-database')
183207
.send({ name: 'X', psk: fakePsk });
184208
expect(res.status).toBe(201);
185209
});
210+
211+
it('admin bypasses write check (real admin bypass via resolveCallerScope)', async () => {
212+
vi.spyOn(databaseService.channelDatabase, 'getByIdAsync').mockResolvedValue(
213+
{ ...channel1, id: 42 } as any
214+
);
215+
216+
const agent = await harness.loginAs(harness.admin);
217+
const res = await agent
218+
.post('/api/channel-database')
219+
.send({ name: 'AdminChannel', psk: fakePsk });
220+
expect(res.status).toBe(201);
221+
});
186222
});

0 commit comments

Comments
 (0)