Browse Source

Allow support to edit users (#463)

* Allow support to edit users

* Refactor assignable roles

* Better spec descriptions
pull/465/head
baarkerlounger 3 years ago committed by GitHub
parent
commit
fb0c6b8bd4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 13
      app/controllers/users_controller.rb
  2. 24
      app/helpers/user_helper.rb
  3. 2
      app/views/organisations/users.html.erb
  4. 12
      app/views/users/show.html.erb
  5. 2
      config/locales/en.yml
  6. 101
      spec/helpers/user_helper_spec.rb
  7. 464
      spec/requests/users_controller_spec.rb

13
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

24
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

2
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| %>

12
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()

2
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:

101
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

464
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
it "cannot assign support role to the new user" do
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
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

Loading…
Cancel
Save