Skip to content

Commit 67114b2

Browse files
PierreBrisorgueilPierreBrisorgueil
authored
fix(auth,orgs,audit): replace silent .catch(() => {}) with error logging (#3366)
* fix(auth,orgs,audit): replace silent .catch(() => {}) with error logging Replace all silent .catch(() => {}) with logger.error (DB writes / critical ops) and logger.warn (fire-and-forget emails) across auth controllers, org services, and audit middleware. Adds logger mock to analytics unit tests to avoid config dependency. Closes #3360 * test(auth,orgs): add unit tests for error logging on silent-catch paths Add unit tests verifying logger.warn/error is called when fire-and-forget emails or DB rollback operations fail in auth controllers, org crud/service, and membership service. Fixes codecov/patch failure on PR #3366. * fix(auth,orgs,audit): use serializable error format in logger calls Switch all logger.warn/error calls from raw Error objects to { message, stack } to ensure correct serialization with Winston JSON format in production. Restore audit middleware .catch() (defensive against mock rejections in tests). Update tests to assert on the new object format.
1 parent 793e1ba commit 67114b2

12 files changed

Lines changed: 571 additions & 12 deletions

‎lib/services/tests/analytics.identify.unit.tests.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,10 @@ describe('Analytics identify on auth events:', () => {
119119
jest.unstable_mockModule('../../helpers/getBaseUrl.js', () => ({
120120
default: jest.fn().mockReturnValue('http://localhost:3000'),
121121
}));
122+
123+
jest.unstable_mockModule('../logger.js', () => ({
124+
default: { error: jest.fn(), warn: jest.fn(), info: jest.fn() },
125+
}));
122126
};
123127

124128
test('should call AnalyticsService.identify after successful signup', async () => {
@@ -260,6 +264,10 @@ describe('Analytics identify on auth events:', () => {
260264
default: jest.fn().mockReturnValue('http://localhost:3000'),
261265
}));
262266

267+
jest.unstable_mockModule('../logger.js', () => ({
268+
default: { error: jest.fn(), warn: jest.fn(), info: jest.fn() },
269+
}));
270+
263271
const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js');
264272

265273
const req = { user: mockUser };

‎modules/audit/middlewares/audit.middleware.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
* Module dependencies
33
*/
44
import AuditService from '../services/audit.service.js';
5+
import logger from '../../../lib/services/logger.js';
56

67
/**
78
* Default route prefixes to skip when auto-capturing audit events.
@@ -126,7 +127,7 @@ const createAuditMiddleware = (options = {}) => {
126127
req,
127128
targetType,
128129
targetId,
129-
}).catch(() => {});
130+
}).catch((err) => logger.error('audit.middleware: audit log write failed', { message: err?.message, stack: err?.stack }));
130131
});
131132

132133
return next();

‎modules/audit/tests/audit.middleware.unit.tests.js‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,11 @@ jest.unstable_mockModule('../services/audit.service.js', () => ({
1313
},
1414
}));
1515

16+
const mockLoggerError = jest.fn();
17+
jest.unstable_mockModule('../../../lib/services/logger.js', () => ({
18+
default: { error: mockLoggerError, warn: jest.fn(), info: jest.fn() },
19+
}));
20+
1621
/**
1722
* Unit tests for audit middleware
1823
*/
@@ -224,8 +229,9 @@ describe('Audit middleware unit tests:', () => {
224229
expect(mockLog).not.toHaveBeenCalled();
225230
});
226231

227-
test('should not throw when AuditService.log rejects', () => {
228-
mockLog = jest.fn().mockRejectedValue(new Error('DB down'));
232+
test('should not throw when AuditService.log rejects and should log the error', async () => {
233+
const dbError = new Error('DB down');
234+
mockLog = jest.fn().mockRejectedValue(dbError);
229235
const middleware = createAuditMiddleware();
230236
const req = createReq();
231237
const res = createRes(200);
@@ -234,5 +240,11 @@ describe('Audit middleware unit tests:', () => {
234240
middleware(req, res, next);
235241
// Should not throw
236242
expect(() => res.emit('finish')).not.toThrow();
243+
// Allow the .catch() handler to run
244+
await new Promise((r) => setTimeout(r, 10));
245+
expect(mockLoggerError).toHaveBeenCalledWith(
246+
'audit.middleware: audit log write failed',
247+
{ message: dbError.message, stack: dbError.stack },
248+
);
237249
});
238250
});

‎modules/auth/controllers/auth.controller.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import AuthOrganizationService from '../../organizations/services/organizations.
1919
import OrganizationCrudService from '../../organizations/services/organizations.crud.service.js';
2020
import MembershipService from '../../organizations/services/organizations.membership.service.js';
2121
import AnalyticsService from '../../../lib/services/analytics.js';
22+
import logger from '../../../lib/services/logger.js';
2223

2324
const tokenCookieOptions = {
2425
httpOnly: true,
@@ -79,7 +80,7 @@ const signup = async (req, res) => {
7980
emailVerificationExpires: Date.now() + 24 * 3600000, // 24 hours
8081
}, 'recover');
8182
// Send verification email (best-effort, do not block signup)
82-
sendVerificationEmail(user, verificationToken).catch(() => {});
83+
sendVerificationEmail(user, verificationToken).catch((err) => logger.warn('auth.signup: verification email failed', { message: err?.message, stack: err?.stack }));
8384
} else {
8485
// No mailer configured — auto-verify so dev/test are not blocked
8586
const brutUser = await UserService.getBrut({ id: user.id });

‎modules/auth/controllers/auth.password.controller.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import getBaseUrl from '../../../lib/helpers/getBaseUrl.js';
1010
import errors from '../../../lib/helpers/errors.js';
1111
import responses from '../../../lib/helpers/responses.js';
1212
import config from '../../../config/index.js';
13+
import logger from '../../../lib/services/logger.js';
1314

1415
const tokenCookieOptions = {
1516
httpOnly: true,
@@ -108,7 +109,7 @@ const reset = async (req, res) => {
108109
appName: config.app.title,
109110
appContact: config.app.contact,
110111
},
111-
}).catch(() => {});
112+
}).catch((err) => logger.warn('auth.password.reset: confirmation email failed', { message: err?.message, stack: err?.stack }));
112113
return res
113114
.status(200)
114115
.cookie('TOKEN', jwt.sign({ userId: user.id }, config.jwt.secret, { expiresIn: config.jwt.expiresIn }), tokenCookieOptions)

‎modules/auth/tests/auth.silent.catch.unit.tests.js‎

Lines changed: 225 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,225 @@
1+
/**
2+
* Module dependencies.
3+
*/
4+
import { jest, describe, test, expect, beforeEach } from '@jest/globals';
5+
6+
/**
7+
* Unit tests — verify that logger.warn/error is called when fire-and-forget
8+
* email sends or DB rollback operations fail (replaces silent .catch(() => {})).
9+
*/
10+
11+
describe('auth.controller silent-catch error logging:', () => {
12+
let mockWarn;
13+
let mockError;
14+
15+
beforeEach(() => {
16+
jest.resetModules();
17+
18+
mockWarn = jest.fn();
19+
mockError = jest.fn();
20+
21+
jest.unstable_mockModule('../../../lib/services/logger.js', () => ({
22+
default: { warn: mockWarn, error: mockError, info: jest.fn() },
23+
}));
24+
});
25+
26+
describe('signup: verification email failure logs a warning', () => {
27+
test('should call logger.warn when sendVerificationEmail rejects', async () => {
28+
const emailError = new Error('SMTP down');
29+
30+
jest.unstable_mockModule('../../../modules/users/services/users.service.js', () => ({
31+
default: {
32+
create: jest.fn().mockResolvedValue({
33+
id: 'u1', email: 'x@y.com', firstName: 'A', lastName: 'B', provider: 'local',
34+
}),
35+
getBrut: jest.fn().mockResolvedValue({ id: 'u1' }),
36+
update: jest.fn().mockResolvedValue({}),
37+
remove: jest.fn(),
38+
},
39+
}));
40+
41+
jest.unstable_mockModule('../../../modules/organizations/services/organizations.service.js', () => ({
42+
default: {
43+
handleSignupOrganization: jest.fn().mockResolvedValue({
44+
organization: null, joined: false, pendingJoin: false,
45+
abilities: [], organizationSetupRequired: false,
46+
emailVerificationRequired: false, suggestedOrganization: null,
47+
}),
48+
},
49+
}));
50+
51+
jest.unstable_mockModule('../../../modules/organizations/services/organizations.crud.service.js', () => ({
52+
default: { autoSetCurrentOrganization: jest.fn() },
53+
}));
54+
55+
jest.unstable_mockModule('../../../modules/organizations/services/organizations.membership.service.js', () => ({
56+
default: { findByUserAndOrganization: jest.fn(), listPendingByUser: jest.fn().mockResolvedValue([]) },
57+
}));
58+
59+
jest.unstable_mockModule('../../../config/index.js', () => ({
60+
default: {
61+
sign: { up: true, in: true },
62+
jwt: { secret: 'test-secret', expiresIn: 3600 },
63+
cookie: { secure: false, sameSite: 'lax' },
64+
organizations: { enabled: false },
65+
app: { title: 'Test', contact: 'test@test.com' },
66+
},
67+
}));
68+
69+
jest.unstable_mockModule('../../../lib/middlewares/model.js', () => ({
70+
default: { getResultFromZod: jest.fn(), checkError: jest.fn() },
71+
}));
72+
73+
// Mailer configured, sendMail rejects
74+
jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({
75+
default: {
76+
isConfigured: jest.fn().mockReturnValue(true),
77+
sendMail: jest.fn().mockRejectedValue(emailError),
78+
},
79+
}));
80+
81+
jest.unstable_mockModule('../../../lib/helpers/responses.js', () => ({
82+
default: {
83+
success: jest.fn().mockReturnValue(jest.fn()),
84+
error: jest.fn().mockReturnValue(jest.fn()),
85+
},
86+
}));
87+
88+
jest.unstable_mockModule('../../../lib/helpers/errors.js', () => ({
89+
default: { getMessage: jest.fn().mockReturnValue('error') },
90+
}));
91+
92+
jest.unstable_mockModule('../../../lib/helpers/AppError.js', () => ({
93+
default: class AppError extends Error {
94+
constructor(msg, opts) {
95+
super(msg);
96+
this.code = opts?.code;
97+
this.details = opts?.details;
98+
}
99+
},
100+
}));
101+
102+
jest.unstable_mockModule('../../../modules/users/models/users.schema.js', () => ({
103+
default: { User: {} },
104+
}));
105+
106+
jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({
107+
default: { defineAbilityFor: jest.fn().mockResolvedValue({}) },
108+
}));
109+
110+
jest.unstable_mockModule('../../../lib/helpers/abilities.js', () => ({
111+
default: jest.fn().mockReturnValue([]),
112+
}));
113+
114+
jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({
115+
default: jest.fn().mockReturnValue('http://localhost:3000'),
116+
}));
117+
118+
jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({
119+
default: { identify: jest.fn(), groupIdentify: jest.fn() },
120+
}));
121+
122+
const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js');
123+
124+
const req = { body: { email: 'x@y.com', firstName: 'A', lastName: 'B', password: 'P@ss1234!' } };
125+
const res = {
126+
status: jest.fn().mockReturnThis(),
127+
cookie: jest.fn().mockReturnThis(),
128+
json: jest.fn().mockReturnThis(),
129+
};
130+
131+
await AuthController.signup(req, res);
132+
133+
// Allow the fire-and-forget promise to settle
134+
await new Promise((r) => setTimeout(r, 10));
135+
136+
expect(mockWarn).toHaveBeenCalledWith(
137+
'auth.signup: verification email failed',
138+
{ message: emailError.message, stack: emailError.stack },
139+
);
140+
});
141+
});
142+
});
143+
144+
describe('auth.password.controller silent-catch error logging:', () => {
145+
let mockWarn;
146+
let mockError;
147+
148+
beforeEach(() => {
149+
jest.resetModules();
150+
151+
mockWarn = jest.fn();
152+
mockError = jest.fn();
153+
154+
jest.unstable_mockModule('../../../lib/services/logger.js', () => ({
155+
default: { warn: mockWarn, error: mockError, info: jest.fn() },
156+
}));
157+
});
158+
159+
describe('reset: confirmation email failure logs a warning', () => {
160+
test('should call logger.warn when confirmation email rejects', async () => {
161+
const emailError = new Error('SMTP unavailable');
162+
163+
jest.unstable_mockModule('../../../modules/users/services/users.service.js', () => ({
164+
default: {
165+
getBrut: jest.fn().mockResolvedValue({
166+
id: 'u1', email: 'a@b.com', firstName: 'A', lastName: 'B',
167+
resetPasswordToken: 'tok', resetPasswordExpires: Date.now() + 3600000,
168+
}),
169+
update: jest.fn().mockResolvedValue({ id: 'u1', email: 'a@b.com', firstName: 'A', lastName: 'B' }),
170+
},
171+
}));
172+
173+
jest.unstable_mockModule('../../../modules/auth/services/auth.service.js', () => ({
174+
default: { hashPassword: jest.fn().mockResolvedValue('hashed') },
175+
}));
176+
177+
// sendMail rejects
178+
jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({
179+
default: { sendMail: jest.fn().mockRejectedValue(emailError) },
180+
}));
181+
182+
jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({
183+
default: jest.fn().mockReturnValue('http://localhost:3000'),
184+
}));
185+
186+
jest.unstable_mockModule('../../../lib/helpers/errors.js', () => ({
187+
default: { getMessage: jest.fn().mockReturnValue('error') },
188+
}));
189+
190+
jest.unstable_mockModule('../../../lib/helpers/responses.js', () => ({
191+
default: {
192+
success: jest.fn().mockReturnValue(jest.fn()),
193+
error: jest.fn().mockReturnValue(jest.fn()),
194+
},
195+
}));
196+
197+
jest.unstable_mockModule('../../../config/index.js', () => ({
198+
default: {
199+
jwt: { secret: 'test-secret', expiresIn: 3600 },
200+
cookie: { secure: false, sameSite: 'lax' },
201+
app: { title: 'Test', contact: 'test@test.com' },
202+
},
203+
}));
204+
205+
const { default: PasswordController } = await import('../../../modules/auth/controllers/auth.password.controller.js');
206+
207+
const req = { body: { token: 'tok', newPassword: 'NewP@ss1!' } };
208+
const res = {
209+
status: jest.fn().mockReturnThis(),
210+
cookie: jest.fn().mockReturnThis(),
211+
json: jest.fn().mockReturnThis(),
212+
};
213+
214+
await PasswordController.reset(req, res);
215+
216+
// Allow fire-and-forget to settle
217+
await new Promise((r) => setTimeout(r, 10));
218+
219+
expect(mockWarn).toHaveBeenCalledWith(
220+
'auth.password.reset: confirmation email failed',
221+
{ message: emailError.message, stack: emailError.stack },
222+
);
223+
});
224+
});
225+
});

‎modules/organizations/services/organizations.crud.service.js‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import AppError from '../../../lib/helpers/AppError.js';
55
import { assertEmailVerified } from '../../../lib/helpers/emailVerification.js';
66
import config from '../../../config/index.js';
7+
import logger from '../../../lib/services/logger.js';
78

89
/**
910
* @desc Escape regex-special characters in a user-provided string.
@@ -112,8 +113,8 @@ const create = async (body, user) => {
112113
await UserService.updateById(user.id || user._id, { currentOrganization: result._id });
113114
} catch (err) {
114115
// Rollback partially created artifacts
115-
if (membership) await MembershipRepository.deleteMany({ _id: membership._id }).catch(() => {});
116-
await OrganizationsRepository.remove(result).catch(() => {});
116+
if (membership) await MembershipRepository.deleteMany({ _id: membership._id }).catch((e) => logger.error('organizations.crud.create: rollback membership failed', { message: e?.message, stack: e?.stack }));
117+
await OrganizationsRepository.remove(result).catch((e) => logger.error('organizations.crud.create: rollback organization failed', { message: e?.message, stack: e?.stack }));
117118
throw err;
118119
}
119120

‎modules/organizations/services/organizations.membership.service.js‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import crypto from 'crypto';
55

66
import config from '../../../config/index.js';
7+
import logger from '../../../lib/services/logger.js';
78
import getBaseUrl from '../../../lib/helpers/getBaseUrl.js';
89
import mailer from '../../../lib/helpers/mailer/index.js';
910
import { assertEmailVerified } from '../../../lib/helpers/emailVerification.js';
@@ -164,7 +165,7 @@ const createJoinRequest = async (userId, organizationId) => {
164165
url: `${getBaseUrl()}/users/organizations/${organizationId}`,
165166
appName: config.app.title,
166167
},
167-
}).catch(() => {});
168+
}).catch((err) => logger.warn('organizations.membership.createJoinRequest: admin notification email failed', { message: err?.message, stack: err?.stack }));
168169
}
169170
}
170171
}
@@ -206,7 +207,7 @@ const approveRequest = async (membership) => {
206207
orgName: org.name,
207208
appName: config.app.title,
208209
},
209-
}).catch(() => {});
210+
}).catch((err) => logger.warn('organizations.membership.approveRequest: approval email failed', { message: err?.message, stack: err?.stack }));
210211
}
211212
}
212213

@@ -235,7 +236,7 @@ const rejectRequest = async (membership) => {
235236
orgName: org.name,
236237
appName: config.app.title,
237238
},
238-
}).catch(() => {});
239+
}).catch((err) => logger.warn('organizations.membership.rejectRequest: rejection email failed', { message: err?.message, stack: err?.stack }));
239240
}
240241
}
241242
return MembershipRepository.remove(membership);

0 commit comments

Comments
 (0)