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

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

Merged
merged 1 commit into from Dec 22, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/helpers/audits_helper.rb
Expand Up @@ -46,6 +46,8 @@ def details(audit)
next if change.nil? || change.to_s.empty?
if name == 'template'
(_("Provisioning Template content changed %s") % (link_to 'view diff', audit_path(audit))).html_safe if audit_template? audit
elsif name == "password_changed"
name = _("Password has been changed")
elsif name == "owner_id" || name == "owner_type"
_("Owner changed to %s") % (audit.revision.owner rescue _('N/A'))
elsif name == 'global_status'
Expand Down
16 changes: 16 additions & 0 deletions app/models/user.rb
Expand Up @@ -16,7 +16,9 @@ class User < ActiveRecord::Base

validates_lengths_from_database :except => [:firstname, :lastname, :format, :mail, :login]
attr_accessor :password, :password_confirmation, :current_password
attr_reader :password_changed
after_save :ensure_default_role
after_save :unset_password_changed
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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.

before_destroy EnsureNotUsedBy.new([:direct_hosts, :hosts]), :ensure_hidden_users_are_not_deleted, :ensure_last_admin_is_not_deleted

belongs_to :auth_source
Expand Down Expand Up @@ -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.present? }
before_save :set_lower_login

after_create :welcome_mail
Expand Down Expand Up @@ -447,6 +450,19 @@ def self.try_to_auto_create_user(login, password)
user
end

def password_changed_changed?
Copy link
Contributor

Choose a reason for hiding this comment

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

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

changed.include?('password_changed')
end

def set_password_changed
@password_changed = true
attribute_will_change!('password_changed')
end

def unset_password_changed
@password_changed = false
end

private

def prepare_password
Expand Down
65 changes: 65 additions & 0 deletions test/models/user_test.rb
Expand Up @@ -320,6 +320,71 @@ def setup_user(operation)
assert_includes record.errors.keys, :admin
end

context "audits for password change" do

Choose a reason for hiding this comment

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

Block has too many lines. [57/25]

def setup_user_for_audits
user = FactoryGirl.create(:user)
User.find_by_id(user.id) #to clear the value of user.password
end

test "audit of password change should be saved only once, second time audited changes should not contain password_changed" do
user = setup_user_for_audits
as_admin do
user.password = "newpassword"
assert_valid user
assert user.password_changed_changed?
assert user.password_changed
assert_includes user.changed, "password_changed"
assert user.save
assert_includes Audit.last.audited_changes, "password_changed"
#testing after_save
refute user.password_changed_changed?
refute user.password_changed
refute_includes user.changed, "password_changed"
end
end

test "audit of password change should be saved" do
user = setup_user_for_audits
as_admin do
user.password = "newpassword"
assert_valid user
assert user.password_changed_changed?
assert user.password_changed
assert_includes user.changed, "password_changed"
assert user.save
assert_includes Audit.last.audited_changes, "password_changed"
end
end

test "audit of password change should not be saved - due to no password change" do
user = setup_user_for_audits
as_admin do
user.firstname = "Johnny"
assert_valid user
refute user.password_changed_changed?
refute user.password_changed
refute_includes user.changed, "password_changed"
assert user.save
refute_includes Audit.last.audited_changes, "password_changed"
end
end

test "audit of name change sholud contain only firstname and not password_changed" do
user = setup_user_for_audits
as_admin do
user.firstname = "Johnny"
assert_valid user
assert_includes user.changed, "firstname"
refute user.password_changed_changed?
refute user.password_changed
refute_includes user.changed, "password_changed"
assert user.save
assert_includes Audit.last.audited_changes, "firstname"
refute_includes Audit.last.audited_changes, "password_changed"
end
end
end

test "user can save user if he does not change roles" do
setup_user "edit"
record = users(:two)
Expand Down