From 610b76def7c1cc94db98cdb8f482b43438a36e63 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Mon, 16 Oct 2023 12:47:19 +0100 Subject: [PATCH 1/2] Fix bug in UserUpdate to prevent loss of permissions Trello: https://trello.com/c/boKPl89S We use the UserUpdate class in `Account::RoleOrganisationsController` to update the user's role and organisation. We're not passing `supported_permission_ids` in the `params` parameter to `UserUpdate#initialize` because we're not interested in changing a user's permissions in these operations. Unfortunately it turns out that `UserUpdate` expects to have the user's existing permissions passed in if you want them to remain unchanged. Otherwise `UserUpdate#filtered_user_params` will reset/clear all permissions for that user. I've chosen to fix this in `UserUpdate#filtered_user_params` because it seems reasonable to me that leaving `supported_permission_ids` out of `params` should be interpreted as meaning that supported permissions shouldn't be changed. Note that this bug would've caused superadmins to lose their permissions if they changed their role or organisation, and admins to lose their permissions if they changed their organisation. In fixing this problem I discovered that `ManagePermissionsController` was relying on the previous behaviour to allow users to remove all their permissions. I've solved this in the same way that it's solved in `UsersController#update` by copying the `allow_no_application_access` method from that controller. --- .../account/manage_permissions_controller.rb | 6 ++++++ app/services/user_update.rb | 2 ++ test/services/user_update_test.rb | 14 ++++++++++++++ 3 files changed, 22 insertions(+) diff --git a/app/controllers/account/manage_permissions_controller.rb b/app/controllers/account/manage_permissions_controller.rb index b24690636..5319510d6 100644 --- a/app/controllers/account/manage_permissions_controller.rb +++ b/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 diff --git a/app/services/user_update.rb b/app/services/user_update.rb index c188c1edc..1ad8580a2 100644 --- a/app/services/user_update.rb +++ b/app/services/user_update.rb @@ -29,6 +29,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 diff --git a/test/services/user_update_test.rb b/test/services/user_update_test.rb index 99485c8b6..3afe30525 100644 --- a/test/services/user_update_test.rb +++ b/test/services/user_update_test.rb @@ -78,4 +78,18 @@ 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 end From df0065316a0cb73a9cebeb8ed0a1968db95a99f1 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Mon, 16 Oct 2023 13:17:15 +0100 Subject: [PATCH 2/2] Record EventLog when organisation is changed We appear to record an event when anything else is changed by `UserUpdate` so it seems reasonable that we should also record an event when the organisation is changed. Superadmins and Admins can change users' organisations from or to "None" so I've extracted the `Organisation::NONE` constant to avoid some duplication between the template and the tests. --- app/helpers/users_helper.rb | 2 +- app/models/event_log.rb | 5 ++++ app/models/organisation.rb | 1 + app/services/user_update.rb | 13 ++++++++++ test/services/user_update_test.rb | 43 +++++++++++++++++++++++++++++++ 5 files changed, 63 insertions(+), 1 deletion(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 874474763..328a0e2d1 100644 --- a/app/helpers/users_helper.rb +++ b/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) diff --git a/app/models/event_log.rb b/app/models/event_log.rb index 5c32d83af..d764b97c1 100644 --- a/app/models/event_log.rb +++ b/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 diff --git a/app/models/organisation.rb b/app/models/organisation.rb index 0d56b6177..045006516 100644 --- a/app/models/organisation.rb +++ b/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 diff --git a/app/services/user_update.rb b/app/services/user_update.rb index 1ad8580a2..1da5ec0d0 100644 --- a/app/services/user_update.rb +++ b/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 @@ -89,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] diff --git a/test/services/user_update_test.rb b/test/services/user_update_test.rb index 3afe30525..a36c87a4a 100644 --- a/test/services/user_update_test.rb +++ b/test/services/user_update_test.rb @@ -92,4 +92,47 @@ class UserUpdateTest < ActionView::TestCase 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