diff --git a/app/controllers/users/roles_controller.rb b/app/controllers/users/roles_controller.rb new file mode 100644 index 000000000..5ad380d8a --- /dev/null +++ b/app/controllers/users/roles_controller.rb @@ -0,0 +1,38 @@ +class Users::RolesController < ApplicationController + layout "admin_layout" + + before_action :authenticate_user! + before_action :load_user + before_action :authorize_user + before_action :redirect_to_account_page_if_acting_on_own_user, only: %i[edit] + + def edit; end + + def update + updater = UserUpdate.new(@user, user_params, current_user, user_ip_address) + if updater.call + redirect_to edit_user_path(@user), notice: "Updated user #{@user.email} successfully" + else + render :edit + end + end + +private + + def load_user + @user = User.find(params[:user_id]) + end + + def authorize_user + authorize(@user) + authorize(@user, :assign_role?) + end + + def user_params + params.require(:user).permit(*current_user.permitted_params.intersection([:role])) + end + + def redirect_to_account_page_if_acting_on_own_user + redirect_to edit_account_role_path if current_user == @user + end +end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index a5d7a3cd1..1850bed3d 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -112,7 +112,7 @@ def user_params end def permitted_user_params - @permitted_user_params ||= params.require(:user).permit(:user, :email, :organisation_id, :require_2sv, :role, :skip_update_user_permissions, supported_permission_ids: []).to_h + @permitted_user_params ||= params.require(:user).permit(:user, :organisation_id, :require_2sv, :skip_update_user_permissions, supported_permission_ids: []).to_h end def filter_params diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 328a0e2d1..20c7661bc 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -92,4 +92,16 @@ def formatted_permission_name(application_name, permission_name) permission_name end end + + def user_role_select_hint + render "govuk_publishing_components/components/list", { + visible_counters: true, + items: [ + "Superadmins can create and edit all user types and edit applications.", + "Admins can create and edit normal users.", + "Super Organisation Admins can unlock and unsuspend their organisation and related organisation accounts.", + "Organisation Admins can unlock and unsuspend their organisation accounts.", + ], + } + end end diff --git a/app/views/devise/invitations/new.html.erb b/app/views/devise/invitations/new.html.erb index f5933bb7c..7c5bc0a75 100644 --- a/app/views/devise/invitations/new.html.erb +++ b/app/views/devise/invitations/new.html.erb @@ -60,23 +60,11 @@ } %> <% if policy(User).assign_role? %> - <% role_hint = capture do %> - <%= render "govuk_publishing_components/components/list", { - visible_counters: true, - items: [ - "Superadmins can create and edit all user types and edit applications.", - "Admins can create and edit normal users.", - "Super Organisation Admins can unlock and unsuspend their organisation and related organisation accounts.", - "Organisation Admins can unlock and unsuspend their organisation accounts.", - ] - } %> - <% end %> - <%= render "govuk_publishing_components/components/select", { id: "user_role", name: "user[role]", label: "Role", - hint: role_hint, + hint: user_role_select_hint, options: options_for_role_select(selected: f.object.role), } %> <% end %> diff --git a/app/views/users/_form_fields.html.erb b/app/views/users/_form_fields.html.erb index 1a3e70988..b70686413 100644 --- a/app/views/users/_form_fields.html.erb +++ b/app/views/users/_form_fields.html.erb @@ -12,22 +12,14 @@ <% end %>

-<% if policy(User).assign_role? %> - <% if @user.exempt_from_2sv? %> -

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.

- <% else %> -

- <%= f.label :role %>
- <%= 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' %> - - Superadmins can create and edit all user types and edit applications.
- Admins can create and edit normal users.
- Super Organisation Admins can unlock and unsuspend their organisation and related organisation accounts.
- Organisation Admins can unlock and unsuspend their organisation accounts. -
-

+

+ Role: <%= @user.role.humanize %> + <% if policy(User).assign_role? %> + <%= link_to edit_user_role_path(@user) do %> + Change + <% end %> <% end %> -<% end %> +

<% if policy(@user).mandate_2sv? %>
diff --git a/app/views/users/roles/edit.html.erb b/app/views/users/roles/edit.html.erb new file mode 100644 index 000000000..918812011 --- /dev/null +++ b/app/views/users/roles/edit.html.erb @@ -0,0 +1,66 @@ +<% content_for :title_caption, "Manage other users" %> +<% content_for :title, "Change role for #{@user.name}" %> + +<% content_for :breadcrumbs, + render("govuk_publishing_components/components/breadcrumbs", { + collapse_on_mobile: true, + breadcrumbs: [ + { + title: "Dashboard", + url: root_path, + }, + { + title: "Users", + url: users_path, + }, + { + title: @user.name, + url: edit_user_path(@user), + }, + { + title: "Change role", + } + ] + }) +%> + +<% if @user.errors.count > 0 %> + <% content_for :error_summary do %> + <%= render "govuk_publishing_components/components/error_summary", { + title: "There is a problem", + items: @user.errors.map do |error| + { + text: error.full_message, + href: "#user_#{error.attribute}", + } + end, + } %> + <% end %> +<% end %> + +
+
+ <%= form_for @user, url: user_role_path(@user) do %> + <% if @user.exempt_from_2sv? %> + <%= render "govuk_publishing_components/components/inset_text", { + text: "This user's role is set to #{@user.role.humanize}. They are currently exempted from 2-step verification, meaning that their role cannot be changed as admins are required to have 2-step verification.", + } %> + <% else %> + <%= render "govuk_publishing_components/components/select", { + id: "user_role", + name: "user[role]", + label: "Role", + hint: user_role_select_hint, + options: current_user.manageable_roles.map { |role| { text: role.humanize, value: role, selected: @user.role == role } }, + error_message: @user.errors[:role].any? ? @user.errors.full_messages_for(:role).to_sentence : nil + } %> +
+ <%= render "govuk_publishing_components/components/button", { + text: "Change role", + } %> + <%= link_to "Cancel", edit_user_path(@user), class: "govuk-link govuk-link--no-visited-state" %> +
+ <% end %> + <% end %> +
+
diff --git a/config/routes.rb b/config/routes.rb index 79f846c7d..74d79e37e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -48,6 +48,7 @@ put :resend_email_change delete :cancel_email_change end + resource :role, only: %i[edit update], controller: "users/roles" end get "user", to: "oauth_users#show" diff --git a/test/controllers/users/roles_controller_test.rb b/test/controllers/users/roles_controller_test.rb new file mode 100644 index 000000000..b0e15ece5 --- /dev/null +++ b/test/controllers/users/roles_controller_test.rb @@ -0,0 +1,258 @@ +require "test_helper" + +class Users::RolesControllerTest < ActionController::TestCase + context "GET edit" do + context "signed in as Superadmin user" do + setup do + @superadmin = create(:superadmin_user) + sign_in(@superadmin) + end + + should "display form with role field" do + user = create(:user, role: Roles::Normal.role_name) + + get :edit, params: { user_id: user } + + assert_template :edit + assert_select "form[action='#{user_role_path(user)}']" do + assert_select "select[name='user[role]'] option", value: Roles::Normal.role_name + assert_select "button[type='submit']", text: "Change role" + assert_select "a[href='#{edit_user_path(user)}']", text: "Cancel" + end + end + + should "not display form if user is exempt from 2SV" do + user = create(:two_step_exempted_user, role: Roles::Normal.role_name) + + get :edit, params: { user_id: user } + + assert_select "form[action='#{user_role_path(user)}']" do + assert_select "select[name='user[role]'] option", count: 0 + assert_select ".govuk-inset-text", text: /their role cannot be changed/ + end + end + + should "authorize access if UserPolicy#edit? and UserPolicy#assign_role? return true" do + user = create(:user) + + user_policy = stub_everything("user-policy", edit?: true, assign_role?: true) + UserPolicy.stubs(:new).returns(user_policy) + + get :edit, params: { user_id: user } + + assert_response :success + end + + should "not authorize access if UserPolicy#edit? returns false" do + user = create(:user) + + user_policy = stub_everything("user-policy", edit?: false, assign_role?: true) + UserPolicy.stubs(:new).returns(user_policy) + + get :edit, params: { user_id: user } + + assert_not_authorised + end + + should "not authorize access if UserPolicy#assign_role? returns false" do + user = create(:user) + + user_policy = stub_everything("user-policy", edit?: true, assign_role?: false) + UserPolicy.stubs(:new).returns(user_policy) + + get :edit, params: { user_id: user } + + assert_not_authorised + end + + should "redirect to account edit role page if admin is acting on their own user" do + get :edit, params: { user_id: @superadmin } + + assert_redirected_to edit_account_role_path + end + end + + context "signed in as Normal user" do + setup do + sign_in(create(:user)) + end + + should "not be authorized" do + user = create(:user) + + get :edit, params: { user_id: user } + + assert_not_authorised + end + end + + context "not signed in" do + should "not be allowed access" do + user = create(:user) + + get :edit, params: { user_id: user } + + assert_not_authenticated + end + end + end + + context "PUT update" do + context "signed in as Superadmin user" do + setup do + @superadmin = create(:superadmin_user) + sign_in(@superadmin) + end + + should "update user role" do + user = create(:user_in_organisation, role: Roles::Normal.role_name) + + put :update, params: { user_id: user, user: { role: Roles::OrganisationAdmin.role_name } } + + assert_equal Roles::OrganisationAdmin.role_name, user.reload.role + end + + should "record account updated & role change events" do + user = create(:user_in_organisation, role: Roles::Normal.role_name) + + @controller.stubs(:user_ip_address).returns("1.1.1.1") + + EventLog.expects(:record_event).with( + user, + EventLog::ACCOUNT_UPDATED, + initiator: @superadmin, + ip_address: "1.1.1.1", + ) + + EventLog.expects(:record_role_change).with( + user, + Roles::Normal.role_name, + Roles::OrganisationAdmin.role_name, + @superadmin, + ) + + put :update, params: { user_id: user, user: { role: Roles::OrganisationAdmin.role_name } } + end + + should "should not record role change event if role has not changed" do + user = create(:user, role: Roles::Normal.role_name) + + EventLog.expects(:record_role_change).never + + put :update, params: { user_id: user, user: { role: Roles::Normal.role_name } } + end + + should "push changes out to apps" do + user = create(:user_in_organisation) + PermissionUpdater.expects(:perform_on).with(user).once + + put :update, params: { user_id: user, user: { role: Roles::OrganisationAdmin.role_name } } + end + + should "redirect to user page and display success notice" do + user = create(:user_in_organisation, email: "user@gov.uk") + + put :update, params: { user_id: user, user: { role: Roles::OrganisationAdmin.role_name } } + + assert_redirected_to edit_user_path(user) + assert_equal "Updated user user@gov.uk successfully", flash[:notice] + end + + should "update user role if UserPolicy#update? and UserPolicy#assign_role? return true" do + user = create(:user_in_organisation, role: Roles::Normal.role_name) + + user_policy = stub_everything("user-policy", update?: true, assign_role?: true) + UserPolicy.stubs(:new).returns(user_policy) + + put :update, params: { user_id: user, user: { role: Roles::OrganisationAdmin.role_name } } + + assert_equal Roles::OrganisationAdmin.role_name, user.reload.role + end + + should "not update user role if UserPolicy#update? returns false" do + user = create(:user_in_organisation, role: Roles::Normal.role_name) + + user_policy = stub_everything("user-policy", update?: false, assign_role?: true) + UserPolicy.stubs(:new).returns(user_policy) + + put :update, params: { user_id: user, user: { role: Roles::OrganisationAdmin.role_name } } + + assert_equal Roles::Normal.role_name, user.reload.role + assert_not_authorised + end + + should "not update user role if UserPolicy#assign_role? returns false" do + user = create(:user_in_organisation, role: Roles::Normal.role_name) + + user_policy = stub_everything("user-policy", update?: true, assign_role?: false) + UserPolicy.stubs(:new).returns(user_policy) + + put :update, params: { user_id: user, user: { role: Roles::OrganisationAdmin.role_name } } + + assert_equal Roles::Normal.role_name, user.reload.role + assert_not_authorised + end + + should "not update user role if user is exempt from 2SV" do + user = create(:two_step_exempted_user, :in_organisation, role: Roles::Normal.role_name) + + put :update, params: { user_id: user, user: { role: Roles::OrganisationAdmin.role_name } } + + assert_equal Roles::Normal.role_name, user.reload.role + + assert_select ".govuk-error-summary" do + assert_select "li", text: /#{Roles::OrganisationAdmin.role_name} users cannot be exempted from 2SV/ + end + end + + should "redisplay form if role is not valid" do + user = create(:user_in_organisation, role: Roles::Normal.role_name) + + put :update, params: { user_id: user, user: { role: "invalid-role" } } + + assert_template :edit + assert_select "form[action='#{user_role_path(user)}']" do + assert_select "select[name='user[role]'] option", value: Roles::Normal.role_name + end + end + + should "display errors if role is not valid" do + user = create(:user_in_organisation) + + put :update, params: { user_id: user, user: { role: "invalid-role" } } + + assert_select ".govuk-error-summary" do + assert_select "a", href: "#user_role", text: "Role is not included in the list" + end + assert_select ".govuk-form-group" do + assert_select ".govuk-error-message", text: "Error: Role is not included in the list" + assert_select "select[name='user[role]'].govuk-select--error" + end + end + end + + context "signed in as Normal user" do + setup do + sign_in(create(:user)) + end + + should "not be authorized" do + user = create(:user_in_organisation, role: Roles::Normal.role_name) + + put :update, params: { user_id: user, user: { role: Roles::OrganisationAdmin.role_name } } + + assert_not_authorised + end + end + + context "not signed in" do + should "not be allowed access" do + user = create(:user_in_organisation) + + get :update, params: { user_id: user } + + assert_not_authenticated + end + end + end +end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index f047db879..0d4c2515f 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -601,12 +601,6 @@ class UsersControllerTest < ActionController::TestCase assert_nil user.reload.organisation end - should "not let you set the role" do - not_an_admin = create(:user) - put :update, params: { id: not_an_admin.id, user: { role: Roles::Admin.role_name } } - assert_equal "normal", not_an_admin.reload.role - end - should "push changes out to apps" do another_user = create(:user, name: "Old Name") PermissionUpdater.expects(:perform_on).with(another_user).once @@ -647,28 +641,6 @@ class UsersControllerTest < ActionController::TestCase end end - context "signed in as a Superadmin user" do - setup do - @superadmin = create(:superadmin_user) - sign_in @superadmin - end - - should "update the user's role" do - not_an_admin = create(:user) - put :update, params: { id: not_an_admin.id, user: { role: Roles::Admin.role_name } } - assert_equal Roles::Admin.role_name, not_an_admin.reload.role - end - - context "changing a role" do - should "log an event" do - another_user = create(:admin_user) - put :update, params: { id: another_user.id, user: { role: "normal" } } - - assert_equal 1, EventLog.where(event_id: EventLog::ROLE_CHANGED.id, uid: another_user.uid, initiator_id: @superadmin.id).count - end - end - end - context "signed in as Organisation Admin user" do setup do @organisation_admin = create(:organisation_admin_user) diff --git a/test/factories/users.rb b/test/factories/users.rb index 0cd29c0a1..69cf2f997 100644 --- a/test/factories/users.rb +++ b/test/factories/users.rb @@ -106,10 +106,12 @@ locked_at { Time.zone.now } end - factory :user_in_organisation, parent: :user do + trait :in_organisation do association :organisation, factory: :organisation end + factory :user_in_organisation, parent: :user, traits: [:in_organisation] + factory :api_user do transient do with_permissions { {} } diff --git a/test/integration/change_user_role_test.rb b/test/integration/change_user_role_test.rb index faf1ba1e9..dd6bbb3e5 100644 --- a/test/integration/change_user_role_test.rb +++ b/test/integration/change_user_role_test.rb @@ -16,9 +16,10 @@ def sign_in_as_and_edit_user(sign_in_as, user_to_edit) should "be able to change the role of a user who is not exempt from 2SV" do user = create(:user) sign_in_as_and_edit_user(@super_admin, user) + click_link "Change role" select "Admin", from: "Role" - click_button "Update User" + click_button "Change role" assert user.reload.admin? end @@ -26,10 +27,11 @@ def sign_in_as_and_edit_user(sign_in_as, user_to_edit) should "not be able to change the role of a user who is exempt from 2SV" do user = create(:two_step_exempted_user) sign_in_as_and_edit_user(@super_admin, user) + click_link "Change role" assert page.has_no_select?("Role") - assert page.has_text? "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." + assert page.has_text? "This user's role is set to #{user.role.humanize}. They are currently exempted from 2-step verification, meaning that their role cannot be changed as admins are required to have 2-step verification." end end