From bb7c71a5be5b3d357572a3eabf573b566022d9d5 Mon Sep 17 00:00:00 2001 From: Dmytro Trofimov <3643368+trdmitry@users.noreply.github.com> Date: Fri, 27 May 2022 17:09:08 +0300 Subject: [PATCH 1/2] Password recovery (#313) - merge with template engine - AccountController tests added - fixed view if user not found by email - added new page with errors - fixed validation - user not need to enter email twice --- .../Controllers/AccountControllerTests.cs | 203 ++++++++++++++++++ .../Controllers/AccountController.cs | 43 ++-- .../Resources/SharedResource.en.resx | 11 +- .../Resources/SharedResource.uk.resx | 14 +- .../ForgotPasswordConfirmation.cshtml | 6 +- .../Account/Password/ResetPassword.cshtml | 4 +- .../Password/ResetPasswordFailed.cshtml | 16 ++ 7 files changed, 279 insertions(+), 18 deletions(-) create mode 100644 OutOfSchool/OutOfSchool.IdentityServer.Tests/Controllers/AccountControllerTests.cs create mode 100644 OutOfSchool/OutOfSchool.IdentityServer/Views/Account/Password/ResetPasswordFailed.cshtml diff --git a/OutOfSchool/OutOfSchool.IdentityServer.Tests/Controllers/AccountControllerTests.cs b/OutOfSchool/OutOfSchool.IdentityServer.Tests/Controllers/AccountControllerTests.cs new file mode 100644 index 0000000000..74222402e0 --- /dev/null +++ b/OutOfSchool/OutOfSchool.IdentityServer.Tests/Controllers/AccountControllerTests.cs @@ -0,0 +1,203 @@ +using IdentityServer4.Models; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Localization; +using Microsoft.Extensions.Logging; +using Moq; +using NUnit.Framework; +using OutOfSchool.EmailSender; +using OutOfSchool.IdentityServer.Controllers; +using OutOfSchool.IdentityServer.ViewModels; +using Microsoft.AspNetCore.Identity; +using OutOfSchool.Services.Models; +using System.Threading.Tasks; +using OutOfSchool.RazorTemplatesData.Services; + +namespace OutOfSchool.IdentityServer.Tests.Controllers +{ + public class AccountControllerTests + { + private AccountController accountController; + private readonly Mock fakeSignInManager; + private readonly Mock fakeUserManager; + private readonly Mock fakeEmailSender; + private readonly Mock> fakeLogger; + private readonly Mock> fakeLocalizer; + private readonly Mock fakeRazorViewToStringRenderer; + + public AccountControllerTests() + { + fakeSignInManager = new Mock(); + fakeUserManager = new Mock(); + fakeEmailSender = new Mock(); + fakeLogger = new Mock>(); + fakeLocalizer = new Mock>(); + fakeRazorViewToStringRenderer = new Mock(); + } + + [SetUp] + public void Setup() + { + fakeLocalizer + .Setup(localizer => localizer[It.IsAny()]) + .Returns(new LocalizedString("mock", "error")); + + accountController = new AccountController( + fakeSignInManager.Object, + fakeUserManager.Object, + fakeEmailSender.Object, + fakeLogger.Object, + fakeLocalizer.Object, + fakeRazorViewToStringRenderer.Object + ); + } + + [Test] + public void ForgotPassword_ReturnsViewResult() + { + // Arrange + var returnUrl = "Return url"; + + // Act + IActionResult result = accountController.ForgotPassword(returnUrl); + var forgotPasswordResultModel = (ForgotPasswordViewModel)((ViewResult)result).Model; + + // Assert + Assert.That(forgotPasswordResultModel.ReturnUrl, Is.EqualTo(returnUrl)); + Assert.IsInstanceOf(result); + } + + #region ResetPasswordTests + + [Test] + public async Task ResetPasswordGet_EmptyFields_ReturnsBadViewResult() + { + // Arrange + + // Act + IActionResult result = await accountController.ResetPassword(null, null); + + // Assert + Assert.IsInstanceOf(result); + } + + [Test] + public async Task ResetPasswordGet_EmailNotFound_ReturnsErrorMessage() + { + // Arrange + fakeUserManager.Setup(userManager => userManager.FindByEmailAsync(It.IsAny())) + .ReturnsAsync((User)null); + + // Act + IActionResult result = await accountController.ResetPassword("token", "email"); + string modelMessage = (LocalizedString)((ViewResult)result).Model; + + // Assert + Assert.AreEqual(modelMessage, "error"); + } + + [Test] + public async Task ResetPasswordGet_TokenInvalid_ReturnsErrorMessage() + { + // Arrange + fakeUserManager.Setup(userManager => userManager.FindByEmailAsync(It.IsAny())) + .ReturnsAsync(new User()); + + fakeUserManager.Setup(userManager => userManager.VerifyUserTokenAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(false); + + // Act + IActionResult result = await accountController.ResetPassword("token", "email"); + string modelMessage = (LocalizedString)((ViewResult)result).Model; + + // Assert + Assert.AreEqual(modelMessage, "error"); + } + + [Test] + public async Task ResetPasswordGet_TokenValid_ReturnsResetPasswordViewModel() + { + // Arrange + var token = "token"; + var email = "test@email.com"; + fakeUserManager.Setup(userManager => userManager.FindByEmailAsync(It.IsAny())) + .ReturnsAsync(new User()); + + fakeUserManager.Setup(userManager => userManager.VerifyUserTokenAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(true); + + // Act + IActionResult result = await accountController.ResetPassword(token, email); + ResetPasswordViewModel resetPasswordViewModel = (ResetPasswordViewModel)((ViewResult)result).Model; + + // Assert + Assert.AreEqual(resetPasswordViewModel.Token, token); + Assert.AreEqual(resetPasswordViewModel.Email, email); + } + + [Test] + public async Task ResetPasswordPost_InvalidModel_ReturnsResetPasswordViewModel() + { + // Arrange + var fakeErrorMessage = "Model is invalid"; + accountController.ModelState.AddModelError(string.Empty, fakeErrorMessage); + + // Act + IActionResult result = await accountController.ResetPassword(new ResetPasswordViewModel()); + ResetPasswordViewModel resetPasswordViewModelResult = (ResetPasswordViewModel)((ViewResult)result).Model; + + // Assert + Assert.IsInstanceOf(resetPasswordViewModelResult); + } + + [Test] + public async Task ResetPasswordPost_EmailNotFound_ReturnsResetPasswordViewModel() + { + // Arrange + fakeUserManager.Setup(userManager => userManager.FindByEmailAsync(It.IsAny())) + .ReturnsAsync((User)null); + + // Act + IActionResult result = await accountController.ResetPassword(new ResetPasswordViewModel()); + string modelMessage = (LocalizedString)((ViewResult)result).Model; + + // Assert + Assert.AreEqual(modelMessage, "error"); + } + + [Test] + public async Task ResetPasswordPost_Success_ReturnsResetPasswordConfirmation() + { + // Arrange + fakeUserManager.Setup(userManager => userManager.FindByEmailAsync(It.IsAny())) + .ReturnsAsync(new User()); + fakeUserManager.Setup(userManager => userManager.ResetPasswordAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(IdentityResult.Success); + + // Act + IActionResult result = await accountController.ResetPassword(new ResetPasswordViewModel()); + ViewResult viewResult = (ViewResult)result; + + // Assert + Assert.AreEqual(viewResult.ViewName, "Password/ResetPasswordConfirmation"); + } + + [Test] + public async Task ResetPasswordPost_Failed_ReturnsResetPasswordConfirmation() + { + // Arrange + fakeUserManager.Setup(userManager => userManager.FindByEmailAsync(It.IsAny())) + .ReturnsAsync(new User()); + fakeUserManager.Setup(userManager => userManager.ResetPasswordAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(IdentityResult.Failed(null)); + + // Act + IActionResult result = await accountController.ResetPassword(new ResetPasswordViewModel()); + ViewResult viewResult = (ViewResult)result; + + // Assert + Assert.AreEqual(viewResult.ViewName, "Password/ResetPasswordFailed"); + } + #endregion + + } +} diff --git a/OutOfSchool/OutOfSchool.IdentityServer/Controllers/AccountController.cs b/OutOfSchool/OutOfSchool.IdentityServer/Controllers/AccountController.cs index daacb3c932..89febf9b7d 100644 --- a/OutOfSchool/OutOfSchool.IdentityServer/Controllers/AccountController.cs +++ b/OutOfSchool/OutOfSchool.IdentityServer/Controllers/AccountController.cs @@ -1,4 +1,5 @@ -using System.Linq; +using System; +using System.Linq; using System.Text.Encodings.Web; using System.Threading.Tasks; using Microsoft.AspNetCore.Authorization; @@ -40,6 +41,7 @@ public AccountController( this.userManager = userManager; this.emailSender = emailSender; this.logger = logger; + this.localizer = localizer; this.renderer = renderer; this.localizer = localizer; } @@ -263,7 +265,7 @@ public async Task ForgotPassword(ForgotPasswordViewModel model) } var token = await userManager.GeneratePasswordResetTokenAsync(user); - var callBackUrl = Url.Action("ResetPassword", "Account", new { token }, Request.Scheme); + var callBackUrl = Url.Action("ResetPassword", "Account", new { token, user.Email }, Request.Scheme); var email = model.Email; var subject = "Reset Password"; @@ -271,7 +273,7 @@ public async Task ForgotPassword(ForgotPasswordViewModel model) { FirstName = user.FirstName, LastName = user.LastName, - ActionUrl = HtmlEncoder.Default.Encode(callBackUrl), + ActionUrl = callBackUrl, }; var htmlMessage = await renderer.GetHtmlStringAsync(RazorTemplates.ResetPassword, userActionViewModel); @@ -284,17 +286,35 @@ public async Task ForgotPassword(ForgotPasswordViewModel model) } [HttpGet] - public IActionResult ResetPassword(string token = null) + public async Task ResetPassword(string token = null, string email = null) { logger.LogDebug($"{path} started. User(id): {userId}"); - if (token == null) + if (string.IsNullOrWhiteSpace(token) || string.IsNullOrWhiteSpace(email)) + { + logger.LogError($"{path} Token or email was not supplied for reset password. User(id): {userId}"); + return BadRequest("A token and email must be supplied for password reset."); + } + + var user = await userManager.FindByEmailAsync(email); + if (user == null) + { + logger.LogError($"{path} User not found. Email: {email}"); + + // If message will be "user not found", someone can use this url to check registered emails. I decide to show "invalid token" + return View("Password/ResetPasswordFailed", localizer["Change password invalid token"]); + } + + var purpose = UserManager.ResetPasswordTokenPurpose; + var checkToken = await userManager.VerifyUserTokenAsync(user, userManager.Options.Tokens.PasswordResetTokenProvider, purpose, token); + if (!checkToken) { - logger.LogError($"{path} Token was not supplied for reset password. User(id): {userId}"); - return BadRequest("A token must be supplied for password reset."); + logger.LogError($"{path} Token is not valid for user: {user.Id}"); + + return View("Password/ResetPasswordFailed", localizer["Change password invalid token"]); } - return View("Password/ResetPassword", new ResetPasswordViewModel() { Token = token }); + return View("Password/ResetPassword", new ResetPasswordViewModel() { Token = token, Email = email }); } [HttpPost] @@ -306,7 +326,7 @@ public async Task ResetPassword(ResetPasswordViewModel model) { logger.LogError($"{path} Input data was not valid. User(id): {userId}"); - return BadRequest(ModelState); + return View("Password/ResetPassword", new ResetPasswordViewModel()); } var user = await userManager.FindByEmailAsync(model.Email); @@ -314,7 +334,7 @@ public async Task ResetPassword(ResetPasswordViewModel model) { logger.LogError($"{path} User with Email:{model.Email} was not found. User(id): {userId}"); - return View("Password/ResetPasswordConfirmation"); + return View("Password/ResetPasswordFailed", localizer["Change password failed"]); } var result = await userManager.ResetPasswordAsync(user, model.Token, model.Password); @@ -328,8 +348,7 @@ public async Task ResetPassword(ResetPasswordViewModel model) logger.LogError($"{path} Reset password was failed. User(id): {userId}. " + $"{string.Join(System.Environment.NewLine, result.Errors.Select(e => e.Description))}"); - // TODO: In my opinion we shouldn't return Ok in this cause. - return Ok(); + return View("Password/ResetPasswordFailed", localizer["Change password failed"]); } [HttpGet] diff --git a/OutOfSchool/OutOfSchool.IdentityServer/Resources/SharedResource.en.resx b/OutOfSchool/OutOfSchool.IdentityServer/Resources/SharedResource.en.resx index 71607ddf94..c81a7a5659 100644 --- a/OutOfSchool/OutOfSchool.IdentityServer/Resources/SharedResource.en.resx +++ b/OutOfSchool/OutOfSchool.IdentityServer/Resources/SharedResource.en.resx @@ -327,4 +327,13 @@ Email {0} dont match with current email - \ No newline at end of file + + Confirm password change + + + Token is not valid + + + Change password failed. Please contact administrator. + + diff --git a/OutOfSchool/OutOfSchool.IdentityServer/Resources/SharedResource.uk.resx b/OutOfSchool/OutOfSchool.IdentityServer/Resources/SharedResource.uk.resx index 686d3098f5..ea05321c87 100644 --- a/OutOfSchool/OutOfSchool.IdentityServer/Resources/SharedResource.uk.resx +++ b/OutOfSchool/OutOfSchool.IdentityServer/Resources/SharedResource.uk.resx @@ -242,7 +242,8 @@ Введіть ваш емейл на який буде надіслано інструкцію для скидання пароля - Перевірте вашу пошту для отримання інструкцій + Інструкція з відновлення паролю була надіслана на ваш емейл. +Зайдіть на емейл і виконайте дії вказані в листі. Підтвердження зміни паролю @@ -332,4 +333,13 @@ Login: {0} Вказаний email {0} не співпадеє з поточним - \ No newline at end of file + + Підтвердіть зміну паролю + + + Token не пройшов перевірку + + + Помилка при зміні паролю. Зверниться до адміністратора. + + diff --git a/OutOfSchool/OutOfSchool.IdentityServer/Views/Account/Password/ForgotPasswordConfirmation.cshtml b/OutOfSchool/OutOfSchool.IdentityServer/Views/Account/Password/ForgotPasswordConfirmation.cshtml index 8063ccfd87..2d35ee9462 100644 --- a/OutOfSchool/OutOfSchool.IdentityServer/Views/Account/Password/ForgotPasswordConfirmation.cshtml +++ b/OutOfSchool/OutOfSchool.IdentityServer/Views/Account/Password/ForgotPasswordConfirmation.cshtml @@ -13,5 +13,9 @@

@SharedLocalizer["Reset password confirmation"]

@SharedLocalizer["Check your email for instructions"]

+ +
+ Згадали пароль? @SharedLocalizer["Sign In"] +
- \ No newline at end of file + diff --git a/OutOfSchool/OutOfSchool.IdentityServer/Views/Account/Password/ResetPassword.cshtml b/OutOfSchool/OutOfSchool.IdentityServer/Views/Account/Password/ResetPassword.cshtml index e707929aca..9fefa4c8fe 100644 --- a/OutOfSchool/OutOfSchool.IdentityServer/Views/Account/Password/ResetPassword.cshtml +++ b/OutOfSchool/OutOfSchool.IdentityServer/Views/Account/Password/ResetPassword.cshtml @@ -22,7 +22,7 @@
- +
@@ -38,7 +38,7 @@
- +
diff --git a/OutOfSchool/OutOfSchool.IdentityServer/Views/Account/Password/ResetPasswordFailed.cshtml b/OutOfSchool/OutOfSchool.IdentityServer/Views/Account/Password/ResetPasswordFailed.cshtml new file mode 100644 index 0000000000..c06a505079 --- /dev/null +++ b/OutOfSchool/OutOfSchool.IdentityServer/Views/Account/Password/ResetPasswordFailed.cshtml @@ -0,0 +1,16 @@ +@using Microsoft.AspNetCore.Mvc.Localization +@using OutOfSchool.IdentityServer + +@inject IHtmlLocalizer SharedLocalizer + +@{ + ViewData["Title"] = SharedLocalizer["Reset password confirmation"]; + Layout = "_Layout"; +} +
+
+

@SharedLocalizer["Reset password"]

+ +

@Model

+
+
\ No newline at end of file From c5df891a34139cb9089177c35e7b913ce6e079ef Mon Sep 17 00:00:00 2001 From: Dmytro Trofimov <3643368+trdmitry@users.noreply.github.com> Date: Fri, 27 May 2022 17:30:49 +0300 Subject: [PATCH 2/2] Fixed sonar code smells --- .../Controllers/AccountControllerTests.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/OutOfSchool/OutOfSchool.IdentityServer.Tests/Controllers/AccountControllerTests.cs b/OutOfSchool/OutOfSchool.IdentityServer.Tests/Controllers/AccountControllerTests.cs index 74222402e0..b4513fed1c 100644 --- a/OutOfSchool/OutOfSchool.IdentityServer.Tests/Controllers/AccountControllerTests.cs +++ b/OutOfSchool/OutOfSchool.IdentityServer.Tests/Controllers/AccountControllerTests.cs @@ -92,7 +92,7 @@ public async Task ResetPasswordGet_EmailNotFound_ReturnsErrorMessage() string modelMessage = (LocalizedString)((ViewResult)result).Model; // Assert - Assert.AreEqual(modelMessage, "error"); + Assert.AreEqual("error", modelMessage); } [Test] @@ -110,7 +110,7 @@ public async Task ResetPasswordGet_TokenInvalid_ReturnsErrorMessage() string modelMessage = (LocalizedString)((ViewResult)result).Model; // Assert - Assert.AreEqual(modelMessage, "error"); + Assert.AreEqual("error", modelMessage); } [Test] @@ -161,7 +161,7 @@ public async Task ResetPasswordPost_EmailNotFound_ReturnsResetPasswordViewModel( string modelMessage = (LocalizedString)((ViewResult)result).Model; // Assert - Assert.AreEqual(modelMessage, "error"); + Assert.AreEqual("error", modelMessage); } [Test] @@ -178,7 +178,7 @@ public async Task ResetPasswordPost_Success_ReturnsResetPasswordConfirmation() ViewResult viewResult = (ViewResult)result; // Assert - Assert.AreEqual(viewResult.ViewName, "Password/ResetPasswordConfirmation"); + Assert.AreEqual("Password/ResetPasswordConfirmation", viewResult.ViewName); } [Test] @@ -195,7 +195,7 @@ public async Task ResetPasswordPost_Failed_ReturnsResetPasswordConfirmation() ViewResult viewResult = (ViewResult)result; // Assert - Assert.AreEqual(viewResult.ViewName, "Password/ResetPasswordFailed"); + Assert.AreEqual("Password/ResetPasswordFailed", viewResult.ViewName); } #endregion