Only data coordinators can change role, dpo, keycontact

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

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" %>
     </div>

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" %>
     </div>

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 %>
   </div>

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

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