coderabbit feedback

Author: dblythy

spec/AuditLogController.spec.js

@@ -482,10 +482,4 @@ describe('AuditLogController', () => {
       expect(controller.isEnabled()).toBe(false);
     });
   });
-
-  describe('expectedAdapterType', () => {
-    it('should return AuditLogAdapter', () => {
-      expect(controller.expectedAdapterType()).toBe(AuditLogAdapter);
-    });
-  });
 });

spec/Auth.spec.js

@@ -320,8 +320,11 @@ describe('Audit Logging - User Authentication', () => {
 
     try {
       await Parse.User.logIn('audituser2', 'wrongpassword');
+      fail('Expected login to fail with wrong password');
     } catch (error) {
-      // Expected error
+      // Verify this is the expected authentication failure
+      expect(error.code).toBe(Parse.Error.OBJECT_NOT_FOUND);
+      expect(error.message).toContain('Invalid username/password');
     }
 
     await new Promise(resolve => setTimeout(resolve, 200));

spec/ParseObject.spec.js

@@ -2371,8 +2371,6 @@ describe('Audit Logging - CRUD Operations', () => {
   });
 
   it('should not log internal master key operations without user', async () => {
-    Parse.Cloud.useMasterKey();
-
     const TestClass = Parse.Object.extend('AuditCRUDInternal');
     const obj = new TestClass();
     obj.set('name', 'internal');

src/Adapters/Logger/AuditLogger.js

@@ -63,7 +63,25 @@ export function configureAuditLogger({
   maxFiles,
 } = {}) {
   if (!auditLogFolder) {
-    // Audit logging disabled
+    // Audit logging disabled - close and remove any existing transports
+    try {
+      if (auditLogger.transports && auditLogger.transports.length > 0) {
+        // Close all transports
+        auditLogger.transports.forEach(transport => {
+          try {
+            if (transport.close) {
+              transport.close();
+            }
+          } catch (err) {
+            // Ignore errors during transport cleanup
+          }
+        });
+        // Clear all transports
+        auditLogger.clear();
+      }
+    } catch (err) {
+      // Ignore errors during cleanup
+    }
     return;
   }
 
@@ -77,6 +95,23 @@ export function configureAuditLogger({
   } catch (e) {
     // eslint-disable-next-line no-console
     console.error('Failed to create audit log folder:', e);
+    // Clean up existing transports since audit logging cannot be enabled
+    try {
+      if (auditLogger.transports && auditLogger.transports.length > 0) {
+        auditLogger.transports.forEach(transport => {
+          try {
+            if (transport.close) {
+              transport.close();
+            }
+          } catch (err) {
+            // Ignore errors during transport cleanup
+          }
+        });
+        auditLogger.clear();
+      }
+    } catch (err) {
+      // Ignore errors during cleanup
+    }
     return;
   }
 

src/Controllers/AuditLogController.js

@@ -1,14 +1,14 @@
-import AdaptableController from './AdaptableController';
 import { AuditLogAdapter } from '../Adapters/Logger/AuditLogAdapter';
 
 /**
  * AuditLogController
  * Controller for managing GDPR-compliant audit logging
  */
-export class AuditLogController extends AdaptableController {
+export class AuditLogController {
   constructor(adapter, appId, options = {}) {
-    super(adapter, appId, options);
     this.adapter = adapter;
+    this.appId = appId;
+    this.options = options;
   }
 
   /**
@@ -311,11 +311,7 @@ export class AuditLogController extends AdaptableController {
    * @returns {boolean} True if enabled
    */
   isEnabled() {
-    return this.adapter && this.adapter.isEnabled();
-  }
-
-  expectedAdapterType() {
-    return AuditLogAdapter;
+    return !!(this.adapter && this.adapter.isEnabled());
   }
 }
 

src/RestQuery.js

@@ -978,8 +978,7 @@ _UnsafeRestQuery.prototype.logAuditDataView = function () {
     });
   } catch (error) {
     // Don't fail the query if audit logging fails
-    // eslint-disable-next-line no-console
-    console.error('Audit logging error:', error);
+    this.config.loggerController.error('Audit logging error in RestQuery', { error });
   }
 
   return Promise.resolve();

src/RestWrite.js

@@ -1803,7 +1803,9 @@ RestWrite.prototype.logAuditDataWrite = function () {
     return Promise.resolve();
   }
 
-  if ((this.auth.isMaster || this.auth.isMaintenance) && !this.auth.user) {
+  // Skip only master key operations without a user context
+  // Maintenance mode operations should still be audited
+  if (this.auth.isMaster && !this.auth.user) {
     return Promise.resolve();
   }
 
@@ -1815,8 +1817,10 @@ RestWrite.prototype.logAuditDataWrite = function () {
   const isCreate = !this.query;
   const isUpdate = !!this.query;
 
-  const aclModified = this.originalData && this.originalData.ACL && this.data.ACL &&
-    JSON.stringify(this.originalData.ACL) !== JSON.stringify(this.data.ACL);
+  // Check if ACL was modified, including cases where ACL was added or removed
+  const originalACL = this.originalData?.ACL ?? null;
+  const newACL = this.data?.ACL ?? null;
+  const aclModified = isUpdate && JSON.stringify(originalACL) !== JSON.stringify(newACL);
 
   try {
     if (isCreate) {
@@ -1849,15 +1853,14 @@ RestWrite.prototype.logAuditDataWrite = function () {
         req: { config: this.config },
         className: this.className,
         objectId: objectId,
-        oldACL: this.originalData.ACL,
-        newACL: this.data.ACL,
+        oldACL: originalACL,
+        newACL: newACL,
         success: true,
       });
     }
   } catch (error) {
     // Don't fail the write if audit logging fails
-    // eslint-disable-next-line no-console
-    console.error('Audit logging error:', error);
+    this.config.loggerController.error('Audit logging error in RestWrite', { error });
   }
 
   return Promise.resolve();

src/Routers/UsersRouter.js

@@ -317,7 +317,7 @@ export class UsersRouter extends ClassesRouter {
 
       if (req.config.auditLogController) {
         req.config.auditLogController.logUserLogin({
-          auth: { ...req.auth, user: afterLoginUser, sessionToken: user.sessionToken },
+          auth: { ...req.auth, user: afterLoginUser, sessionToken: user.sessionToken ? '***masked***' : undefined },
           req,
           username: user.username || user.email,
           success: true,
@@ -393,7 +393,7 @@ export class UsersRouter extends ClassesRouter {
       if (req.config.auditLogController) {
         const afterLoginUser = Parse.User.fromJSON(Object.assign({ className: '_User' }, user));
         req.config.auditLogController.logUserLogin({
-          auth: { ...req.auth, user: afterLoginUser, sessionToken: user.sessionToken },
+          auth: { ...req.auth, user: afterLoginUser, sessionToken: user.sessionToken ? '***masked***' : undefined },
           req,
           username: user.username || user.email || userId,
           success: true,