Skip to content

Commit

Permalink
Move UserUpdate#record_2sv_mandated -> User callback
Browse files Browse the repository at this point in the history
This is now called *automatically* via an `after_update` callback which
means it's called more reliably than `UserUpdate#record_2sv_mandated`
was called.

I've changed the implementation very slightly to make use of other magic
methods provided by `ActiveModel::Dirty#previous_changes`. I don't
believe any of these changes will have changed the behaviour.

The `Current.user` guard condition is necessary, because
`LogEvent::TWO_STEP_MANDATED` has `require_initiator` set to true and so
would trigger a validation error if `Current.user` was not set.

The advantage of recording the event in a callback like this is that we
don't need to remember to use `UserUpdate#call` to make any changes to a
user and we may now catch some places where 2SV was being mandated for a
user but no event was being recorded.
  • Loading branch information
floehopper committed Jan 4, 2024
1 parent a49a644 commit 76c4a55
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 12 deletions.
8 changes: 8 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ class User < ApplicationRecord
after_update :record_role_change
after_update :record_organisation_change
after_update :record_2sv_exemption_removed
after_update :record_2sv_mandated

scope :web_users, -> { where(api_user: false) }

Expand Down Expand Up @@ -499,4 +500,11 @@ def record_2sv_exemption_removed

EventLog.record_event(self, EventLog::TWO_STEP_EXEMPTION_REMOVED, initiator: true, ip_address: true)
end

def record_2sv_mandated
return if Current.user.blank?
return unless require_2sv && require_2sv_previously_changed?

EventLog.record_event(self, EventLog::TWO_STEP_MANDATED, initiator: true, ip_address: true)
end
end
12 changes: 0 additions & 12 deletions app/services/user_update.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ def call
user.application_permissions.reload

record_permission_changes(old_permissions)
record_2sv_mandated
send_two_step_mandated_notification
perform_permissions_update
record_email_change_and_notify
Expand Down Expand Up @@ -63,17 +62,6 @@ def record_permission_changes(old_permissions)
end
end

def record_2sv_mandated
return unless user.require_2sv && user.previous_changes[:require_2sv]

EventLog.record_event(
user,
EventLog::TWO_STEP_MANDATED,
initiator: true,
ip_address: true,
)
end

def perform_permissions_update
PermissionUpdater.perform_on(user)
end
Expand Down

0 comments on commit 76c4a55

Please sign in to comment.