Merge pull request #2435 from alphagov/fix-bug-in-user-update

Fix permissions bug in UserUpdate

app/controllers/account/manage_permissions_controller.rb

@@ -4,6 +4,7 @@ class Account::ManagePermissionsController < ApplicationController
 
   before_action :authenticate_user!
   before_action :authorise_user
+  before_action :allow_no_application_access, only: [:update]
 
   def show
     @application_permissions = all_applications_and_permissions_for(current_user)
@@ -21,6 +22,11 @@ def update
 
 private
 
+  def allow_no_application_access
+    params[:user] ||= {}
+    params[:user][:supported_permission_ids] ||= []
+  end
+
   def authorise_user
     authorize %i[account manage_permissions]
   end

app/helpers/users_helper.rb

@@ -16,7 +16,7 @@ def organisation_options(form_builder)
   end
 
   def organisation_select_options
-    { include_blank: current_user.publishing_manager? ? false : "None" }
+    { include_blank: current_user.publishing_manager? ? false : Organisation::NONE }
   end
 
   def user_email_tokens(user = current_user)

app/models/event_log.rb

@@ -48,6 +48,7 @@ class EventLog < ApplicationRecord
     ACCESS_GRANTS_DELETED = LogEntry.new(id: 43, description: "Access grants deleted", require_uid: true),
     ACCESS_TOKENS_DELETED = LogEntry.new(id: 44, description: "Access tokens deleted", require_uid: true),
     ACCOUNT_DELETED = LogEntry.new(id: 45, description: "Account deleted", require_uid: true),
+    ORGANISATION_CHANGED = LogEntry.new(id: 46, description: "Organisation changed", require_uid: true, require_initiator: true),
 
     # We no longer expire passwords, but we keep this event for history purposes
     PASSWORD_EXPIRED = LogEntry.new(id: 6, description: "Password expired"),
@@ -106,6 +107,10 @@ def self.record_role_change(user, previous_role, new_role, initiator)
     record_event(user, ROLE_CHANGED, initiator:, trailing_message: "from #{previous_role} to #{new_role}")
   end
 
+  def self.record_organisation_change(user, previous_organisation, new_organisation, initiator)
+    record_event(user, ORGANISATION_CHANGED, initiator:, trailing_message: "from #{previous_organisation} to #{new_organisation}")
+  end
+
   def self.record_account_invitation(user, initiator)
     record_event(user, ACCOUNT_INVITED, initiator:)
   end

app/models/organisation.rb

@@ -1,5 +1,6 @@
 class Organisation < ApplicationRecord
   GDS_ORG_CONTENT_ID = "af07d5a5-df63-4ddc-9383-6a666845ebe9".freeze
+  NONE = "None".freeze
 
   has_ancestry
 

app/services/user_update.rb

@@ -18,6 +18,7 @@ def call
     record_update
     record_permission_changes(old_permissions)
     record_role_change
+    record_organisation_change
     record_2sv_exemption_removed
     record_2sv_mandated
     send_two_step_mandated_notification
@@ -29,6 +30,8 @@ def call
 private
 
   def filtered_user_params
+    return user_params unless user_params.key?(:supported_permission_ids)
+
     filter = SupportedPermissionParameterFilter.new(current_user, user, user_params)
     user_params.merge(supported_permission_ids: filter.filtered_supported_permission_ids)
   end
@@ -87,6 +90,18 @@ def record_role_change
     )
   end
 
+  def record_organisation_change
+    organisation_change = user.previous_changes[:organisation_id]
+    return unless organisation_change
+
+    EventLog.record_organisation_change(
+      user,
+      Organisation.find_by(id: organisation_change.first)&.name || Organisation::NONE,
+      Organisation.find_by(id: organisation_change.last)&.name || Organisation::NONE,
+      current_user,
+    )
+  end
+
   def record_2sv_exemption_removed
     return unless user.require_2sv && user.previous_changes[:reason_for_2sv_exemption]
 

test/services/user_update_test.rb

@@ -78,4 +78,61 @@ class UserUpdateTest < ActionView::TestCase
 
     assert_equal 1, EventLog.where(event_id: EventLog::TWO_STEP_MANDATED.id).count
   end
+
+  should "not lose permissions when supported_permissions are absent from params" do
+    current_user = create(:superadmin_user)
+    ip_address = "1.2.3.4"
+
+    affected_user = create(:user)
+    app = create(:application)
+    affected_user.grant_application_signin_permission(app)
+    assert affected_user.has_access_to?(app)
+
+    UserUpdate.new(affected_user, {}, current_user, ip_address).call
+
+    assert affected_user.has_access_to?(app)
+  end
+
+  should "record when organisation has been changed" do
+    current_user = create(:superadmin_user)
+    ip_address = "1.2.3.4"
+
+    organisation_1 = create(:organisation, name: "organisation-1")
+    organisation_2 = create(:organisation, name: "organisation-2")
+    affected_user = create(:user, organisation: organisation_1)
+
+    params = { organisation_id: organisation_2.id }
+    UserUpdate.new(affected_user, params, current_user, ip_address).call
+
+    assert_equal 1, EventLog.where(event_id: EventLog::ORGANISATION_CHANGED.id).count
+    assert_equal "from organisation-1 to organisation-2", EventLog.where(event_id: EventLog::ORGANISATION_CHANGED.id).last.trailing_message
+  end
+
+  should "record when organisation has been changed from 'None'" do
+    current_user = create(:superadmin_user)
+    ip_address = "1.2.3.4"
+
+    organisation = create(:organisation, name: "organisation-name")
+    affected_user = create(:user, organisation: nil)
+
+    params = { organisation_id: organisation.id }
+    UserUpdate.new(affected_user, params, current_user, ip_address).call
+
+    assert_equal 1, EventLog.where(event_id: EventLog::ORGANISATION_CHANGED.id).count
+    assert_equal "from None to organisation-name", EventLog.where(event_id: EventLog::ORGANISATION_CHANGED.id).last.trailing_message
+  end
+
+  should "record when organisation has been changed to 'None'" do
+    current_user = create(:superadmin_user)
+    ip_address = "1.2.3.4"
+
+    organisation = create(:organisation, name: "organisation-name")
+    affected_user = create(:user, organisation:)
+
+    params = { organisation_id: nil }
+    UserUpdate.new(affected_user, params, current_user, ip_address).call
+
+    assert_equal 1, EventLog.where(event_id: EventLog::ORGANISATION_CHANGED.id).count
+    assert_equal "from organisation-name to None", EventLog.where(event_id: EventLog::ORGANISATION_CHANGED.id).last.trailing_message
+  end
 end