Add Audit Event for Password Change #1600

micahlee · 2020-06-08T16:58:16Z

NOTE: This PR is blocked until the parent branch is merged.

What does this PR do?

What's changed? Why were these changes made?

This PR adds an audit log message for password changes events using the API endpoint PUT /authn/:account/password

How should the reviewer approach this PR, especially if manual tests are required?

The container image registry.tld/conjur-appliance:5.9.0-20200610203923-d5e10a1 may be used to evaluate this change manually, for example in dap-intro.

Example audit message

Successful Password Change:

<86>1 2020-06-10T21:04:53.745+00:00 60aa5330d24b conjur fb98c0b8-136e-4f6f-ab35-1f545acadba0 password [auth@43868 user="demo:user:admin"][subject@43868 user="demo:user:admin"][action@43868 result="success" operation="change"][meta sequenceId="3"] demo:user:admin successfully changed their password

Failed Password Change:

<84>1 2020-06-10T21:03:32.205+00:00 60aa5330d24b conjur 74acf74b-7743-41d7-be1d-1dc060356d26 password [auth@43868 user="demo:user:admin"][subject@43868 user="demo:user:admin"][action@43868 result="failure" operation="change"][meta sequenceId="5"] demo:user:admin failed to change their password: password CONJ00046E The password you have chosen does not meet the complexity requirements. Choose a password that includes: 12-128 characters, 2 uppercase letters, 2 lowercase letters, 1 digit, 1 special character

Appliance Build

https://jenkins.conjur.net/job/conjurinc--appliance/job/1548-audit-password-change/

What ticket does this PR close?

Connected to #1548

Checklists

Change log

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code changes, or
The changes in this PR do not require tests

Documentation

This PR does not require updating any documentation, or
Docs (e.g. READMEs) were updated in this PR, and/or there is a follow-on issue to update docs

R&D Docs (dap-wiki) PR: cyberark/dap-wiki#29

app/controllers/credentials_controller.rb

app/models/audit/event/password.rb

jonahx

This looks terrific. All comments minor.

app/models/audit/event/password.rb

jonahx · 2020-06-11T17:15:55Z

app/domain/commands/credentials/change_password.rb

+  module Credentials
+
+    ChangePassword ||= CommandClass.new(
+      dependencies: {},


Two problems here:

A CommandClass with no dependencies doesn't make any sense, conceptually.

We actually do have dependencies here, but they sneak in implicitly via our input, namely the role. Specifically, credentials.save(raise_on_save_failure: true) is a database interaction.

Unfortunately, I think fixing this would require completely reworking the credentials controller, which will be too big of a sidetrack right now.

I recommend instead that we keep your code mostly as is, but just make it an ordinary object rather than a CommandClass, and add a comment to a placeholder issue for refactoring the controller. This will allow your test code to remain unchanged, or close to it as well.

So the way this is set up, with factoring out the command class first, and then adding audit to it, it only appears to have no dependencies in this step.

The audit log is added in the next commit as a dependency to this command class.

Does this change your assessment at all?

Could you share, hypothetically, what your ideal command class for this would look like (even in just pseudocode)? If you could have exactly what you wanted as inputs from the CredentialsController?

That would help me better grasp what the desired end state would be from your perspective.

jonahx · 2020-06-11T17:37:16Z

spec/app/domain/commands/credentials/change_password_spec.rb

+    # Expect the command to raise the original exception
+    expect { subject.call(role: role,  password: password) }.to raise_error(err_message)
+  end
+end


Given the existing code, and our decision to delay changing it, these tests look perfect.

jonahx · 2020-06-11T18:00:41Z

app/domain/commands/credentials/change_password.rb

-      dependencies: {},
+      dependencies: {
+        audit_log: ::Audit.logger
+      },


Ok so this somewhat changes my comment on previous commit, but it remains the case that one of our dependencies is still sneaking in through the input.

My recommendation might change now to:

Keep the command class.

Add a comment stating the problem above dependencies and inputs and link to issue.

app/domain/commands/credentials/change_password.rb

This provides a better base for adding audit to password change events.

codeclimate · 2020-06-12T19:18:21Z

Code Climate has analyzed commit 29dede2 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 97.7% (50% is the threshold).

This pull request will bring the total coverage in the repository to 85.8% (0.0% change).

View more on Code Climate.

jonahx reviewed Jun 8, 2020

View reviewed changes

app/controllers/credentials_controller.rb Outdated Show resolved Hide resolved

jonahx reviewed Jun 8, 2020

View reviewed changes

app/models/audit/event/password.rb Outdated Show resolved Hide resolved

jonahx reviewed Jun 8, 2020

View reviewed changes

micahlee force-pushed the 1548-audit-password-change branch from c7d95d2 to d79dcdf Compare June 8, 2020 18:22

jonahx reviewed Jun 8, 2020

View reviewed changes

app/models/audit/event/password.rb Outdated Show resolved Hide resolved

micahlee force-pushed the 1548-audit-password-change branch 2 times, most recently from 39f482e to c6dc923 Compare June 8, 2020 18:36

micahlee marked this pull request as draft June 8, 2020 18:42

micahlee changed the title ~~Add Audit Event for Password Change~~ [WIP] Add Audit Event for Password Change Jun 8, 2020

jonahx force-pushed the 1573-simplify-audit branch from 778457d to 2097a84 Compare June 8, 2020 18:51

micahlee force-pushed the 1548-audit-password-change branch from c6dc923 to e21b0f7 Compare June 8, 2020 19:16

jonahx force-pushed the 1573-simplify-audit branch from 2097a84 to c03026f Compare June 8, 2020 19:23

micahlee force-pushed the 1548-audit-password-change branch 6 times, most recently from 20183c8 to c72245a Compare June 9, 2020 13:00

micahlee changed the title ~~[WIP] Add Audit Event for Password Change~~ Add Audit Event for Password Change Jun 9, 2020

jonahx force-pushed the 1573-simplify-audit branch from 455b52b to e2569b8 Compare June 9, 2020 14:20

micahlee force-pushed the 1548-audit-password-change branch 2 times, most recently from 2991100 to 087643f Compare June 9, 2020 17:06

jonahx force-pushed the 1573-simplify-audit branch 5 times, most recently from 99c9b02 to abf59b4 Compare June 11, 2020 05:47

micahlee force-pushed the 1548-audit-password-change branch 2 times, most recently from 893a2af to b2625a1 Compare June 11, 2020 14:29

jonahx force-pushed the 1573-simplify-audit branch from 2369fad to b1b4e90 Compare June 11, 2020 14:53

micahlee force-pushed the 1548-audit-password-change branch 2 times, most recently from fce8b5d to c752227 Compare June 11, 2020 15:00

Base automatically changed from 1573-simplify-audit to master June 11, 2020 15:33

micahlee force-pushed the 1548-audit-password-change branch 3 times, most recently from ce31fcc to eb2622c Compare June 11, 2020 16:03

micahlee marked this pull request as ready for review June 11, 2020 16:24

micahlee requested a review from a team June 11, 2020 16:39

jonahx reviewed Jun 11, 2020

View reviewed changes

app/domain/commands/credentials/change_password.rb Outdated Show resolved Hide resolved

micahlee mentioned this pull request Jun 11, 2020

Implicit database dependency in credentials command classes #1611

Open

micahlee force-pushed the 1548-audit-password-change branch 2 times, most recently from de62ac8 to 866a567 Compare June 11, 2020 18:29

jonahx previously approved these changes Jun 11, 2020

View reviewed changes

micahlee added 2 commits June 12, 2020 15:02

Factor password change operation into command class

811ceb4

This provides a better base for adding audit to password change events.

Add audit message for password change events

29dede2

micahlee dismissed jonahx’s stale review via 29dede2 June 12, 2020 19:02

micahlee force-pushed the 1548-audit-password-change branch from 866a567 to 29dede2 Compare June 12, 2020 19:02

jonahx approved these changes Jun 12, 2020

View reviewed changes

micahlee merged commit 180217f into master Jun 12, 2020

micahlee deleted the 1548-audit-password-change branch June 12, 2020 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Audit Event for Password Change #1600

Add Audit Event for Password Change #1600

micahlee commented Jun 8, 2020 •

edited

Loading

jonahx left a comment

jonahx Jun 11, 2020 •

edited

Loading

micahlee Jun 11, 2020

micahlee Jun 11, 2020

jonahx Jun 11, 2020

jonahx Jun 11, 2020

codeclimate bot commented Jun 12, 2020

Add Audit Event for Password Change #1600

Add Audit Event for Password Change #1600

Conversation

micahlee commented Jun 8, 2020 • edited Loading

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

jonahx left a comment

Choose a reason for hiding this comment

jonahx Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

micahlee Jun 11, 2020

Choose a reason for hiding this comment

micahlee Jun 11, 2020

Choose a reason for hiding this comment

jonahx Jun 11, 2020

Choose a reason for hiding this comment

jonahx Jun 11, 2020

Choose a reason for hiding this comment

codeclimate bot commented Jun 12, 2020

micahlee commented Jun 8, 2020 •

edited

Loading

jonahx Jun 11, 2020 •

edited

Loading