From c5e0ef90c3f8aa88fb28bd289dd2358866ba7e07 Mon Sep 17 00:00:00 2001 From: Warren Buckley Date: Mon, 10 Feb 2020 14:27:54 +0000 Subject: [PATCH 1/4] Verify & check SecurityStamp --- src/Umbraco.Web/Editors/AuthenticationController.cs | 7 ++++--- src/Umbraco.Web/Editors/BackOfficeController.cs | 12 +++++++++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Web/Editors/AuthenticationController.cs b/src/Umbraco.Web/Editors/AuthenticationController.cs index c2c481e8e4..c7c08d1b42 100644 --- a/src/Umbraco.Web/Editors/AuthenticationController.cs +++ b/src/Umbraco.Web/Editors/AuthenticationController.cs @@ -301,7 +301,7 @@ namespace Umbraco.Web.Editors if (user != null) { var code = await UserManager.GeneratePasswordResetTokenAsync(identityUser.Id); - var callbackUrl = ConstructCallbackUrl(identityUser.Id, code); + var callbackUrl = ConstructCallbackUrl(identityUser.Id, code, identityUser.SecurityStamp.GenerateHash()); var message = Services.TextService.Localize("resetPasswordEmailCopyFormat", // Ensure the culture of the found user is used for the email! @@ -506,7 +506,7 @@ namespace Umbraco.Web.Editors return response; } - private string ConstructCallbackUrl(int userId, string code) + private string ConstructCallbackUrl(int userId, string code, string userSecurityStamp) { // Get an mvc helper to get the url var http = EnsureHttpContext(); @@ -516,7 +516,8 @@ namespace Umbraco.Web.Editors { area = GlobalSettings.GetUmbracoMvcArea(), u = userId, - r = code + r = code, + s = userSecurityStamp }); // Construct full URL using configured application URL (which will fall back to request) diff --git a/src/Umbraco.Web/Editors/BackOfficeController.cs b/src/Umbraco.Web/Editors/BackOfficeController.cs index e77a1b70f2..04c4627d85 100644 --- a/src/Umbraco.Web/Editors/BackOfficeController.cs +++ b/src/Umbraco.Web/Editors/BackOfficeController.cs @@ -296,11 +296,21 @@ namespace Umbraco.Web.Editors } [HttpGet] - public async Task ValidatePasswordResetCode([Bind(Prefix = "u")]int userId, [Bind(Prefix = "r")]string resetCode) + public async Task ValidatePasswordResetCode([Bind(Prefix = "u")]int userId, [Bind(Prefix = "r")]string resetCode, [Bind(Prefix = "s")]string stampHash) { var user = UserManager.FindById(userId); if (user != null) { + // Check security stamp that has been generated in forgotten password email link is the same we have stored for user + // ie the user has not been marked inactive or password changed by an admin etc + if(user.SecurityStamp.GenerateHash() != stampHash) + { + // Password, email or something changed to the user since the password reset email requested + // Add error and redirect for it to be displayed + TempData[ViewDataExtensions.TokenPasswordResetCode] = new[] { Services.TextService.Localize("login/resetCodeExpired") }; + return RedirectToLocal(Url.Action("Default", "BackOffice")); + } + var result = await UserManager.UserTokenProvider.ValidateAsync("ResetPassword", resetCode, UserManager, user); if (result) { From 6ae88f6c76373ca79939e28993572eb1c378d4f7 Mon Sep 17 00:00:00 2001 From: Warren Buckley Date: Tue, 11 Feb 2020 09:56:30 +0000 Subject: [PATCH 2/4] Revert "Verify & check SecurityStamp" This reverts commit c5e0ef90c3f8aa88fb28bd289dd2358866ba7e07. --- src/Umbraco.Web/Editors/AuthenticationController.cs | 7 +++---- src/Umbraco.Web/Editors/BackOfficeController.cs | 12 +----------- 2 files changed, 4 insertions(+), 15 deletions(-) diff --git a/src/Umbraco.Web/Editors/AuthenticationController.cs b/src/Umbraco.Web/Editors/AuthenticationController.cs index c7c08d1b42..c2c481e8e4 100644 --- a/src/Umbraco.Web/Editors/AuthenticationController.cs +++ b/src/Umbraco.Web/Editors/AuthenticationController.cs @@ -301,7 +301,7 @@ namespace Umbraco.Web.Editors if (user != null) { var code = await UserManager.GeneratePasswordResetTokenAsync(identityUser.Id); - var callbackUrl = ConstructCallbackUrl(identityUser.Id, code, identityUser.SecurityStamp.GenerateHash()); + var callbackUrl = ConstructCallbackUrl(identityUser.Id, code); var message = Services.TextService.Localize("resetPasswordEmailCopyFormat", // Ensure the culture of the found user is used for the email! @@ -506,7 +506,7 @@ namespace Umbraco.Web.Editors return response; } - private string ConstructCallbackUrl(int userId, string code, string userSecurityStamp) + private string ConstructCallbackUrl(int userId, string code) { // Get an mvc helper to get the url var http = EnsureHttpContext(); @@ -516,8 +516,7 @@ namespace Umbraco.Web.Editors { area = GlobalSettings.GetUmbracoMvcArea(), u = userId, - r = code, - s = userSecurityStamp + r = code }); // Construct full URL using configured application URL (which will fall back to request) diff --git a/src/Umbraco.Web/Editors/BackOfficeController.cs b/src/Umbraco.Web/Editors/BackOfficeController.cs index 04c4627d85..e77a1b70f2 100644 --- a/src/Umbraco.Web/Editors/BackOfficeController.cs +++ b/src/Umbraco.Web/Editors/BackOfficeController.cs @@ -296,21 +296,11 @@ namespace Umbraco.Web.Editors } [HttpGet] - public async Task ValidatePasswordResetCode([Bind(Prefix = "u")]int userId, [Bind(Prefix = "r")]string resetCode, [Bind(Prefix = "s")]string stampHash) + public async Task ValidatePasswordResetCode([Bind(Prefix = "u")]int userId, [Bind(Prefix = "r")]string resetCode) { var user = UserManager.FindById(userId); if (user != null) { - // Check security stamp that has been generated in forgotten password email link is the same we have stored for user - // ie the user has not been marked inactive or password changed by an admin etc - if(user.SecurityStamp.GenerateHash() != stampHash) - { - // Password, email or something changed to the user since the password reset email requested - // Add error and redirect for it to be displayed - TempData[ViewDataExtensions.TokenPasswordResetCode] = new[] { Services.TextService.Localize("login/resetCodeExpired") }; - return RedirectToLocal(Url.Action("Default", "BackOffice")); - } - var result = await UserManager.UserTokenProvider.ValidateAsync("ResetPassword", resetCode, UserManager, user); if (result) { From a7ff82f29a9fb41cba693d023b391ef17ae1e25a Mon Sep 17 00:00:00 2001 From: Warren Buckley Date: Tue, 11 Feb 2020 11:52:44 +0000 Subject: [PATCH 3/4] Implements Shans notes on changins security stamp for chaning email/username --- .../Repositories/Implement/UserRepository.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs index 96abc37662..3be5102b83 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/UserRepository.cs @@ -557,6 +557,16 @@ ORDER BY colName"; } } + // If userlogin or the email has changed then need to reset security stamp + if (changedCols.Contains("userLogin") || changedCols.Contains("userEmail")) + { + userDto.EmailConfirmedDate = null; + userDto.SecurityStampToken = entity.SecurityStamp = Guid.NewGuid().ToString(); + + changedCols.Add("emailConfirmedDate"); + changedCols.Add("securityStampToken"); + } + //only update the changed cols if (changedCols.Count > 0) { From e90e32b871b1d86269f47f7b60b159f8629a4773 Mon Sep 17 00:00:00 2001 From: Warren Buckley Date: Wed, 12 Feb 2020 10:11:28 +0000 Subject: [PATCH 4/4] Add unit test to verify SecurityStamp changes/invalidates when the userlogin changes --- .../Repositories/UserRepositoryTest.cs | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs b/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs index 3ba00e54cf..bbefb79f6b 100644 --- a/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs +++ b/src/Umbraco.Tests/Persistence/Repositories/UserRepositoryTest.cs @@ -421,6 +421,35 @@ namespace Umbraco.Tests.Persistence.Repositories } } + [Test] + public void Can_Invalidate_SecurityStamp_On_Username_Change() + { + // Arrange + var provider = TestObjects.GetScopeProvider(Logger); + using (var scope = provider.CreateScope()) + { + var repository = CreateRepository(provider); + var userGroupRepository = CreateUserGroupRepository(provider); + + var user = CreateAndCommitUserWithGroup(repository, userGroupRepository); + var originalSecurityStamp = user.SecurityStamp; + + // Ensure when user generated a security stamp is present + Assert.That(user.SecurityStamp, Is.Not.Null); + Assert.That(user.SecurityStamp, Is.Not.Empty); + + // Update username + user.Username = user.Username + "UPDATED"; + repository.Save(user); + + // Get the user + var updatedUser = repository.Get(user.Id); + + // Ensure the Security Stamp is invalidated & no longer the same + Assert.AreNotEqual(originalSecurityStamp, updatedUser.SecurityStamp); + } + } + private void AssertPropertyValues(IUser updatedItem, IUser originalUser) { Assert.That(updatedItem.Id, Is.EqualTo(originalUser.Id));