Skip to content

HEEDLS-350 Validate password if not null before ModelState validation #469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 8, 2021
Merged

HEEDLS-350 Validate password if not null before ModelState validation #469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6147055
HEEDLS-350 Validate password if not null before ModelState validation
AlexJacksonDS Jul 5, 2021
e4d274c
HEEDLS-350 Refactor ChangePasswordController and unit tests
AlexJacksonDS Jul 7, 2021
5eb7f9c
HEEDLS-350 Add empty UserAccountSet constructor
AlexJacksonDS Jul 8, 2021
47736d8
HEEDLS-350 Fix incorrectly refactored test
AlexJacksonDS Jul 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions DigitalLearningSolutions.Data.Tests/Models/User/UserAccountSetTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<DelegateUser>();

// When
var result = new UserAccountSet(adminUser, delegates).Any();
var result = new UserAccountSet().Any();

// Then
result.Should().BeFalse();
Expand Down
15 changes: 5 additions & 10 deletions DigitalLearningSolutions.Data.Tests/Services/UserServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>._, A<string>._, A<string>._, null, A<int>._))
.MustHaveHappened();
A.CallTo(() => userDataService.UpdateDelegateUsers(A<string>._, A<string>._, A<string>._, null, A<int[]>._))
Expand Down Expand Up @@ -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<string>._, A<string>._, A<string>._, null, A<int[]>._))
.MustHaveHappened();
A.CallTo(() => userDataService.UpdateAdminUser(A<string>._, A<string>._, A<string>._, null, A<int>._))
Expand Down Expand Up @@ -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<string>._, A<string>._, A<string>._, null, A<int[]>._))
.MustHaveHappened();
A.CallTo(() => userDataService.UpdateAdminUser(A<string>._, A<string>._, A<string>._, null, A<int>._))
Expand Down Expand Up @@ -321,13 +317,12 @@ public void TryUpdateUsers_with_incorrect_password_doesnt_update()
A.CallTo(() => userDataService.GetDelegateUsersByEmailAddress(signedInEmail))
.Returns(new List<DelegateUser>());
A.CallTo(() => loginService.VerifyUsers(password, A<AdminUser>._, A<List<DelegateUser>>._))
.Returns(new UserAccountSet(null, new List<DelegateUser>()));
.Returns(new UserAccountSet());

// When
var result = userService.TryUpdateUserAccountDetails(accountDetailsData, centreAnswersData);
userService.UpdateUserAccountDetails(accountDetailsData, centreAnswersData);

// Then
result.Should().BeFalse();
A.CallTo(() => userDataService.UpdateDelegateUsers(A<string>._, A<string>._, A<string>._, null, A<int[]>._))
.MustNotHaveHappened();
A.CallTo(() => userDataService.UpdateAdminUser(A<string>._, A<string>._, A<string>._, null, A<int>._))
Expand Down
2 changes: 2 additions & 0 deletions DigitalLearningSolutions.Data/Models/User/UserAccountSet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ public class UserAccountSet
public readonly AdminUser? AdminAccount;
public readonly List<DelegateUser> DelegateAccounts;

public UserAccountSet() : this(null, null) { }

public UserAccountSet(AdminUser? adminAccount, IEnumerable<DelegateUser?>? delegateAccounts)
{
AdminAccount = adminAccount;
Expand Down
20 changes: 11 additions & 9 deletions DigitalLearningSolutions.Data/Services/UserService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,16 @@ List<DelegateUser> delegateUsers

public List<CentreUserDetails> GetUserCentres(AdminUser? adminUser, List<DelegateUser> delegateUsers);

public bool TryUpdateUserAccountDetails(
public void UpdateUserAccountDetails(
AccountDetailsData accountDetailsData,
CentreAnswersData? centreAnswersData = null
);

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
Expand Down Expand Up @@ -103,7 +105,7 @@ public List<CentreUserDetails> GetUserCentres(AdminUser? adminUser, List<Delegat
return availableCentres.OrderByDescending(ac => ac.IsAdmin).ThenBy(ac => ac.CentreName).ToList();
}

public bool TryUpdateUserAccountDetails(
public void UpdateUserAccountDetails(
AccountDetailsData accountDetailsData,
CentreAnswersData? centreAnswersData = null
)
Expand All @@ -115,11 +117,6 @@ public bool TryUpdateUserAccountDetails(
accountDetailsData.Password
);

if (verifiedAdminUser == null && verifiedDelegateUsers.Count == 0)
{
return false;
}

if (verifiedAdminUser != null)
{
userDataService.UpdateAdminUser(
Expand Down Expand Up @@ -156,8 +153,6 @@ public bool TryUpdateUserAccountDetails(
);
}
}

return true;
}

public bool NewEmailAddressIsValid(string emailAddress, int? adminUserId, int? delegateUserId, int centreId)
Expand Down Expand Up @@ -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();
}
}
}
18 changes: 12 additions & 6 deletions DigitalLearningSolutions.Web.Tests/Controllers/ChangePasswordControllerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public void SetUp()
passwordService = A.Fake<IPasswordService>();
authenticatedController = new ChangePasswordController(passwordService, userService)
.WithDefaultContext()
.WithMockUser(isAuthenticated: true, adminId: LoggedInAdminId, delegateId: LoggedInDelegateId);
.WithMockUser(true, adminId: LoggedInAdminId, delegateId: LoggedInDelegateId);
}

[Test]
Expand Down Expand Up @@ -76,13 +76,15 @@ public async Task Post_returns_success_page_if_model_and_password_valid()
{
// Given
var user = Builder<AdminUser>.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"
}
);

Expand All @@ -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"
}
);

Expand All @@ -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"
}
);

Expand All @@ -152,7 +158,7 @@ private void GivenPasswordVerificationReturnsUsers(UserAccountSet users, string
private void GivenPasswordVerificationFails()
{
A.CallTo(() => userService.GetVerifiedLinkedUsersAccounts(A<int>._, A<int>._, A<string>._))
.Returns(new UserAccountSet(null, new List<DelegateUser>()));
.Returns(new UserAccountSet());
}

private void ThenMustHaveChangedPasswordForUsersOnce(string newPassword, IEnumerable<User> expectedUsers)
Expand Down
2 changes: 1 addition & 1 deletion DigitalLearningSolutions.Web.Tests/Controllers/Login/LoginControllerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ public async Task Bad_password_should_render_basic_form_with_error()
A.CallTo(() => userService.GetUsersByUsername(A<string>._))
.Returns((UserTestHelper.GetDefaultAdminUser(), new List<DelegateUser>()));
A.CallTo(() => loginService.VerifyUsers(A<string>._, A<AdminUser>._, A<List<DelegateUser>>._))
.Returns(new UserAccountSet(null, new List<DelegateUser>()));
.Returns(new UserAccountSet());

// When
var result = await controller.Index(LoginTestHelper.GetDefaultLoginViewModel());
Expand Down
Loading