From bf50575d6e7de685926ce1c3357c10084a8bad67 Mon Sep 17 00:00:00 2001 From: Daniel Bloxham Date: Wed, 8 Jun 2022 15:00:47 +0100 Subject: [PATCH 1/4] HEEDLS-958 - Refactor Promote To Admin to reactivate inactive account at centre if it exists --- .../DataServices/PasswordDataServiceTests.cs | 18 +- .../AdminUserDataServiceTests.cs | 15 ++ .../Services/RegistrationServiceTests.cs | 156 +++++++++++++++--- .../DataServices/PasswordDataService.cs | 14 ++ .../UserDataService/AdminUserDataService.cs | 19 ++- .../UserDataService/UserDataService.cs | 2 + .../Services/RegistrationService.cs | 77 ++++++--- .../Delegates/PromoteToAdminController.cs | 4 +- 8 files changed, 245 insertions(+), 60 deletions(-) diff --git a/DigitalLearningSolutions.Data.Tests/DataServices/PasswordDataServiceTests.cs b/DigitalLearningSolutions.Data.Tests/DataServices/PasswordDataServiceTests.cs index 321cb9a561..6b20eb377f 100644 --- a/DigitalLearningSolutions.Data.Tests/DataServices/PasswordDataServiceTests.cs +++ b/DigitalLearningSolutions.Data.Tests/DataServices/PasswordDataServiceTests.cs @@ -14,9 +14,9 @@ public class PasswordDataServiceTests { private const string PasswordHashNotYetInDb = "I haven't used this password before!"; + private SqlConnection connection = null!; private PasswordDataService passwordDataService = null!; private UserDataService userDataService = null!; - private SqlConnection connection = null!; [SetUp] public void Setup() @@ -47,6 +47,22 @@ public void Set_password_by_candidate_number_should_set_password_correctly() } } + [Test] + public void SetPasswordByAdminId_should_set_password_correctly() + { + using var transaction = new TransactionScope(); + // Given + const string? password = "hashedPassword"; + const int adminId = 1; + + // When + passwordDataService.SetPasswordByAdminId(adminId, password); + var result = userDataService.GetAdminUserById(1)!.Password; + + // Then + result.Should().Be(password); + } + [Test] public async Task Setting_password_by_email_sets_password_for_matching_admins() { diff --git a/DigitalLearningSolutions.Data.Tests/DataServices/UserDataServiceTests/AdminUserDataServiceTests.cs b/DigitalLearningSolutions.Data.Tests/DataServices/UserDataServiceTests/AdminUserDataServiceTests.cs index 59e33ae48d..f29893711f 100644 --- a/DigitalLearningSolutions.Data.Tests/DataServices/UserDataServiceTests/AdminUserDataServiceTests.cs +++ b/DigitalLearningSolutions.Data.Tests/DataServices/UserDataServiceTests/AdminUserDataServiceTests.cs @@ -221,6 +221,21 @@ public void DeactivateAdminUser_updates_user() } } + [Test] + public void ActivateAdminUser_updates_user() + { + using var transaction = new TransactionScope(); + // Given + const int adminId = 8; + + // When + userDataService.ActivateAdmin(adminId); + var updatedAdminUser = userDataService.GetAdminUserById(adminId)!; + + // Then + updatedAdminUser.Active.Should().Be(true); + } + [Test] public void DeleteAdmin_deletes_admin_record() { diff --git a/DigitalLearningSolutions.Data.Tests/Services/RegistrationServiceTests.cs b/DigitalLearningSolutions.Data.Tests/Services/RegistrationServiceTests.cs index cef287e062..931acb5bea 100644 --- a/DigitalLearningSolutions.Data.Tests/Services/RegistrationServiceTests.cs +++ b/DigitalLearningSolutions.Data.Tests/Services/RegistrationServiceTests.cs @@ -17,6 +17,7 @@ namespace DigitalLearningSolutions.Data.Tests.Services using DigitalLearningSolutions.Data.Tests.TestHelpers; using FakeItEasy; using FluentAssertions; + using FluentAssertions.Execution; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Logging.Abstractions; using NUnit.Framework; @@ -32,7 +33,6 @@ public class RegistrationServiceTests private ICentresDataService centresDataService = null!; private IConfiguration config = null!; private IEmailService emailService = null!; - private IFrameworkNotificationService frameworkNotificationService = null!; private IPasswordDataService passwordDataService = null!; private IPasswordResetService passwordResetService = null!; private IRegistrationDataService registrationDataService = null!; @@ -50,7 +50,6 @@ public void Setup() centresDataService = A.Fake(); config = A.Fake(); supervisorDelegateService = A.Fake(); - frameworkNotificationService = A.Fake(); userDataService = A.Fake(); A.CallTo(() => config["CurrentSystemBaseUrl"]).Returns(OldSystemBaseUrl); @@ -72,7 +71,6 @@ public void Setup() centresDataService, config, supervisorDelegateService, - frameworkNotificationService, userDataService, new NullLogger() ); @@ -543,7 +541,7 @@ public void RegisterDelegateByCentre_schedules_welcome_email_if_notify_date_set( model.Email, NewCandidateNumber, baseUrl, - model.NotifyDate.Value, + model.NotifyDate!.Value, "RegisterDelegateByCentre_Refactor" ) ).MustHaveHappened(1, Times.Exactly); @@ -659,7 +657,8 @@ public void PromoteDelegateToAdmin_throws_AdminCreationFailedException_if_delega } [Test] - public void PromoteDelegateToAdmin_throws_email_in_use_AdminCreationFailedException_if_admin_already_exists() + public void + PromoteDelegateToAdmin_throws_email_in_use_AdminCreationFailedException_if_active_admin_already_exists() { // Given var delegateUser = UserTestHelper.GetDefaultDelegateUser(); @@ -674,7 +673,80 @@ public void PromoteDelegateToAdmin_throws_email_in_use_AdminCreationFailedExcept ); // Then - result.Error.Should().Be(AdminCreationError.EmailAlreadyInUse); + using (new AssertionScope()) + { + UpdateToExistingAdminAccountMustNotHaveHappened(); + result.Error.Should().Be(AdminCreationError.EmailAlreadyInUse); + } + } + + [Test] + public void + PromoteDelegateToAdmin_throws_email_in_use_AdminCreationFailedException_if_inactive_admin_at_different_centre_already_exists() + { + // Given + var delegateUser = UserTestHelper.GetDefaultDelegateUser(); + var adminUser = UserTestHelper.GetDefaultAdminUser(centreId: 3, active: false); + var adminRoles = new AdminRoles(true, true, true, true, true, true, true); + A.CallTo(() => userDataService.GetDelegateUserById(A._)).Returns(delegateUser); + A.CallTo(() => userDataService.GetAdminUserByEmailAddress(A._)).Returns(adminUser); + + // When + var result = Assert.Throws( + () => registrationService.PromoteDelegateToAdmin(adminRoles, 1, 1) + ); + + // Then + using (new AssertionScope()) + { + UpdateToExistingAdminAccountMustNotHaveHappened(); + result.Error.Should().Be(AdminCreationError.EmailAlreadyInUse); + } + } + + [Test] + public void PromoteDelegateToAdmin_updates_existing_admin_if_inactive_admin_at_same_centre_already_exists() + { + // Given + const int categoryId = 1; + var delegateUser = UserTestHelper.GetDefaultDelegateUser(); + var adminUser = UserTestHelper.GetDefaultAdminUser(active: false); + var adminRoles = new AdminRoles(true, true, true, true, true, true, true); + A.CallTo(() => userDataService.GetDelegateUserById(A._)).Returns(delegateUser); + A.CallTo(() => userDataService.GetAdminUserByEmailAddress(A._)).Returns(adminUser); + + // When + registrationService.PromoteDelegateToAdmin(adminRoles, categoryId, 1); + + // Then + using (new AssertionScope()) + { + A.CallTo(() => userDataService.ActivateAdmin(adminUser.Id)).MustHaveHappenedOnceExactly(); + A.CallTo( + () => userDataService.UpdateAdminUser( + delegateUser.FirstName!, + delegateUser.LastName, + delegateUser.EmailAddress!, + delegateUser.ProfileImage, + adminUser.Id + ) + ).MustHaveHappenedOnceExactly(); + A.CallTo(() => passwordDataService.SetPasswordByAdminId(adminUser.Id, delegateUser.Password!)) + .MustHaveHappenedOnceExactly(); + A.CallTo( + () => userDataService.UpdateAdminUserPermissions( + adminUser.Id, + adminRoles.IsCentreAdmin, + adminRoles.IsSupervisor, + adminRoles.IsNominatedSupervisor, + adminRoles.IsTrainer, + adminRoles.IsContentCreator, + adminRoles.IsContentManager, + adminRoles.ImportOnly, + categoryId + ) + ).MustHaveHappenedOnceExactly(); + } } [Test] @@ -690,27 +762,31 @@ public void PromoteDelegateToAdmin_calls_data_service_with_expected_value() registrationService.PromoteDelegateToAdmin(adminRoles, 1, 1); // Then - A.CallTo( - () => registrationDataService.RegisterAdmin( - A.That.Matches( - a => - a.FirstName == delegateUser.FirstName && - a.LastName == delegateUser.LastName && - a.Email == delegateUser.EmailAddress && - a.Centre == delegateUser.CentreId && - a.PasswordHash == delegateUser.Password && - a.Active && - a.Approved && - a.IsCentreAdmin == adminRoles.IsCentreAdmin && - !a.IsCentreManager && - a.IsContentManager == adminRoles.IsContentManager && - a.ImportOnly == adminRoles.IsCmsAdministrator && - a.IsContentCreator == adminRoles.IsContentCreator && - a.IsTrainer == adminRoles.IsTrainer && - a.IsSupervisor == adminRoles.IsSupervisor + using (new AssertionScope()) + { + A.CallTo( + () => registrationDataService.RegisterAdmin( + A.That.Matches( + a => + a.FirstName == delegateUser.FirstName && + a.LastName == delegateUser.LastName && + a.Email == delegateUser.EmailAddress && + a.Centre == delegateUser.CentreId && + a.PasswordHash == delegateUser.Password && + a.Active && + a.Approved && + a.IsCentreAdmin == adminRoles.IsCentreAdmin && + !a.IsCentreManager && + a.IsContentManager == adminRoles.IsContentManager && + a.ImportOnly == adminRoles.IsCmsAdministrator && + a.IsContentCreator == adminRoles.IsContentCreator && + a.IsTrainer == adminRoles.IsTrainer && + a.IsSupervisor == adminRoles.IsSupervisor + ) ) - ) - ).MustHaveHappened(); + ).MustHaveHappened(); + UpdateToExistingAdminAccountMustNotHaveHappened(); + } } private void GivenNoPendingSupervisorDelegateRecordsForEmail() @@ -735,5 +811,33 @@ private void GivenPendingSupervisorDelegateIdsForEmailAre(IEnumerable super ) .Returns(supervisorDelegates); } + + private void UpdateToExistingAdminAccountMustNotHaveHappened() + { + A.CallTo(() => userDataService.ActivateAdmin(A._)).MustNotHaveHappened(); + A.CallTo( + () => userDataService.UpdateAdminUser( + A._, + A._, + A._, + A._, + A._ + ) + ).MustNotHaveHappened(); + A.CallTo(() => passwordDataService.SetPasswordByAdminId(A._, A._)).MustNotHaveHappened(); + A.CallTo( + () => userDataService.UpdateAdminUserPermissions( + A._, + A._, + A._, + A._, + A._, + A._, + A._, + A._, + A._ + ) + ).MustNotHaveHappened(); + } } } diff --git a/DigitalLearningSolutions.Data/DataServices/PasswordDataService.cs b/DigitalLearningSolutions.Data/DataServices/PasswordDataService.cs index a6f6cb1ea9..1a18f9371f 100644 --- a/DigitalLearningSolutions.Data/DataServices/PasswordDataService.cs +++ b/DigitalLearningSolutions.Data/DataServices/PasswordDataService.cs @@ -11,7 +11,11 @@ public interface IPasswordDataService { void SetPasswordByCandidateNumber(string candidateNumber, string passwordHash); + + void SetPasswordByAdminId(int adminId, string passwordHash); + Task SetPasswordByEmailAsync(string email, string passwordHash); + Task SetPasswordForUsersAsync(IEnumerable users, string passwordHash); } @@ -34,6 +38,16 @@ public void SetPasswordByCandidateNumber(string candidateNumber, string password ); } + public void SetPasswordByAdminId(int adminId, string passwordHash) + { + connection.Query( + @"UPDATE AdminUsers + SET Password = @passwordHash + WHERE AdminID = @adminID", + new { passwordHash, adminId } + ); + } + public async Task SetPasswordByEmailAsync( string email, string passwordHash diff --git a/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs b/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs index 0532c45627..9b63951d44 100644 --- a/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs +++ b/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs @@ -143,7 +143,7 @@ int categoryId isContentManager, importOnly, categoryId, - adminId + adminId, } ); } @@ -151,12 +151,12 @@ int categoryId public void UpdateAdminUserFailedLoginCount(int adminId, int updatedCount) { connection.Execute( - @"UPDATE AdminUsers + @"UPDATE AdminUsers SET FailedLoginCount = @updatedCount WHERE AdminID = @adminId", - new { adminId, updatedCount} - ); + new { adminId, updatedCount } + ); } public void DeactivateAdmin(int adminId) @@ -170,6 +170,17 @@ public void DeactivateAdmin(int adminId) ); } + public void ActivateAdmin(int adminId) + { + connection.Execute( + @"UPDATE AdminUsers + SET + Active = 1 + WHERE AdminID = @adminId", + new { adminId } + ); + } + public void DeleteAdminUser(int adminId) { connection.Execute( diff --git a/DigitalLearningSolutions.Data/DataServices/UserDataService/UserDataService.cs b/DigitalLearningSolutions.Data/DataServices/UserDataService/UserDataService.cs index 6aaddb6dec..ac49fb37f1 100644 --- a/DigitalLearningSolutions.Data/DataServices/UserDataService/UserDataService.cs +++ b/DigitalLearningSolutions.Data/DataServices/UserDataService/UserDataService.cs @@ -115,6 +115,8 @@ void UpdateDelegateUserCentrePrompts( void DeactivateAdmin(int adminId); + void ActivateAdmin(int adminId); + void ActivateDelegateUser(int delegateId); int? GetDelegateUserLearningHubAuthId(int delegateId); diff --git a/DigitalLearningSolutions.Data/Services/RegistrationService.cs b/DigitalLearningSolutions.Data/Services/RegistrationService.cs index f5a47d9b53..dc347dbf88 100644 --- a/DigitalLearningSolutions.Data/Services/RegistrationService.cs +++ b/DigitalLearningSolutions.Data/Services/RegistrationService.cs @@ -35,7 +35,6 @@ public class RegistrationService : IRegistrationService private readonly ICentresDataService centresDataService; private readonly IConfiguration config; private readonly IEmailService emailService; - private readonly IFrameworkNotificationService frameworkNotificationService; private readonly ILogger logger; private readonly IPasswordDataService passwordDataService; private readonly IPasswordResetService passwordResetService; @@ -51,7 +50,6 @@ public RegistrationService( ICentresDataService centresDataService, IConfiguration config, ISupervisorDelegateService supervisorDelegateService, - IFrameworkNotificationService frameworkNotificationService, IUserDataService userDataService, ILogger logger ) @@ -64,7 +62,6 @@ ILogger logger this.userDataService = userDataService; this.config = config; this.supervisorDelegateService = supervisorDelegateService; - this.frameworkNotificationService = frameworkNotificationService; this.userDataService = userDataService; this.logger = logger; } @@ -162,33 +159,59 @@ public void PromoteDelegateToAdmin(AdminRoles adminRoles, int categoryId, int de var adminUser = userDataService.GetAdminUserByEmailAddress(delegateUser.EmailAddress); - if (adminUser != null) + if (adminUser?.Active is false && adminUser.CentreId == delegateUser.CentreId) { - throw new AdminCreationFailedException(AdminCreationError.EmailAlreadyInUse); + userDataService.ActivateAdmin(adminUser.Id); + userDataService.UpdateAdminUser( + delegateUser.FirstName, + delegateUser.LastName, + delegateUser.EmailAddress, + delegateUser.ProfileImage, + adminUser.Id + ); + passwordDataService.SetPasswordByAdminId(adminUser.Id, delegateUser.Password); + // Set various roles - TODO do we want to force IsCentreManager to false like we do when registering? + userDataService.UpdateAdminUserPermissions( + adminUser.Id, + adminRoles.IsCentreAdmin, + adminRoles.IsSupervisor, + adminRoles.IsNominatedSupervisor, + adminRoles.IsTrainer, + adminRoles.IsContentCreator, + adminRoles.IsContentManager, + adminRoles.ImportOnly, + categoryId + ); } + else if (adminUser == null) + { + var adminRegistrationModel = new AdminRegistrationModel( + delegateUser.FirstName, + delegateUser.LastName, + delegateUser.EmailAddress, + delegateUser.CentreId, + delegateUser.Password, + true, + true, + delegateUser.ProfessionalRegistrationNumber, + categoryId, + adminRoles.IsCentreAdmin, + false, + adminRoles.IsSupervisor, + adminRoles.IsNominatedSupervisor, + adminRoles.IsTrainer, + adminRoles.IsContentCreator, + adminRoles.IsCmsAdministrator, + adminRoles.IsCmsManager, + delegateUser.ProfileImage + ); - var adminRegistrationModel = new AdminRegistrationModel( - delegateUser.FirstName, - delegateUser.LastName, - delegateUser.EmailAddress, - delegateUser.CentreId, - delegateUser.Password, - true, - true, - delegateUser.ProfessionalRegistrationNumber, - categoryId, - adminRoles.IsCentreAdmin, - false, - adminRoles.IsSupervisor, - adminRoles.IsNominatedSupervisor, - adminRoles.IsTrainer, - adminRoles.IsContentCreator, - adminRoles.IsCmsAdministrator, - adminRoles.IsCmsManager, - delegateUser.ProfileImage - ); - - registrationDataService.RegisterAdmin(adminRegistrationModel); + registrationDataService.RegisterAdmin(adminRegistrationModel); + } + else + { + throw new AdminCreationFailedException(AdminCreationError.EmailAlreadyInUse); + } } public string RegisterDelegateByCentre(DelegateRegistrationModel delegateRegistrationModel, string baseUrl) diff --git a/DigitalLearningSolutions.Web/Controllers/TrackingSystem/Delegates/PromoteToAdminController.cs b/DigitalLearningSolutions.Web/Controllers/TrackingSystem/Delegates/PromoteToAdminController.cs index d27e95c4bd..2409791407 100644 --- a/DigitalLearningSolutions.Web/Controllers/TrackingSystem/Delegates/PromoteToAdminController.cs +++ b/DigitalLearningSolutions.Web/Controllers/TrackingSystem/Delegates/PromoteToAdminController.cs @@ -51,9 +51,9 @@ ILogger logger public IActionResult Index(int delegateId) { var centreId = User.GetCentreId(); - var delegateUser = userDataService.GetDelegateUserCardById(delegateId)!; + var delegateUser = userDataService.GetDelegateUserCardById(delegateId); - if (delegateUser.IsAdmin || !delegateUser.IsPasswordSet) + if (delegateUser!.IsAdmin || !delegateUser.IsPasswordSet) { return NotFound(); } From 6b9fd0167069637928bdeccbc887d23d73ca2426 Mon Sep 17 00:00:00 2001 From: Daniel Bloxham Date: Wed, 8 Jun 2022 15:14:34 +0100 Subject: [PATCH 2/4] HEEDLS-958 - tidy data service strings and remove comment. --- .../DataServices/PasswordDataService.cs | 6 +++--- .../DataServices/UserDataService/AdminUserDataService.cs | 7 +++---- .../Services/RegistrationService.cs | 1 - 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/DigitalLearningSolutions.Data/DataServices/PasswordDataService.cs b/DigitalLearningSolutions.Data/DataServices/PasswordDataService.cs index 1a18f9371f..765ce77220 100644 --- a/DigitalLearningSolutions.Data/DataServices/PasswordDataService.cs +++ b/DigitalLearningSolutions.Data/DataServices/PasswordDataService.cs @@ -41,9 +41,9 @@ public void SetPasswordByCandidateNumber(string candidateNumber, string password public void SetPasswordByAdminId(int adminId, string passwordHash) { connection.Query( - @"UPDATE AdminUsers - SET Password = @passwordHash - WHERE AdminID = @adminID", + @"UPDATE AdminUsers SET + Password = @passwordHash + WHERE AdminID = @adminID", new { passwordHash, adminId } ); } diff --git a/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs b/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs index 9b63951d44..a3763664ad 100644 --- a/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs +++ b/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs @@ -173,10 +173,9 @@ public void DeactivateAdmin(int adminId) public void ActivateAdmin(int adminId) { connection.Execute( - @"UPDATE AdminUsers - SET - Active = 1 - WHERE AdminID = @adminId", + @"UPDATE AdminUsers SET + Active = 1 + WHERE AdminID = @adminId", new { adminId } ); } diff --git a/DigitalLearningSolutions.Data/Services/RegistrationService.cs b/DigitalLearningSolutions.Data/Services/RegistrationService.cs index dc347dbf88..9cc6a179fb 100644 --- a/DigitalLearningSolutions.Data/Services/RegistrationService.cs +++ b/DigitalLearningSolutions.Data/Services/RegistrationService.cs @@ -170,7 +170,6 @@ public void PromoteDelegateToAdmin(AdminRoles adminRoles, int categoryId, int de adminUser.Id ); passwordDataService.SetPasswordByAdminId(adminUser.Id, delegateUser.Password); - // Set various roles - TODO do we want to force IsCentreManager to false like we do when registering? userDataService.UpdateAdminUserPermissions( adminUser.Id, adminRoles.IsCentreAdmin, From 012ddfcece2aea05d9d1b41043a1ec277b7ac59f Mon Sep 17 00:00:00 2001 From: Daniel Bloxham Date: Thu, 9 Jun 2022 08:52:54 +0100 Subject: [PATCH 3/4] HEEDLS-958 - revoke high level permissions from reactivated admin --- .../AdminUserDataServiceTests.cs | 14 ++++++++++---- .../Services/RegistrationServiceTests.cs | 4 ++-- .../UserDataService/AdminUserDataService.cs | 11 +++++++++-- .../UserDataService/UserDataService.cs | 2 +- .../Services/RegistrationService.cs | 2 +- 5 files changed, 23 insertions(+), 10 deletions(-) diff --git a/DigitalLearningSolutions.Data.Tests/DataServices/UserDataServiceTests/AdminUserDataServiceTests.cs b/DigitalLearningSolutions.Data.Tests/DataServices/UserDataServiceTests/AdminUserDataServiceTests.cs index f29893711f..d6be315261 100644 --- a/DigitalLearningSolutions.Data.Tests/DataServices/UserDataServiceTests/AdminUserDataServiceTests.cs +++ b/DigitalLearningSolutions.Data.Tests/DataServices/UserDataServiceTests/AdminUserDataServiceTests.cs @@ -6,6 +6,7 @@ using Dapper; using DigitalLearningSolutions.Data.Tests.TestHelpers; using FluentAssertions; + using FluentAssertions.Execution; using NUnit.Framework; public partial class UserDataServiceTests @@ -222,18 +223,23 @@ public void DeactivateAdminUser_updates_user() } [Test] - public void ActivateAdminUser_updates_user() + public void ReactivateAdminUser_activates_user_and_resets_admin_permissions() { using var transaction = new TransactionScope(); // Given - const int adminId = 8; + const int adminId = 16; // When - userDataService.ActivateAdmin(adminId); + userDataService.ReactivateAdmin(adminId); var updatedAdminUser = userDataService.GetAdminUserById(adminId)!; // Then - updatedAdminUser.Active.Should().Be(true); + using (new AssertionScope()) + { + updatedAdminUser.Active.Should().Be(true); + updatedAdminUser.IsCentreManager.Should().Be(false); + updatedAdminUser.IsUserAdmin.Should().Be(false); + } } [Test] diff --git a/DigitalLearningSolutions.Data.Tests/Services/RegistrationServiceTests.cs b/DigitalLearningSolutions.Data.Tests/Services/RegistrationServiceTests.cs index 931acb5bea..4deec91162 100644 --- a/DigitalLearningSolutions.Data.Tests/Services/RegistrationServiceTests.cs +++ b/DigitalLearningSolutions.Data.Tests/Services/RegistrationServiceTests.cs @@ -721,7 +721,7 @@ public void PromoteDelegateToAdmin_updates_existing_admin_if_inactive_admin_at_s // Then using (new AssertionScope()) { - A.CallTo(() => userDataService.ActivateAdmin(adminUser.Id)).MustHaveHappenedOnceExactly(); + A.CallTo(() => userDataService.ReactivateAdmin(adminUser.Id)).MustHaveHappenedOnceExactly(); A.CallTo( () => userDataService.UpdateAdminUser( delegateUser.FirstName!, @@ -814,7 +814,7 @@ private void GivenPendingSupervisorDelegateIdsForEmailAre(IEnumerable super private void UpdateToExistingAdminAccountMustNotHaveHappened() { - A.CallTo(() => userDataService.ActivateAdmin(A._)).MustNotHaveHappened(); + A.CallTo(() => userDataService.ReactivateAdmin(A._)).MustNotHaveHappened(); A.CallTo( () => userDataService.UpdateAdminUser( A._, diff --git a/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs b/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs index a3763664ad..43fda70129 100644 --- a/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs +++ b/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs @@ -170,11 +170,18 @@ public void DeactivateAdmin(int adminId) ); } - public void ActivateAdmin(int adminId) + /// + /// When we reactivate an admin, we must ensure the admin permissions are not + /// greater than basic levels. Otherwise, a basic admin would be able to + /// "create" admins with more permissions than themselves. + /// + public void ReactivateAdmin(int adminId) { connection.Execute( @"UPDATE AdminUsers SET - Active = 1 + Active = 1, + IsCentreManager = 0, + UserAdmin = 0 WHERE AdminID = @adminId", new { adminId } ); diff --git a/DigitalLearningSolutions.Data/DataServices/UserDataService/UserDataService.cs b/DigitalLearningSolutions.Data/DataServices/UserDataService/UserDataService.cs index ac49fb37f1..943d55c5ab 100644 --- a/DigitalLearningSolutions.Data/DataServices/UserDataService/UserDataService.cs +++ b/DigitalLearningSolutions.Data/DataServices/UserDataService/UserDataService.cs @@ -115,7 +115,7 @@ void UpdateDelegateUserCentrePrompts( void DeactivateAdmin(int adminId); - void ActivateAdmin(int adminId); + void ReactivateAdmin(int adminId); void ActivateDelegateUser(int delegateId); diff --git a/DigitalLearningSolutions.Data/Services/RegistrationService.cs b/DigitalLearningSolutions.Data/Services/RegistrationService.cs index 9cc6a179fb..72b089e587 100644 --- a/DigitalLearningSolutions.Data/Services/RegistrationService.cs +++ b/DigitalLearningSolutions.Data/Services/RegistrationService.cs @@ -161,7 +161,7 @@ public void PromoteDelegateToAdmin(AdminRoles adminRoles, int categoryId, int de if (adminUser?.Active is false && adminUser.CentreId == delegateUser.CentreId) { - userDataService.ActivateAdmin(adminUser.Id); + userDataService.ReactivateAdmin(adminUser.Id); userDataService.UpdateAdminUser( delegateUser.FirstName, delegateUser.LastName, From 21220da6cb48b8af08cf333adc4c24b6b1b0f1c9 Mon Sep 17 00:00:00 2001 From: Daniel Bloxham Date: Fri, 10 Jun 2022 08:54:53 +0100 Subject: [PATCH 4/4] HEEDLS-958 - review markups --- .../AdminUserDataServiceTests.cs | 13 +++++++++---- .../TestHelpers/UserTestHelper.cs | 12 ++++++++++++ .../UserDataService/AdminUserDataService.cs | 4 ++-- .../Services/RegistrationService.cs | 2 +- 4 files changed, 24 insertions(+), 7 deletions(-) diff --git a/DigitalLearningSolutions.Data.Tests/DataServices/UserDataServiceTests/AdminUserDataServiceTests.cs b/DigitalLearningSolutions.Data.Tests/DataServices/UserDataServiceTests/AdminUserDataServiceTests.cs index d6be315261..7db6b59399 100644 --- a/DigitalLearningSolutions.Data.Tests/DataServices/UserDataServiceTests/AdminUserDataServiceTests.cs +++ b/DigitalLearningSolutions.Data.Tests/DataServices/UserDataServiceTests/AdminUserDataServiceTests.cs @@ -228,17 +228,22 @@ public void ReactivateAdminUser_activates_user_and_resets_admin_permissions() using var transaction = new TransactionScope(); // Given const int adminId = 16; + connection.SetAdminToInactiveWithCentreManagerAndSuperAdminPermissions(adminId); // When + var deactivatedAdmin = userDataService.GetAdminUserById(adminId)!; userDataService.ReactivateAdmin(adminId); - var updatedAdminUser = userDataService.GetAdminUserById(adminId)!; + var reactivatedAdmin = userDataService.GetAdminUserById(adminId)!; // Then using (new AssertionScope()) { - updatedAdminUser.Active.Should().Be(true); - updatedAdminUser.IsCentreManager.Should().Be(false); - updatedAdminUser.IsUserAdmin.Should().Be(false); + deactivatedAdmin.Active.Should().Be(false); + deactivatedAdmin.IsCentreManager.Should().Be(true); + deactivatedAdmin.IsUserAdmin.Should().Be(true); + reactivatedAdmin.Active.Should().Be(true); + reactivatedAdmin.IsCentreManager.Should().Be(false); + reactivatedAdmin.IsUserAdmin.Should().Be(false); } } diff --git a/DigitalLearningSolutions.Data.Tests/TestHelpers/UserTestHelper.cs b/DigitalLearningSolutions.Data.Tests/TestHelpers/UserTestHelper.cs index 48ea51cae7..76bfc57e90 100644 --- a/DigitalLearningSolutions.Data.Tests/TestHelpers/UserTestHelper.cs +++ b/DigitalLearningSolutions.Data.Tests/TestHelpers/UserTestHelper.cs @@ -214,6 +214,18 @@ public static CentreAnswersData GetDefaultCentreAnswersData( return new CentreAnswersData(centreId, jobGroupId, answer1, answer2, answer3, answer4, answer5, answer6); } + public static void SetAdminToInactiveWithCentreManagerAndSuperAdminPermissions(this DbConnection connection, int adminId) + { + connection.Execute( + @"UPDATE AdminUsers SET + Active = 0, + IsCentreManager = 1, + UserAdmin = 1 + WHERE AdminID = @adminId", + new { adminId } + ); + } + public static void GivenDelegateUserIsInDatabase(DelegateUser user, SqlConnection sqlConnection) { sqlConnection.Execute( diff --git a/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs b/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs index 43fda70129..eb52db9dad 100644 --- a/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs +++ b/DigitalLearningSolutions.Data/DataServices/UserDataService/AdminUserDataService.cs @@ -180,8 +180,8 @@ public void ReactivateAdmin(int adminId) connection.Execute( @"UPDATE AdminUsers SET Active = 1, - IsCentreManager = 0, - UserAdmin = 0 + IsCentreManager = 0, + UserAdmin = 0 WHERE AdminID = @adminId", new { adminId } ); diff --git a/DigitalLearningSolutions.Data/Services/RegistrationService.cs b/DigitalLearningSolutions.Data/Services/RegistrationService.cs index 72b089e587..b30c476f13 100644 --- a/DigitalLearningSolutions.Data/Services/RegistrationService.cs +++ b/DigitalLearningSolutions.Data/Services/RegistrationService.cs @@ -159,7 +159,7 @@ public void PromoteDelegateToAdmin(AdminRoles adminRoles, int categoryId, int de var adminUser = userDataService.GetAdminUserByEmailAddress(delegateUser.EmailAddress); - if (adminUser?.Active is false && adminUser.CentreId == delegateUser.CentreId) + if (adminUser?.Active == false && adminUser.CentreId == delegateUser.CentreId) { userDataService.ReactivateAdmin(adminUser.Id); userDataService.UpdateAdminUser(