Fixes #16850 - Added record of password change to audit log #3970

dhlavac · 2016-10-25T13:00:37Z

No description provided.

theforeman-bot · 2016-10-25T13:00:40Z

Can an existing organization member please verify this patch?

theforeman-bot · 2016-10-25T13:00:40Z

Can an existing organization member please verify this patch?

theforeman-bot · 2016-10-25T13:00:40Z

Can an existing organization member please verify this patch?

domcleal · 2016-10-26T08:32:39Z

@theforeman-bot ok to test

domcleal

Some inline comments, but the main thing this is missing is tests for the model changes. Please add some to ensure the auditing happens on password change, and also not on a regular update.

domcleal · 2016-10-26T10:08:26Z

app/models/user.rb

+  end
+
+  def set_password_changed
+    self.password_changed = true


This seems to be missing a conditional, will it not mark password_changed on every model validation?

domcleal · 2016-10-26T10:08:48Z

app/models/user.rb

@@ -445,6 +446,24 @@ def self.try_to_auto_create_user(login, password)
    user
  end

+  def password_changed
+    @password_changed ||= nil


Unsure what ||= nil is achieving here, should this just be an attr_reader :password_changed ?

domcleal · 2016-10-26T10:11:20Z

app/models/user.rb

+    @password_changed = value
+  end
+
+  def password_changed_changed?


Is this necessary? Doesn't AM::Dirty provide it?

domcleal · 2016-10-26T10:11:54Z

app/models/user.rb

+
+  def set_password_changed
+    self.password_changed = true
+    self.audit_comment = 'Password has been changed'


Is this necessary if the attribute's recorded?

dhlavac · 2016-11-22T13:38:25Z

@domcleal I changed pull request and to yours previous comments: 1.attr_reader is fine, thank you, 2. password_changed_changed? is neccessary because it working with virtual attribute and other things I rosolve please could yo look at that ?

domcleal · 2016-11-22T14:18:32Z

app/models/user.rb

@@ -88,6 +90,7 @@ def self.name_format
  before_validation :verify_current_password, :if => Proc.new {|user| user == User.current},
                    :unless => Proc.new {|user| user.password.empty?}
  before_validation :prepare_password, :normalize_mail
+  before_validation :set_password_changed, :if => Proc.new { |user| user.manage_password? && !user.password.blank? }


!..blank? should be present?

domcleal · 2016-11-22T14:19:28Z

test/models/user_test.rb

@@ -320,6 +320,32 @@ def setup_user(operation)
    assert_includes record.errors.keys, :admin
  end

+  test "log of password change should be saved" do
+    user = FactoryGirl.create(:user)
+    user.password = nil


This is rather hacky, reload the object from the database instead (User.find_by_id(user.id)) and add a comment to state why (i.e. to clear the value of user.password).

domcleal · 2016-11-22T14:20:01Z

test/models/user_test.rb

+    user.save
+    as_admin do
+      user.password = "newpassword"
+      user.valid?


assert_valid user

domcleal · 2016-11-22T14:20:35Z

test/models/user_test.rb

@@ -320,6 +320,32 @@ def setup_user(operation)
    assert_includes record.errors.keys, :admin
  end

+  test "log of password change should be saved" do


audit, not log

domcleal · 2016-11-22T14:20:49Z

test/models/user_test.rb

+    end
+  end
+
+  test "log of password change shouldnt be saved" do


shouldn't, missing an apostrophe

There is no password change here - it isn't that the log of a password change shouldn't be saved, it's that there is no audit log entry created when there is no password change.

In this test I want to test that , it should not mark password_changed on every model validation

domcleal · 2016-11-22T14:22:00Z

test/models/user_test.rb

+
+  test "log of password change shouldnt be saved" do
+    user = FactoryGirl.create(:user)
+    user.password = nil


ditto all comments above, they apply to this test too.

domcleal · 2016-11-22T14:23:21Z

app/models/user.rb

  after_save :ensure_default_role
+  after_save :unset_password_changed


Missing test coverage? A test should run two saves in a row and check for no second audit entry.

@dhlavac When I see your tests - it looks like you're calling at most .valid? and .save, so I'm not 100% positive this is ggcovered as per the comment above.

dhlavac · 2016-11-22T16:44:56Z

thanks @domcleal, I fixed all your comments, could you please take another look?

domcleal · 2016-11-23T09:58:01Z

test/models/user_test.rb

+      assert_includes user.changed, "password_changed"
+      assert user.save
+    end
+    user = User.find_by_id(user.id) #to clear the value of user.password


This is this meant to test the after_save, isn't it? Replacing the entire user object will also reset the value, so it doesn't test the callback.

dhlavac · 2016-11-24T16:48:10Z

@domcleal I have edited test which testing after_save

dhlavac · 2016-12-13T15:02:35Z

@domcleal I hope Final version added assertion for Audit object to every test

dLobatog · 2016-12-21T08:57:05Z

test/models/user_test.rb

@@ -320,6 +320,68 @@ def setup_user(operation)
    assert_includes record.errors.keys, :admin
  end

+  test "audit of password change should be saved only once, second time audited changes should not contain password_changed" do
+    user = FactoryGirl.create(:user)


Please move the user creation to a setup block, it's the same in all 3 hosts. You can group them in a context audits for password change or similar.

dLobatog · 2016-12-21T08:58:01Z

@dhlavac I've put a minor comment on tests - otherwise I haven't tested it much but it seemed you addressed @domcleal comments.

dLobatog · 2016-12-21T08:59:25Z

app/models/user.rb

  after_save :ensure_default_role
+  after_save :unset_password_changed


@dhlavac When I see your tests - it looks like you're calling at most .valid? and .save, so I'm not 100% positive this is ggcovered as per the comment above.

dhlavac · 2016-12-21T14:06:23Z

@dLobatog Audit log was created before validation and after save I set the variable password_changed to false then I think this is correct, if I tested it is not still true. Thanks for comment tests were added to context and setup block was created.

houndci-bot · 2016-12-21T14:07:18Z

test/models/user_test.rb

+  context "audits for password change" do
+    def setup_user_for_audits
+      user = FactoryGirl.create(:user)
+      user = User.find_by_id(user.id) #to clear the value of user.password


Useless assignment to variable - user.
Use find_by instead of dynamic find_by_id.

houndci-bot · 2016-12-21T14:07:19Z

test/models/user_test.rb

@@ -320,6 +320,71 @@ def setup_user(operation)
    assert_includes record.errors.keys, :admin
  end

+  context "audits for password change" do


Block has too many lines. [57/25]

dLobatog · 2016-12-21T15:41:09Z

Thanks @dhlavac, waiting on @domcleal to approve.

domcleal · 2016-12-22T10:27:43Z

Merged, thanks @dhlavac.

theforeman-bot added Needs testing Not yet reviewed labels Oct 25, 2016

domcleal suggested changes Oct 26, 2016

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Oct 26, 2016

dhlavac force-pushed the fix/16850 branch from 72012ed to d5dade5 Compare November 22, 2016 13:35

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 22, 2016

domcleal suggested changes Nov 22, 2016

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Nov 22, 2016

dhlavac force-pushed the fix/16850 branch from d5dade5 to c211e23 Compare November 22, 2016 16:43

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 22, 2016

domcleal suggested changes Nov 23, 2016

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Nov 23, 2016

dhlavac force-pushed the fix/16850 branch from c211e23 to d8cfc07 Compare November 24, 2016 16:46

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 24, 2016

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Dec 13, 2016

domcleal added Waiting on contributor and removed Needs re-review labels Dec 13, 2016

dhlavac force-pushed the fix/16850 branch from 1b564ff to 51af64e Compare December 13, 2016 15:00

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Dec 13, 2016

dhlavac force-pushed the fix/16850 branch from 51af64e to 64199fb Compare December 14, 2016 11:00

dLobatog reviewed Dec 21, 2016

View reviewed changes

dLobatog suggested changes Dec 21, 2016

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Dec 21, 2016

dhlavac force-pushed the fix/16850 branch from 64199fb to 3dc3930 Compare December 21, 2016 14:07

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Dec 21, 2016

houndci-bot reviewed Dec 21, 2016

View reviewed changes

Fixes #16850 - Added record of password change to audit log

7643b79

dhlavac force-pushed the fix/16850 branch from 3dc3930 to 7643b79 Compare December 21, 2016 14:17

dLobatog approved these changes Dec 21, 2016

View reviewed changes

domcleal approved these changes Dec 22, 2016

View reviewed changes

domcleal merged commit 518a259 into theforeman:develop Dec 22, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #16850 - Added record of password change to audit log #3970

Fixes #16850 - Added record of password change to audit log #3970

dhlavac commented Oct 25, 2016

theforeman-bot commented Oct 25, 2016

theforeman-bot commented Oct 25, 2016

theforeman-bot commented Oct 25, 2016

domcleal commented Oct 26, 2016

domcleal left a comment

domcleal Oct 26, 2016

domcleal Oct 26, 2016

domcleal Oct 26, 2016

domcleal Oct 26, 2016

dhlavac commented Nov 22, 2016 •

edited

domcleal Nov 22, 2016

domcleal Nov 22, 2016

domcleal Nov 22, 2016

domcleal Nov 22, 2016

domcleal Nov 22, 2016

dhlavac Nov 22, 2016

domcleal Nov 22, 2016

domcleal Nov 22, 2016

dLobatog Dec 21, 2016

dhlavac commented Nov 22, 2016

domcleal Nov 23, 2016

dhlavac commented Nov 24, 2016

dhlavac commented Dec 13, 2016

dLobatog Dec 21, 2016

dLobatog commented Dec 21, 2016

dLobatog Dec 21, 2016

dhlavac commented Dec 21, 2016

houndci-bot Dec 21, 2016

houndci-bot Dec 21, 2016

dLobatog commented Dec 21, 2016

domcleal commented Dec 22, 2016

		after_save :ensure_default_role
		after_save :unset_password_changed

Fixes #16850 - Added record of password change to audit log #3970

Fixes #16850 - Added record of password change to audit log #3970

Conversation

dhlavac commented Oct 25, 2016

theforeman-bot commented Oct 25, 2016

theforeman-bot commented Oct 25, 2016

theforeman-bot commented Oct 25, 2016

domcleal commented Oct 26, 2016

domcleal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhlavac commented Nov 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhlavac commented Nov 22, 2016

Choose a reason for hiding this comment

dhlavac commented Nov 24, 2016

dhlavac commented Dec 13, 2016

Choose a reason for hiding this comment

dLobatog commented Dec 21, 2016

Choose a reason for hiding this comment

dhlavac commented Dec 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dLobatog commented Dec 21, 2016

domcleal commented Dec 22, 2016

dhlavac commented Nov 22, 2016 •

edited