Move UpdateUser methods into after_update callbacks on User
#2582
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
floehopper
changed the title
floehopper
changed the title
UpdateUser methods into after_update callbacks on User
floehopper
force-pushed
the
spike-using-active-support-current-attributes-for-current-user
branch
5 times, most recently
from
f246514 to
5a4c684
Compare
The `Current` class inherits from `ActiveSupport::CurrentAttributes` [1] which means that its singleton attribute accessors are "thread-isolated". This means that we can set them in a new `before_action` in `ApplicationController` and then make use of them in model code without needing access to a controller. I'm hoping to use this to simplify calls to `EventLog` methods and ultimately move a bunch of functionality in `UserUpdate#call` into model callbacks. [1]: https://api.rubyonrails.org/v7.0.8/classes/ActiveSupport/CurrentAttributes.html
I think this makes the tests more robust. Also it'll make some upcoming changes easier.
Unfortunately a bunch of the tests were relying on the `initiator` defaulting to the user being updated, so for the moment I've replicated that in the new implementation of `EventLog.record_email_change`. However, I plan to remove that in a subsequent commit.
Now that we're using `Current.user` to obtain the `initiator`, there's no need for the default - `Current.user` should always be set. This means we have to explicitly set `Current.user` in some tests to avoid validation errors due to `EventLog#initiator_id` being missing.
Previously I found it confusing that the `previous_organisation` & `new_organisation` arguments were actually the names of the organisations rather than the organisations themselves. Renaming them to `previous_organisation_name` & `new_organisation_name` respectively makes things less confusing.
The `ip_address` option for this method is optional and not all callsites were setting it. This means we only record the current user's IP address in some cases. To this end I've repurposed the `ip_address` option to be a boolean value indicating whether or not we want to record the user's IP address.
The `initiator` option for this method is optional and not all callsites were setting it. This means we only record the initiating user in some cases. To this end I've repurposed the `initiator` option to be a boolean value indicating whether or not we want to record the initiating user.
This is now called *automatically* via an `after_update` callback which means it's called more reliably and more frequently than `UserUpdate#record_update` was called. The `ActiveModel::Dirty#previous_changes` guard condition avoids recording events when none of the user's attributes have changed which would be pretty pointless. The `Current.user` guard condition is necessary, because `LogEvent::ACCOUNT_UPDATED` has `require_initiator` set to true and so would trigger a validation error if `Current.user` was not set. I've had to add a few stubs for extra `LogEvent::ACCOUNT_UPDATED` event log records which reflect a slight change in behaviour. However, I don't think these extra even log records are necessarily a bad thing! 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. This in turn means that we will now be recording events when an API user's attributes are updated. which we weren't previously because the `ApiUsersController` wasn't calling `UserUpdate#call`.
This is now called *automatically* via an `after_update` callback which means it's called more reliably than `UserUpdate#record_role_change` 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::ROLE_CHANGED` 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 a user's role was being updated but no event was being recorded.
This is now called *automatically* via an `after_update` callback which means it's called more reliably than `UserUpdate#record_organisation_change` 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::ORGANISATION_CHANGED` 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 a user's organisation was being updated but no event was being recorded.
This is now called *automatically* via an `after_update` callback which means it's called more reliably than `UserUpdate#record_2sv_exemption_removed` 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_EXEMPTION_REMOVED` 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 a user's 2SV was being removed but no event was being recorded.
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.
TODO: Previously this might have been called when only a user's permissions had changed, i.e. none of its actual attributes, which would mean that the `after_update` callback on `User` would not be called. This is now called *automatically* via an `after_update` callback which means it's called more reliably than `UserUpdate#perform_permissions_update` was called. I've had to add a call `#reload` on the `User#authorisations` association in the test for `User#suspend`, because somehow the call to `User#perform_permissions_update` was leaving the in-memory `User#authorisations` out-of-date. It's safe to remove a bunch of calls to `PermissionUpdater.perform_on`, because in all cases the relevant user is being updated and so the new callback will be fired. The advantage of queueing the `PermissionUpdater` 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 a user was being updated, but the job was not being queued. TODO: I've removed a test in `ApiUsersControllerTest`, because you can no longer update name or email. However, it might be possible to get it to work when updating permissions.
floehopper
force-pushed
the
spike-using-active-support-current-attributes-for-current-user
branch
from
5a4c684 to
4517a1d
Compare
|
I'm going to try to get the bulk of this merged in #2623. Closing this PR for now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Questions
EventLogrecords makes sense to do in callbacks onUser, but I'm slightly less sure about sending email notifications. I think it still makes sense, but I'll probably need to be very careful that the logic is implemented correctly so that e.g. we don't end up sending out emails multiple times.Userrecord. However, I think we can either use callbacks on theUserApplicationPermissionjoin model or implement some customActiveModel::Dirty-esque methods to detect changes to a user's permissions. The former would be conceptually simpler, but it would mean that we'd lose the ability to group permission changes together, so we'd end up e.g. creating multipleEventLogrecords (one for each user permission that's changed) and sending multiple SSO Push Updates out to each app. The latter might not be a problem though.