diff --git a/app/controllers/users/names_controller.rb b/app/controllers/users/names_controller.rb new file mode 100644 index 000000000..8c22fb12a --- /dev/null +++ b/app/controllers/users/names_controller.rb @@ -0,0 +1,32 @@ +class Users::NamesController < ApplicationController + layout "admin_layout" + + before_action :authenticate_user! + before_action :load_user + before_action :authorize_user + + def edit; end + + def update + if @user.update(user_params) + EventLog.record_event(@user, EventLog::ACCOUNT_UPDATED, initiator: current_user, ip_address: user_ip_address) + redirect_to 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 + end + + def user_params + params.require(:user).permit(*current_user.permitted_params.intersection([:name])) + end +end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index cc94aaf7d..4801a00d1 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -127,7 +127,7 @@ def user_params end def permitted_user_params - @permitted_user_params ||= params.require(:user).permit(:user, :name, :email, :organisation_id, :require_2sv, :role, :skip_update_user_permissions, supported_permission_ids: []).to_h + @permitted_user_params ||= params.require(:user).permit(:user, :email, :organisation_id, :require_2sv, :role, :skip_update_user_permissions, supported_permission_ids: []).to_h end def filter_params diff --git a/app/models/user.rb b/app/models/user.rb index a893ca8a5..20d131f45 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -320,6 +320,10 @@ def manageable_organisations role_class.manageable_organisations_for(self).order(:name) end + def permitted_params + role_class.permitted_user_params + end + # Make devise send all emails using ActiveJob def send_devise_notification(notification, *args) devise_mailer.send(notification, self, *args).deliver_later diff --git a/app/views/layouts/admin_layout.html.erb b/app/views/layouts/admin_layout.html.erb index 88385e9ed..39b664876 100644 --- a/app/views/layouts/admin_layout.html.erb +++ b/app/views/layouts/admin_layout.html.erb @@ -37,6 +37,9 @@
+ <% if yield(:title_caption).present? %> + <%= yield(:title_caption) %> + <% end %>

<% if yield(:page_heading).present? %> <%= yield(:page_heading) %> diff --git a/app/views/users/_form_fields.html.erb b/app/views/users/_form_fields.html.erb index b67d36731..4784d2139 100644 --- a/app/views/users/_form_fields.html.erb +++ b/app/views/users/_form_fields.html.erb @@ -1,6 +1,5 @@ -

- <%= f.label :name %> - <%= f.text_field :name, autofocus: true, autocomplete: "off", class: 'form-control input-md-6 ' %> +

+ Name: <%= @user.name %> <%= link_to 'Change'.html_safe, edit_user_name_path(@user) %>

diff --git a/app/views/users/names/edit.html.erb b/app/views/users/names/edit.html.erb new file mode 100644 index 000000000..64ff41a06 --- /dev/null +++ b/app/views/users/names/edit.html.erb @@ -0,0 +1,60 @@ +<% content_for :title_caption, "Manage other users" %> +<% content_for :title, "Change name for #{@user.name_was}" %> + +<% 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_was, + url: edit_user_path(@user), + }, + { + title: "Change name", + } + ] + }) +%> + +<% 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_name_path do |f| %> + <%= render "govuk_publishing_components/components/input", { + label: { + text: "Name" + }, + name: "user[name]", + type: "name", + id: "user_name", + value: @user.name, + autocomplete: "off", + error_items: @user.errors.full_messages_for(:name).map { |message| { text: message } } + } %> + <%= render "govuk_publishing_components/components/button", { + text: "Change name" + } %> + <% end %> +
+
diff --git a/config/routes.rb b/config/routes.rb index 03ba38e81..77766e91e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -45,6 +45,7 @@ patch :reset_two_step_verification get :require_2sv end + resource :name, only: %i[edit update], controller: "users/names" end get "user", to: "oauth_users#show" diff --git a/test/controllers/users/names_controller_test.rb b/test/controllers/users/names_controller_test.rb new file mode 100644 index 000000000..0c65c89da --- /dev/null +++ b/test/controllers/users/names_controller_test.rb @@ -0,0 +1,190 @@ +require "test_helper" + +class Users::NamesControllerTest < ActionController::TestCase + context "GET edit" do + context "signed in as Admin user" do + setup do + sign_in(create(:admin_user)) + end + + should "display form with name field" do + user = create(:user, name: "user-name") + + get :edit, params: { user_id: user } + + assert_template :edit + assert_select "form[action='#{user_name_path(user)}']" do + assert_select "input[name='user[name]']", value: "user-name" + end + end + + should "authorize access if UserPolicy#edit? returns true" do + user = create(:user) + + user_policy = stub_everything("user-policy", edit?: true) + UserPolicy.stubs(:new).returns(user_policy) + + get :edit, params: { user_id: user } + + assert_template :edit + end + + should "not authorize access if UserPolicy#edit? returns false" do + user = create(:user) + + user_policy = stub_everything("user-policy", edit?: false) + UserPolicy.stubs(:new).returns(user_policy) + + get :edit, params: { user_id: user } + + assert_not_authorised + 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_redirected_to new_user_session_path + end + end + end + + context "PUT update" do + context "signed in as Admin user" do + setup do + @admin = create(:admin_user) + sign_in(@admin) + end + + should "update user name" do + user = create(:user, name: "user-name") + + put :update, params: { user_id: user, user: { name: "new-user-name" } } + + assert_equal "new-user-name", user.reload.name + end + + should "record account updated event" do + user = create(:user) + + @controller.stubs(:user_ip_address).returns("1.1.1.1") + + EventLog.expects(:record_event).with( + user, + EventLog::ACCOUNT_UPDATED, + initiator: @admin, + ip_address: "1.1.1.1", + ) + + put :update, params: { user_id: user, user: { name: "new-user-name" } } + end + + should "redirect to user page and display success notice" do + user = create(:user, email: "user@gov.uk") + + put :update, params: { user_id: user, user: { name: "new-user-name" } } + + assert_redirected_to user_path(user), notice: "Updated name of user@gov.uk successfully" + end + + should "update user name if UserPolicy#update? returns true" do + user = create(:user, name: "user-name") + + user_policy = stub_everything("user-policy", update?: true) + UserPolicy.stubs(:new).returns(user_policy) + + put :update, params: { user_id: user, user: { name: "new-user-name" } } + + assert_equal "new-user-name", user.reload.name + end + + should "not update user name if UserPolicy#update? returns false" do + user = create(:user, name: "user-name") + + user_policy = stub_everything("user-policy", update?: false) + UserPolicy.stubs(:new).returns(user_policy) + + put :update, params: { user_id: user, user: { name: "new-user-name" } } + + assert_equal "user-name", user.reload.name + assert_not_authorised + end + + should "redisplay form if name is not valid" do + user = create(:user, name: "user-name") + + put :update, params: { user_id: user, user: { name: "" } } + + assert_template :edit + assert_select "form[action='#{user_name_path(user)}']" do + assert_select "input[name='user[name]']", value: "" + end + end + + should "use original name in page title, heading & breadcrumbs if new name was not valid" do + user = create(:user, name: "user-name") + + put :update, params: { user_id: user, user: { name: "" } } + + assert_select "head title", text: /^Change name for user-name/ + assert_select "h1", text: "Change name for user-name" + assert_select ".govuk-breadcrumbs li", text: "user-name" + end + + should "display errors if name is not valid" do + user = create(:user) + + put :update, params: { user_id: user, user: { name: "" } } + + assert_select ".govuk-error-summary" do + assert_select "a", href: "#user_name", text: "Name can't be blank" + end + assert_select ".govuk-form-group" do + assert_select ".govuk-error-message", text: "Error: Name can't be blank" + assert_select "input[name='user[name]'].govuk-input--error" + end + end + end + + context "signed in as Normal user" do + setup do + sign_in(create(:user)) + end + + should "update user name" do + user = create(:user, name: "user-name") + + put :update, params: { user_id: user, user: { name: "new-user-name" } } + + 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_redirected_to new_user_session_path + end + end + end +end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index af028e6f8..fba7508dc 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -262,10 +262,19 @@ class UsersControllerTest < ActionController::TestCase end end - should "show the form" do + should "display the user's name and a link to change the name" do + not_an_admin = create(:user, name: "user-name") + get :edit, params: { id: not_an_admin.id } + assert_select "*", text: /Name: user-name/ + assert_select "a", href: edit_user_name_path(not_an_admin), text: "Change name" + end + + should "show the form with an email field" do not_an_admin = create(:user) get :edit, params: { id: not_an_admin.id } - assert_select "input[name='user[email]'][value='#{not_an_admin.email}']" + assert_select "form[action='#{user_path(not_an_admin)}']" do + assert_select "input[name='user[email]'][value='#{not_an_admin.email}']" + end end should "show the pending email if applicable" do @@ -556,24 +565,22 @@ class UsersControllerTest < ActionController::TestCase sign_in @user end - context "GET edit" do - context "when current user tries to edit their own user" do - should "redirect to the account page" do - get :edit, params: { id: @user } + context "when current user tries to edit their own user" do + should "redirect to the account page" do + get :edit, params: { id: @user } - assert_redirected_to account_path - end + assert_redirected_to account_path end + end - context "when current user tries to edit another user" do - should "redirect to the dashboard and explain user does not have permission" do - another_user = create(:user) + context "when current user tries to edit another user" do + should "redirect to the dashboard and explain user does not have permission" do + another_user = create(:user) - get :edit, params: { id: another_user } + get :edit, params: { id: another_user } - assert_redirected_to root_path - assert_equal "You do not have permission to perform this action.", flash[:alert] - end + assert_redirected_to root_path + assert_equal "You do not have permission to perform this action.", flash[:alert] end end end @@ -586,15 +593,6 @@ class UsersControllerTest < ActionController::TestCase sign_in @user end - should "update the user" do - another_user = create(:user, name: "Old Name") - put :update, params: { id: another_user.id, user: { name: "New Name" } } - - assert_equal "New Name", another_user.reload.name - assert_redirected_to users_path - assert_equal "Updated user #{another_user.email} successfully", flash[:notice] - end - should "not be able to update superadmins" do superadmin = create(:superadmin_user) @@ -612,12 +610,6 @@ class UsersControllerTest < ActionController::TestCase assert_nil user.reload.organisation end - should "redisplay the form if save fails" do - another_user = create(:user) - put :update, params: { id: another_user.id, user: { name: "" } } - assert_select "form#edit_user_#{another_user.id}" - 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 } } @@ -752,7 +744,7 @@ class UsersControllerTest < ActionController::TestCase organisation = @organisation_admin.organisation organisation_admin_for_same_organisation = create(:organisation_admin_user, organisation:) - put :update, params: { id: organisation_admin_for_same_organisation.id, user: { name: "" } } + put :update, params: { id: organisation_admin_for_same_organisation.id, user: { email: "" } } assert_select "form#edit_user_#{organisation_admin_for_same_organisation.id}" end @@ -768,7 +760,7 @@ class UsersControllerTest < ActionController::TestCase organisation = @super_organisation_admin.organisation super_organisation_admin_for_same_organisation = create(:super_organisation_admin_user, organisation:) - put :update, params: { id: super_organisation_admin_for_same_organisation.id, user: { name: "" } } + put :update, params: { id: super_organisation_admin_for_same_organisation.id, user: { email: "" } } assert_select "form#edit_user_#{super_organisation_admin_for_same_organisation.id}" end diff --git a/test/integration/change_user_name_test.rb b/test/integration/change_user_name_test.rb new file mode 100644 index 000000000..096ea330d --- /dev/null +++ b/test/integration/change_user_name_test.rb @@ -0,0 +1,20 @@ +require "test_helper" + +class ChangeUserNameTest < ActionDispatch::IntegrationTest + context "when signed in as any kind of admin" do + setup do + @superadmin = create(:superadmin_user) + @user = create(:user, name: "user-name") + end + + should "be able to change a normal user's name" do + visit root_path + signin_with(@superadmin) + visit edit_user_path(@user) + click_link "Change name" + fill_in "Name", with: "new-user-name" + click_button "Change name" + assert_equal "new-user-name", @user.reload.name + end + end +end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index b29a22d5b..f7b95e0a2 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -1238,6 +1238,15 @@ def setup end end + context "#permitted_params" do + should "return the params editable by the user's role" do + user = build(:user, role: Roles::Admin.role_name) + Roles::Admin.stubs(:permitted_user_params).returns(%i[abc xyz]) + + assert_equal %i[abc xyz], user.permitted_params + end + end + def authenticate_access(user, app) Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: app.id) end