From e079455f65372dcdef152c74d59a29c853acdf30 Mon Sep 17 00:00:00 2001 From: Samuel Young Date: Wed, 29 Apr 2026 16:16:43 +0100 Subject: [PATCH] CLDC-4332: Check that a support user can only be in certain orgs (#3305) * CLDC-4332: Check that a support user can only be in certain orgs * CLDC-4332: Add tests * CLDC-4332: Lint * CLDC-4332: Only show the check for wrong org on edit * CLDC-4332: Add the error to role if role is wrong * fixup! CLDC-4332: Only show the check for wrong org on edit lint * CLDC-4332: TEMP: Add MHCLG filter for review apps * Revert "CLDC-4332: TEMP: Add MHCLG filter for review apps" This reverts commit b65c630edea1f06403b4f837ec79d2541b7fb46b. --- app/controllers/users_controller.rb | 10 ++++-- app/models/user.rb | 17 +++++++++++ app/services/feature_toggle.rb | 6 ++++ config/locales/en.yml | 4 +++ spec/models/user_spec.rb | 47 +++++++++++++++++++++++++++++ 5 files changed, 82 insertions(+), 2 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 57036cabe..677925679 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -221,8 +221,14 @@ private @user.errors.add :phone end - if user_params.key?(:organisation_id) && user_params[:organisation_id].blank? - @user.errors.add :organisation_id, :blank + if user_params.key?(:organisation_id) + if user_params[:organisation_id].blank? + @user.errors.add :organisation_id, :blank + elsif !@user.role_is_allowed_to_be_in_organisation?(override_organisation_id: user_params[:organisation_id].to_i) && @user.id.present? + # this will also be flagged by the validation in user.rb. + # for convenience we show the error early before they go through the change org flow (involves reassigning logs). + @user.errors.add :organisation_id, I18n.t("validations.user.support_user_in_wrong_organisation.change_organisation") + end end end diff --git a/app/models/user.rb b/app/models/user.rb index 92fd37dc8..b2ab58c11 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -25,6 +25,7 @@ class User < ApplicationRecord validates :organisation_id, presence: true validate :organisation_not_merged + validate :support_user_is_in_correct_organisation has_paper_trail ignore: %w[last_sign_in_at current_sign_in_at @@ -390,6 +391,12 @@ class User < ApplicationRecord end end + def role_is_allowed_to_be_in_organisation?(override_organisation_id: nil) + return true unless support? && FeatureToggle.support_organisation_allow_list.present? + + FeatureToggle.support_organisation_allow_list.include?(override_organisation_id || organisation_id) + end + protected # Checks whether a password is needed or not. For validations only. @@ -407,6 +414,16 @@ private end end + def support_user_is_in_correct_organisation + return if role_is_allowed_to_be_in_organisation? + + if role_changed? + errors.add :role, I18n.t("validations.user.support_user_in_wrong_organisation.change_role") + else + errors.add :organisation_id, I18n.t("validations.user.support_user_in_wrong_organisation.change_organisation") + end + end + def send_data_protection_confirmation_reminder return unless persisted? return unless is_dpo? diff --git a/app/services/feature_toggle.rb b/app/services/feature_toggle.rb index 2f301294c..7404309f2 100644 --- a/app/services/feature_toggle.rb +++ b/app/services/feature_toggle.rb @@ -34,4 +34,10 @@ class FeatureToggle def self.sales_export_enabled? Time.zone.now >= Time.zone.local(2025, 4, 1) || (Rails.env.review? || Rails.env.staging?) end + + # IDs of organisations a user must be in to be allowed the support role + # if nil this feature will be disabled + def self.support_organisation_allow_list + [1] if Rails.env.production? + end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 26a834e5d..843d1da8b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -260,6 +260,10 @@ en: blank: "Enter an email address." role: invalid: "Role must be data accessor, data provider or data coordinator." + user: + support_user_in_wrong_organisation: + change_role: "You cannot create a support account type for a user in this organisation. Support accounts should only be created for MHCLG and contractor staff as they are administrator level accounts with access to all organisations' data. Any support accounts for housing organisations would be a data protection breach." + change_organisation: "You cannot move a user with a support account to a non-MHCLG organisation. If you need to move the user, change their role type to data coordinator or data provider." setup: saledate: diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 495d5ba13..b461b60e5 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -540,6 +540,53 @@ RSpec.describe User, type: :model do .to raise_error(ActiveRecord::RecordInvalid, error_message) end end + + describe "#support_user_is_in_correct_organisation" do + let(:organisation) { create(:organisation) } + + context "when the user is not a support user" do + let(:user) { build(:user, :data_coordinator, organisation:) } + + it "is valid regardless of the allow list" do + allow(FeatureToggle).to receive(:support_organisation_allow_list).and_return([999]) + expect(user).to be_valid + end + end + + context "when the user is a support user" do + let(:user) { build(:user, :support, organisation:) } + + context "and the allow list is nil" do + before do + allow(FeatureToggle).to receive(:support_organisation_allow_list).and_return(nil) + end + + it "is valid" do + expect(user).to be_valid + end + end + + context "and the organisation is in the allow list" do + before do + allow(FeatureToggle).to receive(:support_organisation_allow_list).and_return([organisation.id]) + end + + it "is valid" do + expect(user).to be_valid + end + end + + context "and the organisation is not in the allow list" do + before do + allow(FeatureToggle).to receive(:support_organisation_allow_list).and_return([organisation.id + 1]) + end + + it "is not valid" do + expect(user).not_to be_valid + end + end + end + end end describe "delete" do