Conversation
WalkthroughThis update introduces a comprehensive audit logging system into the application. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpressApp
participant AuditMiddleware
participant Controller
participant AuditService
participant AppDataSource
participant AuditLogEntity
Client->>ExpressApp: HTTP Request
ExpressApp->>AuditMiddleware: Passes Request
AuditMiddleware->>ExpressApp: Attaches auditContext, dataSource
ExpressApp->>Controller: Handles route
Controller->>AuditService: Calls createAuditLogs/findAuditLogs
AuditService->>AppDataSource: Uses repository
AppDataSource->>AuditLogEntity: Persists or queries audit logs
AuditLogEntity-->>AuditService: Returns result
AuditService-->>Controller: Returns result
Controller-->>ExpressApp: Sends response
ExpressApp-->>Client: HTTP Response
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (17)
src/middlewares/audit.middleware.ts (2)
2-2: Remove unused TypeORM import.The
getConnectionimport from TypeORM is not used in this file. Remove it to keep the codebase clean.-import { getConnection } from "typeorm";
32-34: Remove unnecessary comment and whitespace.There's an empty line and a comment that doesn't add value. Clean up this section for better readability.
- // Add user context to request for use in route handlers - req.auditContext = userContext;src/app.ts (1)
116-118: Remove unnecessary comment and whitespace.The comment on line 116 doesn't provide additional information and there's extra whitespace. Clean up this section.
-// Make dataSource available in request for services - app.use(auditMiddleware as RequestHandler);src/controllers/AuditLogController.ts (1)
28-33: Consider logging or propagating unknown errors
The currentelsebranch swallows non-Errorexceptions and gives a generic 500. Addingconsole.error(error)(or your logger) helps troubleshooting in prod.src/entities/subscribers/entity.subscriber.ts (3)
40-50: Hard-coded “system” user will hide real actors
Using fixeduserId/userEmailprevents traceability of changes triggered by HTTP requests (where you do have user context from the audit middleware). Consider passing the request-scoped context throughAsyncLocalStorage, the new TypeORMIsolationLevel, or another DI mechanism so subscribers can obtain the true user identity.
68-76:anytypes flagged by ESLint – tighten typing
The loop operates onRecord<string, unknown>; declaring them as such removes the five@typescript-eslint/no-explicit-anyerrors without behaviour change.-const oldValues: Record<string, any> = {}; -const newValues: Record<string, any> = {}; +const oldValues: Record<string, unknown> = {}; +const newValues: Record<string, unknown> = {};🧰 Tools
🪛 ESLint
[error] 68-68: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 69-69: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
80-96: Possible performance hit when many columns are unchanged
updatedColumnsonly lists changed columns, butevent.entitymay still have large blobs/relations that you spread into the audit record. To keep audit rows lean (and avoid JSON size limits in some DBs), store only the changed column/value pairs already computed above.src/routes/auditLogRoutes.ts (1)
46-53:asyncHandlercan reuse existing community util
Instead of rolling your own wrapper, consider usingexpress-async-handlerwhich is well-tested and removes a few LOC.src/entities/AuditLog.entity.ts (1)
27-32: Consider using a more specific type thanany.Using
Record<string, any>foroldValuesandnewValuesprovides flexibility but reduces type safety. Consider using a more specific type if possible.- @Column("json", { nullable: true }) - oldValues?: Record<string, any>; // used when there is an update, so we can see - // what value was there previously + @Column("json", { nullable: true }) + oldValues?: Record<string, unknown>; // used when there is an update, so we can see + // what value was there previously - @Column("json", { nullable: true }) - newValues?: Record<string, any>; + @Column("json", { nullable: true }) + newValues?: Record<string, unknown>;🧰 Tools
🪛 ESLint
[error] 28-28: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 32-32: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/interfaces/audit.interface.ts (1)
7-8: Consider using a more specific type thanany.Similar to the entity file, consider using
unknowninstead ofanyfor better type safety while maintaining flexibility.- oldValues?: Record<string, any>; - newValues?: Record<string, any>; + oldValues?: Record<string, unknown>; + newValues?: Record<string, unknown>;🧰 Tools
🪛 ESLint
[error] 7-7: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 8-8: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/services/Audit.service.ts (1)
32-32: Remove commented-out code.There are commented-out lines that appear to be leftover from refactoring. These should be removed to keep the codebase clean.
- // const auditLogRepository = this.dataSource.getRepository(AuditLog);- // const auditLogRepository = this.dataSource.getRepository(AuditLog);Also applies to: 46-46
src/services/AuditConfig.service.ts (3)
16-16: Fix typo in method name.There's a grammatical error in the method name
configuresSensitiveFields.- private configuresSensitiveFields() { + private configureSensitiveFields() {
13-13: Update method call to match the corrected method name.If you fix the typo in the method name, update the method call in the constructor as well.
- this.configuresSensitiveFields(); + this.configureSensitiveFields();
1-4: Consider usingunknowninstead ofanyfor better type safety.While flexibility is needed for handling different entity types, using
unknowninstead ofanycan improve type safety while maintaining flexibility.interface SensitiveFieldConfig { fieldName: string; - maskFunction?: (value: any) => any; + maskFunction?: (value: unknown) => unknown; } //... and in the maskSensitiveData method: maskSensitiveData( entityName: string, - data: Record<string, any>, - ): Record<string, any> { + data: Record<string, unknown>, + ): Record<string, unknown> {Also applies to: 39-43
🧰 Tools
🪛 ESLint
[error] 3-3: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 3-3: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/middlewares/entity-audit.middleware.ts (3)
26-31: Consider handling data source initialization error more gracefully.Currently, when the DataSource is not available, you're logging an error and continuing with next(), but it might be better to explicitly return an error status or use a more robust error handling approach. This could help with debugging and ensure consistent behavior.
if (!dataSource) { - console.error("DataSource not available in request"); + console.error("DataSource not available in request for entity audit"); + // Consider returning an appropriate error status instead of silently continuing return next(); }
39-46: Optimize entity retrieval based on request method.Currently, you're fetching the entity for all request types, but this is unnecessary for POST requests (which create new entities). Consider skipping this step for POST requests to improve performance.
+ // Only fetch existing entity for non-POST requests + if (req.method !== "POST") { // Get the entity before any changes const entity = await entityRepository.findOne({ where: { id: entityId }, }); if (!entity) { return next(); } // Store the entity in the request for comparison after operation req.preAuditEntity = entity; + } req.entityType = entityType;
95-97: Enhance error logging for audit failures.The current error logging is minimal. Consider adding more context to help with debugging, such as the entity type and ID that failed to audit.
} catch (error) { - console.error("Error creating audit log:", error); + console.error(`Error creating audit log for ${entityType} (ID: ${entityId}):`, error); + // Consider sending this error to a monitoring service }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
src/app.ts(7 hunks)src/config/db.ts(2 hunks)src/controllers/AuditLogController.ts(1 hunks)src/controllers/AuthController.ts(2 hunks)src/controllers/PaymentLink.controller.ts(1 hunks)src/controllers/twoFactorAuthController.ts(1 hunks)src/controllers/validateTwoFactorAuthentication.ts(1 hunks)src/entities/AuditLog.entity.ts(1 hunks)src/entities/subscribers/entity.subscriber.ts(1 hunks)src/enums/AuditLogAction.ts(1 hunks)src/index.ts(1 hunks)src/interfaces/audit.interface.ts(1 hunks)src/middlewares/audit.middleware.ts(1 hunks)src/middlewares/authMiddleware.ts(2 hunks)src/middlewares/entity-audit.middleware.ts(1 hunks)src/routes/auditLogRoutes.ts(1 hunks)src/routes/health.routes.ts(1 hunks)src/routes/transactionReports.routes.ts(1 hunks)src/services/Audit.service.ts(1 hunks)src/services/AuditConfig.service.ts(1 hunks)src/services/AuthService.ts(1 hunks)src/services/UserService.ts(1 hunks)src/services/emailVerification.service.ts(1 hunks)src/services/inAppNotificationService.ts(1 hunks)src/services/merchant.service.ts(1 hunks)src/services/merchantWebhookQueue.service.ts(1 hunks)src/services/rateLimitMonitoring.service.ts(1 hunks)src/services/session.service.ts(1 hunks)src/services/wallet-verification.service.ts(2 hunks)src/services/webhook.service.ts(1 hunks)src/types/express.d.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
src/services/wallet-verification.service.ts (1)
src/config/db.ts (1)
AppDataSource(16-39)
src/enums/AuditLogAction.ts (1)
src/interfaces/audit.interface.ts (1)
AuditLogActionsEnum(25-25)
src/services/emailVerification.service.ts (1)
src/config/db.ts (1)
AppDataSource(16-39)
src/entities/subscribers/entity.subscriber.ts (2)
src/services/Audit.service.ts (1)
AuditService(9-101)src/interfaces/audit.interface.ts (1)
AuditLogActionsEnum(25-25)
src/controllers/AuditLogController.ts (2)
src/services/Audit.service.ts (1)
AuditService(9-101)src/interfaces/audit.interface.ts (1)
AuditLogActionsEnum(25-25)
src/services/Audit.service.ts (2)
src/interfaces/audit.interface.ts (2)
CreateAditLogParams(3-13)FindAuditLogParams(15-24)src/services/AuditConfig.service.ts (1)
auditConfigService(61-61)
src/entities/AuditLog.entity.ts (1)
src/interfaces/audit.interface.ts (1)
AuditLogActionsEnum(25-25)
src/app.ts (2)
src/config/db.ts (1)
AppDataSource(16-39)src/middlewares/audit.middleware.ts (1)
auditMiddleware(8-37)
🪛 ESLint
src/middlewares/authMiddleware.ts
[error] 30-30: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/types/express.d.ts
[error] 17-17: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/entities/subscribers/entity.subscriber.ts
[error] 28-28: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 59-59: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 68-68: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 69-69: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 102-102: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/services/AuditConfig.service.ts
[error] 3-3: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 3-3: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 41-41: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 42-42: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/entities/AuditLog.entity.ts
[error] 28-28: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 32-32: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/middlewares/entity-audit.middleware.ts
[error] 60-60: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 61-61: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/interfaces/audit.interface.ts
[error] 7-7: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 8-8: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🔇 Additional comments (35)
src/services/UserService.ts (1)
2-2: Update to named import for AppDataSource
The import has been switched to a named export to match the changes insrc/config/db.ts. Please verify thatAppDataSourceis correctly exported as a named constant in yourdb.tsfile and remove any leftover default‐import patterns in this file.src/services/webhook.service.ts (1)
3-3: Switch to named AppDataSource import
You’ve updated the import to a named export from../config/db. Confirm thatAppDataSourceis exported by name indb.ts, and ensure consistency across all services and controllers.src/routes/transactionReports.routes.ts (1)
12-12: Use named import for AppDataSource
This change aligns with the refactored export insrc/config/db.ts. Verify thatAppDataSourceis initialized before this module runs and that no default imports remain.src/services/session.service.ts (1)
2-2: Named import of AppDataSource
The import has been updated to use the namedAppDataSource. Ensure that session cleanup, creation, and retrieval still operate as expected with the refactored data source.src/services/inAppNotificationService.ts (1)
2-2: Align import to named AppDataSource export
You’ve changed the default import to a named import. Confirm thedb.tsexport and remove any outdated import patterns to maintain consistency.src/routes/health.routes.ts (1)
2-2: Align import with new export style for AppDataSource
The change from a default to a named import matches the updated export insrc/config/db.ts. Confirm thatAppDataSourceis correctly exported as a named constant and that there are no residual default exports.src/services/merchant.service.ts (1)
5-5: Ensure named import of AppDataSource
The import statement now correctly uses a named import forAppDataSource, aligning with its definition insrc/config/db.ts. No further action needed.src/controllers/validateTwoFactorAuthentication.ts (1)
3-3: Update AppDataSource import to named export
The import forAppDataSourcehas been updated to a named import, consistent with the changes insrc/config/db.ts. Verify that this matches the export and functions as expected.src/controllers/PaymentLink.controller.ts (1)
5-5: Convert AppDataSource import to named export
This change updates the import to a named import forAppDataSource, matching the new export in the database config. No additional adjustments are needed.src/controllers/twoFactorAuthController.ts (1)
4-4: Switch to named import for AppDataSource
The import has been changed to a named import, ensuring consistency with the export insrc/config/db.ts. Confirm there’s no default export conflict.src/services/merchantWebhookQueue.service.ts (1)
8-8: Import style updated for consistencyThe import has been modified from a default import to a named import for
AppDataSource. This change aligns with the broader refactoring of database access patterns across the codebase.src/services/wallet-verification.service.ts (2)
2-2: Import style updated for consistencyThe import has been changed from a default import to a named import for
AppDataSourcefrom the database configuration, consistent with other files in the codebase.
18-20: Repository initialization updated to use AppDataSourceThis change updates the repository initialization to use the consistent
AppDataSourcepattern, maintaining functionality while aligning with the standardized database access approach.src/services/AuthService.ts (1)
5-5: Import style updated for consistencyThe import has been changed from a default import to a named import for
AppDataSource, aligning with the broader refactoring of how the database data source is imported throughout the application.src/services/emailVerification.service.ts (2)
8-8: Import style updated for consistencyThe import has been changed from a default import to a named import for
AppDataSource, aligning with the standardized approach for database access across the codebase.
16-17: Repository initialization updated to use AppDataSourceThis change updates the repository initialization to use the consistent
AppDataSourcepattern, maintaining functionality while aligning with the standardized database access approach.src/index.ts (2)
2-4: Updated imports for audit logging integrationImport statements have been updated to use named imports and to include the new
EntityAuditSubscriberwhich is essential for the audit logging feature.
12-14: Added database subscriber for audit loggingThis code adds the
EntityAuditSubscriberto the database connection, enabling automatic audit logging of entity changes. This is a crucial component of the audit logging system being implemented in this PR.The subscriber is correctly initialized with the database connection and added to the subscribers array before the server starts, ensuring all database operations will be audited.
src/enums/AuditLogAction.ts (1)
1-5: Well-structured enum for audit log actions.The enum follows TypeScript best practices with clear, descriptive string values for create, update, and delete operations. This will provide type safety throughout the audit logging system.
Consider adding JSDoc comments to explain the purpose of each action value for better documentation.
src/config/db.ts (2)
12-12: Good import addition for the new AuditLog entity.This import correctly brings in the new entity class that will be added to the database schema.
35-35: AuditLog entity correctly added to the entities array.This ensures that TypeORM will manage the AuditLog entity and create the corresponding database table.
src/controllers/AuthController.ts (2)
9-11: Good imports for audit logging functionality.The imports correctly bring in the necessary types and services for audit logging.
23-23: Informative console log.This log provides clarity on the completion of the registration process.
src/middlewares/authMiddleware.ts (4)
11-11: Verify TypeORM import is necessary here.The
DataSourceimport from TypeORM is used in the extended Request interface, but since this file is primarily concerned with authentication, consider moving the database-related extension to another file likesrc/types/express.d.tsto maintain separation of concerns.
13-13: Good change: Extending the correct Express module.Updating the module declaration from "express" to "express-serve-static-core" is correct, as this is the proper module to extend when adding properties to the Express Request interface.
33-33:Details
❌ Incorrect review comment
Make
dataSourceoptional or handle missing cases.Since
dataSourceis not marked as optional but the Request interface is used throughout the application, ensure all request handlers have access to the data source or make this property optional to prevent TypeScript errors.
🏁 Script executed:
#!/bin/bash # Check if there are any places where Request is used without ensuring dataSource exists rg -A 2 -B 2 "req\.(dataSource|db)" --type=tsLength of output: 1427
No change needed:
dataSourceis guaranteed by global middleware
The app registers a top-level middleware insrc/app.tsthat assignsAppDataSourcetoreq.dataSourcebefore any handlers run, ensuring it’s always present. Keeping the property non-optional preserves type safety. The audit middleware even logs a warning if it somehow isn’t set.Likely an incorrect or invalid review comment.
23-34:Details
✅ Verification successful
Improve type safety by avoiding
anytype.The
preAuditEntityproperty usesanywhich reduces type safety. Consider using a generic type parameter or a more specific type that represents entities in your system.- preAuditEntity?: any; + preAuditEntity?: Record<string, unknown>;Also, since this is the auth middleware file but contains audit-related types, consider moving these interface extensions to a dedicated types file for better separation of concerns.
I wasn’t able to locate any other usages or more specific entity types in the repo that would let us tighten
preAuditEntitybeyondany. Also, no existing “Audit” types file exists, so moving just these small bits may be premature.That said, we should still avoid
any. If you don’t have a concrete entity interface today, fall back to the safe, structural type:- preAuditEntity?: any; + preAuditEntity?: Record<string, unknown>;You can later replace
Record<string, unknown>with a proper interface (or a generic type parameter) once your domain models are defined.No further changes required in other files at this time.
🧰 Tools
🪛 ESLint
[error] 30-30: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/middlewares/audit.middleware.ts (2)
20-23: Good implementation of user context extraction.The code correctly extracts user information from the request or falls back to a system user when not available. The IP address extraction also properly handles forwarded IPs, which is good practice for applications behind proxies.
25-30: Good practice: Normalizing user ID to string.Converting the user ID to a string with
String(user.id)is good practice, as it ensures consistency regardless of whether the ID is a number or string in different parts of the application.src/app.ts (2)
96-99: Good implementation: Attaching DataSource to requests.Attaching the
AppDataSourceto each request is a good approach that ensures all route handlers have access to the database connection without needing to import it directly.
130-131: Good implementation: Adding audit logging routes.Adding a dedicated route for audit logs provides a clean API for accessing audit information. This is a good separation of concerns.
src/controllers/AuditLogController.ts (1)
8-8:req.dataSourcemay not be typed onRequest– compile-time error risk
req.dataSourcerelies on an ExpressRequestaugmentation that is not part of the default typings. If the declaration-merging file (src/@types/express/index.d.tsor similar) is missing, the project will failtsc/ESLint.Would you double-check that the custom
declare global { namespace Express { … }}block is present in the repo? If not, please add it or change the controller signature to accept an injectedDataSourceinstance instead.src/entities/AuditLog.entity.ts (1)
1-48: Well-structured audit log entity design.This is a well-designed entity that captures comprehensive audit information including entity details, action type, before/after values, user information, and timestamps.
🧰 Tools
🪛 ESLint
[error] 28-28: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 32-32: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/services/AuditConfig.service.ts (1)
48-57: Good implementation of sensitive data masking.The masking implementation correctly handles undefined fields and properly applies the custom mask functions. The approach of creating a new object to avoid modifying the original data is also good practice.
src/middlewares/entity-audit.middleware.ts (1)
1-106: LGTM! Well-structured middleware factory for entity auditing.Overall, this middleware appears well-designed. It follows the middleware pattern appropriately, handles asynchronous operations correctly, and has good separation of concerns. The factory pattern allows for reuse across different entity types.
With the suggested improvements for type safety, data masking, and performance optimization, this will be a robust solution for audit logging.
🧰 Tools
🪛 ESLint
[error] 60-60: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 61-61: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
| dotenv.config(); | ||
|
|
||
| const AppDataSource = new DataSource({ | ||
| export const AppDataSource = new DataSource({ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Export style change from default to named export.
The switch from a default export to a named export improves code consistency and makes imports more explicit throughout the codebase.
🏁 Script executed:
#!/bin/bash
# Check if all imports of AppDataSource have been updated to use named imports
grep -r "import AppDataSource from" --include="*.ts" .Length of output: 601
Update test imports to use named export
Several test files still import AppDataSource as a default export. Please update these to use the named export syntax.
Files to update:
- src/tests/PaymentService.test.ts
- src/tests/auth.test.ts
- src/tests/health.test.ts
- src/tests/routes/merchantRoutes.test.ts
- src/tests/services/UserService.test.ts
- src/tests/sessionController.test.ts
- src/tests/sessionService.test.ts
Example change:
- import AppDataSource from "../config/db";
+ import { AppDataSource } from "../config/db";Ensure all occurrences are corrected so tests import with braces.
| userId: req.user!.id, | ||
| email: req.user!.email, | ||
| }; |
There was a problem hiding this comment.
Potential runtime error using non-null assertion.
Changing from optional chaining (?.) to non-null assertion (!.) assumes that req.user will always be defined when handling rate limit events. This could lead to runtime errors if any rate-limited endpoint can be accessed without authentication.
Consider adding a null check or using safer fallbacks:
- userId: req.user!.id,
- email: req.user!.email,
+ userId: req.user?.id,
+ email: req.user?.email,Or with fallbacks:
- userId: req.user!.id,
- email: req.user!.email,
+ userId: req.user?.id || undefined,
+ email: req.user?.email || undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| userId: req.user!.id, | |
| email: req.user!.email, | |
| }; | |
| userId: req.user?.id, | |
| email: req.user?.email, |
| userId: req.user!.id, | |
| email: req.user!.email, | |
| }; | |
| userId: req.user?.id || undefined, | |
| email: req.user?.email || undefined, |
| console.log("Done with registration", { user }); | ||
| const auditService = new AuditService(req.dataSource); | ||
| await auditService.createAuditLogs({ | ||
| entityType: "User", | ||
| entityId: String(user.id), | ||
| action: AuditLogActionsEnum.CREATE, | ||
| newValues: req.body, | ||
| userId: String(user.id), | ||
| userEmail: user.email, | ||
| ipAddress: req.auditContext?.ipAddress || "0.0.0.0", | ||
| userAgent: req.auditContext?.userAgent || "Unknown", | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Audit logging implementation for user registration.
The audit logging implementation captures comprehensive information about the registration event, including entity details, user information, and request metadata with appropriate fallbacks.
Consider wrapping the audit logging in a try-catch block to prevent audit logging failures from affecting the user experience:
console.log("Done with registration", { user });
- const auditService = new AuditService(req.dataSource);
- await auditService.createAuditLogs({
- entityType: "User",
- entityId: String(user.id),
- action: AuditLogActionsEnum.CREATE,
- newValues: req.body,
- userId: String(user.id),
- userEmail: user.email,
- ipAddress: req.auditContext?.ipAddress || "0.0.0.0",
- userAgent: req.auditContext?.userAgent || "Unknown",
- });
+ try {
+ const auditService = new AuditService(req.dataSource);
+ await auditService.createAuditLogs({
+ entityType: "User",
+ entityId: String(user.id),
+ action: AuditLogActionsEnum.CREATE,
+ newValues: req.body,
+ userId: String(user.id),
+ userEmail: user.email,
+ ipAddress: req.auditContext?.ipAddress || "0.0.0.0",
+ userAgent: req.auditContext?.userAgent || "Unknown",
+ });
+ } catch (auditError) {
+ console.error("Failed to create audit log for user registration:", auditError);
+ // Continue with the response, don't fail the registration if audit logging fails
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("Done with registration", { user }); | |
| const auditService = new AuditService(req.dataSource); | |
| await auditService.createAuditLogs({ | |
| entityType: "User", | |
| entityId: String(user.id), | |
| action: AuditLogActionsEnum.CREATE, | |
| newValues: req.body, | |
| userId: String(user.id), | |
| userEmail: user.email, | |
| ipAddress: req.auditContext?.ipAddress || "0.0.0.0", | |
| userAgent: req.auditContext?.userAgent || "Unknown", | |
| }); | |
| console.log("Done with registration", { user }); | |
| try { | |
| const auditService = new AuditService(req.dataSource); | |
| await auditService.createAuditLogs({ | |
| entityType: "User", | |
| entityId: String(user.id), | |
| action: AuditLogActionsEnum.CREATE, | |
| newValues: req.body, | |
| userId: String(user.id), | |
| userEmail: user.email, | |
| ipAddress: req.auditContext?.ipAddress || "0.0.0.0", | |
| userAgent: req.auditContext?.userAgent || "Unknown", | |
| }); | |
| } catch (auditError) { | |
| console.error("Failed to create audit log for user registration:", auditError); | |
| // Continue with the response, don't fail the registration if audit logging fails | |
| } |
| // Store the original methods that we'll be wrapping | ||
| const originalJson = res.json; | ||
| const originalSend = res.send; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused response method wrapping.
The middleware stores original res.json and res.send methods but never uses them. Remove these lines since they don't serve any purpose.
- // Store the original methods that we'll be wrapping
- const originalJson = res.json;
- const originalSend = res.send;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Store the original methods that we'll be wrapping | |
| const originalJson = res.json; | |
| const originalSend = res.send; |
| const originalJson = res.json; | ||
| const originalSend = res.send; | ||
|
|
||
| console.log("Entered here ", { originalJson, originalSend }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debugging console.log statement.
Debug logging statements should not be included in production code. Remove this console.log statement.
- console.log("Entered here ", { originalJson, originalSend });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("Entered here ", { originalJson, originalSend }); |
| CreateAditLogParams, | ||
| FindAuditLogParams, | ||
| } from "src/interfaces/audit.interface"; |
There was a problem hiding this comment.
Update import to match the corrected interface name.
If you fix the typo in the interface name CreateAditLogParams to CreateAuditLogParams, don't forget to update the import here.
import {
- CreateAditLogParams,
+ CreateAuditLogParams,
FindAuditLogParams,
} from "src/interfaces/audit.interface";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CreateAditLogParams, | |
| FindAuditLogParams, | |
| } from "src/interfaces/audit.interface"; | |
| import { | |
| CreateAuditLogParams, | |
| FindAuditLogParams, | |
| } from "src/interfaces/audit.interface"; |
| async createAuditLogs(payload: CreateAditLogParams) { | ||
| try { | ||
| const maskedOldValues = payload.oldValues | ||
| ? auditConfigService.maskSensitiveData( | ||
| payload.entityType, | ||
| payload.oldValues, | ||
| ) | ||
| : undefined; | ||
|
|
||
| const maskedNewValues = payload.newValues | ||
| ? auditConfigService.maskSensitiveData( | ||
| payload.entityType, | ||
| payload.newValues, | ||
| ) | ||
| : undefined; | ||
|
|
||
| // const auditLogRepository = this.dataSource.getRepository(AuditLog); | ||
| const auditLog = this.auditLogRepository.create({ | ||
| ...payload, | ||
| oldValues: maskedOldValues, | ||
| newValues: maskedNewValues, | ||
| }); | ||
|
|
||
| return this.auditLogRepository.save(auditLog); | ||
| } catch (error) { | ||
| console.log({ error }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in createAuditLogs.
The method currently only logs errors to the console without returning any error status or throwing an exception. This could lead to silent failures that are difficult to debug.
async createAuditLogs(payload: CreateAditLogParams) {
try {
const maskedOldValues = payload.oldValues
? auditConfigService.maskSensitiveData(
payload.entityType,
payload.oldValues,
)
: undefined;
const maskedNewValues = payload.newValues
? auditConfigService.maskSensitiveData(
payload.entityType,
payload.newValues,
)
: undefined;
// const auditLogRepository = this.dataSource.getRepository(AuditLog);
const auditLog = this.auditLogRepository.create({
...payload,
oldValues: maskedOldValues,
newValues: maskedNewValues,
});
return this.auditLogRepository.save(auditLog);
} catch (error) {
- console.log({ error });
+ console.error('Failed to create audit log:', error);
+ throw new Error(`Failed to create audit log: ${error instanceof Error ? error.message : String(error)}`);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async createAuditLogs(payload: CreateAditLogParams) { | |
| try { | |
| const maskedOldValues = payload.oldValues | |
| ? auditConfigService.maskSensitiveData( | |
| payload.entityType, | |
| payload.oldValues, | |
| ) | |
| : undefined; | |
| const maskedNewValues = payload.newValues | |
| ? auditConfigService.maskSensitiveData( | |
| payload.entityType, | |
| payload.newValues, | |
| ) | |
| : undefined; | |
| // const auditLogRepository = this.dataSource.getRepository(AuditLog); | |
| const auditLog = this.auditLogRepository.create({ | |
| ...payload, | |
| oldValues: maskedOldValues, | |
| newValues: maskedNewValues, | |
| }); | |
| return this.auditLogRepository.save(auditLog); | |
| } catch (error) { | |
| console.log({ error }); | |
| } | |
| } | |
| async createAuditLogs(payload: CreateAditLogParams) { | |
| try { | |
| const maskedOldValues = payload.oldValues | |
| ? auditConfigService.maskSensitiveData( | |
| payload.entityType, | |
| payload.oldValues, | |
| ) | |
| : undefined; | |
| const maskedNewValues = payload.newValues | |
| ? auditConfigService.maskSensitiveData( | |
| payload.entityType, | |
| payload.newValues, | |
| ) | |
| : undefined; | |
| // const auditLogRepository = this.dataSource.getRepository(AuditLog); | |
| const auditLog = this.auditLogRepository.create({ | |
| ...payload, | |
| oldValues: maskedOldValues, | |
| newValues: maskedNewValues, | |
| }); | |
| return this.auditLogRepository.save(auditLog); | |
| } catch (error) { | |
| console.error('Failed to create audit log:', error); | |
| throw new Error( | |
| `Failed to create audit log: ${ | |
| error instanceof Error ? error.message : String(error) | |
| }` | |
| ); | |
| } | |
| } |
| await auditService.createAuditLogs({ | ||
| action: action, | ||
| entityId: entityId, | ||
| entityType: entityType, | ||
| oldValues, | ||
| newValues, | ||
| ipAddress: req.auditContext!.ipAddress, | ||
| userAgent: req.auditContext!.userAgent, | ||
| userEmail: req.auditContext!.userEmail, | ||
| userId: req.auditContext!.userId, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider masking sensitive data before logging.
The audit logs include both old and new values of the entity, which might contain sensitive information like passwords, API keys, or personal data. Consider implementing data masking for sensitive fields.
You could implement a sanitization function:
function sanitizeEntityData(data: Record<string, unknown>, entityType: string): Record<string, unknown> {
if (!data) return data;
const sensitiveFields = getSensitiveFieldsForEntity(entityType);
const sanitized = { ...data };
for (const field of sensitiveFields) {
if (field in sanitized) {
sanitized[field] = '***REDACTED***';
}
}
return sanitized;
}And then use it before passing values to createAuditLogs:
await auditService.createAuditLogs({
action: action,
entityId: entityId,
entityType: entityType,
- oldValues,
- newValues,
+ oldValues: oldValues ? sanitizeEntityData(oldValues, entityType) : undefined,
+ newValues: newValues ? sanitizeEntityData(newValues, entityType) : undefined,
ipAddress: req.auditContext!.ipAddress,
userAgent: req.auditContext!.userAgent,
userEmail: req.auditContext!.userEmail,
userId: req.auditContext!.userId,
});| req.preAuditEntity = entity; | ||
| req.entityType = entityType; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extend the Request type definition for custom properties.
You're adding custom properties to the Request object (preAuditEntity and entityType), but these aren't defined in the Express Request interface. This could lead to TypeScript errors or runtime issues.
Make sure to extend the Express Request interface in your type definitions (likely in src/types/express.d.ts) to include these properties:
declare namespace Express {
export interface Request {
// Existing properties like auditContext
preAuditEntity?: any; // Use proper entity type instead of any
entityType?: string;
}
}| let oldValues: Record<string, any> | undefined; | ||
| let newValues: Record<string, any> | undefined; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any with more specific types.
Using any type reduces type safety. Consider defining more specific types for better type checking and maintainability.
- let oldValues: Record<string, any> | undefined;
- let newValues: Record<string, any> | undefined;
+ let oldValues: Record<string, unknown> | undefined;
+ let newValues: Record<string, unknown> | undefined;Alternatively, you could define a more specific entity interface based on your entity model.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let oldValues: Record<string, any> | undefined; | |
| let newValues: Record<string, any> | undefined; | |
| let oldValues: Record<string, unknown> | undefined; | |
| let newValues: Record<string, unknown> | undefined; |
🧰 Tools
🪛 ESLint
[error] 60-60: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 61-61: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
MPSxDev
left a comment
MPSxDev
left a comment
There was a problem hiding this comment.
Great Job @nwokporochukwuebuka Please solve potential issues and refactor suggestion. and make sure it build correctly.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/app.ts (2)
84-92: Remove redundant commented-out database initialization codeThis commented-out database initialization code should be removed completely as it creates confusion. According to the codebase, initialization already happens in src/index.ts and health routes.
142-142: Update all imports of app throughout the codebaseYou've changed the export from default to named export, which will break existing imports.
As noted in previous review comments, several test files still use the default import. Update all import statements in files like:
- src/tests/health.test.ts
- src/tests/auth.test.ts
- src/tests/transactionReportsRoute.test.ts
- src/tests/routes/merchantRoutes.test.ts
Example change:
-import app from "../app"; +import { app } from "../app";Let's verify that all imports have been updated:
#!/bin/bash # Find all imports of app that may need updating rg "import .* from ['\"].*app['\"]" --type=ts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/app.ts(6 hunks)src/config/db.ts(2 hunks)src/controllers/PaymentLink.controller.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/controllers/PaymentLink.controller.ts
- src/config/db.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/app.ts (1)
src/config/db.ts (1)
AppDataSource(18-42)
🪛 ESLint
src/app.ts
[error] 95-95: 'AppDataSource' is not defined.
(no-undef)
[error] 114-114: 'stellarContractRoutes' is not defined.
(no-undef)
🔇 Additional comments (1)
src/app.ts (1)
7-7: LGTM - New imports added for audit functionalityThese imports support the new audit logging functionality being introduced. The
NextFunctiontype is needed for the new middleware, andauditMiddlewarewill be used to track user actions.Also applies to: 36-36
| import emailVerification from "./routes/emailVerification.routes"; | ||
| import PaymentRoute from "./routes/PaymentLink.routes"; | ||
| import authRoutes from "./routes/authRoutes"; | ||
| import auditLogRoute from "./routes/auditLogRoutes"; |
There was a problem hiding this comment.
Import added but route is commented out
You've imported auditLogRoute but the route registration on line 113 is commented out. Either uncomment the route registration or remove this import if it's not being used yet.
| app.use(((req: Request, res: Response, next: NextFunction) => { | ||
| req.dataSource = AppDataSource; | ||
| next(); | ||
| }) as RequestHandler); |
There was a problem hiding this comment.
Missing import for AppDataSource
The middleware attaches AppDataSource to the request object, but AppDataSource is not imported. This will cause runtime errors.
Add the import at the top of the file:
import logger from "./utils/logger";
import { auth } from "express-openid-connect";
import { auditMiddleware } from "./middlewares/audit.middleware";
+import { AppDataSource } from "./config/db";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| app.use(((req: Request, res: Response, next: NextFunction) => { | |
| req.dataSource = AppDataSource; | |
| next(); | |
| }) as RequestHandler); | |
| import logger from "./utils/logger"; | |
| import { auth } from "express-openid-connect"; | |
| import { auditMiddleware } from "./middlewares/audit.middleware"; | |
| import { AppDataSource } from "./config/db"; |
🧰 Tools
🪛 ESLint
[error] 95-95: 'AppDataSource' is not defined.
(no-undef)
| //app.use("/audit-log", auditLogRoute); | ||
| app.use("/api/v1/stellar", stellarContractRoutes); |
There was a problem hiding this comment.
Inconsistent route configuration
There are two issues here:
- The audit log route is commented out (line 113), which is inconsistent with the purpose of this PR
stellarContractRoutesis being used (line 114) but its import on line 24 is commented out
Fix these issues with the following changes:
-//app.use("/audit-log", auditLogRoute);
-app.use("/api/v1/stellar", stellarContractRoutes);
+app.use("/audit-log", auditLogRoute);
+//app.use("/api/v1/stellar", stellarContractRoutes);Or uncomment the import on line 24:
-//import stellarContractRoutes from "./routes/stellar-contract.routes";
+import stellarContractRoutes from "./routes/stellar-contract.routes";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //app.use("/audit-log", auditLogRoute); | |
| app.use("/api/v1/stellar", stellarContractRoutes); | |
| // In src/app.ts, replace lines 113–114: | |
| app.use("/audit-log", auditLogRoute); | |
| //app.use("/api/v1/stellar", stellarContractRoutes); |
🧰 Tools
🪛 ESLint
[error] 114-114: 'stellarContractRoutes' is not defined.
(no-undef)
|
If there's no progress by this weekend, I'll unassign it to reassign in the next OD @nwokporochukwuebuka |
3 participants
Pull Request Overview
📝 Summary
Provide a brief overview of what this PR accomplishes.
This PR adds audit logs and also database subscribers
🔗 Related Issues
🔄 Changes Made
Please provide a general description of your changes. Include any relevant background information or context that may help reviewers understand the purpose of this PR.
This PR gives two things how to handle logs through out the application using a middleware that helps users log any create, update and delete request.
Also there is a database subscriber that listens to every insert, update and delete and records the action
🖼️ Current Output
Give evidence like screenshots of what your job looks like.
That is my POSTMAN docs of fetching the audit logs
🧪 Testing
If applicable, describe the tests performed. Provide screenshots, test outputs, or any resources that can help reviewers understand how the changes were tested.
💬 Comments
Any additional context, questions, or considerations for the reviewers.
Yes, a lot of people working on this codebase don't test their code and its causing a big isse
Summary by CodeRabbit
New Features
Bug Fixes
Chores