diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index bf4130bf9..0c44b679b 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -34,6 +34,8 @@ class UsersController < ApplicationController @resource.errors.add :email, I18n.t("validations.email.blank") elsif !email_valid?(user_params["email"]) @resource.errors.add :email, I18n.t("validations.email.invalid") + elsif user_params[:role] && !current_user.assignable_roles.key?(user_params[:role].to_sym) + @resource.errors.add :role, I18n.t("validations.role.invalid") end if @resource.errors.present? render :new, status: :unprocessable_entity @@ -78,12 +80,12 @@ private def user_params if @user == current_user - if current_user.data_coordinator? + if current_user.data_coordinator? || current_user.support? params.require(:user).permit(:email, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact) else params.require(:user).permit(:email, :name, :password, :password_confirmation) end - elsif current_user.data_coordinator? + elsif current_user.data_coordinator? || current_user.support? params.require(:user).permit(:email, :name, :role, :is_dpo, :is_key_contact) end end @@ -94,11 +96,12 @@ private def authenticate_scope! if action_name == "create" - head :unauthorized and return unless current_user.data_coordinator? + head :unauthorized and return unless current_user.data_coordinator? || current_user.support? else - render_not_found and return unless current_user.organisation == @user.organisation + render_not_found and return unless (current_user.organisation == @user.organisation) || current_user.support? render_not_found and return if action_name == "edit_password" && current_user != @user - render_not_found and return unless action_name == "show" || current_user.data_coordinator? || current_user == @user + render_not_found and return unless action_name == "show" || + current_user.data_coordinator? || current_user.support? || current_user == @user end end end diff --git a/app/helpers/user_helper.rb b/app/helpers/user_helper.rb index a9f6c920d..329920346 100644 --- a/app/helpers/user_helper.rb +++ b/app/helpers/user_helper.rb @@ -6,4 +6,28 @@ module UserHelper def pronoun(user, current_user) current_user == user ? "you" : "they" end + + def can_edit_names?(user, current_user) + current_user == user || current_user.data_coordinator? || current_user.support? + end + + def can_edit_emails?(user, current_user) + current_user == user || current_user.data_coordinator? || current_user.support? + end + + def can_edit_password?(user, current_user) + current_user == user + end + + def can_edit_roles?(_user, current_user) + current_user.data_coordinator? || current_user.support? + end + + def can_edit_dpo?(_user, current_user) + current_user.data_coordinator? || current_user.support? + end + + def can_edit_key_contact?(_user, current_user) + current_user.data_coordinator? || current_user.support? + end end diff --git a/app/views/organisations/users.html.erb b/app/views/organisations/users.html.erb index 33c7bc5d0..304528016 100644 --- a/app/views/organisations/users.html.erb +++ b/app/views/organisations/users.html.erb @@ -4,7 +4,7 @@ <%= "Users" %> <% end %> -<% if current_user.data_coordinator? %> +<% if current_user.data_coordinator? || current_user.support? %> <%= govuk_button_link_to "Invite user", new_user_path, html: { method: :get } %> <% end %> <%= govuk_table do |table| %> diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 6222fc471..846472ca1 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -14,7 +14,7 @@ <%= summary_list.row do |row| row.key { "Name" } row.value { @user.name } - if current_user == @user || current_user.data_coordinator? + if can_edit_names?(@user, current_user) row.action(visually_hidden_text: "name", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-name" }) else row.action() @@ -24,7 +24,7 @@ <%= summary_list.row() do |row| row.key { "Email address" } row.value { @user.email } - if current_user == @user || current_user.data_coordinator? + if can_edit_emails?(@user, current_user) row.action(visually_hidden_text: "email address", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-email-address" }) else row.action() @@ -34,7 +34,7 @@ <%= summary_list.row do |row| row.key { "Password" } row.value { "••••••••" } - if current_user == @user + if can_edit_password?(@user, current_user) row.action(visually_hidden_text: "password", href: edit_password_account_path, html_attributes: { "data-qa": "change-password" }) else row.action() @@ -50,7 +50,7 @@ <%= summary_list.row do |row| row.key { "Role" } row.value { @user.role.humanize } - if current_user.data_coordinator? + if can_edit_roles?(@user, current_user) row.action(visually_hidden_text: "role", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-role" }) else row.action() @@ -60,7 +60,7 @@ <%= summary_list.row do |row| row.key { "Data protection officer" } row.value { @user.is_data_protection_officer? ? "Yes" : "No" } - if current_user.data_coordinator? + if can_edit_dpo?(@user, current_user) row.action(visually_hidden_text: "are #{pronoun(@user, current_user)} a data protection officer?", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-are-#{pronoun(@user, current_user)}-a-data-protection-officer" }) else row.action() @@ -70,7 +70,7 @@ <%= summary_list.row do |row| row.key { "Key contact" } row.value { @user.is_key_contact? ? "Yes" : "No" } - if current_user.data_coordinator? + if can_edit_key_contact?(@user, current_user) row.action(visually_hidden_text: "are #{pronoun(@user, current_user)} a key contact?", href: aliased_user_edit(@user, current_user), html_attributes: { "data-qa": "change-are-#{pronoun(@user, current_user)}-a-key-contact" }) else row.action() diff --git a/config/locales/en.yml b/config/locales/en.yml index 2837d1ea7..d73de31f6 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -47,6 +47,8 @@ en: taken: "Email already exists" invalid: "Enter an email address in the correct format, like name@example.com" blank: "Enter an email address" + role: + invalid: "Role must be data accessor, data provider or data coordinator" setup: intermediate_rent_product_name: diff --git a/spec/helpers/user_helper_spec.rb b/spec/helpers/user_helper_spec.rb index 6386ddd4f..5ec525c69 100644 --- a/spec/helpers/user_helper_spec.rb +++ b/spec/helpers/user_helper_spec.rb @@ -35,4 +35,105 @@ RSpec.describe UserHelper do end end end + + describe "change button permissions" do + context "when the user is a data provider viewing their own details" do + let(:current_user) { FactoryBot.create(:user, :data_provider) } + let(:user) { current_user } + + it "allows changing name" do + expect(can_edit_names?(user, current_user)).to be true + end + + it "allows changing email" do + expect(can_edit_emails?(user, current_user)).to be true + end + + it "allows changing password" do + expect(can_edit_password?(user, current_user)).to be true + end + + it "does not allow changing roles" do + expect(can_edit_roles?(user, current_user)).to be false + end + + it "does not allow changing dpo" do + expect(can_edit_dpo?(user, current_user)).to be false + end + + it "does not allow changing key contact" do + expect(can_edit_key_contact?(user, current_user)).to be false + end + end + + context "when the user is a data coordinator viewing another user's details" do + it "allows changing name" do + expect(can_edit_names?(user, current_user)).to be true + end + + it "allows changing email" do + expect(can_edit_emails?(user, current_user)).to be true + end + + it "allows changing password" do + expect(can_edit_password?(user, current_user)).to be false + end + + it "does not allow changing roles" do + expect(can_edit_roles?(user, current_user)).to be true + end + + it "does not allow changing dpo" do + expect(can_edit_dpo?(user, current_user)).to be true + end + + it "does not allow changing key contact" do + expect(can_edit_key_contact?(user, current_user)).to be true + end + + context "when the user is a data coordinator viewing their own details" do + let(:user) { current_user } + + it "allows changing password" do + expect(can_edit_password?(user, current_user)).to be true + end + end + end + + context "when the user is a support user viewing another user's details" do + let(:current_user) { FactoryBot.create(:user, :support) } + + it "allows changing name" do + expect(can_edit_names?(user, current_user)).to be true + end + + it "allows changing email" do + expect(can_edit_emails?(user, current_user)).to be true + end + + it "allows changing password" do + expect(can_edit_password?(user, current_user)).to be false + end + + it "does not allow changing roles" do + expect(can_edit_roles?(user, current_user)).to be true + end + + it "does not allow changing dpo" do + expect(can_edit_dpo?(user, current_user)).to be true + end + + it "does not allow changing key contact" do + expect(can_edit_key_contact?(user, current_user)).to be true + end + + context "when the user is a support user viewing their own details" do + let(:user) { current_user } + + it "allows changing password" do + expect(can_edit_password?(user, current_user)).to be true + end + end + end + end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index bef6baa13..3da36083b 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -132,7 +132,7 @@ RSpec.describe UsersController, type: :request do end end - context "when the current user does not matches the user ID" do + context "when the current user does not match the user ID" do before do sign_in user get "/users/#{other_user.id}", headers: headers, params: {} @@ -188,7 +188,7 @@ RSpec.describe UsersController, type: :request do end end - context "when the current user does not matches the user ID" do + context "when the current user does not match the user ID" do before do sign_in user get "/users/#{other_user.id}/edit", headers: headers, params: {} @@ -216,7 +216,7 @@ RSpec.describe UsersController, type: :request do end end - context "when the current user does not matches the user ID" do + context "when the current user does not match the user ID" do before do sign_in user get "/users/#{other_user.id}/edit", headers: headers, params: {} @@ -272,7 +272,7 @@ RSpec.describe UsersController, type: :request do end end - context "when the current user does not matches the user ID" do + context "when the current user does not match the user ID" do let(:params) { { id: other_user.id, user: { name: new_name } } } before do @@ -356,7 +356,7 @@ RSpec.describe UsersController, type: :request do end end - context "when the current user does not matches the user ID" do + context "when the current user does not match the user ID" do before do sign_in user get "/users/#{other_user.id}", headers: headers, params: {} @@ -419,7 +419,7 @@ RSpec.describe UsersController, type: :request do end end - context "when the current user does not matches the user ID" do + context "when the current user does not match the user ID" do before do sign_in user get "/users/#{other_user.id}/edit", headers: headers, params: {} @@ -469,7 +469,7 @@ RSpec.describe UsersController, type: :request do end end - context "when the current user does not matches the user ID" do + context "when the current user does not match the user ID" do before do sign_in user end @@ -531,7 +531,7 @@ RSpec.describe UsersController, type: :request do end end - context "when the current user does not matches the user ID" do + context "when the current user does not match the user ID" do before do sign_in user end @@ -566,7 +566,7 @@ RSpec.describe UsersController, type: :request do expect(page).to have_content(other_user.reload.email.to_s) end - context "when we try to update the user password" do + context "when the data coordinator tries to update the user’s password" do let(:params) do { id: user.id, user: { password: new_name, password_confirmation: new_name, name: "new name" } @@ -585,7 +585,7 @@ RSpec.describe UsersController, type: :request do end end - context "when the current user does not matches the user ID" do + context "when the current user does not match the user ID" do context "when the user is not part of the same organisation as the current user" do let(:other_user) { FactoryBot.create(:user) } let(:params) { { id: other_user.id, user: { name: new_name } } } @@ -642,10 +642,425 @@ RSpec.describe UsersController, type: :request do expect(response).to redirect_to("/organisations/#{user.organisation.id}/users") end + context "when the email is already taken" do + before do + FactoryBot.create(:user, email: "new_user@example.com") + end + + it "shows an error" do + request + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content(I18n.t("validations.email.taken")) + end + end + + context "when trying to assign support role" do + let(:params) do + { + "user": { + name: "new user", + email: "new_user@example.com", + role: "support", + }, + } + end + + it "shows an error" do + request + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content(I18n.t("validations.role.invalid")) + end + end + end + + describe "#new" do + before do + sign_in user + end + it "cannot assign support role to the new user" do - request + get "/users/new" expect(page).not_to have_field("user-role-support-field") end + end + end + + context "when user is signed in as a support user" do + let(:user) { FactoryBot.create(:user, :support) } + let(:other_user) { FactoryBot.create(:user, organisation: user.organisation) } + + before do + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + end + + describe "#show" do + context "when the current user matches the user ID" do + before do + sign_in user + get "/users/#{user.id}", headers: headers, params: {} + end + + it "show the user details" do + expect(page).to have_content("Your account") + end + + it "allows changing name, email, password, role, dpo and key contact" do + expect(page).to have_link("Change", text: "name") + expect(page).to have_link("Change", text: "email address") + expect(page).to have_link("Change", text: "password") + expect(page).to have_link("Change", text: "role") + expect(page).to have_link("Change", text: "are you a data protection officer?") + expect(page).to have_link("Change", text: "are you a key contact?") + end + end + + context "when the current user does not match the user ID" do + before do + sign_in user + get "/users/#{other_user.id}", headers: headers, params: {} + end + + context "when the user is part of the same organisation as the current user" do + it "returns 200" do + expect(response).to have_http_status(:ok) + end + + it "shows the user details page" do + expect(page).to have_content("#{other_user.name}’s account") + end + + it "allows changing name, email, role, dpo and key contact" do + expect(page).to have_link("Change", text: "name") + expect(page).to have_link("Change", text: "email address") + expect(page).not_to have_link("Change", text: "password") + expect(page).to have_link("Change", text: "role") + expect(page).to have_link("Change", text: "are they a data protection officer?") + expect(page).to have_link("Change", text: "are they a key contact?") + end + end + + context "when the user is not part of the same organisation as the current user" do + let(:other_user) { FactoryBot.create(:user) } + + it "returns 200" do + expect(response).to have_http_status(:ok) + end + + it "shows the user details page" do + expect(page).to have_content("#{other_user.name}’s account") + end + + it "allows changing name, email, role, dpo and key contact" do + expect(page).to have_link("Change", text: "name") + expect(page).to have_link("Change", text: "email address") + expect(page).not_to have_link("Change", text: "password") + expect(page).to have_link("Change", text: "role") + expect(page).to have_link("Change", text: "are they a data protection officer?") + expect(page).to have_link("Change", text: "are they a key contact?") + end + end + end + end + + describe "#edit" do + context "when the current user matches the user ID" do + before do + sign_in user + get "/users/#{user.id}/edit", headers: headers, params: {} + end + + it "show the edit personal details page" do + expect(page).to have_content("Change your personal details") + end + + it "has fields for name, email, role, dpo and key contact" do + expect(page).to have_field("user[name]") + expect(page).to have_field("user[email]") + expect(page).to have_field("user[role]") + expect(page).to have_field("user[is_dpo]") + expect(page).to have_field("user[is_key_contact]") + end + + it "allows setting the role to `support`" do + expect(page).to have_field("user-role-support-field") + end + end + + context "when the current user does not match the user ID" do + before do + sign_in user + get "/users/#{other_user.id}/edit", headers: headers, params: {} + end + + context "when the user is part of the same organisation as the current user" do + it "returns 200" do + expect(response).to have_http_status(:ok) + end + + it "shows the user details page" do + expect(page).to have_content("Change #{other_user.name}’s personal details") + end + + it "has fields for name, email, role, dpo and key contact" do + expect(page).to have_field("user[name]") + expect(page).to have_field("user[email]") + expect(page).to have_field("user[role]") + expect(page).to have_field("user[is_dpo]") + expect(page).to have_field("user[is_key_contact]") + end + end + + context "when the user is not part of the same organisation as the current user" do + let(:other_user) { FactoryBot.create(:user) } + + it "returns 200" do + expect(response).to have_http_status(:ok) + end + + it "shows the user details page" do + expect(page).to have_content("Change #{other_user.name}’s personal details") + end + + it "has fields for name, email, role, dpo and key contact" do + expect(page).to have_field("user[name]") + expect(page).to have_field("user[email]") + expect(page).to have_field("user[role]") + expect(page).to have_field("user[is_dpo]") + expect(page).to have_field("user[is_key_contact]") + end + end + end + end + + describe "#edit_password" do + context "when the current user matches the user ID" do + before do + sign_in user + get "/account/edit/password", headers: headers, params: {} + end + + it "shows the edit password page" do + expect(page).to have_content("Change your password") + end + + it "shows the password requirements hint" do + expect(page).to have_css("#user-password-hint") + end + end + + context "when the current user does not match the user ID" do + before do + sign_in user + end + + it "there is no route" do + expect { + get "/users/#{other_user.id}/password/edit", headers: headers, params: {} + }.to raise_error(ActionController::RoutingError) + end + end + end + + describe "#update" do + context "when the current user matches the user ID" do + before do + sign_in user + patch "/users/#{user.id}", headers: headers, params: params + end + + it "updates the user" do + user.reload + expect(user.name).to eq(new_name) + end + + it "tracks who updated the record" do + user.reload + whodunnit_actor = user.versions.last.actor + expect(whodunnit_actor).to be_a(User) + expect(whodunnit_actor.id).to eq(user.id) + end + + context "when user changes email, dpo and key contact" do + let(:params) { { id: user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } } + + it "allows changing email and dpo" do + user.reload + expect(user.email).to eq(new_email) + expect(user.is_data_protection_officer?).to be true + expect(user.is_key_contact?).to be true + end + end + + context "when we update the user password" do + let(:params) do + { + id: user.id, user: { password: new_name, password_confirmation: "something_else" } + } + end + + before do + sign_in user + patch "/users/#{user.id}", headers: headers, params: params + end + + it "shows an error if passwords don't match" do + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_selector("#error-summary-title") + end + end + end + + context "when the current user does not match the user ID" do + before do + sign_in user + end + + context "when the user is part of the same organisation as the current user" do + it "updates the user" do + expect { patch "/users/#{other_user.id}", headers: headers, params: params } + .to change { other_user.reload.name }.from(other_user.name).to(new_name) + end + + it "tracks who updated the record" do + expect { patch "/users/#{other_user.id}", headers: headers, params: params } + .to change { other_user.reload.versions.last.actor&.id }.from(nil).to(user.id) + end + + context "when user changes email, dpo, key_contact" do + let(:params) { { id: other_user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } } + + it "allows changing email, dpo, key_contact" do + patch "/users/#{other_user.id}", headers: headers, params: params + other_user.reload + expect(other_user.email).to eq(new_email) + expect(other_user.is_data_protection_officer?).to be true + expect(other_user.is_key_contact?).to be true + end + end + + it "does not bypass sign in for the support user" do + patch "/users/#{other_user.id}", headers: headers, params: params + follow_redirect! + expect(page).to have_content("#{other_user.reload.name}’s account") + expect(page).to have_content(other_user.reload.email.to_s) + end + + context "when the support user tries to update the user’s password" do + let(:params) do + { + id: user.id, user: { password: new_name, password_confirmation: new_name, name: "new name" } + } + end + + it "does not update the password" do + expect { patch "/users/#{other_user.id}", headers: headers, params: params } + .not_to change(other_user, :encrypted_password) + end + + it "does update other values" do + expect { patch "/users/#{other_user.id}", headers: headers, params: params } + .to change { other_user.reload.name }.from("Danny Rojas").to("new name") + end + end + end + + context "when the current user does not match the user ID" do + context "when the user is not part of the same organisation as the current user" do + let(:other_user) { FactoryBot.create(:user) } + let(:params) { { id: other_user.id, user: { name: new_name } } } + + before do + sign_in user + end + + it "updates the user" do + expect { patch "/users/#{other_user.id}", headers: headers, params: params } + .to change { other_user.reload.name }.from(other_user.name).to(new_name) + end + + it "tracks who updated the record" do + expect { patch "/users/#{other_user.id}", headers: headers, params: params } + .to change { other_user.reload.versions.last.actor&.id }.from(nil).to(user.id) + end + + context "when user changes email, dpo, key_contact" do + let(:params) { { id: other_user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } } + + it "allows changing email, dpo, key_contact" do + patch "/users/#{other_user.id}", headers: headers, params: params + other_user.reload + expect(other_user.email).to eq(new_email) + expect(other_user.is_data_protection_officer?).to be true + expect(other_user.is_key_contact?).to be true + end + end + + it "does not bypass sign in for the support user" do + patch "/users/#{other_user.id}", headers: headers, params: params + follow_redirect! + expect(page).to have_content("#{other_user.reload.name}’s account") + expect(page).to have_content(other_user.reload.email.to_s) + end + + context "when the support user tries to update the user’s password" do + let(:params) do + { + id: user.id, user: { password: new_name, password_confirmation: new_name, name: "new name" } + } + end + + it "does not update the password" do + expect { patch "/users/#{other_user.id}", headers: headers, params: params } + .not_to change(other_user, :encrypted_password) + end + + it "does update other values" do + expect { patch "/users/#{other_user.id}", headers: headers, params: params } + .to change { other_user.reload.name }.from("Danny Rojas").to("new name") + end + end + end + end + end + + context "when the update fails to persist" do + before do + sign_in user + allow(User).to receive(:find_by).and_return(user) + allow(user).to receive(:update).and_return(false) + patch "/users/#{user.id}", headers: headers, params: params + end + + it "show an error" do + expect(response).to have_http_status(:unprocessable_entity) + end + end + end + + describe "#create" do + let(:params) do + { + "user": { + name: "new user", + email: "new_user@example.com", + role: "data_coordinator", + }, + } + end + let(:request) { post "/users/", headers: headers, params: params } + + before do + sign_in user + end + + it "invites a new user" do + expect { request }.to change(User, :count).by(1) + end + + it "redirects back to organisation users page" do + request + expect(response).to redirect_to("/organisations/#{user.organisation.id}/users") + end context "when the email is already taken" do before do @@ -658,6 +1073,33 @@ RSpec.describe UsersController, type: :request do expect(page).to have_content(I18n.t("validations.email.taken")) end end + + context "when trying to assign support role" do + let(:params) do + { + "user": { + name: "new user", + email: "new_user@example.com", + role: "support", + }, + } + end + + it "creates a new support user" do + expect(User.last.role).to eq("support") + end + end + end + + describe "#new" do + before do + sign_in user + end + + it "can assign support role to the new user" do + get "/users/new" + expect(page).to have_field("user-role-support-field") + end end end