CLDC-771: Allow Data Coordinator to change details for users in their organisation (#428)

* Update controller auth

* Change data protection officer from role to attribute

* Allow changing DPO

* Update wording

* Use radio buttons rather than check box

* Add some integration tests

* Make hidden text dynamic

app/controllers/users_controller.rb

@@ -7,8 +7,10 @@ class UsersController < ApplicationController
 
   def update
     if @user.update(user_params)
+      if @user == current_user
         bypass_sign_in @user
         flash[:notice] = I18n.t("devise.passwords.updated") if user_params.key?("password")
+      end
       redirect_to user_path(@user)
     elsif user_params.key?("password")
       format_error_messages
@@ -73,7 +75,11 @@ private
   end
 
   def user_params
-    params.require(:user).permit(:email, :name, :password, :password_confirmation, :role)
+    if @user == current_user
+      params.require(:user).permit(:email, :name, :password, :password_confirmation, :role, :is_dpo)
+    else
+      params.require(:user).permit(:email, :name, :role, :is_dpo)
+    end
   end
 
   def find_resource
@@ -81,6 +87,8 @@ private
   end
 
   def authenticate_scope!
-    render_not_found if current_user != @user
+    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
   end
 end

app/models/user.rb

@@ -14,7 +14,6 @@ class User < ApplicationRecord
     data_accessor: 0,
     data_provider: 1,
     data_coordinator: 2,
-    data_protection_officer: 3,
   }.freeze
 
   enum role: ROLES
@@ -37,4 +36,12 @@ class User < ApplicationRecord
   def reset_password_notify_template
     last_sign_in_at ? RESET_PASSWORD_TEMPLATE_ID : SET_PASSWORD_TEMPLATE_ID
   end
+
+  def is_data_protection_officer?
+    is_dpo
+  end
+
+  def is_data_protection_officer!
+    update!(is_dpo: true)
+  end
 end

app/services/imports/data_protection_confirmation_import_service.rb

@@ -11,14 +11,14 @@ module Imports
       dp_officer = User.find_by(
         name: record_field_value(xml_document, "dp-user"),
         organisation: org,
-        role: "data_protection_officer",
+        is_dpo: true,
       )
 
       if dp_officer.blank?
         dp_officer = User.new(
           name: record_field_value(xml_document, "dp-user"),
           organisation: org,
-          role: "data_protection_officer",
+          is_dpo: true,
           encrypted_password: SecureRandom.hex(10),
         )
         dp_officer.save!(validate: false)

app/views/users/edit.html.erb

@@ -1,4 +1,4 @@
-<% content_for :title, "Change your personal details" %>
+<% content_for :title, current_user == @user ? "Change your personal details" : "Change #{@user.name}’s personal details" %>
 
 <% content_for :before_content do %>
   <%= govuk_back_link(
@@ -7,7 +7,7 @@
   ) %>
 <% end %>
 
-<%= form_for(current_user, as: :user, html: { method: :patch }) do |f| %>
+<%= form_for(@user, as: :user, html: { method: :patch }) do |f| %>
   <div class="govuk-grid-row">
     <div class="govuk-grid-column-two-thirds">
       <%= f.govuk_error_summary %>
@@ -26,6 +26,14 @@
         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_submit "Save changes" %>
     </div>
   </div>

app/views/users/new.html.erb

@@ -31,6 +31,14 @@
         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_submit "Continue" %>
     </div>
   </div>

app/views/users/show.html.erb

@@ -1,4 +1,4 @@
-<% content_for :title, "Your account" %>
+<% content_for :title, current_user == @user ? "Your account" : "#{@user.name}’s account" %>
 
 <div class="govuk-grid-row">
   <div class="govuk-grid-column-two-thirds">
@@ -13,33 +13,43 @@
     <%= govuk_summary_list do |summary_list| %>
       <%= summary_list.row do |row|
         row.key { 'Name' }
-        row.value { current_user.name }
+        row.value { @user.name }
         row.action(visually_hidden_text: 'name', href: edit_user_path, html_attributes: { 'data-qa': 'change-name' })
       end %>
 
       <%= summary_list.row() do |row|
         row.key { 'Email address' }
-        row.value { current_user.email }
+        row.value { @user.email }
         row.action(visually_hidden_text: 'email address', href: edit_user_path, html_attributes: { 'data-qa': 'change-email' })
       end %>
 
       <%= summary_list.row do |row|
         row.key { 'Password' }
         row.value { '••••••••' }
+        if current_user == @user
           row.action(visually_hidden_text: 'password', href: password_edit_user_path, html_attributes: { 'data-qa': 'change-password' })
+        else
+          row.action()
+        end
       end %>
 
       <%= summary_list.row do |row|
         row.key { 'Organisation' }
-        row.value { current_user.organisation.name }
+        row.value { @user.organisation.name }
         row.action()
       end %>
 
       <%= summary_list.row do |row|
         row.key { 'Role' }
-        row.value { current_user.role.humanize }
+        row.value { @user.role.humanize }
         row.action()
       end %>
+
+      <%= 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" })
+      end %>
     <% end %>
   </div>
 </div>

db/migrate/20220328105332_change_dpo_to_attribute.rb

@@ -0,0 +1,7 @@
+class ChangeDpoToAttribute < ActiveRecord::Migration[7.0]
+  def change
+    change_table :users, bulk: true do |t|
+      t.column :is_dpo, :boolean, default: false
+    end
+  end
+end

db/schema.rb

@@ -315,6 +315,7 @@ ActiveRecord::Schema[7.0].define(version: 202202071123100) do
     t.integer "failed_attempts", default: 0
     t.string "unlock_token"
     t.datetime "locked_at", precision: nil
+    t.boolean "is_dpo", default: false
     t.string "phone"
     t.index ["email"], name: "index_users_on_email", unique: true
     t.index ["organisation_id"], name: "index_users_on_organisation_id"

spec/factories/user.rb

@@ -9,7 +9,7 @@ FactoryBot.define do
       role { "data_coordinator" }
     end
     trait :data_protection_officer do
-      role { "data_protection_officer" }
+      is_dpo { true }
     end
     created_at { Time.zone.now }
     updated_at { Time.zone.now }

spec/features/user_spec.rb

@@ -236,5 +236,48 @@ RSpec.describe "User Features" do
       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 and is_dpo" 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")
+      click_button("Continue")
+      expect(
+        User.find_by(name: "New User", email: "newuser@example.com", role: "data_provider", is_dpo: 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
+
+  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")
+      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)).to be_a(User)
+    end
   end
 end

spec/models/user_spec.rb

@@ -46,6 +46,15 @@ RSpec.describe User, type: :model do
       expect(user.data_provider?).to be true
       expect(user.data_coordinator?).to be false
     end
+
+    it "is not a data protection officer by default" do
+      expect(user.is_data_protection_officer?).to be false
+    end
+
+    it "can be set to data protection officer" do
+      expect { user.is_data_protection_officer! }
+        .to change { user.reload.is_data_protection_officer? }.from(false).to(true)
+    end
   end
 
   describe "paper trail" do

spec/requests/users_controller_spec.rb

@@ -2,11 +2,12 @@ require "rails_helper"
 
 RSpec.describe UsersController, type: :request do
   let(:user) { FactoryBot.create(:user) }
-  let(:unauthorised_user) { FactoryBot.create(:user) }
+  let(:other_user) { FactoryBot.create(:user) }
   let(:headers) { { "Accept" => "text/html" } }
   let(:page) { Capybara::Node::Simple.new(response.body) }
-  let(:new_value) { "new test name" }
-  let(:params) { { id: user.id, user: { name: new_value } } }
+  let(:new_name) { "new test name" }
+  let(:new_email) { "new@example.com" }
+  let(:params) { { id: user.id, user: { name: new_name } } }
   let(:notify_client) { instance_double(Notifications::Client) }
   let(:devise_notify_mailer) { DeviseNotifyMailer.new }
 
@@ -56,7 +57,7 @@ RSpec.describe UsersController, type: :request do
         context "when the reset token is valid" do
           let(:params) do
             {
-              id: user.id, user: { password: new_value, password_confirmation: "something_else" }
+              id: user.id, user: { password: new_name, password_confirmation: "something_else" }
             }
           end
 
@@ -78,8 +79,8 @@ RSpec.describe UsersController, type: :request do
             {
               id: user.id,
               user: {
-                password: new_value,
-                password_confirmation: new_value,
+                password: new_name,
+                password_confirmation: new_name,
                 reset_password_token: raw,
               },
             }
@@ -109,6 +110,7 @@ RSpec.describe UsersController, type: :request do
     end
   end
 
+  context "when user is signed in as a data provider" do
     describe "#show" do
       context "when the current user matches the user ID" do
         before do
@@ -124,7 +126,7 @@ RSpec.describe UsersController, type: :request do
       context "when the current user does not matches the user ID" do
         before do
           sign_in user
-        get "/users/#{unauthorised_user.id}", headers: headers, params: {}
+          get "/users/#{other_user.id}", headers: headers, params: {}
         end
 
         it "returns not found 404" do
@@ -152,7 +154,7 @@ RSpec.describe UsersController, type: :request do
       context "when the current user does not matches the user ID" do
         before do
           sign_in user
-        get "/users/#{unauthorised_user.id}/edit", headers: headers, params: {}
+          get "/users/#{other_user.id}/edit", headers: headers, params: {}
         end
 
         it "returns not found 404" do
@@ -180,7 +182,7 @@ RSpec.describe UsersController, type: :request do
       context "when the current user does not matches the user ID" do
         before do
           sign_in user
-        get "/users/#{unauthorised_user.id}/edit", headers: headers, params: {}
+          get "/users/#{other_user.id}/edit", headers: headers, params: {}
         end
 
         it "returns not found 404" do
@@ -198,7 +200,7 @@ RSpec.describe UsersController, type: :request do
 
         it "updates the user" do
           user.reload
-        expect(user.name).to eq(new_value)
+          expect(user.name).to eq(new_name)
         end
 
         it "tracks who updated the record" do
@@ -207,6 +209,16 @@ RSpec.describe UsersController, type: :request do
           expect(whodunnit_actor).to be_a(User)
           expect(whodunnit_actor.id).to eq(user.id)
         end
+
+        context "when user changes email and dpo" do
+          let(:params) { { id: user.id, user: { name: new_name, email: new_email, is_dpo: "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
+          end
+        end
       end
 
       context "when the update fails to persist" do
@@ -223,22 +235,183 @@ RSpec.describe UsersController, type: :request do
       end
 
       context "when the current user does not matches the user ID" do
-      let(:params) { { id: unauthorised_user.id, user: { name: new_value } } }
+        let(:params) { { id: other_user.id, user: { name: new_name } } }
+
+        before do
+          sign_in user
+          patch "/users/#{other_user.id}", headers: headers, params: params
+        end
+
+        it "returns not found 404" do
+          expect(response).to have_http_status(:not_found)
+        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
+  end
+
+  context "when user is signed in as a data coordinator" do
+    let(:user) { FactoryBot.create(:user, :data_coordinator) }
+    let(:other_user) { FactoryBot.create(:user, organisation: user.organisation) }
+
+    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
+      end
+
+      context "when the current user does not matches 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
+        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 not found 404" do
+            expect(response).to have_http_status(:not_found)
+          end
+
+          it "shows the 404 view" do
+            expect(page).to have_content("Page not found")
+          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
+      end
+
+      context "when the current user does not matches 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
+        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 not found 404" do
+            expect(response).to have_http_status(:not_found)
+          end
+        end
+      end
+    end
 
+    describe "#edit_password" do
+      context "when the current user matches the user ID" do
         before do
           sign_in user
-        patch "/users/#{unauthorised_user.id}", headers: headers, params: params
+          get "/users/#{user.id}/password/edit", 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 matches the user ID" do
+        before do
+          sign_in user
+          get "/users/#{other_user.id}/password/edit", headers: headers, params: {}
         end
 
         it "returns not found 404" do
           expect(response).to have_http_status(:not_found)
         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 and dpo" do
+          let(:params) { { id: user.id, user: { name: new_name, email: new_email, is_dpo: "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
+          end
+        end
 
         context "when we update the user password" do
           let(:params) do
             {
-          id: user.id, user: { password: new_value, password_confirmation: "something_else" }
+              id: user.id, user: { password: new_name, password_confirmation: "something_else" }
             }
           end
 
@@ -254,6 +427,91 @@ RSpec.describe UsersController, type: :request do
         end
       end
 
+      context "when the current user does not matches 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 and dpo" do
+            let(:params) { { id: other_user.id, user: { name: new_name, email: new_email, is_dpo: "true" } } }
+
+            it "allows changing email and dpo" 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
+            end
+          end
+
+          it "does not bypass sign in for the coordinator" 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 we try to update the user 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 matches 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
+              patch "/users/#{other_user.id}", headers: headers, params: params
+            end
+
+            it "returns not found 404" do
+              expect(response).to have_http_status(:not_found)
+            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
+  end
+
   describe "#create" do
     let(:params) do
       {

spec/services/imports/data_protection_confirmation_import_service_spec.rb

@@ -34,7 +34,7 @@ RSpec.describe Imports::DataProtectionConfirmationImportService do
         it "creates a data protection officer without sign in credentials" do
           expect { import_service.create_data_protection_confirmations("data_protection_directory") }
             .to change(User, :count).by(1)
-          data_protection_officer = User.find_by(organisation:, role: "data_protection_officer")
+          data_protection_officer = User.find_by(organisation:, is_dpo: true)
           expect(data_protection_officer.email).to eq("")
         end