Tidy InvitationsController #2401
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 was actually removed from devise_invitable in v1.2.0 (not v1.2.1 as the TODO comment suggests) [1]. We're now using the latest version of devise_invitable which is v2.0.8. Having examined the source code of Devise::InvitationsController [2], I'm confident we can remove this method as the TODO comment suggests. [1]: scambra/devise_invitable@296247a [2]: https://github.com/scambra/devise_invitable/blob/v2.0.8/app/controllers/devise/invitations_controller.rb
The change described in the TODO seems to have been made in this commit from 6 years ago [1] which was part of the upgrade to Rails v5.1.4 [2]. However, the TODO comment was not removed at the same time, presumably due to an oversight. I've tried to understand more about what the TODO comment meant, but it's pretty hard to work it out. The Devise::InvitationsController from the current version of devise_invitable (v2.0.8) inherits from DeviseController in the current version of devise (v4.9.2). And the latter implements a `resource_params` method [3] which still uses `params.fetch()`. However, our InvitationsController which inherits from Devise::InvitationsController implements its own `resource_params` method which overwrites (no call to `super`) the implementation in DeviseController. The Devise::InvitationsController [4] from devise_invitable doesn't seem to call the `resource_params` method. However, our InvitationsController does call our own `resource_params` method. So things are all a bit confusing - not least because I think some of the methods in our current InvitationsController seem to be based on an old implementation of Devise::InvitationsController. However, I don't propose to address that in this commit. Anyway, based on the above and the fact that the app seems to have been working fine for the last 6 years, I think it's safe to delete this TODO comment! If we don't do it now, we never will!! [1]: d28ff3c [2]: #593 [3]: https://github.com/heartcombo/devise/blob/8b0b849a67c46b10827743aa0ccb0679d69e5396/app/controllers/devise_controller.rb#L208-L210 [4]: https://github.com/scambra/devise_invitable/blob/v2.0.8/app/controllers/devise/invitations_controller.rb
* 1st paragraph of NOTE > `current_user` doesn't exist for `#edit` and `#update` actions as > implemented in our current (out-of-date) versions of Devise and > DeviseInvitable I'm working on the assumption that "`current_user` doesn't exist" in the NOTE meant that the return value from the `current_user` method was `nil` rather than that the `current_user` method did not exist. The fact that `current_user` is `nil` for `#edit` & `#update` actions is *still* true for the current version (v4.9.2) of devise and the current version (v2.0.8) of devise_invitable. There is a slight implication in the NOTE that this is only true because we're using an out-of-date version of devise & devise_invitable. However, I'm pretty confident this is by design in that there is a `require_no_authentication` before action set for `#edit`, `#update` & `#destroy` [1]. This before action is defined in `DeviseController` [2]. * 2nd paragraph, 1st sentence of NOTE > With the old `attr_accessible` approach, this would fall back to the > default whitelist (i.e. equivalent to the `:normal` role) and this this > preserves that behaviour. This seems to be talking about functionality which was removed 8 years ago [3] as part of the upgrade to Rails v4 [4]. In turn that functionality seems to have been added [5] just before that as part of a move to use strong parameters [6] which was the same PR in which this NOTE was added [7]. While that paragraph gives some useful context, I don't think it's important/current enough to warrant continuing to live as a comment in the code. Hopefully this commit note can serve a similar purpose for future developers! Regarding the fall back to the "normal" role, I note that the first two tests in `InvitingUsersTest` [12] fail if this fall back is removed, so the tests serve as documentation that can't get out-of-sync with the code. * 2nd paragraph, 2nd sentence > In fact, a user accepting an invitation only needs to modify `password` and `password_confirmation` so we could only permit those two params for the `edit` and `update` actions. At the time the NOTE was added [7] the `current_user_role` was used in `UserParameterSanitiser` to determine which attributes of a `User` the current user was allowed to change. A normal user could edit the following attributes: `uid`, `name`, `email`, `password` and `password_confirmation` [8]. This is still the case [9] and so the suggestion is still valid, i.e. a user probably ought not to be able to change their `uid` or `name` when accepting an invitation which they could currently do by crafting suitable HTTP requests. I'm going to bear this in mind as I make changes to the `InvitationsController` as part of this Trello card [10]. I note that this PR comment [11] suggests that the functionality deciding which parameters should be available for which roles would probably be better if it lived in a Pundit policy. I tend to agree and I think that would make it easier to restrict parameters on a per-controller-action basis too. * Summary All in all, given all the above, I'm happy to delete the comment! [1]: https://github.com/scambra/devise_invitable/blob/db2f9a1c0e87addb196d1b82920370f4230c1157/app/controllers/devise/invitations_controller.rb#L4 [2]: https://github.com/heartcombo/devise/blob/8b0b849a67c46b10827743aa0ccb0679d69e5396/app/controllers/devise_controller.rb#L99-L119 [3]: bffb80b [4]: #352 [5]: 1bb38e5 [6]: #336 [7]: 3e939db [8]: https://github.com/alphagov/signon/blob/3e939db4eff7ac551e835aafb40217e803b5be10/lib/roles/normal.rb#L4-L6 [9]: https://github.com/alphagov/signon/blob/591bfe2deca05804e5cba43221c4f8694e6d6be3/lib/roles/normal.rb#L3C1-L5 [10]: https://trello.com/c/umBDQZUc [11]: #336 (comment) [12]: https://github.com/alphagov/signon/blob/591bfe2deca05804e5cba43221c4f8694e6d6be3/test/integration/inviting_users_test.rb#L6-L32
This is a small refactoring which I think is slightly easier to read. A `User` has its role set to "normal" by default due to a default value on the `users.role` database column. I did contemplate explicitly setting the `User#role` to "normal", but I thought that would make things harder if we ever wanted to change the default role. I'm confident in making this change, because I know that two tests in `InvitingUsersTest` [1] fail if the fallback is not in place. [1]: https://github.com/alphagov/signon/blob/591bfe2deca05804e5cba43221c4f8694e6d6be3/test/integration/inviting_users_test.rb#L6-L32
I want to add contexts for when the user is not authenticated. Making this change first will make it easier to see what's going on. This diff is best viewed with the `--ignore-all-space` option in order to ignore the indentation changes.
Before making some changes to the implementation of InvitationsController I want to ensure that behaviour is covered by tests.
Previously these actions were only implemented in the superclass and it was far from obvious that this was the case. Although adding these methods isn't strictly necessary, I think doing so makes things a bit clearer. I've also added a comment to make it clearer that the `edit` action renders the custom `app/views/devise/invitations/edit.html.erb` template. Note that I did contemplate adding some test coverage for these actions in InvitationsControllerTest, but since we're not customizing their behaviour, we can probably safely rely on the tests within devise_invitable. Also our InvitingUsersTest integration test provides some good coverage of `edit` & `update` actions. Note We don't seem to be using the `destroy` action anywhere in our code. I did wonder if it was intended to be requested from a link in an email, especially since it responds to GET requests, but it doesn't appear in our `app/views/user_mailer/invitation_instructions.text.erb` or in either of the equivalent templates in devise_invitable [1]; it only appears in tests in devise_invitable. So perhaps we can remove it! [1]: https://github.com/scambra/devise_invitable/tree/v2.0.8/app/views/devise/mailer
floehopper
force-pushed
the
tidy-invitations-controller
branch
from
893fcc0 to
72c132d
Compare
chrisroos
approved these changes
Oct 4, 2023
There was a problem hiding this comment.
This all looks good to me, @floehopper!
I've left a couple of comments in the code but I'll leave you to decide whether to address them.
The two commits I most struggled to understand were Use built-in strong params for InvitationsController#update and Use built-in strong params for InvitationsController#create but I trust you and the tests that the behaviour is still as expected :-)
I also noticed a few improvements that you could make to commit notes, but again I'll leave it to decide whether to address them:
Make InvitationsController more like its superclass
The superclass, Devise::InvitationsController, prepends the
authenticate_inviter!before action [1] for thenew&create
actions. This before action callsauthenticate_user!anyway [2],
albeit with theforceoption set totrue.
The [2] links to https://github.com/scambra/devise_invitable/blob/db2f9a1c0e87addb196d1b82920370f4230c1157/app/controllers/devise/invitations_controller.rb#L4 which doesn't seem to make sense in this context.
So, all in all, I think it's clearer and more consistent with the
superclass implementation to addrequire_no_authentication!as a
before action fornew,createANDresendactions.
I read this assuming that I'd see a call to require_no_authentication! in this commit but there isn't one. Does it instead mean that we're relying on the call to require_no_authentication! in the superclass?
Rename "user" to "inviter" in InvitationsControllerTest
named "user". I've changed the relevant instances of "user" to "invitee"
I think "invitee" should be "inviter".
Extract more contexts in InvitationsControllerTest
to require 2S when the invitee's organisation does not require 2SV.
Missing "V" from "2SV".
Make InvitationsControllerTest test names more explanatory
they will be assined to an organisation for which 2SV is not required,
Typo in "assigned".
Use built-in strong params for InvitationsController#create
Lack of line breaks makes it hard to read.
The superclass, `Devise::InvitationsController`, prepends the `authenticate_inviter!` before action [1] for the `new` & `create` actions. This before action calls `authenticate_user!` anyway [2], albeit with the `force` option set to `true`. Although the `authenticate_user!` before action looked like it applied to all actions, it was being bypassed by the superclass prepending the `require_no_authentication` before action [3] for the `edit`, `update` & `destroy` actions. We've previously added the `resend` action which effectively does the same as `create` but for an existing user who has already been invited. So this needs the same protection as `new` & `create`. There's some question in my mind about whether instead of using this action, we could re-use the `create` action but with the `resend_invitation` configuration option [4]. However, I'm not going to tackle that yet. So, all in all, I think it's clearer and more consistent with the superclass implementation to remove the `authenticate_user!` before action from `InvitationsController` and add `authenticate_inviter!` as a before action but for for `new`, `create` AND `resend` actions instead of just for `new` & `create` as it is in the superclass. I've also changed the `verify_authorized` after action from using the `except` option to the `only` option to match the `authenticate_inviter!` before action. While I accept this introduces a risk that we'll forget to add any new action to these `only` option arrays, I think the improved consistency and clarity is worth it. [1]: https://github.com/scambra/devise_invitable/blob/db2f9a1c0e87addb196d1b82920370f4230c1157/app/controllers/devise/invitations_controller.rb#L2 [2]: https://github.com/scambra/devise_invitable/blob/db2f9a1c0e87addb196d1b82920370f4230c1157/lib/devise_invitable/controllers/helpers.rb#L21-L23 [3]: https://github.com/scambra/devise_invitable/blob/db2f9a1c0e87addb196d1b82920370f4230c1157/app/controllers/devise/invitations_controller.rb#L4 [4]: https://github.com/scambra/devise_invitable/blob/db2f9a1c0e87addb196d1b82920370f4230c1157/lib/devise_invitable.rb#L52-L59
This comment was out-of-date, because it talked about `accepts_nested_attributes_for` which hasn't been used for some time. I don't think the error it talks about is still pertinent either, because the permissions are now created via `InvitationsController#grant_default_permissions` which iterates through the default supported permissions calling `User#grant_permission` for each. The latter is idempotent, so if the user already has the permission, it won't be added again. I suppose one minor issue is that it won't remove any permissions that have become non-default permissions since they were added to the user the first time round, but that doesn't seem like a big issue. Anyway, I'm happy to remove this somewhat confusing comment, because we have a test in place [1] for the relevant behaviour. We can think about whether we still need that behaviour separately. [1]: https://github.com/alphagov/signon/blob/591bfe2deca05804e5cba43221c4f8694e6d6be3/test/controllers/invitations_controller_test.rb#L43-L50
`User.new` is perfectly capable of assigning `User#organisation_id` if the `:organisation_id` key exists in `all_params`. So assigning it explicitly is not necessary.
This adds a controller test for the scenario when the user is not invited because there were validation errors. While there is already an integration test for this scenario in InvitingUsersTest, I think this is still worthwhile.
Previously we were recording the "Account was invited" event even if the invitation wasn't sent due to validation errors on the form. This commit adds test coverage and fixes the bug.
This non-happy path conditional branch makes the happy path code harder to read and makes the action less consistent with the superclass implementation. I'm confident that we have enough test coverage around this behaviour to make the change withouth worrying about breaking anything.
I was finding reading the InvitationsControllerTest very confusing, because there are two users involved in each of the tests - the inviter and the invitee, but both were often referred to using text or variables named "user". I've changed the relevant instances of "user" to "inviter" and I plan to make a similar change to rename the remaining instances of "user" to "invitee" in a subsequent commit.
|
@chrisroos Many thanks for all your feedback. See my responses inline below:
Oh, yes - well spotted - the links were in quite a muddle. I've hopefully fixed them now!
No, it was a typo.
Yes, you're correct - I've fixed the typo.
Yes, I've fixed the typo.
Typo fixed.
Ah, yes - sorry - now fixed! I'll address your inline comments before I force-push the changes. |
I was finding reading the InvitationsControllerTest very confusing, because there are two users involved in each of the tests - the inviter and the invitee, but both were often referred to using text or variables named "user". This renames the remaining non-invitee users appropriately.
This is another small step in trying to make the test easy to read by being clear that the parameters relate to the invitee and not the inviter. I've also made the domain used for all email addresses consistently used gov.uk, because the differences were distracting me.
Previously we were using the `role_name` method on the relevant role class in all cases except for the normal role which we were just hard-coding as a magic string. This changes the test to use `Roles::Normal.role_name` for those instances to make them consistent.
I found the lines like this very distracting:
@inviter.update!(role: Roles::OrganisationAdmin.role_name, organisation_id: create(:organisation).id)
There's no reason we can't call `ActionController::TestCase#sign_in` [1]
multiple times and so instead of the above I've introduced an extra
level of context where we need the inviter to have a different role.
That means we can call `sign_in` again and just use the relevant user
factory. I think this makes the tests easier to read.
Note that I can't see any need to set the inviter's `organisation_id`,
because the relevant user factories do that by default.
[1]: https://github.com/alphagov/signon/blob/243f9eb898eb70be40cab6ffea55009b16f8e7cf/test/test_helper.rb#L35-L43
While we were previously testing scenarios where the inviter should not allowed access, we weren't testing scenarios where the inviter should have access. This fixes that.
Move the "happy path" tests to the top of each context. I think it's more useful to prioritise these as they characterise the most significant behaviour of the controller.
This introduces new contexts for when the invitee's organisation does or does not require 2SV. My main motivation for doing this was to be able to expand the test names to explain that the "2SV form" (i.e. the one rendered by `UsersController#require_2sv`) only allows the inviter to choose whether to require 2SV when the invitee's organisation does not require 2SV. Previously I don't think this was very obvious - or at least I found the tests rather confusing. Two of the new contexts have the same name where the organisation does not require 2SV. I thought I'd do that in a subsequent commit to make the diffs less confusing.
This makes the tests somewhat terser and idiomatic.
This improves some test name so that they explain why the inviter cannot choose whether 2SV is required for any kind of admin invitee even if they will be assigned to an organisation for which 2SV is not required, i.e. because 2SV is required for all kinds of admin invitees irrespective of the organanisation-level setting.
This inlines `app/views/users/_form_fields.html.erb` into `app/views/devise/invitations/new.html.erb`, but leaves the former in place so it can still be used by `UsersController#edit`. There's a bunch of code in this partial which is irrelevant for a new user. I plan to remove that from `app/views/devise/invitations/new.html.erb` in subsequent commits. Similarly there is almost certainly some code in the partial which is irrelevant for a persisted user. I'll try to remove that too. While this comes at the cost of some duplication, I think it'll be worth it to make it clearer what user attributes can be set or updated in each of the scenarios.
The user (i.e. `resource`) provided to `app/views/devise/invitations/new.html.erb` will always be a new record and so `User#persisted?` will always be `false` and so we can remove this code.
The user (i.e. `resource`) provided to `app/views/devise/invitations/new.html.erb` will always be a new record and so `User#unconfirmed_email` will always be `nil` and so we can remove this code.
Only govuk admins (i.e. admins & superadmins) can invite users (see `UserPolicy#new?`; publishing managers (i.e. organisation admins & super organisation admins) cannot invite users and so they will never see this page and we can safely remove this code.
Previously both the `if` *and* the `elsif` included the `policy(User).assign_role?` conditional. By moving the rest of the `if` condition into its own nested `if` and pairing it with an `else` we can make things clearer. Ideally I would've done this *before* I inlined the form fields into the new invitation template, but it would be awkward to rewrite the git history, so I'm taking the lazy option and updating it in both places.
The user (i.e. `resource`) provided to `app/views/devise/invitations/new.html.erb` will always be a new record and so `User#reason_for_2sv_exemption` will always be blank and so we can remove the code in the `else` block.
The user provided to `app/views/users/_form_fields.html.erb` from `UsersController#edit` will always be a persisted record and so `User#persisted?` will always be `true` and so we can remove this code.
By combining the creation of the relevant user type and the act of signing in, we can reduce the number of levels of context nesting by one. Ideally I'd re-write the git history to include this, but that's quite awkward and I don't think it's worth the effort.
The saving of the invitee isn't specific to whether or not their organisation requires 2SV or not.
* Invitee user is created with the appropriate attributes * Invitee user is assigned default supported permissions * Admin inviter cannot assign invitee role * Invitee is invited via email by inviter
* User#require_2sv is set to true if Organisation#require_2sv? is true. * User#require_2sv is set to true if invitee will have any kind of admin role, even if Organisation#require_2sv? is false.
It's not immediately obvious but this is the action which is used for accepting an invitation at which point the user is required to set their password. While we're just using the inherited `Devise::InvitationsController#update` [1] unchanged, we *are* customizing some of the methods that it uses, e.g. `#resource_params` & `#update_resource_params`. So while to some extent we can rely on the tests in `devise_invitable`, I want to get some more confidence about the current behaviour. In particular I find the way the code decides whether certain params are permitted (for bulk assignment) or not is quite hard to follow, because it's spread across a number of methods & classes, e.g. * `InvitationsController#resource_params` * `InvitationsController#unsanitised_user_params` * `InvitationsController#current_user_role` * `UserParameterSanitiser` * `Roles::***.permitted_user_params` For example, the reason the invitation acceptance form can't be hacked to update the invitee's role (which seems quite important) is because `InvitationsController#current_user_role` will be "normal" and so `UserParameterSanitiser#sanitise` will exclude the `role` param, because `Roles::Normal.permitted_user_params` does not include `role`. This isn't immediately obvious! [1]: https://github.com/scambra/devise_invitable/blob/v2.0.8/app/controllers/devise/invitations_controller.rb
It's not immediately obvious but this is the action which is used for
accepting an invitation at which point the user is required to set their
password.
Previously we were overriding
`Devise::InvitationsController#update_resource_params` to return the
params from `InvitationsController#resource_params` which in turn was
overriding `Devise::InvitationsController#resource_params`.
`InvitationsController#resource_params` was using
`UserParameterSanitiser#sanitise` to further restrict the params
permitted in `InvitationsController#unsanitised_user_params`.
The latter permitted the following keys:
* `name`
* `email`
* `organisation_id`
* `invitation_token`
* `password`
* `password_confirmation`
* `require_2sv`
* `role`
* `supported_permission_ids`
And then since `current_user` is always `nil` in `InvitationsController#update`,
`UserParameterSanitiser#sanitise` was further restricting the permitted
params by only allowing keys returned from `Roles::Normal.permitted_user_params`:
* `uid`
* `name`
* `email`
* `password`
* `password_confirmation`
As you can see `uid` is never permitted by
`InvitationsController#unsanitised_user_params` and so the only keys
permitted by `UserParameterSanitiser#sanitise` were:
* `name`
* `email`
* `password`
* `password_confirmation`
`InvitationsController#resource_params` was then adding
`invitation_token`, because it was handling the `update` action. Giving
a final list of:
* `name`
* `email`
* `password`
* `password_confirmation`
* `invitation_token`
This seems like an extremely complicated and overly permissive way to
permit the params that are actually needed for this action, i.e.
`invitation_token`, `password` & `password_confirmation` which are the
fields in the form in `app/views/devise/invitations/edit.html.erb` and
thence `app/views/devise/passwords/_change_password_panel.html.erb`
(`current_password` is not rendered as part of
`InvitationsController#edit`, because the `updating_password` local is
not set).
In this commit I've attempted to come up with a simpler solution which
is also more in keeping with `devise_invitable` as follows:
* I've renamed `InvitationsController#resource_params` to
`#invite_params` and change the implementation of `#create` so it uses
the renamed method. The new method name matches the method that's used
in the superclass [1]. The idea is that this method is only used to
provide params to the `#create` action and not the `#update` action.
* I've removed the custom implementation of `#update_resource_params`
and instead fallback to the implementation in the superclass [2]. This
uses the Strong Parameters support built into `devise_invitable` [3] to
permit `invitation_token`, `password` & `password_confirmation`, i.e.
the only params that we require.
* Since the method renamed to `#invite_params` is no longer used for the
`#update` action, we can remove the `if`/`else` statement. And since
the `#invitation_token` method is not longer used in that statement, it
can also be removed.
These changes have the effect of tightening up the list of permitted
params so that `name` & `email` are no longer permitted. Previously it
was theoretically possible to hack the invitation acceptance form to
change your name & email, but I strongly suspect this was not
intentional and possibly even a security risk.
As an aside, it was rather odd that the fallback value of
`#invitation_token` was `{}`! I would've epxected it to be "" or just
`nil`.
[1]: https://github.com/scambra/devise_invitable/blob/db2f9a1c0e87addb196d1b82920370f4230c1157/app/controllers/devise/invitations_controller.rb#L106C9-L108
[2]: https://github.com/scambra/devise_invitable/blob/db2f9a1c0e87addb196d1b82920370f4230c1157/app/controllers/devise/invitations_controller.rb#L110-L112
[3]: https://github.com/scambra/devise_invitable/blob/v2.0.8/README.rdoc#label-Strong+Parameters
This template is only rendered from `InvitationsController#new` and the `authorize User` statement calls `UserPolicy#new?` which only allows access to admins and superadmins. The conditional in the template was calling `UserPolicy#assign_organisations?` which is aliased to `UserPolicy#new?`, i.e. anyone who can view the page is allowed to assign an organisation to the invitee. Thus the conditional is redundant and can be removed.
It's not immediately obvious but this is the action which is used for creating a user and sending them an invitation. Previously we were overriding `Devise::InvitationsController#invite_params` to use `UserParameterSanitiser#sanitise` to further restrict the params permitted in `InvitationsController#unsanitised_user_params`. The latter permitted the following keys: * `name` * `email` * `organisation_id` * `invitation_token` * `password` * `password_confirmation` * `require_2sv` * `role` * `supported_permission_ids` And then since `current_user` is always and admin or a superadmin in `InvitationsController#create`, `UserParameterSanitiser#sanitise` was further restricting the permitted params by only allowing keys returned from `Roles::Admin.permitted_user_params` or `Roles::Superadmin.permitted_user_params`: * `uid` * `name` * `email` * `organisation_id` * `password` * `password_confirmation` * `require_2sv` * `role` (only for superadmins) * `supported_permission_ids` * `unconfirmed_email` * `confirmation_token` As you can see `uid`, `invitation_token`, `unconfirmed_email` & `confirmation_token` were never permitted by `InvitationsController#unsanitised_user_params` and so the only keys permitted by `UserParameterSanitiser#sanitise` were: * `name` * `email` * `organisation_id` * `password` * `password_confirmation` * `require_2sv` * `role` (only for superadmins) * `supported_permission_ids` This seems like an extremely complicated and overly permissive way to permit the params that are actually needed for this action, i.e. `name`, `email`, `organisation_id`, `supported_permission_ids` & `role` (the latter only for superadmins) which are the fields in the form in `app/views/devise/invitations/new.html.erb` and thence `app/views/shared/_user_permissions.html.erb`. In this commit I've attempted to come up with a simpler solution which is also more in keeping with `devise_invitable` as follows: * I've removed `InvitationsController#invite_params` which means that the `#create` action now relies on the superclass implementation [1]. This only permits the `email` param by default. * I've added a new `configure_permitted_parameters` before action (just for the `create` action) as recommended in the `devise_invitable` documentation for Strong Parameters [2]. The `#configure_permitted_parameters` method permits `name`, `organisation_id` & `supported_permission_ids`. It also permits `role`, but only if the current user is allowed to assign a role, i.e. they are a superadmin (this is the only difference between the keys returned by `Roles::Superadmin.permitted_user_params` & `Roles::Admin.permitted_user_params`). I believe these are the only params that should be submitted by the `app/views/devise/invitations/new.html.erb` form (see above). * I've removed `#unsanitised_user_params` & `#current_user_role` which are both now redundant. These changes have the effect of tightening up the list of permitted params so that `password`, `password_confirmation` & `require_2sv` are no longer permitted. Previously it was theoretically possible to hack the new invitation form to change your these attributes on the created user, but I strongly suspect this was not intentional. [1]: https://github.com/scambra/devise_invitable/blob/db2f9a1c0e87addb196d1b82920370f4230c1157/app/controllers/devise/invitations_controller.rb#L106C9-L108 [2]: https://github.com/scambra/devise_invitable/blob/v2.0.8/README.rdoc#label-Strong+Parameters
Previously we were only temporarily instantiating a user in order to (implicitly) pass it (as `record`) to `UserPolicy#create?`. The implementation of the latter was the same as `UserPolicy#new?` except that it prevented admins from creating a superadmin invitee. However, since only superadmins (and not admins) are allowed to assign a role (see `UserPolicy#assign_role?`, admins can only ever create invitees with the "normal" role. So it seems simpler to change the authorization for the `#create` action to work exactly the same way as the `#new` action. It also has the benefit that we can legitimately remove the slightly confusiing comment about a "workaround". I'm not 100% sure the changes to `UserPolicyTest` are correct, because I found them rather confusing. However, I've tried to change the tests for `UserPolicy#create?` to be the same as for `UserPolicy#new?`. To do this I've moved `create` from `user_management_actions` to `primary_management_actions` which is where `new` lives. And I've had to remove two existing explicit tests for `UserPolicy#create?`, because `primary_management_actions` was creating duplicate tests.
To `#invitee_requires_2sv`. I think this is more consistent with the naming in the rest of the controller.
I suspect this was previously necessary (if it ever was), because we were converting an instance of `ActionController::Parameters` to a `Hash` by calling `#to_h` in what was the `#resource_params` before the set of changes in this PR. However, now that we're using the `#invite_params` implemented in the superclass, we're now working directly with an instance of `ActionController::Parameters` which seems to allow access via either string or symbol keys. Thus the call to `#symbolize_keys` is redundant and can be removed.
Given this method argument is not used, I think it's clearer not to give it a name, particularly one which is easily confused with the `#resource` accessor method.
This seems more consistent with the code in the rest of this action.
|
Force-pushing the changes I've made in the light of @chrisroos' feedback in preparation for merging. |
floehopper
force-pushed
the
tidy-invitations-controller
branch
from
72c132d to
cc50bed
Compare
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/umBDQZUc
This is in preparation for splitting the page handled by the
InvitationsController#new&#createactions into two pages with the second handling the setting of permissions. TheInvitationsControlleris made more complicated than a typical controller for a few reasons, e.g.:#new,#create&#destroyactions is admins & superadmins inviting new users; whereas the audience for the#edit&#updateactions is new users accepting their invitations and setting their password.Devise::InvitationsControllerprovided bydevise_invitablebut significantly customises its behaviour and its template(s) inapp/views/devise/invitations/.There's definitely a question in my mind about how much benefit we're getting from inheriting from
Devise::InvitationsController, particularly given how much we are customising it and how easy it is for the code to get out-of-date with the gem as it gets updated with Dependabot. However, I decided that this is out-of-scope for this PR.Currently there's an optional 2nd page for when an admin can choose whether or not the invited user must use 2SV which further complicates the controller. I'm hoping that I can combine this page into the 1st page in a subsequent PR.
While this PR has a lot of commits, most of them are relatively small and only make changes to two files:
app/controllers/invitations_controller.rbtest/controllers/invitations_controller_test.rbThe main changes in this PR are:
InvitationsControllerTestand try to makeInvitationsControllerTesteasier to follow.InvitationsControllerand bring it more into line with the current version ofDevise::InvitationsControllerUserParameterSanitiser. I'm not convinced that the latter is a useful abstraction, because it makes it much harder to see which parameters are permitted for a given controller/action and (in combination withRoles::***.permitted_user_paramsmethods it seems to contain logic that probably belongs in Pundit policies.app/views/devise/invitations/new.html.erbtemplate rather than re-usingapp/views/users/_form_fields.html.erb- the latter was unnecessarily complicated and made it hard to see the wood for the trees. Admittedly this comes at the cost of a little duplication, but I think the benefits outweigh the costs.Notable fixes in this PR
Use built-in strong params for InvitationsController#update
Use built-in strong params for InvitationsController#create