Skip to content

Commit 064f3a5

Browse files
Ayush4958Ayush4958
authored
Standarized the API Response in Public-Api middleware (#310)
* feat(public-api): update middlewares for AppError and ApiResponse * fix(public-api): update middleware tests for AppError usage * Prevent the error leaking
1 parent fa41f98 commit 064f3a5

9 files changed

Lines changed: 123 additions & 148 deletions

‎apps/public-api/src/__tests__/authorizeReadOperation.test.js‎

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,20 @@
11
'use strict';
22

3-
const authorizeReadOperation = require('../middlewares/authorizeReadOperation');
3+
jest.mock('@urbackend/common', () => {
4+
class MockAppError extends Error {
5+
constructor(statusCode, message, error) {
6+
super(message);
7+
this.statusCode = statusCode;
8+
this.error = error;
9+
this.isOperational = true;
10+
}
11+
}
12+
return {
13+
AppError: MockAppError
14+
};
15+
});
416

17+
const authorizeReadOperation = require('../middlewares/authorizeReadOperation');
518
// ---------------------------------------------------------------------------
619
// Helpers
720
// ---------------------------------------------------------------------------
@@ -80,9 +93,8 @@ describe('authorizeReadOperation middleware', () => {
8093

8194
await authorizeReadOperation(req, res, next);
8295

83-
expect(res.statusCode).toBe(404);
84-
expect(res.body.error).toBe('Collection not found');
85-
expect(next).not.toHaveBeenCalled();
96+
expect(next).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 404, message: 'Collection not found' }));
97+
expect(res.status).not.toHaveBeenCalled();
8698
});
8799

88100
test('rls disabled allows public read', async () => {
@@ -121,9 +133,8 @@ describe('authorizeReadOperation middleware', () => {
121133

122134
await authorizeReadOperation(req, res, next);
123135

124-
expect(res.statusCode).toBe(401);
125-
expect(res.body.error).toBe('Authentication required');
126-
expect(next).not.toHaveBeenCalled();
136+
expect(next).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 401, message: 'Provide a valid user Bearer token for private reads.' }));
137+
expect(res.status).not.toHaveBeenCalled();
127138
});
128139

129140
test('private mode sets owner filter when authed', async () => {

‎apps/public-api/src/__tests__/authorizeWriteOperation.test.js‎

Lines changed: 39 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -8,20 +8,31 @@ const mockFindById = jest.fn();
88
const mockSelect = jest.fn();
99
const mockLean = jest.fn();
1010

11-
jest.mock('@urbackend/common', () => ({
12-
getConnection: jest.fn().mockResolvedValue({}),
13-
getCompiledModel: jest.fn().mockReturnValue({
14-
findById: (...args) => {
15-
mockFindById(...args);
16-
return {
17-
select: (field) => {
18-
mockSelect(field);
19-
return { lean: mockLean };
20-
},
21-
};
22-
},
23-
}),
24-
}));
11+
jest.mock('@urbackend/common', () => {
12+
class MockAppError extends Error {
13+
constructor(statusCode, message, error) {
14+
super(message);
15+
this.statusCode = statusCode;
16+
this.error = error;
17+
this.isOperational = true;
18+
}
19+
}
20+
return {
21+
AppError: MockAppError,
22+
getConnection: jest.fn().mockResolvedValue({}),
23+
getCompiledModel: jest.fn().mockReturnValue({
24+
findById: (...args) => {
25+
mockFindById(...args);
26+
return {
27+
select: (field) => {
28+
mockSelect(field);
29+
return { lean: mockLean };
30+
},
31+
};
32+
},
33+
}),
34+
};
35+
});
2536

2637
jest.mock('mongoose', () => ({
2738
Types: {
@@ -138,10 +149,8 @@ describe('authorizeWriteOperation middleware', () => {
138149

139150
await authorizeWriteOperation(req, res, next);
140151

141-
expect(res.statusCode).toBe(403);
142-
expect(res.body.success).toBe(false);
143-
expect(res.body.error).toBe('Write blocked for publishable key');
144-
expect(next).not.toHaveBeenCalled();
152+
expect(next).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 403, error: 'Write blocked for publishable key' }));
153+
expect(res.status).not.toHaveBeenCalled();
145154
});
146155

147156
test('blocked with 401 when RLS enabled but no auth token provided', async () => {
@@ -150,10 +159,8 @@ describe('authorizeWriteOperation middleware', () => {
150159

151160
await authorizeWriteOperation(req, res, next);
152161

153-
expect(res.statusCode).toBe(401);
154-
expect(res.body.success).toBe(false);
155-
expect(res.body.error).toBe('Authentication required');
156-
expect(next).not.toHaveBeenCalled();
162+
expect(next).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 401 }));
163+
expect(res.status).not.toHaveBeenCalled();
157164
});
158165
});
159166

@@ -171,10 +178,8 @@ describe('authorizeWriteOperation middleware', () => {
171178

172179
await authorizeWriteOperation(req, res, next);
173180

174-
expect(res.statusCode).toBe(403);
175-
expect(res.body.success).toBe(false);
176-
expect(res.body.error).toBe('RLS owner mismatch');
177-
expect(next).not.toHaveBeenCalled();
181+
expect(next).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 403, error: 'RLS owner mismatch' }));
182+
expect(res.status).not.toHaveBeenCalled();
178183
});
179184

180185
// In the new atomic design, the middleware does not block based on document owner
@@ -293,10 +298,8 @@ describe('authorizeWriteOperation middleware', () => {
293298

294299
await authorizeWriteOperation(req, res, next);
295300

296-
expect(res.statusCode).toBe(404);
297-
expect(res.body.success).toBe(false);
298-
expect(res.body.error).toBe('Collection not found');
299-
expect(next).not.toHaveBeenCalled();
301+
expect(next).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 404, error: 'Collection not found' }));
302+
expect(res.status).not.toHaveBeenCalled();
300303
});
301304

302305
test('returns 403 when RLS ownerField is _id for a POST insert', async () => {
@@ -325,10 +328,8 @@ describe('authorizeWriteOperation middleware', () => {
325328

326329
await authorizeWriteOperation(req, res, next);
327330

328-
expect(res.statusCode).toBe(403);
329-
expect(res.body.success).toBe(false);
330-
expect(res.body.error).toBe('Insert denied');
331-
expect(next).not.toHaveBeenCalled();
331+
expect(next).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 403, error: 'Insert denied' }));
332+
expect(res.status).not.toHaveBeenCalled();
332333
});
333334

334335
test('returns 400 for an invalid document id on PUT', async () => {
@@ -342,10 +343,8 @@ describe('authorizeWriteOperation middleware', () => {
342343

343344
await authorizeWriteOperation(req, res, next);
344345

345-
expect(res.statusCode).toBe(400);
346-
expect(res.body.success).toBe(false);
347-
expect(res.body.error).toBe('Invalid ID format.');
348-
expect(next).not.toHaveBeenCalled();
346+
expect(next).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 400, error: 'Invalid ID format' }));
347+
expect(res.status).not.toHaveBeenCalled();
349348
});
350349

351350

@@ -361,10 +360,8 @@ describe('authorizeWriteOperation middleware', () => {
361360

362361
await authorizeWriteOperation(req, res, next);
363362

364-
expect(res.statusCode).toBe(403);
365-
expect(res.body.success).toBe(false);
366-
expect(res.body.error).toBe('Owner field immutable');
367-
expect(next).not.toHaveBeenCalled();
363+
expect(next).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 403, error: 'Owner field immutable' }));
364+
expect(res.status).not.toHaveBeenCalled();
368365
});
369366
});
370367
});

‎apps/public-api/src/__tests__/verifyApiKey.test.js‎

Lines changed: 31 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5,18 +5,29 @@ const mockSelect = jest.fn().mockReturnThis();
55
const mockPopulate = jest.fn().mockReturnThis();
66
const mockLean = jest.fn();
77

8-
jest.mock('@urbackend/common', () => ({
9-
Project: {
10-
findOne: jest.fn(() => ({
11-
select: mockSelect,
12-
populate: mockPopulate,
13-
lean: mockLean,
14-
})),
15-
},
16-
hashApiKey: jest.fn((key) => `hashed_${key}`),
17-
getProjectByApiKeyCache: jest.fn().mockResolvedValue(null),
18-
setProjectByApiKeyCache: jest.fn().mockResolvedValue(undefined),
19-
}));
8+
jest.mock('@urbackend/common', () => {
9+
class MockAppError extends Error {
10+
constructor(statusCode, message, error) {
11+
super(message);
12+
this.statusCode = statusCode;
13+
this.error = error;
14+
this.isOperational = true;
15+
}
16+
}
17+
return {
18+
AppError: MockAppError,
19+
Project: {
20+
findOne: jest.fn(() => ({
21+
select: mockSelect,
22+
populate: mockPopulate,
23+
lean: mockLean,
24+
})),
25+
},
26+
hashApiKey: jest.fn((key) => `hashed_${key}`),
27+
getProjectByApiKeyCache: jest.fn().mockResolvedValue(null),
28+
setProjectByApiKeyCache: jest.fn().mockResolvedValue(undefined),
29+
};
30+
});
2031

2132
const { hashApiKey, getProjectByApiKeyCache, Project } = require('@urbackend/common');
2233
const verifyApiKey = require('../middlewares/verifyApiKey');
@@ -100,9 +111,8 @@ describe('verifyApiKey middleware', () => {
100111

101112
await verifyApiKey(req, res, next);
102113

103-
expect(next).not.toHaveBeenCalled();
104-
expect(res.status).toHaveBeenCalledWith(401);
105-
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ error: 'API key not found' }));
114+
expect(next).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 401, message: 'API key not found' }));
115+
expect(res.status).not.toHaveBeenCalled();
106116
});
107117

108118
test('header takes precedence when both x-api-key header and ?key= query param are present', async () => {
@@ -139,9 +149,8 @@ describe('verifyApiKey middleware', () => {
139149

140150
await verifyApiKey(req, res, next);
141151

142-
expect(next).not.toHaveBeenCalled();
143-
expect(res.status).toHaveBeenCalledWith(401);
144-
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ error: 'API key not found' }));
152+
expect(next).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 401, message: 'API key not found' }));
153+
expect(res.status).not.toHaveBeenCalled();
145154
});
146155

147156
test('returns 401 when project is not found in DB', async () => {
@@ -151,9 +160,8 @@ describe('verifyApiKey middleware', () => {
151160

152161
await verifyApiKey(req, res, next);
153162

154-
expect(next).not.toHaveBeenCalled();
155-
expect(res.status).toHaveBeenCalledWith(401);
156-
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ error: 'API key is expired or invalid.' }));
163+
expect(next).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 401, error: 'API key is expired or invalid.' }));
164+
expect(res.status).not.toHaveBeenCalled();
157165
});
158166

159167
test('returns 401 when owner is not verified', async () => {
@@ -163,9 +171,8 @@ describe('verifyApiKey middleware', () => {
163171

164172
await verifyApiKey(req, res, next);
165173

166-
expect(next).not.toHaveBeenCalled();
167-
expect(res.status).toHaveBeenCalledWith(401);
168-
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ error: 'Owner not verified' }));
174+
expect(next).toHaveBeenCalledWith(expect.objectContaining({ statusCode: 401, error: 'Owner not verified' }));
175+
expect(res.status).not.toHaveBeenCalled();
169176
});
170177

171178
test('uses cache when available and does not query DB', async () => {

‎apps/public-api/src/middlewares/authorizeReadOperation.js‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
const { AppError } = require('@urbackend/common');
12
module.exports = async (req, res, next) => {
23
try {
34
if (req.keyRole === 'secret') {
@@ -10,7 +11,7 @@ module.exports = async (req, res, next) => {
1011
const collectionConfig = project.collections.find(c => c.name === collectionName);
1112

1213
if (!collectionConfig) {
13-
return res.status(404).json({ error: 'Collection not found' });
14+
return next(new AppError(404, 'Collection not found'));
1415
}
1516

1617
const rls = collectionConfig.rls || {};
@@ -24,10 +25,7 @@ module.exports = async (req, res, next) => {
2425

2526
if (mode === 'private') {
2627
if (!req.authUser?.userId) {
27-
return res.status(401).json({
28-
error: 'Authentication required',
29-
message: 'Provide a valid user Bearer token for private reads.'
30-
});
28+
return next(new AppError(401, 'Provide a valid user Bearer token for private reads.', 'Authentication required'));
3129
}
3230

3331
const ownerField = rls.ownerField || 'userId';
@@ -40,8 +38,9 @@ module.exports = async (req, res, next) => {
4038
return next();
4139
}
4240

43-
return res.status(403).json({ error: 'Unsupported RLS mode' });
41+
return next(new AppError(403, 'Unsupported RLS mode'));
4442
} catch (err) {
45-
return res.status(500).json({ error: err.message });
43+
console.error('[authorizeReadOperation] Unexpected error:', err);
44+
return next(new AppError(500, 'Internal Server Error'));
4645
}
4746
};

0 commit comments

Comments
 (0)