We have moved to GitHub Issues
Created by Stephen Roberts 01 Nov 2017, 13:31:53 Updated by Harvey Williams 14 Nov 2017, 17:01:42Tags: Unscheduled
I have been following @Shannon's 2FA example at https://gist.github.com/Shazwazza/2fbbbe6567a2b0509f5215af8ba9ab37 My implementation is stock, except i have some database logic to store some custom data and controllers to support he setup of 2FA etc
I'm having an issue where all users are validated against the default admin user. i.e. Admin user (id:0) is perfectly fine, but any other users fail to signin, unless they use the admin users 2FA token!
What i've found In [AuthenticationController.cs|https://github.com/umbraco/Umbraco-CMS/blob/e0025db56d52b770d2b3aedbd48a3b804fd15ef0/src/Umbraco.Web/Editors/AuthenticationController.cs#L368] the local value userName is correct, and is referring to the correct user. However the call to actually do the signin SignInManager.TwoFactorSignInAsync doesnt pass in the user to validate, this comes from elsewhere.
Debugging, From the above method call (TwoFactorSignInAsync) it jumps through external code to FindByIdAsync in the BackOfficeUserStore with a parameter value for userId of 0, then it calls the correct provider implmenting DataProtectorTokenProvider.ValidateAsync with a user model of BackOfficeIdentityUser which is for the default admin user 0 and not the correct user
What i think is happening is in TwoFactorSignInAsync (which is within the [SignInManager|https://aspnetidentity.codeplex.com/SourceControl/latest#src/Microsoft.AspNet.Identity.Owin/SignInManager.cs] it is calling GetVerifiedUserIdAsync which is failing and returning a default id of 0, (if you look at the code, the default(TKey) is suspect!
This is on the latest version and at least 7.6.10
Indeed, I have reproduced the problem.
Problem is in
Umbraco.Web.Editors.AuthenticationController, we're asking for:
var result = await SignInManager.TwoFactorSignInAsync(model.Provider, model.Code, isPersistent: true, rememberBrowser: false);
TwoFactorSignInAsync determines on it's own what the User Id should be. That isn't quite compatible with our own methods of determining the user Id.
TwoFactorSignInAsync method in core should help. Testing that now.
That was exactly it, great find @steroberts89 !
Ah this is truly annoying. It's all to do with the Identity code not making things flexible and not making
GetVerifiedUserIdAsync() overridable, instead we are shadowing it but when that happens the base class isn't calling our shadowed method (because it's not overridden) and unfortunately the SignInManager is not an interface so it can't be overridden by explicit implementation either.
I guess the way we should have implemented TKey would have been
Nullable<int> instead of just
int and unfortunately we can just change the BackOfficeUserManager to inherit from
SignInManager<BackOfficeIdentityUser, int> since that would be a breaking change.
If only the admin Id wasn't
There will be other issues with this however because there's other methods that we we need to override that suffer from the same problem, basically anything that uses
GetVerifiedUserIdAsync and there is one method we can't override which is HasBeenVerifiedAsync that calls this method. I'll have a think about this.
I've added another commit: 95f632e1eac742a053c08d597c224d2d22d59ed3 which overrides SendTwoFactorCodeAsync which would suffer from the same problem and also updates the code to check for -1 instead of null (since it will never be null). There's unfortunately nothing we can do about the
HasBeenVerifiedAsync method unless we change the BackOfficeSignInManager signature. I don't think this will affect anything now since we don't use this API anywhere but maybe in the future it might.
I have merged. @sebastiaan let me know if you have any questions about my changes.
This fix (on 7.6.12) has fixed the issue on the Umbraco 2FA package. Thanks for the quick turn around!
Backwards Compatible: True
Affected versions: 7.6.10, 7.6.11
Due in version: 7.7.5, 7.6.12
Sprint: Sprint 71