Merge pull request #2401 from alphagov/tidy-invitations-controller

Tidy InvitationsController

Author: floehopper

app/controllers/invitations_controller.rb

@@ -1,7 +1,11 @@
 # https://raw.githubusercontent.com/scambra/devise_invitable/master/app/controllers/devise/invitations_controller.rb
 class InvitationsController < Devise::InvitationsController
-  before_action :authenticate_user!
-  after_action :verify_authorized, except: %i[edit update] # rubocop:disable Rails/LexicallyScopedActionFilter
+  before_action :authenticate_inviter!, only: %i[new create resend]
+  after_action :verify_authorized, only: %i[new create resend]
+
+  before_action :redirect_if_invitee_already_exists, only: :create
+  before_action :configure_permitted_parameters, only: :create
+
   layout "admin_layout", only: %i[edit update]
 
   include UserPermissionsControllerMethods
@@ -13,34 +17,37 @@ def new
   end
 
   def create
-    # Prevent an error when devise_invitable invites/updates an existing user,
-    # and accepts_nested_attributes_for tries to create duplicate permissions.
-    if (self.resource = User.find_by(email: params[:user][:email]))
-      authorize resource
-      flash[:alert] = "User already invited. If you want to, you can click 'Resend signup email'."
-      respond_with resource, location: users_path
+    authorize User
+
+    all_params = invite_params
+    all_params[:require_2sv] = invitee_requires_2sv(all_params)
+
+    self.resource = resource_class.invite!(all_params, current_inviter)
+    if resource.errors.empty?
+      grant_default_permissions(resource)
+      EventLog.record_account_invitation(resource, current_user)
+      set_flash_message :notice, :send_instructions, email: resource.email
+      respond_with resource, location: after_invite_path_for(resource)
     else
-      # workaround for invitatable not providing a build_invitation which could be authorised before saving
-      all_params = resource_params
-      all_params[:require_2sv] = new_user_requires_2sv(all_params.symbolize_keys)
-
-      user = User.new(all_params)
-      user.organisation_id = all_params[:organisation_id]
-      authorize user
-
-      self.resource = resource_class.invite!(all_params, current_inviter)
-      if resource.errors.empty?
-        grant_default_permissions(resource)
-        set_flash_message :notice, :send_instructions, email: resource.email
-        respond_with resource, location: after_invite_path_for(resource)
-      else
-        respond_with_navigational(resource) { render :new }
-      end
-
-      EventLog.record_account_invitation(@user, current_user)
+      respond_with_navigational(resource) { render :new }
     end
   end
 
+  # rubocop:disable Lint/UselessMethodDefinition
+  # Renders app/views/devise/invitations/edit.html.erb
+  def edit
+    super
+  end
+
+  def update
+    super
+  end
+
+  def destroy
+    super
+  end
+  # rubocop:enable Lint/UselessMethodDefinition
+
   def resend
     user = User.find(params[:id])
     authorize user
@@ -52,86 +59,39 @@ def resend
 
 private
 
-  def after_invite_path_for(_resource)
-    if new_user_requires_2sv(resource)
+  def after_invite_path_for(_)
+    if invitee_requires_2sv(resource)
       users_path
     else
       require_2sv_user_path(resource)
     end
   end
 
-  # TODO: remove this method when we're on a version of devise_invitable which
-  # no longer expects it to exist (v1.2.1 onwards)
-  def build_resource
-    self.resource = resource_class.new(resource_params)
-  end
-
-  def resource_params
-    sanitised_params = UserParameterSanitiser.new(
-      user_params: unsanitised_user_params,
-      current_user_role:,
-    ).sanitise
-
-    if params[:action] == "update"
-      sanitised_params.to_h.merge(invitation_token:)
-    else
-      sanitised_params.to_h
+  def grant_default_permissions(user)
+    SupportedPermission.default.each do |default_permission|
+      user.grant_permission(default_permission)
     end
   end
 
-  # TODO: once we've upgraded Devise and DeviseInvitable, `resource_params`
-  # hopefully won't be being called for actions like `#new` anymore and we
-  # can change the following `params.fetch(:user)` to
-  # `params.require(:user)`. See
-  # https://github.com/scambra/devise_invitable/blob/v1.1.5/app/controllers/devise/invitations_controller.rb#L10
-  # and
-  # https://github.com/plataformatec/devise/blob/v2.2/app/controllers/devise_controller.rb#L99
-  # for details :)
-  def unsanitised_user_params
-    params.require(:user).permit(
-      :name,
-      :email,
-      :organisation_id,
-      :invitation_token,
-      :password,
-      :password_confirmation,
-      :require_2sv,
-      :role,
-      supported_permission_ids: [],
-    ).to_h
-  end
-
-  # NOTE: `current_user` doesn't exist for `#edit` and `#update` actions as
-  # implemented in our current (out-of-date) versions of Devise
-  # (https://github.com/plataformatec/devise/blob/v2.2/app/controllers/devise_controller.rb#L117)
-  # and DeviseInvitable
-  # (https://github.com/scambra/devise_invitable/blob/v1.1.5/app/controllers/devise/invitations_controller.rb#L5)
-  #
-  # 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. 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.
-  def current_user_role
-    current_user.try(:role).try(:to_sym) || :normal
+  def organisation(params)
+    Organisation.find_by(id: params[:organisation_id])
   end
 
-  def invitation_token
-    unsanitised_user_params.fetch(:invitation_token, {})
+  def invitee_requires_2sv(params)
+    organisation(params)&.require_2sv? || User.admin_roles.include?(params[:role])
   end
 
-  def update_resource_params
-    resource_params
-  end
-
-  def grant_default_permissions(user)
-    SupportedPermission.default.each do |default_permission|
-      user.grant_permission(default_permission)
+  def redirect_if_invitee_already_exists
+    if (resource = User.find_by(email: params[:user][:email]))
+      authorize resource
+      flash[:alert] = "User already invited. If you want to, you can click 'Resend signup email'."
+      respond_with resource, location: users_path
     end
   end
 
-  def new_user_requires_2sv(params)
-    (params[:organisation_id].present? && Organisation.find(params[:organisation_id]).require_2sv?) ||
-      %w[superadmin admin organisation_admin super_organisation_admin].include?(params[:role])
+  def configure_permitted_parameters
+    keys = [:name, :organisation_id, { supported_permission_ids: [] }]
+    keys << :role if policy(User).assign_role?
+    devise_parameter_sanitizer.permit(:invite, keys:)
   end
 end

app/policies/user_policy.rb

@@ -10,9 +10,7 @@ def new?
   alias_method :assign_organisations?, :new?
 
   # invitations#create
-  def create?
-    current_user.superadmin? || (current_user.admin? && !record.superadmin?)
-  end
+  alias_method :create?, :new?
 
   def edit?
     return false if current_user == record

app/views/devise/invitations/new.html.erb

@@ -5,7 +5,36 @@
 <%= form_for resource, :as => resource_name, :url => invitation_path(resource_name), :html => {:method => :post, :class => 'well'} do |f| %>
   <%= render "devise/shared/error_messages", resource: resource %>
 
-  <%= render partial: "users/form_fields", locals: { f: f } %>
+  <p class="form-group">
+    <%= f.label :name %>
+    <%= f.text_field :name, autofocus: true, autocomplete: "off", class: 'form-control input-md-6 ' %>
+  </p>
+
+  <p class="form-group">
+    <%= f.label :email %>
+    <%= f.text_field :email, autocomplete: "off", class: 'form-control input-md-6 add-label-margin' %>
+  </p>
+
+  <% if policy(User).assign_role? %>
+    <p class="form-group">
+      <%= f.label :role %><br />
+      <%= f.select :role, options_for_select(assignable_user_roles.map(&:humanize).zip(assignable_user_roles), f.object.role), {}, class: "chosen-select form-control", 'data-module' => 'chosen' %>
+      <span class="help-block">
+        <strong>Superadmins</strong> can create and edit all user types and edit applications.<br />
+        <strong>Admins</strong> can create and edit normal users.<br />
+        <strong>Super Organisation Admins</strong> can unlock and unsuspend their organisation and related organisation accounts.<br />
+        <strong>Organisation Admins</strong> can unlock and unsuspend their organisation accounts.
+      </span>
+    </p>
+  <% end %>
+
+  <p class="form-group">
+    <%= f.label :organisation_id, "Organisation" %><br />
+    <%= f.select :organisation_id, organisation_options(f), organisation_select_options, { class: "chosen-select form-control", 'data-module' => 'chosen' } %>
+  </p>
+
+  <h2 class="add-vertical-margins">Permissions</h2>
+  <%= render partial: "shared/user_permissions", locals: { user_object: f.object }%>
 
   <%= f.submit "Create user and send email", :class => 'btn btn-success' %>
 <% end %>

app/views/users/_form_fields.html.erb

@@ -6,10 +6,8 @@
 <p class="form-group">
   <%= f.label :email %>
   <%= f.text_field :email, autocomplete: "off", class: 'form-control input-md-6 add-label-margin' %>
-  <% if f.object.persisted? %>
-    <% if f.object.invited_but_not_yet_accepted? %>
-      <span class="help-block">Changes will trigger a new signup email.</span>
-    <% end %>
+  <% if f.object.invited_but_not_yet_accepted? %>
+    <span class="help-block">Changes will trigger a new signup email.</span>
   <% end %>
 </p>
 
@@ -25,22 +23,24 @@
   </p>
 <% end %>
 
-<% if policy(User).assign_role? && @user.reason_for_2sv_exemption.blank? %>
-  <p class="form-group">
-    <%= f.label :role %><br />
-    <%= f.select :role, options_for_select(assignable_user_roles.map(&:humanize).zip(assignable_user_roles), f.object.role), {}, class: "chosen-select form-control", 'data-module' => 'chosen' %>
-    <span class="help-block">
-      <strong>Superadmins</strong> can create and edit all user types and edit applications.<br />
-      <strong>Admins</strong> can create and edit normal users.<br />
-      <strong>Super Organisation Admins</strong> can unlock and unsuspend their organisation and related organisation accounts.<br />
-      <strong>Organisation Admins</strong> can unlock and unsuspend their organisation accounts.
-    </span>
-  </p>
-<% elsif policy(User).assign_role? %>
-  <p>This user's role is set to <%= @user.role %>. They are currently exempted from 2-step verification, meaning that their role cannot be changed as admins are required to have 2-step verification.</p>
+<% if policy(User).assign_role? %>
+  <% if @user.reason_for_2sv_exemption.blank? %>
+    <p class="form-group">
+      <%= f.label :role %><br />
+      <%= f.select :role, options_for_select(assignable_user_roles.map(&:humanize).zip(assignable_user_roles), f.object.role), {}, class: "chosen-select form-control", 'data-module' => 'chosen' %>
+      <span class="help-block">
+        <strong>Superadmins</strong> can create and edit all user types and edit applications.<br />
+        <strong>Admins</strong> can create and edit normal users.<br />
+        <strong>Super Organisation Admins</strong> can unlock and unsuspend their organisation and related organisation accounts.<br />
+        <strong>Organisation Admins</strong> can unlock and unsuspend their organisation accounts.
+      </span>
+    </p>
+  <% else %>
+    <p>This user's role is set to <%= @user.role %>. They are currently exempted from 2-step verification, meaning that their role cannot be changed as admins are required to have 2-step verification.</p>
+  <% end %>
 <% end %>
 
-<% if policy(@user).mandate_2sv? && @user.persisted? %>
+<% if policy(@user).mandate_2sv? %>
   <dl>
     <dt>Account security</dt>
     <dd>
@@ -77,7 +77,7 @@
         <br/>
         User will be prompted to set up 2-step verification again the next time they sign in.</p>
       <% end %>
-      <% if @user.persisted? && policy(@user).exempt_from_two_step_verification? && @user.reason_for_2sv_exemption.nil? %>
+      <% if policy(@user).exempt_from_two_step_verification? && @user.reason_for_2sv_exemption.nil? %>
           <p>
             <%= link_to 'Exempt user from 2-step verification', edit_two_step_verification_exemption_path(@user) %>
             <br/>

lib/roles.rb

@@ -28,6 +28,10 @@ def admin_role_classes
       role_classes - [Roles::Normal, Roles::Base]
     end
 
+    def admin_roles
+      admin_role_classes.map(&:role_name)
+    end
+
     def roles
       role_classes.sort_by(&:level).map(&:role_name)
     end

test/controllers/invitations_controller_test.rb

@@ -35,6 +35,19 @@ class Subject
     end
   end
 
+  context ".admin_roles" do
+    should "only include admin role names" do
+      expected_role_names = [
+        Roles::Admin.role_name,
+        Roles::Superadmin.role_name,
+        Roles::OrganisationAdmin.role_name,
+        Roles::SuperOrganisationAdmin.role_name,
+      ]
+
+      assert_same_elements expected_role_names, Subject.admin_roles
+    end
+  end
+
   context ".roles" do
     should "order roles by their level" do
       expected_ordered_roles = %w[

test/lib/roles_test.rb

@@ -12,8 +12,8 @@ class UserPolicyTest < ActiveSupport::TestCase
     @gds = create(:organisation, name: "Government Digital Services", content_id: Organisation::GDS_ORG_CONTENT_ID)
   end
 
-  primary_management_actions = %i[new assign_organisations]
-  user_management_actions = %i[edit create update unlock suspension cancel_email_change resend_email_change event_logs reset_2sv mandate_2sv]
+  primary_management_actions = %i[new create assign_organisations]
+  user_management_actions = %i[edit update unlock suspension cancel_email_change resend_email_change event_logs reset_2sv mandate_2sv]
   superadmin_actions = %i[assign_role]
   two_step_verification_exemption_actions = %i[exempt_from_two_step_verification]
 
@@ -42,8 +42,6 @@ class UserPolicyTest < ActiveSupport::TestCase
         assert permit?(user, build(:superadmin_user), permission)
       end
 
-      next if permission == :create
-
       should "not allow for #{permission} for the logged in user" do
         user = create(:superadmin_user)
 
@@ -102,8 +100,6 @@ class UserPolicyTest < ActiveSupport::TestCase
         assert forbid?(user, build(:superadmin_user), permission)
       end
 
-      next if permission == :create
-
       should "not allow for #{permission} for the logged in user" do
         user = create(:admin_user)
 
@@ -140,10 +136,6 @@ class UserPolicyTest < ActiveSupport::TestCase
       assert permit?(build(:super_organisation_admin_user), User, :index)
     end
 
-    should "not allow for create" do
-      assert forbid?(build(:super_organisation_admin_user), User, :create)
-    end
-
     primary_management_actions.each do |permission|
       should "not allow for #{permission}" do
         assert forbid?(build(:super_organisation_admin_user), User, permission)
@@ -201,10 +193,6 @@ class UserPolicyTest < ActiveSupport::TestCase
       assert permit?(build(:organisation_admin_user), User, :index)
     end
 
-    should "not allow for create" do
-      assert forbid?(build(:organisation_admin_user), User, :create)
-    end
-
     primary_management_actions.each do |permission|
       should "not allow for #{permission}" do
         assert forbid?(build(:organisation_admin_user), User, permission)