diff --git a/DigitalLearningSolutions.Data.Tests/Models/User/UserAccountSetTests.cs b/DigitalLearningSolutions.Data.Tests/Models/User/UserAccountSetTests.cs index d39d65cbf6..f42ed80b00 100644 --- a/DigitalLearningSolutions.Data.Tests/Models/User/UserAccountSetTests.cs +++ b/DigitalLearningSolutions.Data.Tests/Models/User/UserAccountSetTests.cs @@ -13,12 +13,8 @@ public class UserAccountSetTests [Test] public void Any_returns_false_if_adminUser_null_and_delegateUsers_empty() { - // Given - AdminUser? adminUser = null; - var delegates = new List(); - // When - var result = new UserAccountSet(adminUser, delegates).Any(); + var result = new UserAccountSet().Any(); // Then result.Should().BeFalse(); diff --git a/DigitalLearningSolutions.Data.Tests/Services/UserServiceTests.cs b/DigitalLearningSolutions.Data.Tests/Services/UserServiceTests.cs index 61570a8a8d..787ea9658c 100644 --- a/DigitalLearningSolutions.Data.Tests/Services/UserServiceTests.cs +++ b/DigitalLearningSolutions.Data.Tests/Services/UserServiceTests.cs @@ -212,10 +212,9 @@ public void TryUpdateUsers_with_null_delegate_only_updates_admin() .DoesNothing(); // When - var result = userService.TryUpdateUserAccountDetails(accountDetailsData); + userService.UpdateUserAccountDetails(accountDetailsData); // Then - result.Should().BeTrue(); A.CallTo(() => userDataService.UpdateAdminUser(A._, A._, A._, null, A._)) .MustHaveHappened(); A.CallTo(() => userDataService.UpdateDelegateUsers(A._, A._, A._, null, A._)) @@ -246,11 +245,9 @@ public void TryUpdateUsers_with_null_admin_only_updates_delegate() .DoesNothing(); // When - var result = - userService.TryUpdateUserAccountDetails(accountDetailsData, centreAnswersData); + userService.UpdateUserAccountDetails(accountDetailsData, centreAnswersData); // Then - result.Should().BeTrue(); A.CallTo(() => userDataService.UpdateDelegateUsers(A._, A._, A._, null, A._)) .MustHaveHappened(); A.CallTo(() => userDataService.UpdateAdminUser(A._, A._, A._, null, A._)) @@ -288,10 +285,9 @@ public void TryUpdateUsers_with_both_admin_and_delegate_updates_both() .DoesNothing(); // When - var result = userService.TryUpdateUserAccountDetails(accountDetailsData, centreAnswersData); + userService.UpdateUserAccountDetails(accountDetailsData, centreAnswersData); // Then - result.Should().BeTrue(); A.CallTo(() => userDataService.UpdateDelegateUsers(A._, A._, A._, null, A._)) .MustHaveHappened(); A.CallTo(() => userDataService.UpdateAdminUser(A._, A._, A._, null, A._)) @@ -321,13 +317,12 @@ public void TryUpdateUsers_with_incorrect_password_doesnt_update() A.CallTo(() => userDataService.GetDelegateUsersByEmailAddress(signedInEmail)) .Returns(new List()); A.CallTo(() => loginService.VerifyUsers(password, A._, A>._)) - .Returns(new UserAccountSet(null, new List())); + .Returns(new UserAccountSet()); // When - var result = userService.TryUpdateUserAccountDetails(accountDetailsData, centreAnswersData); + userService.UpdateUserAccountDetails(accountDetailsData, centreAnswersData); // Then - result.Should().BeFalse(); A.CallTo(() => userDataService.UpdateDelegateUsers(A._, A._, A._, null, A._)) .MustNotHaveHappened(); A.CallTo(() => userDataService.UpdateAdminUser(A._, A._, A._, null, A._)) diff --git a/DigitalLearningSolutions.Data/Models/User/UserAccountSet.cs b/DigitalLearningSolutions.Data/Models/User/UserAccountSet.cs index 0511bad57a..dfc7b0a96a 100644 --- a/DigitalLearningSolutions.Data/Models/User/UserAccountSet.cs +++ b/DigitalLearningSolutions.Data/Models/User/UserAccountSet.cs @@ -8,6 +8,8 @@ public class UserAccountSet public readonly AdminUser? AdminAccount; public readonly List DelegateAccounts; + public UserAccountSet() : this(null, null) { } + public UserAccountSet(AdminUser? adminAccount, IEnumerable? delegateAccounts) { AdminAccount = adminAccount; diff --git a/DigitalLearningSolutions.Data/Services/UserService.cs b/DigitalLearningSolutions.Data/Services/UserService.cs index afe9b730c7..636edafe57 100644 --- a/DigitalLearningSolutions.Data/Services/UserService.cs +++ b/DigitalLearningSolutions.Data/Services/UserService.cs @@ -18,7 +18,7 @@ List delegateUsers public List GetUserCentres(AdminUser? adminUser, List delegateUsers); - public bool TryUpdateUserAccountDetails( + public void UpdateUserAccountDetails( AccountDetailsData accountDetailsData, CentreAnswersData? centreAnswersData = null ); @@ -26,6 +26,8 @@ public bool TryUpdateUserAccountDetails( public bool NewEmailAddressIsValid(string emailAddress, int? adminUserId, int? delegateUserId, int centreId); UserAccountSet GetVerifiedLinkedUsersAccounts(int? adminId, int? delegateId, string password); + + public bool IsPasswordValid(int? adminId, int? delegateId, string password); } public class UserService : IUserService @@ -103,7 +105,7 @@ public List GetUserCentres(AdminUser? adminUser, List ac.IsAdmin).ThenBy(ac => ac.CentreName).ToList(); } - public bool TryUpdateUserAccountDetails( + public void UpdateUserAccountDetails( AccountDetailsData accountDetailsData, CentreAnswersData? centreAnswersData = null ) @@ -115,11 +117,6 @@ public bool TryUpdateUserAccountDetails( accountDetailsData.Password ); - if (verifiedAdminUser == null && verifiedDelegateUsers.Count == 0) - { - return false; - } - if (verifiedAdminUser != null) { userDataService.UpdateAdminUser( @@ -156,8 +153,6 @@ public bool TryUpdateUserAccountDetails( ); } } - - return true; } public bool NewEmailAddressIsValid(string emailAddress, int? adminUserId, int? delegateUserId, int centreId) @@ -201,5 +196,12 @@ string password return loginService.VerifyUsers(password, adminUser, delegateUsers); } + + public bool IsPasswordValid(int? adminId, int? delegateId, string password) + { + var verifiedLinkedUsersAccounts = GetVerifiedLinkedUsersAccounts(adminId, delegateId, password); + + return verifiedLinkedUsersAccounts.Any(); + } } } diff --git a/DigitalLearningSolutions.Web.Tests/Controllers/ChangePasswordControllerTests.cs b/DigitalLearningSolutions.Web.Tests/Controllers/ChangePasswordControllerTests.cs index 4acefe9d2c..78644f60c4 100644 --- a/DigitalLearningSolutions.Web.Tests/Controllers/ChangePasswordControllerTests.cs +++ b/DigitalLearningSolutions.Web.Tests/Controllers/ChangePasswordControllerTests.cs @@ -29,7 +29,7 @@ public void SetUp() passwordService = A.Fake(); authenticatedController = new ChangePasswordController(passwordService, userService) .WithDefaultContext() - .WithMockUser(isAuthenticated: true, adminId: LoggedInAdminId, delegateId: LoggedInDelegateId); + .WithMockUser(true, adminId: LoggedInAdminId, delegateId: LoggedInDelegateId); } [Test] @@ -76,13 +76,15 @@ public async Task Post_returns_success_page_if_model_and_password_valid() { // Given var user = Builder.CreateNew().Build(); - GivenPasswordVerificationReturnsUsers(new UserAccountSet(user, new DelegateUser[] { }), "current-password"); + GivenPasswordVerificationReturnsUsers(new UserAccountSet(user, null), "current-password"); // When var result = await authenticatedController.Index( new ChangePasswordViewModel { - Password = "new-password", ConfirmPassword = "new-password", CurrentPassword = "current-password", + Password = "new-password", + ConfirmPassword = "new-password", + CurrentPassword = "current-password" } ); @@ -105,7 +107,9 @@ public async Task Post_changes_password_if_model_and_password_valid() await authenticatedController.Index( new ChangePasswordViewModel { - Password = "new-password", ConfirmPassword = "new-password", CurrentPassword = "current-password", + Password = "new-password", + ConfirmPassword = "new-password", + CurrentPassword = "current-password" } ); @@ -129,7 +133,9 @@ public async Task Post_changes_password_only_for_verified_users() await authenticatedController.Index( new ChangePasswordViewModel { - Password = "new-password", ConfirmPassword = "new-password", CurrentPassword = "current-password" + Password = "new-password", + ConfirmPassword = "new-password", + CurrentPassword = "current-password" } ); @@ -152,7 +158,7 @@ private void GivenPasswordVerificationReturnsUsers(UserAccountSet users, string private void GivenPasswordVerificationFails() { A.CallTo(() => userService.GetVerifiedLinkedUsersAccounts(A._, A._, A._)) - .Returns(new UserAccountSet(null, new List())); + .Returns(new UserAccountSet()); } private void ThenMustHaveChangedPasswordForUsersOnce(string newPassword, IEnumerable expectedUsers) diff --git a/DigitalLearningSolutions.Web.Tests/Controllers/Login/LoginControllerTests.cs b/DigitalLearningSolutions.Web.Tests/Controllers/Login/LoginControllerTests.cs index d9bce303c4..d9b4653bc8 100644 --- a/DigitalLearningSolutions.Web.Tests/Controllers/Login/LoginControllerTests.cs +++ b/DigitalLearningSolutions.Web.Tests/Controllers/Login/LoginControllerTests.cs @@ -196,7 +196,7 @@ public async Task Bad_password_should_render_basic_form_with_error() A.CallTo(() => userService.GetUsersByUsername(A._)) .Returns((UserTestHelper.GetDefaultAdminUser(), new List())); A.CallTo(() => loginService.VerifyUsers(A._, A._, A>._)) - .Returns(new UserAccountSet(null, new List())); + .Returns(new UserAccountSet()); // When var result = await controller.Index(LoginTestHelper.GetDefaultLoginViewModel()); diff --git a/DigitalLearningSolutions.Web.Tests/Controllers/MyAccount/MyAccountControllerTests.cs b/DigitalLearningSolutions.Web.Tests/Controllers/MyAccount/MyAccountControllerTests.cs index 118bc5b2c5..84ee34cae7 100644 --- a/DigitalLearningSolutions.Web.Tests/Controllers/MyAccount/MyAccountControllerTests.cs +++ b/DigitalLearningSolutions.Web.Tests/Controllers/MyAccount/MyAccountControllerTests.cs @@ -5,7 +5,6 @@ using DigitalLearningSolutions.Data.Models.CustomPrompts; using DigitalLearningSolutions.Data.Models.User; using DigitalLearningSolutions.Data.Services; - using DigitalLearningSolutions.Data.Tests.Helpers; using DigitalLearningSolutions.Data.Tests.TestHelpers; using DigitalLearningSolutions.Web.Controllers; using DigitalLearningSolutions.Web.Helpers; @@ -21,14 +20,13 @@ public class MyAccountControllerTests { + private const string Email = "test@user.com"; private CustomPromptHelper customPromptHelper = null!; private ICustomPromptsService customPromptsService = null!; private IImageResizeService imageResizeService = null!; private IJobGroupsDataService jobGroupsDataService = null!; private IUserService userService = null!; - private const string Email = "test@user.com"; - [SetUp] public void Setup() { @@ -43,8 +41,13 @@ public void Setup() public void EditDetailsPostSave_with_invalid_model_doesnt_call_services() { // Given - var myAccountController = new MyAccountController - (customPromptsService, userService, imageResizeService, jobGroupsDataService, customPromptHelper).WithDefaultContext().WithMockUser(true); + var myAccountController = new MyAccountController( + customPromptsService, + userService, + imageResizeService, + jobGroupsDataService, + customPromptHelper + ).WithDefaultContext().WithMockUser(true); var model = new EditDetailsViewModel(); myAccountController.ModelState.AddModelError(nameof(EditDetailsViewModel.Email), "Required"); @@ -52,7 +55,8 @@ public void EditDetailsPostSave_with_invalid_model_doesnt_call_services() var result = myAccountController.EditDetails(model, "save"); // Then - A.CallTo(() => userService.NewEmailAddressIsValid(A._, A._, A._, A._)).MustNotHaveHappened(); + A.CallTo(() => userService.NewEmailAddressIsValid(A._, A._, A._, A._)) + .MustNotHaveHappened(); result.As().Model.Should().BeEquivalentTo(model); } @@ -60,18 +64,27 @@ public void EditDetailsPostSave_with_invalid_model_doesnt_call_services() public void EditDetailsPostSave_with_missing_delegate_answers_fails_validation() { // Given - var myAccountController = new MyAccountController - (customPromptsService, userService, imageResizeService, jobGroupsDataService, customPromptHelper).WithDefaultContext().WithMockUser(true, adminId: null); - var customPromptLists = new List { CustomPromptsTestHelper.GetDefaultCustomPrompt(1, mandatory: true) }; + var myAccountController = new MyAccountController( + customPromptsService, + userService, + imageResizeService, + jobGroupsDataService, + customPromptHelper + ).WithDefaultContext().WithMockUser(true, adminId: null); + var customPromptLists = new List + { CustomPromptsTestHelper.GetDefaultCustomPrompt(1, mandatory: true) }; A.CallTo - (() => customPromptsService.GetCustomPromptsForCentreByCentreId(2)).Returns(CustomPromptsTestHelper.GetDefaultCentreCustomPrompts(customPromptLists, 2)); + (() => customPromptsService.GetCustomPromptsForCentreByCentreId(2)).Returns( + CustomPromptsTestHelper.GetDefaultCentreCustomPrompts(customPromptLists, 2) + ); var model = new EditDetailsViewModel(); // When var result = myAccountController.EditDetails(model, "save"); // Then - A.CallTo(() => userService.NewEmailAddressIsValid(A._, A._, A._, A._)).MustNotHaveHappened(); + A.CallTo(() => userService.NewEmailAddressIsValid(A._, A._, A._, A._)) + .MustNotHaveHappened(); result.As().Model.Should().BeEquivalentTo(model); myAccountController.ModelState[nameof(EditDetailsViewModel.JobGroupId)].ValidationState.Should().Be (ModelValidationState.Invalid); @@ -83,10 +96,16 @@ public void EditDetailsPostSave_with_missing_delegate_answers_fails_validation() public void EditDetailsPostSave_for_admin_user_with_missing_delegate_answers_doesnt_fail_validation() { // Given - var myAccountController = new MyAccountController - (customPromptsService, userService, imageResizeService, jobGroupsDataService, customPromptHelper).WithDefaultContext().WithMockUser(true, delegateId: null); + var myAccountController = new MyAccountController( + customPromptsService, + userService, + imageResizeService, + jobGroupsDataService, + customPromptHelper + ).WithDefaultContext().WithMockUser(true, delegateId: null); + A.CallTo(() => userService.IsPasswordValid(7, null, "password")).Returns(true); A.CallTo(() => userService.NewEmailAddressIsValid(Email, 7, null, 2)).Returns(true); - A.CallTo(() => userService.TryUpdateUserAccountDetails(A._, null)).Returns(true); + A.CallTo(() => userService.UpdateUserAccountDetails(A._, null)).DoesNothing(); var model = new EditDetailsViewModel { FirstName = "Test", @@ -100,7 +119,7 @@ public void EditDetailsPostSave_for_admin_user_with_missing_delegate_answers_doe // Then A.CallTo(() => userService.NewEmailAddressIsValid(Email, 7, null, 2)).MustHaveHappened(); - A.CallTo(() => userService.TryUpdateUserAccountDetails(A._, null)).MustHaveHappened(); + A.CallTo(() => userService.UpdateUserAccountDetails(A._, null)).MustHaveHappened(); result.Should().BeRedirectToActionResult().WithActionName("Index"); } @@ -108,11 +127,19 @@ public void EditDetailsPostSave_for_admin_user_with_missing_delegate_answers_doe public void EditDetailsPostSave_with_profile_image_fails_validation() { // Given - var myAccountController = new MyAccountController - (customPromptsService, userService, imageResizeService, jobGroupsDataService, customPromptHelper).WithDefaultContext().WithMockUser(true, adminId: null); - var customPromptLists = new List { CustomPromptsTestHelper.GetDefaultCustomPrompt(1, mandatory: true) }; + var myAccountController = new MyAccountController( + customPromptsService, + userService, + imageResizeService, + jobGroupsDataService, + customPromptHelper + ).WithDefaultContext().WithMockUser(true, adminId: null); + var customPromptLists = new List + { CustomPromptsTestHelper.GetDefaultCustomPrompt(1, mandatory: true) }; A.CallTo - (() => customPromptsService.GetCustomPromptsForCentreByCentreId(2)).Returns(CustomPromptsTestHelper.GetDefaultCentreCustomPrompts(customPromptLists, 2)); + (() => customPromptsService.GetCustomPromptsForCentreByCentreId(2)).Returns( + CustomPromptsTestHelper.GetDefaultCentreCustomPrompts(customPromptLists, 2) + ); var model = new EditDetailsViewModel { ProfileImageFile = A.Fake() @@ -122,18 +149,58 @@ public void EditDetailsPostSave_with_profile_image_fails_validation() var result = myAccountController.EditDetails(model, "save"); // Then - A.CallTo(() => userService.NewEmailAddressIsValid(A._, A._, A._, A._)).MustNotHaveHappened(); + A.CallTo(() => userService.NewEmailAddressIsValid(A._, A._, A._, A._)) + .MustNotHaveHappened(); result.As().Model.Should().BeEquivalentTo(model); myAccountController.ModelState[nameof(EditDetailsViewModel.ProfileImageFile)].ValidationState.Should().Be (ModelValidationState.Invalid); } + [Test] + public void EditDetailsPostSave_with_invalid_password_fails_validation() + { + // Given + var myAccountController = new MyAccountController( + customPromptsService, + userService, + imageResizeService, + jobGroupsDataService, + customPromptHelper + ).WithDefaultContext().WithMockUser(true, delegateId: null); + A.CallTo(() => userService.IsPasswordValid(7, null, "password")).Returns(false); + A.CallTo(() => userService.NewEmailAddressIsValid(Email, 7, null, 2)).Returns(true); + A.CallTo(() => userService.UpdateUserAccountDetails(A._, null)).DoesNothing(); + + var model = new EditDetailsViewModel + { + FirstName = "Test", + LastName = "User", + Email = Email, + Password = "password" + }; + + // When + var result = myAccountController.EditDetails(model, "save"); + + // Then + A.CallTo(() => userService.NewEmailAddressIsValid(A._, A._, A._, A._)) + .MustNotHaveHappened(); + result.As().Model.Should().BeEquivalentTo(model); + myAccountController.ModelState[nameof(EditDetailsViewModel.Password)].ValidationState.Should().Be + (ModelValidationState.Invalid); + } + [Test] public void PostEditRegistrationPrompt_returns_error_with_unexpected_action() { // Given - var myAccountController = new MyAccountController - (customPromptsService, userService, imageResizeService, jobGroupsDataService, customPromptHelper).WithDefaultContext().WithMockUser(true, adminId: null); + var myAccountController = new MyAccountController( + customPromptsService, + userService, + imageResizeService, + jobGroupsDataService, + customPromptHelper + ).WithDefaultContext().WithMockUser(true, adminId: null); const string action = "unexpectedString"; var model = new EditDetailsViewModel(); diff --git a/DigitalLearningSolutions.Web/Controllers/ChangePasswordController.cs b/DigitalLearningSolutions.Web/Controllers/ChangePasswordController.cs index 7c5d8a1c60..f3c67b7ad0 100644 --- a/DigitalLearningSolutions.Web/Controllers/ChangePasswordController.cs +++ b/DigitalLearningSolutions.Web/Controllers/ChangePasswordController.cs @@ -1,6 +1,7 @@ namespace DigitalLearningSolutions.Web.Controllers { using System.Threading.Tasks; + using DigitalLearningSolutions.Data.Models.User; using DigitalLearningSolutions.Data.Services; using DigitalLearningSolutions.Web.Helpers; using DigitalLearningSolutions.Web.ViewModels.MyAccount; @@ -28,27 +29,23 @@ public IActionResult Index() [HttpPost] public async Task Index(ChangePasswordViewModel model) { - if (!ModelState.IsValid) - { - return View(model); - } - - var currentPassword = model.CurrentPassword!; - var adminId = User.GetAdminId(); var delegateId = User.GetCandidateId(); - var verifiedLinkedUsersAccounts = - userService.GetVerifiedLinkedUsersAccounts(adminId, delegateId, currentPassword); + var verifiedLinkedUsersAccounts = string.IsNullOrEmpty(model.CurrentPassword) + ? new UserAccountSet() + : userService.GetVerifiedLinkedUsersAccounts(adminId, delegateId, model.CurrentPassword!); - var passwordIsInvalid = !verifiedLinkedUsersAccounts.Any(); - - if (passwordIsInvalid) + if (!verifiedLinkedUsersAccounts.Any()) { ModelState.AddModelError( - nameof(model.CurrentPassword), + nameof(ChangePasswordViewModel.CurrentPassword), CommonValidationErrorMessages.IncorrectPassword ); + } + + if (!ModelState.IsValid) + { return View(model); } diff --git a/DigitalLearningSolutions.Web/Controllers/MyAccountController.cs b/DigitalLearningSolutions.Web/Controllers/MyAccountController.cs index 528606cc11..2102941367 100644 --- a/DigitalLearningSolutions.Web/Controllers/MyAccountController.cs +++ b/DigitalLearningSolutions.Web/Controllers/MyAccountController.cs @@ -104,6 +104,11 @@ private IActionResult EditDetailsPostSave(EditDetailsViewModel model) "Preview your new profile picture before saving"); } + if (model.Password != null && !userService.IsPasswordValid(userAdminId, userDelegateId, model.Password)) + { + ModelState.AddModelError(nameof(EditDetailsViewModel.Password), CommonValidationErrorMessages.IncorrectPassword); + } + if (!ModelState.IsValid) { return View(model); @@ -118,11 +123,7 @@ private IActionResult EditDetailsPostSave(EditDetailsViewModel model) var (accountDetailsData, centreAnswersData) = MapToUpdateAccountData(model, userAdminId, userDelegateId); - if (!userService.TryUpdateUserAccountDetails(accountDetailsData, centreAnswersData)) - { - ModelState.AddModelError(nameof(EditDetailsViewModel.Password), CommonValidationErrorMessages.IncorrectPassword); - return View(model); - } + userService.UpdateUserAccountDetails(accountDetailsData, centreAnswersData); return RedirectToAction("Index"); }