Audit on user password + profile change (#1800)

* add audit on user password/profile change

* Update api/main_endpoints/routes/User.js

Co-authored-by: Evan Ugarte <[email protected]>

* delete unused endpoint

* fix User tests + lint

---------

Co-authored-by: Evan Ugarte <[email protected]>

Author: marvzhai

api/main_endpoints/routes/User.js

@@ -24,6 +24,8 @@ const {
   SERVER_ERROR,
 } = require('../../util/constants').STATUS_CODES;
 const membershipState = require('../../util/constants').MEMBERSHIP_STATE;
+const AuditLog = require('../models/AuditLog.js');
+const AuditLogActions = require('../util/auditLogActions.js');
 
 const logger = require('../../util/logger');
 
@@ -32,9 +34,6 @@ const crypto = require('crypto');
 
 const ROWS_PER_PAGE = 20;
 
-const AuditLogActions = require('../util/auditLogActions.js');
-const AuditLog = require('../models/AuditLog.js');
-
 // Delete a member
 router.post('/delete', async (req, res) => {
   if (!checkIfTokenSent(req)) {
@@ -198,6 +197,25 @@ router.post('/edit', async (req, res) => {
   const query = { _id: req.body._id };
   let user = req.body;
 
+  // keep track of user in db
+  const existingUser = await User.findById(req.body._id);
+  if (!existingUser) {
+    return res.status(NOT_FOUND).send({ message: 'User not found.' });
+  }
+
+  // Track field changes
+  const fieldChanges = {};
+  const fieldsToTrack = ['firstName', 'lastName', 'email', 'accessLevel', 'major', 'discordID', 'emailOptIn', 'membershipValidUntil'];
+
+  fieldsToTrack.forEach(field => {
+    if (user[field] !== undefined && user[field] !== existingUser[field]) {
+      fieldChanges[field] = {
+        from: existingUser[field],
+        to: user[field]
+      };
+    }
+  });
+
   if (typeof req.body.numberOfSemestersToSignUpFor !== 'undefined') {
     user.membershipValidUntil = getMemberExpirationDate(
       parseInt(req.body.numberOfSemestersToSignUpFor)
@@ -213,6 +231,14 @@ router.post('/edit', async (req, res) => {
       return res.sendStatus(SERVER_ERROR);
     }
     user.password = result;
+
+    // create audit log for password change
+    AuditLog.create({
+      userId: decoded._id,
+      action: AuditLogActions.CHANGE_PW,
+      details: { email: existingUser.email, userId: decoded._id },
+    }).catch(logger.error);
+
   } else {
     // omit password from the object if it is falsy
     // i.e. an empty string, undefined or null
@@ -236,24 +262,28 @@ router.post('/edit', async (req, res) => {
     if (result.nModified < 1) {
       return res
         .status(NOT_FOUND)
-        .send({ message: `${query.email} not found.` });
+        .send({ message: `${existingUser.email} not found.` });
     }
 
-    const sanitizedUser = {...user}; // shallow copy of the user; doesn't affect original
+    if (Object.keys(fieldChanges).length > 0) {
+      const sanitizedUser = {...user};
+      if ('password' in sanitizedUser) {
+        sanitizedUser.password = true;
+      }
 
-    if ('password' in sanitizedUser) {
-      sanitizedUser.password = true;
+      AuditLog.create({
+        userId: decoded._id, // person who did modification
+        action: AuditLogActions.UPDATE_USER,
+        documentId: user._id,
+        details: {
+          updatedInfo: sanitizedUser,
+          fieldChanges: fieldChanges
+        }
+      }).catch(logger.error);
     }
 
-    AuditLog.create({
-      userId: decoded._id, // the user making the update
-      action: AuditLogActions.UPDATE_USER,
-      documentId: user._id, // the user affected by the update
-      details: {updatedInfo: sanitizedUser}
-    }).catch(logger.error);
-
     return res.status(OK).send({
-      message: `${query.email} was updated.`,
+      message: `${existingUser.email} was updated.`,
       membershipValidUntil: user.membershipValidUntil
     });
   });
@@ -272,8 +302,9 @@ router.post('/getPagesPrintedCount', (req, res) => {
         apiEndpoint: 'user/PagesPrintedCount',
         errorDescription: error
       };
+      logger.error(info);
 
-      res.status(BAD_REQUEST).send({ message: 'Bad Request.' });
+      return res.status(BAD_REQUEST).send({ message: 'Bad Request.' });
     }
 
     if (!result) {

test/api/User.js

@@ -21,6 +21,8 @@ const sinon = require('sinon');
 const SceApiTester = require('../util/tools/SceApiTester');
 const {mockDayMonthAndYear, revertClock} = require('../util/mocks/Date.js');
 
+const AuditLog = require('../../api/main_endpoints/models/AuditLog.js');
+const AuditLogActions = require('../../api/main_endpoints/util/auditLogActions.js');
 
 let app = null;
 let test = null;
@@ -44,9 +46,6 @@ const {
 const { MEMBERSHIP_STATE } = require('../../api/util/constants');
 const { getMemberExpirationDate } = require('../../api/main_endpoints/util/userHelpers.js');
 
-const AuditLogActions = require('../../api/main_endpoints/util/auditLogActions.js');
-const AuditLog = require('../../api/main_endpoints/models/AuditLog.js');
-
 chai.should();
 chai.use(chaiHttp);
 
@@ -212,51 +211,158 @@ describe('User', () => {
       expect(result).to.have.status(NOT_FOUND);
     });
 
-    it('Should return statusCode 200 and a message ' +
-      'if a user was edited', async () => {
-      const user = {
-        _id: id,
-        email: '[email protected]',
-        token: token,
-        firstName: 'pinkUnicorn',
-        discordID: '0987654321',
-        numberOfSemestersToSignUpFor: undefined
-      };
-      setTokenStatus(true);
-      const result = await test.sendPostRequestWithToken(
-        token, '/api/User/edit', user);
-      expect(result).to.have.status(OK);
-      result.body.should.be.a('object');
-      result.body.should.have.property('message');
-    });
+    describe('create audit log on user change', async () => {
 
-    it('Should create an audit log when a user is updated', async () => {
-      // ensure Audit log DB starts fresh before this test
-      await AuditLog.deleteMany({});
-      // update email, firstname, password, and discordID
-      const user = {
-        _id: id,
-        email: '[email protected]',
-        password: 'newPassword',
-        token: token,
-        firstName: 'Newname',
-        discordID: '421482148',
-        numberOfSemestersToSignUpFor: undefined
-      };
-      setTokenStatus(true, user);
+      // create clean testUser before each test
+      let testUser;
 
-      const result = await test.sendPostRequestWithToken(
-        token, '/api/User/edit', user
-      );
-      expect(result).to.have.status(OK);
+      beforeEach(async () => {
+        await User.deleteMany({});
+        testUser = await new User({
+          email: '[email protected]',
+          password: 'Passw0rd',
+          firstName: 'first-name',
+          lastName: 'last-name',
+          major: 'Computer Science'
+        }).save();
+
+        setTokenStatus(true, testUser);
+      });
+
+      afterEach(async () => {
+        await AuditLog.deleteMany({});
+      });
+
+      it('Should create an audit log when a user is updated (no password change)' + 'not create an audit log for password change', async () => {
+        const res = await test.sendPostRequestWithToken(token, '/api/User/edit', {
+          _id: testUser._id.toString(),
+          firstName: 'Newname',
+          email: '[email protected]',
+          token
+        });
+
+        expect(res).to.have.status(OK);
+        res.body.should.be.a('object');
+        res.body.should.have.property('message');
+
+        const auditEntry = await AuditLog.findOne({ userId: testUser._id }).lean();
+        expect(auditEntry).to.exist;
+        expect(auditEntry.details.fieldChanges.firstName).to.have.deep.equal({
+          from: 'first-name',
+          to: 'Newname'
+        });
+
+        // make sure pw change log doesn't exist
+        const changePWlog = await AuditLog.findOne({
+          userId: id,
+          action: AuditLogActions.CHANGE_PW
+        }).lean();
+        expect(changePWlog).to.not.exist;
+      });
 
-      const auditEntry = await AuditLog.findOne().lean();
+      it('Should create an audit log when a user changes their password (no profile update)', async () => {
+        const res = await test.sendPostRequestWithToken(token, '/api/User/edit', {
+          _id: testUser._id.toString(),
+          firstName: 'first-name',
+          email: '[email protected]',
+          password: 'Newpassw0rd',
+          token
+        });
+
+        expect(res).to.have.status(OK);
+
+        const auditEntry = await AuditLog.findOne({ userId: testUser._id }).lean();
+        expect(auditEntry).to.exist;
+        expect(auditEntry.action).to.equal(AuditLogActions.CHANGE_PW);
+        expect(auditEntry).to.not.have.property('password');
+      });
+
+      it('Should create both audit logs when password and profile info are updated', async () => {
+        const res = await test.sendPostRequestWithToken(token, '/api/User/edit', {
+          _id: testUser._id.toString(),
+          firstName: 'Newname',
+          email: '[email protected]',
+          password: 'Newpassword1',
+          discordID: 'anotherID',
+          token
+        });
+
+        expect(res).to.have.status(OK);
+
+        const changePwLog = await AuditLog.findOne({ action: AuditLogActions.CHANGE_PW }).lean();
+        const updateUserLog = await AuditLog.findOne({ action: AuditLogActions.UPDATE_USER }).lean();
+
+        expect(changePwLog).to.exist;
+        expect(updateUserLog).to.exist;
+      });
+
+      it('Should track profile changes in audit log with correct from/to values', async () => {
+        const res = await test.sendPostRequestWithToken(token, '/api/User/edit', {
+          _id: testUser._id.toString(),
+          firstName: 'Newname',
+          lastName: 'Newlastname',
+          email: '[email protected]',
+          password: 'Newpassword1',
+          discordID: 'anotherID',
+          major: 'Software Engineering',
+          token
+        });
+
+        expect(res).to.have.status(OK);
+
+        const auditEntry = await AuditLog.findOne({
+          action: AuditLogActions.UPDATE_USER,
+          documentId: testUser._id
+        }).lean();
+
+        expect(auditEntry).to.exist;
+        expect(auditEntry.details).to.have.property('fieldChanges');
+
+        // track firstName change
+        expect(auditEntry.details.fieldChanges).to.have.property('firstName');
+        expect(auditEntry.details.fieldChanges.firstName).to.have.deep.equal({
+          from: 'first-name',
+          to: 'Newname'
+        });
+
+        // track lastName change
+        expect(auditEntry.details.fieldChanges).to.have.property('lastName');
+        expect(auditEntry.details.fieldChanges.lastName).to.have.deep.equal({
+          from: 'last-name',
+          to: 'Newlastname'
+        });
+
+        // track major change
+        expect(auditEntry.details.fieldChanges).to.have.property('major');
+        expect(auditEntry.details.fieldChanges.major).to.have.deep.equal({
+          from: 'Computer Science',
+          to: 'Software Engineering'
+        });
+
+        // Should NOT track unchanged fields + password field
+        expect(auditEntry.details.fieldChanges).to.not.have.property('password');
+        expect(auditEntry.details.fieldChanges).to.not.have.property('email');
+      });
 
-      expect(auditEntry).to.exist;
-      expect(auditEntry).to.have.property('userId');
-      expect(auditEntry).to.have.property('action', AuditLogActions.UPDATE_USER);
-      expect(auditEntry.details.updatedInfo).to.have.property('password', true);
-      await AuditLog.deleteMany({});
+      it('Should not create audit log when no fields actually change', async () => {
+        const res = await test.sendPostRequestWithToken(token, '/api/User/edit', {
+          _id: testUser._id.toString(),
+          email: '[email protected]',
+          password: 'Passw0rd',
+          firstName: 'first-name',
+          lastName: 'last-name',
+          major: 'Computer Science'
+        });
+
+        expect(res).to.have.status(OK);
+
+        const auditEntry = await AuditLog.findOne({
+          action: AuditLogActions.UPDATE_USER || AuditLogActions.CHANGE_PW,
+          documentId: testUser._id
+        }).lean();
+
+        expect(auditEntry).to.not.exist;
+      });
     });
   });
 
@@ -288,14 +394,27 @@ describe('User', () => {
       expect(result).to.have.status(NOT_FOUND);
     });
     it('Should return status code 200 if user is found', async () => {
-      const user = {
-        userID: id,
-        token: token
-      };
-      setTokenStatus(true);
-      const result = await test.sendPostRequestWithToken(token, '/api/User/getUserById', user);
-      expect(result).to.have.status(OK);
-      result.body.should.not.have.property('password');
+
+      const testUser = await new User({
+        email: '[email protected]',
+        password: 'Passw0rd',
+        firstName: 'Get',
+        lastName: 'User',
+        accessLevel: MEMBERSHIP_STATE.ADMIN,
+        emailVerified: true
+      }).save();
+
+      // Set token mock for this user
+      setTokenStatus(true, { _id: testUser._id, accessLevel: MEMBERSHIP_STATE.ADMIN });
+
+      const res = await test.sendPostRequestWithToken('', '/api/User/getUserById', {
+        userID: testUser._id,
+        token: ''
+      });
+
+      expect(res).to.have.status(OK);
+      res.body.should.have.property('email').eql('[email protected]');
+      res.body.should.not.have.property('password');
     });
   });
 
@@ -351,13 +470,17 @@ describe('User', () => {
 
     it('Should return statusCode 200 and a message ' +
       'if a user was deleted', async () => {
-      const user = {
+      const user = await new User({
         _id : id,
+        email: '[email protected]',
+        password: 'Passw0rd',
+        firstName: 'Delete',
+        lastName: 'Me',
         token: token
-      };
-      setTokenStatus(true);
+      }).save();
+      setTokenStatus(true, { _id: user._id, accesslevel: MEMBERSHIP_STATE.ADMIN });
       const result = await test.sendPostRequestWithToken(
-        token, '/api/User/delete', user);
+        token, '/api/User/delete', { _id: user._id, token: token } );
       expect(result).to.have.status(OK);
     });