Skip to content
This repository has been archived by the owner on Jun 28, 2023. It is now read-only.

Audit log now logs to a database table #91

Merged
merged 4 commits into from
Mar 5, 2018
Merged

Audit log now logs to a database table #91

merged 4 commits into from
Mar 5, 2018

Conversation

jshiell
Copy link
Contributor

@jshiell jshiell commented Mar 5, 2018

This changes the audit log to write to a database table.

There's an outstanding question as to whether we want an env var to allow this functionality to be disabled. As per the nature of audit logs, this table will grow (slowly, I hope) forever, which may not be desirable in some environments.

@ben-jones-springer-nature appreciate you're not a Rubyist, but I've added you as a reviewer as your team is the closest thing our internal Bandiera has to an owner, hence it's probably worth keeping you in the loop on upstream changes 😄

params: format(params)
)
audit_record.save
rescue => e

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid rescuing without specifying an error class.

@mattrayner
Copy link
Contributor

@jshiell if we could disable the database via an environment variable, that would be useful for us at @ukparliament

Copy link
Contributor

@dazoakley dazoakley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far, but like you and @mattrayner say, would be good to make the use of the table configurable via an environment variable - maybe make it write to the logs by default and have the DB logging as an opt in?


audit_record = db[:audit_records].first
expect(audit_record).to_not be_nil
expect(audit_record[:timestamp]).to be_between(start_time, end_time)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use a gem called timecop for time based testing in Ruby (https://github.com/travisjeffery/timecop). This would allow you to assert that it is the exact time and not just approximate.

@@ -11,7 +11,12 @@ class InvalidParams < StandardError; end
enable :logging
enable :raise_errors if ENV['AIRBRAKE_API_KEY'] && ENV['AIRBRAKE_PROJECT_ID']

audit_log = LoggingAuditLog.new(Bandiera.logger)
audit_log = if ENV['RECORD_AUDIT_RECORDS'] && ENV['RECORD_AUDIT_RECORDS'].downcase == 'true'
LoggingAuditLog.new(recording = record_audit_records)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use 2 (not -10) spaces for indentation.
Useless assignment to variable - recording.

@@ -11,7 +11,12 @@ class InvalidParams < StandardError; end
enable :logging
enable :raise_errors if ENV['AIRBRAKE_API_KEY'] && ENV['AIRBRAKE_PROJECT_ID']

audit_log = LoggingAuditLog.new(Bandiera.logger)
audit_log = if ENV['RECORD_AUDIT_RECORDS'] && ENV['RECORD_AUDIT_RECORDS'].downcase == 'true'
LoggingAuditLog.new(recording = record_audit_records)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use 2 (not -10) spaces for indentation.
Useless assignment to variable - recording.

@jshiell
Copy link
Contributor Author

jshiell commented Mar 5, 2018

DB auditing should now only be active when env var RECORD_AUDIT_RECORDS is set to true. It seemed fair to default it to off, as this'll save surprises for existing users.

Copy link
Contributor

@dazoakley dazoakley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jshiell
Copy link
Contributor Author

jshiell commented Mar 5, 2018

Cheers sir 😄

@jshiell jshiell merged commit d4b4033 into master Mar 5, 2018
@jshiell jshiell deleted the db-audit-log branch March 5, 2018 13:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants