Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix event audit logging #3771

Closed
2 tasks done
svenaas opened this issue Feb 3, 2022 · 2 comments
Closed
2 tasks done

Fix event audit logging #3771

svenaas opened this issue Feb 3, 2022 · 2 comments
Assignees
Labels

Comments

@svenaas
Copy link
Contributor

svenaas commented Feb 3, 2022

User Story

As an operator I want to be able to rely on audit logging working where we expect it to.

Currently of 33 calls to Event.audit I believe only 15 are using the correct parameter list, and 18 are using a list which does not work.

Background (Optional)

Most of our event logging calls EventCreator.audit, which takes these parameters:

 * @param {string} label The Event label
 * @param {object} model An instance of the target Sequelize model
 * @param {string} message A brief description of the event
 * @param {object} [body] Additional attributes

Here is an example call from the admin Site controller, which appears to be valid:

EventCreator.audit(Event.labels.ADMIN_ACTION, req.user, 'Site Activated', { site });

Activating a site via the admin UI (which I believe should call this method) raises no errors.

But we also have many calls in the codebase which appear to be invalid, like this example, from the [non-admin] Site controller:

EventCreator.audit(req.user, Event.labels.USER_ACTION, 'Site Updated', {
  site: { ...updateParams, id: site.id },
});

Updating a site via the front end (I specified a demo branch and saved) raises this error:

SequelizeValidationError: string violation: label cannot be an array or an object,
Validation error: Invalid event label: [object SequelizeInstance:User],
Validation error: Invalid event model: String
    at InstanceValidator._validate (/app/node_modules/sequelize/lib/instance-validator.js:78:13)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async InstanceValidator._validateAndRunHooks (/app/node_modules/sequelize/lib/instance-validator.js:111:7)
    at async InstanceValidator.validate (/app/node_modules/sequelize/lib/instance-validator.js:93:12)
    at async model.save (/app/node_modules/sequelize/lib/model.js:3877:7)
    at async Function.create (/app/node_modules/sequelize/lib/model.js:2207:12) {"service":"federalistapp-development"}

The error in this case is that the user parameter is being passed before the event label instead of the other way around. Perhaps this method signature changed at some point in the past. In any event I'm guessing it hasn't been noticed because a) it does not prevent the site update from being saved, and b) it doesn't blow up in testing, since this audit call is being made from the controller, which does not currently have its own tests.

Changing the call to

EventCreator.audit(Event.labels.USER_ACTION, req.user, 'Site Updated', {
  site: { ...updateParams, id: site.id },
});

makes the error go away on site update, and I believe that this change needs to be made in multiple places in our codebase.

Acceptance Criteria

  • Fix EventCreator.audit method signatures to be sure event label is the first parameter
  • Change made live

After evaluating, edit this part:

Level of effort - <low/medium/high>

Low

@svenaas svenaas added the bug label Feb 4, 2022
@svenaas svenaas self-assigned this Feb 8, 2022
@svenaas
Copy link
Contributor Author

svenaas commented Feb 11, 2022

Confirmed that this fails, generates a console error, and doesn't show up at admin /events:

EventCreator.audit(req.user, Event.labels.ADMIN_ACTION, 'Organization Activated user first', { organization: activatedOrg });

Whereas this succeeds, generates no error, and does show up at admin /events:

EventCreator.audit(Event.labels.ADMIN_ACTION, req.user, 'Organization Activated label first', { organization: activatedOrg });

@svenaas
Copy link
Contributor Author

svenaas commented Feb 14, 2022

Merged but not yet live.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants