Add separate page for resetting another user's 2SV #2545
Merged
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
This method is more intention-revealing than calling `Object#nil?` on the return value from `User#reason_for_2sv_exemption`. c.f. this commit [1] [1]: f82b289
Previously we were using a mixture of these in `app/views/users/_form_fields.html.erb` which I found confusing. The `f` form builder is only really useful for creating form elements and we won't be able to use those when we move the user edit page (and thus this partial) to use the GOV.UK Design System components. So I think it makes sense to use the instance variable consistently here.
chrislo
approved these changes
Nov 23, 2023
There was a problem hiding this comment.
So it might be worth protecting against this somehow, although I don't think it will do any harm if
User#reset_2sv!&UserMailer.two_step_resetare called for a user that doesn't have 2SV setup - it might just be a bit confusing. What do you think?
Agreed, I don't think it'll cause any issues.
Trello: https://trello.com/c/caETStGa This is a step on the journey to move the edit user page to use the GOV.UK Design System. The new design calls for separate pages for different operations on a user and this is the next one. Unlike in commits like this one [1] where we first split out the new page and then separatly moved the new page to use the Design System, here I've created the new page using the Design System in one go, because there wasn't really any markup in the "edit user" page to extract. The new `app/views/users/two_step_verification_resets/edit.html.erb` template is based on other similar pages which we've recently extracted, e.g. `app/views/users/organisations/edit.html.erb`, although obviously in this case there aren't any form fields; only a button. Previously the "Reset 2-step verification" link on the "edit user" page was enhanced by UJS to prompt the user with a confirmation dialog and then submit a PATCH HTTP request to the controller. Since you now have to click the "Reset 2-step verification" link on the "edit user" page AND then click the "Reset 2-step verification" button on the new page, I don't think we need the confirmation dialog any more. However, I've used the `destructive` option on the button component to indicate that some information is lost when you click it. All this means we no longer need to use UJS for this operation which is a plus for two reasons: (a) UJS is deprecated in Rails v7.0 and we're about to upgrade to Rails v7.1; and (b) this functionality did not work without JS enabled which was an accessibility issue. The new `Users::TwoStepVerificationResetsController` is closely based on the relevant bits of code from `UsersController`. I'm using `UserPolicy#reset_2sv?` for the authorization in the new `Users::TwoStepVerificationResetsController`, because that's what was being used for the `UsersController#reset_2sv` action that we're replacing. You could argue that the old UJS-enhanced "Reset 2-step verification" link was only visible on the "edit user" page if `UserPolicy#edit?` returned `true`, but given that you can't navigate to the new form without going via the "edit user" page, using that policy seems unnecessary. Doing this makes me think @chrisroos might have been right to suggest that we only need a single call to `authorize @user, :<predicate-method>?` in the `authorize_user` methods on the other controllers in the `Users` namespace, but I'm going to leave that for now. There weren't any tests in `UsersControllerTest` for this behaviour, so I had to write `Users::TwoStepVerificationResetsControllerTest` from scratch, but I based it closely on other similar controller tests. Luckily there were a couple of integration tests (`test/integration/managing_two_step_verification_test.rb` & `test/integration/mandate_2sv_for_organisation_test.rb`) covering the behaviour which I've been able to fixup to work with the new UI. Rather than creating a combinatorial explosion of tests in `Users::TwoStepVerificationResetsControllerTest` relating to whether a user with all the different roles can edit another user with all the different roles, I've resorted to stubbing `UserPolicy.new` and relevant predicate methods on the `UserPolicy` instance. Although this is a bit ugly, since `UserPolicy` is thoroughly tested in `UserPolicyTest`, it seems like a pragmatic option. [1]: 899a8a1
When the `UserPolicy#mandate_2sv?` condition was first introduced [1], although it was called `#flag_2sv?` at that point [2], it only wrapped the `require_2sv` checkbox. This makes sense to me, because its that checkbox that implements the mandating 2SV operation. However, since then a bunch of extra 2SV-related functionality has been included inside the condition, none of which is actually related to mandating 2SV. I think this has happened accidentally or due to a misunderstanding. So in this commit I have moved the condition as close as possible to the `require_2sv` checkbox which makes more sense to me. This won't currently make any difference, because `UserPolicy#mandate_2sv?` is aliased to `UserPolicy#edit?` which is required to be true for the whole page. However, I think this change makes things less confusing. This diff is best viewed with the `--ignore-all-space` option, because a bunch of indentation has changed. [1]: 0a8cc4d [2]: 74b25a7
I've tidied things up in the "Account security" section, because I was finding the code hard to read. Hopefully this makes things a bit clearer.
In this commit [1], `UserPolicy#reset_2sv?` was changed from only allowing Superadmins to reset another user's 2SV to alias to `UserPolicy#edit?` which is more permissive and allows Admins, Organisation Admins & Super-Organisation Admins to reset _some_ user's 2SV. Thus the user passed in to `User#reset_2sv!` is no longer always a Superadmin and so the `initiating_superadmin` argument name was confusing. Furthermore there wasn't anything in `User#reset_2sv!` to check that `initiating_superadmin` was actually a Superadmin which was also confusing. [1]: 4d1c86d
I found this test really hard to read, so I thought I'd try to improve matters.
floehopper
force-pushed
the
split-out-edit-user-2sv-pages-from-user-edit-page
branch
from
c208c91 to
a24e13a
Compare
floehopper
deleted the
split-out-edit-user-2sv-pages-from-user-edit-page
branch
This was referenced Nov 27, 2023
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.
Trello: https://trello.com/c/uZD2I9dj & https://trello.com/c/caETStGa
This splits out the resetting of another user's 2SV into a separate page. The latest design calls for the splitting out of a bunch of user operations into separate pages like this. The new "reset user 2SV" page uses the GOV.UK Design System, but the "edit user" page still does not. c.f. #2497, #2509 & #2537.
Previously the "Reset 2-step verification" link on the "edit user" page was enhanced by UJS to prompt the user with a confirmation dialog and then submit a PATCH HTTP request to the controller. Since you now have to click the "Reset 2-step verification" link on the "edit user" page AND then click the "Reset 2-step verification" button on the new page, I don't think we need the confirmation dialog any more. However, I've used the
destructiveoption on the button component to indicate that some information is lost when you click it.All this means we no longer need to use UJS for this operation which is a plus for two reasons: (a) UJS is deprecated in Rails v7.0 and we're about to upgrade to Rails v7.1; and (b) this functionality did not work without JS enabled which was an accessibility issue.
Previously there wasn't anything stopping a hand-crafted request to
UsersController#reset_two_step_verificationfrom callingUser#reset_2sv!&UserMailer.two_step_resetfor a user that didn't have 2SV setup. And I haven't added any such check into the new controller. Since the "Reset 2-step verification" link on the "edit user" page still only shows up if the user has 2SV setup, it's not possible to navigate to the new "Reset 2SV" page if the user doesn't have 2SV setup. However, I suppose it's now slightly easier to hack the URL to get to the new "Reset 2SV" page and submit the form. So it might be worth protecting against this somehow, although I don't think it will do any harm ifUser#reset_2sv!&UserMailer.two_step_resetare called for a user that doesn't have 2SV setup - it might just be a bit confusing. What do you think?Unchanged "Reset 2-step verification" link on "edit user" page
New "reset 2SV" page