Invalidating Reset Password Emails

[warrenbuckley]

This PR will check if the security stamp has not been changed since the forgotten password email has been sent out.

Such as the email/username changing etc...

Testing Notes

See original notes on the Azure Board task/card 4828

[warrenbuckley]

@Shazwazza when testing & writing this PR against the test/repro notes on the Azure Board Card #AB4828

I was under the impression that SecurityStamp would be regenerated/invalidated when the user changes their username/email address from the call at sprint planning this morning?

Can you give me a pointer or two please mate 😄

[Shazwazza]

@warrenbuckley as it turns out the Security Stamp is already part of the underlying check. The code that does this check that you would have seen in the ValidatePasswordResetCode is await UserManager.UserTokenProvider.ValidateAsync. The object that is the UserTokenProvider is an instance of DataProtectorTokenProvider (which we create/set here https://github.com/umbraco/Umbraco-CMS/blob/v8/dev/src/Umbraco.Web/Security/BackOfficeUserManager.cs#L196). The code for this object is and method is here: https://github.com/aspnet/AspNetIdentity/blob/a24b776676f12cf7f0e13944783cf8e379b3ef70/src/Microsoft.AspNet.Identity.Owin/DataProtectorTokenProvider.cs#L96 and you can see at the bottom of the method it's already checking the security stamp, so the idea to combine the token with the security stamp is already implemented.

So based on your comment

I was under the impression that the SecurityStamp would be regenerated when the user changes their email/username.

... I'm assuming you've discovered that the security stamp is not being updated when the email changes? You haven't been clear about that. I would expect that if that was the case, then everything would 'just work' since the underlying logic is already validating the security stamp here.

I believe the underlying problem is how we edit user data since we are using both our IUserService - which directly updates data and bypasses the business logic associated with ASP.NET Identity and we are also using some ASP.NET Identity APIs for things like managing passwords. We will need to steamline this in a way. So to answer you question, yes the security stamp will be updated when changing a user's email when using ASP.Net Identity's APIs, see this method SetEmailAsync:
https://github.com/aspnet/AspNetIdentity/blob/master/src/Microsoft.AspNet.Identity.Core/UserManager.cs#L1116

But in UsersController.PostSaveUser we update the user directly with the IUserService and don't use any ASP.Net Identity APIs. That is ok though. what we really need to do is change update the logic within the IUserRepository.PersistUpdatedItem to have the same business logic as ASP.Net Identity and potentially also make sure we use ASP.Net Identity APIs when changing a user's email in the UsersController.PostSaveUser. You can see there's a couple logical operations that take place in ASP.Net Identity's SetEmailAsync method which we'll need to replicate in the UserRepository.PersisteUpdatedItem https://github.com/aspnet/AspNetIdentity/blob/master/src/Microsoft.AspNet.Identity.Core/UserManager.cs#L1104 , these are:

• Changing the email confirmed flag to false - effectively this changes the IUser.EmailConfirmedDate to null
• Updating the security stamp token

So in UserRepository.PersisteUpdatedItem we'll need to check if the email is changing and if so ensure that we perform the above logic.

With that done, everything should 'just work'

[Shazwazza]

Have tested and all good, i get the screen saying The link you have clicked on is invalid or has expired. All we need now is a unit test, should be pretty easy to write. Once that is there and passing you can merge.

Merged in now unit test passed & got @bergmania to give it the once over