Skip to content

Commit b2d5d36

Browse files
committed
refactor: update account and transaction handling with improved logging and parameter handling
- Changed `null` parameters to `undefined` in account closing and balance retrieval for better API compatibility. - Enhanced logging in account-related functions to provide detailed insights during operations. - Updated transaction listing to handle date parameters more robustly. - Improved OAuth2 routes with detailed logging for authorization and token exchange processes. - Removed outdated authentication tests to streamline the test suite.
1 parent 87e7ae0 commit b2d5d36

File tree

12 files changed

+1149
-117
lines changed

12 files changed

+1149
-117
lines changed

src/auth/oauth2/code.js

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
import crypto from 'crypto';
66
import { executeQuery, getRow, pruneExpiredCodes } from '../../db/authDb.js';
7+
import logger from '../../logging/logger.js';
78

89
/**
910
* Generate and store a short-lived authorization code.
@@ -18,6 +19,14 @@ export const generateAuthCode = async (clientId, userId, redirectUri, scope = 'a
1819
VALUES (?, ?, ?, ?, ?, ?)
1920
`, [code, clientId, userId, redirectUri, scope, expiresAt]);
2021

22+
logger.debug('[OAuth2] Authorization code generated', {
23+
clientId,
24+
userId,
25+
redirectUri,
26+
scope,
27+
expiresAt
28+
});
29+
2130
return code;
2231
};
2332

@@ -35,6 +44,7 @@ const validateAuthCodeFormat = (code) => {
3544
*/
3645
export const validateAuthCode = async (code, clientId, redirectUri) => {
3746
if (!validateAuthCodeFormat(code)) {
47+
logger.warn('[OAuth2] Invalid authorization code format', { clientId, codeLength: code?.length });
3848
throw new Error('Invalid authorization code format');
3949
}
4050
await pruneExpiredCodes();
@@ -43,10 +53,19 @@ export const validateAuthCode = async (code, clientId, redirectUri) => {
4353
WHERE code = ? AND client_id = ? AND redirect_uri = ?
4454
`, [code, clientId, redirectUri]);
4555

46-
if (!row) throw new Error('Invalid or expired authorization code');
56+
if (!row) {
57+
logger.warn('[OAuth2] Invalid or expired authorization code', { clientId, redirectUri });
58+
throw new Error('Invalid or expired authorization code');
59+
}
4760

4861
// One-time use – delete
4962
await executeQuery('DELETE FROM auth_codes WHERE code = ?', [code]);
5063

64+
logger.debug('[OAuth2] Authorization code validated and consumed', {
65+
clientId,
66+
userId: row.user_id,
67+
scope: row.scope || 'api'
68+
});
69+
5170
return { userId: row.user_id, scope: row.scope || 'api' };
5271
};

src/routes/accounts.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ router.post(
7474
validateBody(CloseAccountSchema),
7575
asyncHandler(async (req, res) => {
7676
const { transferAccountId, transferCategoryId } = req.validatedBody;
77-
await accountClose(req.validatedParams.id, transferAccountId || null, transferCategoryId || null);
77+
await accountClose(req.validatedParams.id, transferAccountId || undefined, transferCategoryId || undefined);
7878
sendSuccess(res);
7979
})
8080
);
@@ -93,7 +93,7 @@ router.get(
9393
'/:id/balance',
9494
validateParams(IDSchema),
9595
asyncHandler(async (req, res) => {
96-
const cutoff = req.query.cutoff ? new Date(req.query.cutoff) : null;
96+
const cutoff = req.query.cutoff ? new Date(req.query.cutoff) : undefined;
9797
const balance = await accountBalance(req.validatedParams.id, cutoff);
9898
sendSuccess(res, { balance });
9999
})

src/routes/admin.js

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { asyncHandler } from '../middleware/asyncHandler.js';
1111
import { sendSuccess, sendCreated, throwBadRequest, throwNotFound } from '../middleware/responseHelpers.js';
1212
import { validateBody, validateParams, CreateClientSchema, UpdateClientSchema, ClientIdParamsSchema } from '../middleware/validation-schemas.js';
1313
import { adminLimiter, standardWriteLimiter, deleteLimiter } from '../middleware/rateLimiters.js';
14+
import logger from '../logging/logger.js';
1415

1516
const router = express.Router();
1617

@@ -24,7 +25,9 @@ router.use(adminLimiter);
2425
* List all OAuth clients (without secrets).
2526
*/
2627
router.get('/oauth-clients', asyncHandler(async (req, res) => {
28+
logger.debug('[Admin] Listing OAuth clients', { userId: req.user?.user_id });
2729
const clients = await listClients();
30+
logger.info('[Admin] OAuth clients listed', { userId: req.user?.user_id, count: clients.length });
2831
sendSuccess(res, { clients });
2932
}));
3033

@@ -35,12 +38,15 @@ router.get('/oauth-clients', asyncHandler(async (req, res) => {
3538
*/
3639
router.get('/oauth-clients/:clientId', validateParams(ClientIdParamsSchema), asyncHandler(async (req, res) => {
3740
const { clientId } = req.validatedParams;
41+
logger.debug('[Admin] Getting OAuth client', { userId: req.user?.user_id, clientId });
3842
const client = await getClient(clientId);
3943

4044
if (!client) {
45+
logger.warn('[Admin] OAuth client not found', { userId: req.user?.user_id, clientId });
4146
throwNotFound(`OAuth client '${clientId}' not found`);
4247
}
4348

49+
logger.info('[Admin] OAuth client retrieved', { userId: req.user?.user_id, clientId });
4450
sendSuccess(res, { client });
4551
}));
4652

@@ -54,6 +60,14 @@ router.get('/oauth-clients/:clientId', validateParams(ClientIdParamsSchema), asy
5460
router.post('/oauth-clients', standardWriteLimiter, validateBody(CreateClientSchema), asyncHandler(async (req, res) => {
5561
const { client_id, client_secret, allowed_scopes, redirect_uris } = req.validatedBody;
5662

63+
logger.debug('[Admin] Creating OAuth client', {
64+
userId: req.user?.user_id,
65+
client_id,
66+
hasSecret: !!client_secret,
67+
allowed_scopes,
68+
redirect_uris: redirect_uris ? (Array.isArray(redirect_uris) ? redirect_uris.length : 1) : 0
69+
});
70+
5771
try {
5872
const client = await createClient({
5973
clientId: client_id,
@@ -62,14 +76,29 @@ router.post('/oauth-clients', standardWriteLimiter, validateBody(CreateClientSch
6276
redirectUris: redirect_uris,
6377
});
6478

79+
logger.info('[Admin] OAuth client created', {
80+
userId: req.user?.user_id,
81+
client_id,
82+
allowed_scopes: client.allowed_scopes
83+
});
84+
6585
sendCreated(res, {
6686
client,
6787
message: 'OAuth client created successfully. Save the client_secret now - it will not be shown again.',
6888
});
6989
} catch (error) {
7090
if (error.message.includes('already exists')) {
91+
logger.warn('[Admin] OAuth client creation failed - already exists', {
92+
userId: req.user?.user_id,
93+
client_id
94+
});
7195
throwBadRequest(error.message);
7296
}
97+
logger.error('[Admin] OAuth client creation failed', {
98+
userId: req.user?.user_id,
99+
client_id,
100+
error: error.message
101+
});
73102
throw error;
74103
}
75104
}));
@@ -88,16 +117,35 @@ router.put('/oauth-clients/:clientId',
88117
const { clientId } = req.validatedParams;
89118
const updates = req.validatedBody;
90119

120+
logger.debug('[Admin] Updating OAuth client', {
121+
userId: req.user?.user_id,
122+
clientId,
123+
updateFields: Object.keys(updates.fields || {})
124+
});
125+
91126
try {
92127
const client = await updateClient(clientId, updates);
128+
logger.info('[Admin] OAuth client updated', {
129+
userId: req.user?.user_id,
130+
clientId
131+
});
93132
sendSuccess(res, {
94133
client,
95134
message: 'OAuth client updated successfully',
96135
});
97136
} catch (error) {
98137
if (error.message.includes('not found')) {
138+
logger.warn('[Admin] OAuth client update failed - not found', {
139+
userId: req.user?.user_id,
140+
clientId
141+
});
99142
throwNotFound(error.message);
100143
}
144+
logger.error('[Admin] OAuth client update failed', {
145+
userId: req.user?.user_id,
146+
clientId,
147+
error: error.message
148+
});
101149
throw error;
102150
}
103151
})
@@ -110,12 +158,27 @@ router.put('/oauth-clients/:clientId',
110158
*/
111159
router.delete('/oauth-clients/:clientId', deleteLimiter, validateParams(ClientIdParamsSchema), asyncHandler(async (req, res) => {
112160
const { clientId } = req.validatedParams;
161+
162+
logger.debug('[Admin] Deleting OAuth client', {
163+
userId: req.user?.user_id,
164+
clientId
165+
});
166+
113167
const deleted = await deleteClient(clientId);
114168

115169
if (!deleted) {
170+
logger.warn('[Admin] OAuth client deletion failed - not found', {
171+
userId: req.user?.user_id,
172+
clientId
173+
});
116174
throwNotFound(`OAuth client '${clientId}' not found`);
117175
}
118176

177+
logger.info('[Admin] OAuth client deleted', {
178+
userId: req.user?.user_id,
179+
clientId
180+
});
181+
119182
sendSuccess(res, { message: `OAuth client '${clientId}' deleted successfully` });
120183
}));
121184

src/routes/login.js

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import rateLimit from 'express-rate-limit';
99
import { authenticateUser } from '../auth/user.js';
1010
import { authenticateAdminDashboard } from '../auth/adminDashboard.js';
1111
import { asyncHandler } from '../middleware/asyncHandler.js';
12+
import logger, { logAuthEvent } from '../logging/logger.js';
1213

1314
const router = express.Router();
1415

@@ -56,12 +57,33 @@ const validateReturnTo = (returnTo, baseUrl = '') => {
5657
router.post('/login', loginLimiter, async (req, res) => {
5758
const { username, password, return_to } = req.body;
5859

59-
const { userId } = await authenticateUser(username, password);
60-
req.session.user = { id: userId, username };
60+
logger.debug('[Session] Login attempt', { username, hasReturnTo: !!return_to, ip: req.ip });
6161

62-
// Validate return_to to prevent open redirect attacks
63-
const safeReturnTo = validateReturnTo(return_to, req.get('origin') || req.protocol + '://' + req.get('host'));
64-
res.redirect(safeReturnTo);
62+
try {
63+
const { userId } = await authenticateUser(username, password);
64+
req.session.user = { id: userId, username };
65+
66+
logAuthEvent('SESSION_LOGIN', userId, { username, ip: req.ip }, true);
67+
logger.info('[Session] Login successful', { userId, username });
68+
69+
// Validate return_to to prevent open redirect attacks
70+
const safeReturnTo = validateReturnTo(return_to, req.get('origin') || req.protocol + '://' + req.get('host'));
71+
72+
if (return_to && return_to !== safeReturnTo) {
73+
logger.warn('[Session] Invalid return_to URL sanitized', {
74+
original: return_to,
75+
sanitized: safeReturnTo,
76+
username
77+
});
78+
}
79+
80+
res.redirect(safeReturnTo);
81+
} catch (error) {
82+
logAuthEvent('SESSION_LOGIN_FAILED', null, { username, reason: error.message, ip: req.ip }, false);
83+
logger.warn('[Session] Login failed', { username, error: error.message, ip: req.ip });
84+
// Re-throw to be handled by error middleware
85+
throw error;
86+
}
6587
});
6688

6789
/**
@@ -72,12 +94,20 @@ router.post('/login', loginLimiter, async (req, res) => {
7294
*/
7395
router.post('/logout', (req, res) => {
7496
const sessionCookieName = req.session.cookie.name || 'sessionId';
97+
const userId = req.session.user?.id;
98+
const username = req.session.user?.username;
7599

76100
req.session.destroy((err) => {
77101
if (err) {
102+
logger.error('[Session] Logout failed', { userId, username, error: err.message });
78103
return res.status(500).json({ error: 'Failed to logout' });
79104
}
80105

106+
if (userId) {
107+
logAuthEvent('SESSION_LOGOUT', userId, { username }, true);
108+
logger.info('[Session] Logout successful', { userId, username });
109+
}
110+
81111
// Clear the session cookie with the same options used when creating it
82112
res.clearCookie(sessionCookieName, {
83113
httpOnly: true,

0 commit comments

Comments
 (0)