From 25b3cae0e9e12be609a93930af4e5184eefac919 Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Tue, 29 Mar 2022 16:18:41 +0100 Subject: [PATCH] Only data coordinators can change role, dpo, keycontact --- app/controllers/users_controller.rb | 20 +- app/views/users/edit.html.erb | 32 ++-- app/views/users/new.html.erb | 32 ++-- app/views/users/show.html.erb | 12 +- spec/features/user_spec.rb | 251 ++++++++++++++----------- spec/requests/users_controller_spec.rb | 93 +++++---- 6 files changed, 260 insertions(+), 180 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 1633c879e..7c90ea738 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -3,7 +3,7 @@ class UsersController < ApplicationController include Helpers::Email before_action :authenticate_user! before_action :find_resource, except: %i[new create] - before_action :authenticate_scope!, except: %i[new create] + before_action :authenticate_scope!, except: %i[new] def update if @user.update(user_params) @@ -76,8 +76,12 @@ private def user_params if @user == current_user - params.require(:user).permit(:email, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact) - else + if current_user.data_coordinator? + 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? params.require(:user).permit(:email, :name, :role, :is_dpo, :is_key_contact) end end @@ -87,8 +91,12 @@ private end def authenticate_scope! - render_not_found and return unless current_user.organisation == @user.organisation - render_not_found and return if action_name == "edit_password" && current_user != @user - render_not_found and return unless current_user.role == "data_coordinator" || current_user == @user + if action_name == "create" + head :unauthorized and return unless current_user.data_coordinator? + else + render_not_found and return unless current_user.organisation == @user.organisation + render_not_found and return if action_name == "edit_password" && current_user != @user + render_not_found and return unless current_user.data_coordinator? || current_user == @user + end end end diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb index 2da85f28b..6582903d1 100644 --- a/app/views/users/edit.html.erb +++ b/app/views/users/edit.html.erb @@ -26,21 +26,23 @@ spellcheck: "false" %> - <%= f.govuk_collection_radio_buttons :is_dpo, - [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")], - :id, - :name, - inline: true, - legend: { text: "Are #{current_user == @user ? "you" : "they"} a data protection officer?", size: "m" } - %> - - <%= f.govuk_collection_radio_buttons :is_key_contact, - [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")], - :id, - :name, - inline: true, - legend: { text: "Are #{current_user == @user ? "you" : "they"} a key contact?", size: "m" } - %> + <% if current_user.data_coordinator? %> + <%= f.govuk_collection_radio_buttons :is_dpo, + [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")], + :id, + :name, + inline: true, + legend: { text: "Are #{current_user == @user ? "you" : "they"} a data protection officer?", size: "m" } + %> + + <%= f.govuk_collection_radio_buttons :is_key_contact, + [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")], + :id, + :name, + inline: true, + legend: { text: "Are #{current_user == @user ? "you" : "they"} a key contact?", size: "m" } + %> + <% end %> <%= f.govuk_submit "Save changes" %> diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb index fe9cdb3b9..bc9e0b1f0 100644 --- a/app/views/users/new.html.erb +++ b/app/views/users/new.html.erb @@ -31,21 +31,23 @@ f.govuk_collection_radio_buttons :role, roles, :id, :name, legend: { text: "Role", size: "m" } %> - <%= f.govuk_collection_radio_buttons :is_dpo, - [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")], - :id, - :name, - inline: true, - legend: { text: "Are #{current_user == @user ? "you" : "they"} a data protection officer?", size: "m" } - %> - - <%= f.govuk_collection_radio_buttons :is_key_contact, - [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")], - :id, - :name, - inline: true, - legend: { text: "Are #{current_user == @user ? "you" : "they"} a key contact?", size: "m" } - %> + <% if current_user.data_coordinator? %> + <%= f.govuk_collection_radio_buttons :is_dpo, + [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")], + :id, + :name, + inline: true, + legend: { text: "Are #{current_user == @user ? "you" : "they"} a data protection officer?", size: "m" } + %> + + <%= f.govuk_collection_radio_buttons :is_key_contact, + [OpenStruct.new(id: false, name: "No"), OpenStruct.new(id: true, name: "Yes")], + :id, + :name, + inline: true, + legend: { text: "Are #{current_user == @user ? "you" : "they"} a key contact?", size: "m" } + %> + <% end %> <%= f.govuk_submit "Continue" %> diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index bd38dc611..61a8c8c09 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -48,13 +48,21 @@ <%= summary_list.row do |row| row.key { 'Data protection officer' } row.value { @user.is_data_protection_officer? ? "Yes" : "No" } - row.action(visually_hidden_text: "are #{current_user == @user ? "you" : "they"} a data protection officer?", href: edit_user_path, html_attributes: { "data-qa": "change-are-#{current_user == @user ? "you" : "they"}-a-data-protection-officer" }) + if current_user.data_coordinator? + row.action(visually_hidden_text: "are #{current_user == @user ? "you" : "they"} a data protection officer?", href: edit_user_path, html_attributes: { "data-qa": "change-are-#{current_user == @user ? "you" : "they"}-a-data-protection-officer" }) + else + row.action() + end end %> <%= summary_list.row do |row| row.key { 'Key contact' } row.value { @user.is_key_contact? ? "Yes" : "No" } - row.action(visually_hidden_text: "are #{current_user == @user ? "you" : "they"} a key contact?", href: edit_user_path, html_attributes: { "data-qa": "change-are-#{current_user == @user ? "you" : "they"}-a-key-contact" }) + if current_user.data_coordinator? + row.action(visually_hidden_text: "are #{current_user == @user ? "you" : "they"} a key contact?", href: edit_user_path, html_attributes: { "data-qa": "change-are-#{current_user == @user ? "you" : "they"}-a-key-contact" }) + else + row.action() + end end %> <% end %> diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb index 1e7c7deb3..8ff9bed98 100644 --- a/spec/features/user_spec.rb +++ b/spec/features/user_spec.rb @@ -172,123 +172,156 @@ RSpec.describe "User Features" do end end - context "when viewing your account" do - before do - visit("/logs") - fill_in("user[email]", with: user.email) - fill_in("user[password]", with: "pAssword1") - click_button("Sign in") - end - - it "shows 'Your account' link in navigation if logged in and redirect to correct page" do - visit("/logs") - expect(page).to have_link("Your account") - click_link("Your account") - expect(page).to have_current_path("/users/#{user.id}") - end - - it "can navigate to change your password page from main account page" do - visit("/users/#{user.id}") - find('[data-qa="change-password"]').click - expect(page).to have_content("Change your password") - fill_in("user[password]", with: "Password123!") - fill_in("user[password_confirmation]", with: "Password123!") - click_button("Update") - expect(page).to have_current_path("/users/#{user.id}") - end - - it "allow user to change name" do - visit("/users/#{user.id}") - find('[data-qa="change-name"]').click - expect(page).to have_content("Change your personal details") - fill_in("user[name]", with: "Test New") - click_button("Save changes") - expect(page).to have_current_path("/users/#{user.id}") - expect(page).to have_content("Test New") + context "when signed in as a data provider" do + context "when viewing your account" do + before do + visit("/logs") + fill_in("user[email]", with: user.email) + fill_in("user[password]", with: "pAssword1") + click_button("Sign in") + end + + it "does not have change links for dpo and key contact" do + visit("/users/#{user.id}") + expect(page).not_to have_selector('[data-qa="change-are-you-a-data-protection-officer"]') + expect(page).not_to have_selector('[data-qa="change-are-you-a-key-contact"]') + end + + it "does not have dpo and key contact as editable fields" do + visit("/users/#{user.id}/edit") + expect(page).not_to have_field("user[is_dpo]") + expect(page).not_to have_field("user[is_key_contact]") + end end end - context "when adding a new user" do - before do - visit("/logs") - fill_in("user[email]", with: user.email) - fill_in("user[password]", with: "pAssword1") - click_button("Sign in") - end - - it "validates an email has been provided" do - visit("users/new") - fill_in("user[name]", with: "New User") - click_button("Continue") - expect(page).to have_selector("#error-summary-title") - expect(page).to have_selector("#user-email-field-error") - expect(page).to have_content(/Enter an email address/) - expect(page).to have_title("Error") - end - - it "validates email" do - visit("users/new") - fill_in("user[name]", with: "New User") - fill_in("user[email]", with: "thisis'tanemail") - click_button("Continue") - expect(page).to have_selector("#error-summary-title") - expect(page).to have_selector("#user-email-field-error") - expect(page).to have_content(/Enter an email address in the correct format, like name@example.com/) - expect(page).to have_title("Error") - end - - it "sets name, email, role, is_dpo and is_key_contact fields" do - visit("users/new") - fill_in("user[name]", with: "New User") - fill_in("user[email]", with: "newuser@example.com") - choose("user-role-data-provider-field") - choose("user-is-dpo-true-field") - choose("user-is-key-contact-true-field") - click_button("Continue") - expect(User.find_by( - name: "New User", - email: "newuser@example.com", - role: "data_provider", - is_dpo: true, - is_key_contact: true, - )).to be_a(User) - end + context "when signed in as a data coordinator" do + let!(:user) { FactoryBot.create(:user, :data_coordinator, last_sign_in_at: Time.zone.now) } - it "defaults to is_dpo false" do - visit("users/new") - expect(page).to have_field("user[is_dpo]", with: false) + context "when viewing your account" do + before do + visit("/logs") + fill_in("user[email]", with: user.email) + fill_in("user[password]", with: "pAssword1") + click_button("Sign in") + end + + it "shows 'Your account' link in navigation if logged in and redirect to correct page" do + visit("/logs") + expect(page).to have_link("Your account") + click_link("Your account") + expect(page).to have_current_path("/users/#{user.id}") + end + + it "can navigate to change your password page from main account page" do + visit("/users/#{user.id}") + find('[data-qa="change-password"]').click + expect(page).to have_content("Change your password") + fill_in("user[password]", with: "Password123!") + fill_in("user[password_confirmation]", with: "Password123!") + click_button("Update") + expect(page).to have_current_path("/users/#{user.id}") + end + + it "allow user to change name" do + visit("/users/#{user.id}") + find('[data-qa="change-name"]').click + expect(page).to have_content("Change your personal details") + fill_in("user[name]", with: "Test New") + click_button("Save changes") + expect(page).to have_current_path("/users/#{user.id}") + expect(page).to have_content("Test New") + end + + it "has dpo and key contact as editable fields" do + visit("/users/#{user.id}") + expect(page).to have_selector('[data-qa="change-are-you-a-data-protection-officer"]') + expect(page).to have_selector('[data-qa="change-are-you-a-key-contact"]') + end end - end - - context "when editing someone elses account details" do - let!(:user) { FactoryBot.create(:user, :data_coordinator, last_sign_in_at: Time.zone.now) } - let!(:other_user) { FactoryBot.create(:user, name: "Other name", is_dpo: true, organisation: user.organisation) } - before do - visit("/logs") - fill_in("user[email]", with: user.email) - fill_in("user[password]", with: "pAssword1") - click_button("Sign in") + context "when adding a new user" do + before do + visit("/logs") + fill_in("user[email]", with: user.email) + fill_in("user[password]", with: "pAssword1") + click_button("Sign in") + end + + it "validates an email has been provided" do + visit("users/new") + fill_in("user[name]", with: "New User") + click_button("Continue") + expect(page).to have_selector("#error-summary-title") + expect(page).to have_selector("#user-email-field-error") + expect(page).to have_content(/Enter an email address/) + expect(page).to have_title("Error") + end + + it "validates email" do + visit("users/new") + fill_in("user[name]", with: "New User") + fill_in("user[email]", with: "thisis'tanemail") + click_button("Continue") + expect(page).to have_selector("#error-summary-title") + expect(page).to have_selector("#user-email-field-error") + expect(page).to have_content(/Enter an email address in the correct format, like name@example.com/) + expect(page).to have_title("Error") + end + + it "sets name, email, role, is_dpo and is_key_contact fields" do + visit("users/new") + fill_in("user[name]", with: "New User") + fill_in("user[email]", with: "newuser@example.com") + choose("user-role-data-provider-field") + choose("user-is-dpo-true-field") + choose("user-is-key-contact-true-field") + click_button("Continue") + expect(User.find_by( + name: "New User", + email: "newuser@example.com", + role: "data_provider", + is_dpo: true, + is_key_contact: true, + )).to be_a(User) + end + + it "defaults to is_dpo false" do + visit("users/new") + expect(page).to have_field("user[is_dpo]", with: false) + end end - it "allows updating other users details" do - visit("/organisations/#{user.organisation.id}") - click_link("Users") - click_link(other_user.name) - expect(page).to have_title("Other name’s account") - first(:link, "Change").click - expect(page).to have_field("user[is_dpo]", with: true) - choose("user-is-dpo-field") - choose("user-is-key-contact-true-field") - fill_in("user[name]", with: "Updated new name") - click_button("Save changes") - expect(page).to have_title("Updated new name’s account") - expect(User.find_by( - name: "Updated new name", - role: "data_provider", - is_dpo: false, - is_key_contact: true, - )).to be_a(User) + context "when editing someone elses account details" do + let!(:user) { FactoryBot.create(:user, :data_coordinator, last_sign_in_at: Time.zone.now) } + let!(:other_user) { FactoryBot.create(:user, name: "Other name", is_dpo: true, organisation: user.organisation) } + + before do + visit("/logs") + fill_in("user[email]", with: user.email) + fill_in("user[password]", with: "pAssword1") + click_button("Sign in") + end + + it "allows updating other users details" do + visit("/organisations/#{user.organisation.id}") + click_link("Users") + click_link(other_user.name) + expect(page).to have_title("Other name’s account") + first(:link, "Change").click + expect(page).to have_field("user[is_dpo]", with: true) + choose("user-is-dpo-field") + choose("user-is-key-contact-true-field") + fill_in("user[name]", with: "Updated new name") + click_button("Save changes") + expect(page).to have_title("Updated new name’s account") + expect(User.find_by( + name: "Updated new name", + role: "data_provider", + is_dpo: false, + is_key_contact: true, + )).to be_a(User) + end end end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 4e64c9714..3c06aef5f 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -213,11 +213,11 @@ RSpec.describe UsersController, type: :request do context "when user changes email, dpo, 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 + it "allows changing email but not dpo or key_contact" 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 + expect(user.is_data_protection_officer?).to be false + expect(user.is_key_contact?).to be false end end end @@ -266,6 +266,32 @@ RSpec.describe UsersController, type: :request do 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 "does not invite a new user" do + expect { request }.not_to change(User, :count) + end + + it "returns 401 unauthorized" do + request + expect(response).to have_http_status(:unauthorized) + end + end end context "when user is signed in as a data coordinator" do @@ -513,42 +539,43 @@ RSpec.describe UsersController, type: :request do end 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 + describe "#create" do + let(:user) { FactoryBot.create(:user, :data_coordinator) } + let(:params) do + { + "user": { + name: "new user", + email: "new_user@example.com", + role: "data_coordinator", + }, + } + end + let(:request) { post "/users/", headers: headers, params: params } - context "when the email is already taken" do before do - FactoryBot.create(:user, email: "new_user@example.com") + sign_in user end - it "shows an error" do + 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 have_http_status(:unprocessable_entity) - expect(page).to have_content(I18n.t("validations.email.taken")) + 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 end end