U4-10620 - Umbraco 2FA implementation issues

Created by Stephen Roberts 01 Nov 2017, 13:31:53 Updated by Harvey Williams 14 Nov 2017, 17:01:42

Tags: 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

Thanks Stephen

Comments

Sebastiaan Janssen 07 Nov 2017, 14:31:09

Indeed, I have reproduced the problem.


Sebastiaan Janssen 07 Nov 2017, 15:22:56

Problem is in Umbraco.Web.Editors.AuthenticationController, we're asking for:

var result = await SignInManager.TwoFactorSignInAsync(model.Provider, model.Code, isPersistent: true, rememberBrowser: false);

Where 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.

Implementing the TwoFactorSignInAsync method in core should help. Testing that now.


Sebastiaan Janssen 07 Nov 2017, 15:53:30

That was exactly it, great find @steroberts89 !

PR: https://github.com/umbraco/Umbraco-CMS/pull/2313


Shannon Deminick 08 Nov 2017, 06:06:14

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 0 :)

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.


Shannon Deminick 08 Nov 2017, 06:18:16

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.


Harvey Williams 14 Nov 2017, 17:01:42

This fix (on 7.6.12) has fixed the issue on the Umbraco 2FA package. Thanks for the quick turn around!


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.6.10, 7.6.11

Due in version: 7.7.5, 7.6.12

Sprint: Sprint 71

Story Points:

Cycle: