BypassTowFactor in SignInManager:ExternalLoginSignInAsync should default to False about identity HOT 25 CLOSED

There are two issues with this.
First, my app contains sensitive data, and it makes sense to use 2fa with that. Someone might have a different view on the sensitivity of their Facebook account, and not use 2fa with that. Now when you would leave the concern for 2fa enforcement to the external provider, the very fact that I have a Facebook login possibility on the site, a hacker can use the Facebook login to bypass the 2fa. In other words, if any site has social media logins, then the user needs to put 2fa on all those social media accounts to get the same protection level.

The second issue is that the code currently does not work that way.

var result = await _signInManager.ExternalLoginSignInAsync(info.LoginProvider, 
      info.ProviderKey, isPersistent: false, bypassTwoFactor: true);
if (result.Succeeded)
{
   _logger.LogInformation("{Name} logged in with {LoginProvider} provider.", 
      info.Principal.Identity.Name, info.LoginProvider);
   return LocalRedirect(returnUrl);
}

In the above code, result.Succeeded returns false if my app wants 2fa to be checked (cookie expired). So I think @wijnsema is correct to suggest that the code needs to check on result.RequiresTwoFactor.

from identity.

Exactly, that is how I implemented it in my UI customization

from identity.

If you are using external providers we expect the 2fa enforcement to be their concern, not that of your app.

from identity.

@blowdart
Maybe I don't understand, but the user is able to set userId and password and login. Than the user can set 2fa.
When login-in, the user is able to choose the use of their Microsoft account, Facebook, or twitter to login.
Are you saying that in that case, they should skip 2fa?

from identity.

2fa in that regard is for when they use the app specific username and password, not for when they authenticate via social logins.

from identity.

If you are using external providers we expect the 2fa enforcement to be their concern, not that of your app.

From a theoretical point of view, I agree with you. In practice, with social providers like Facebook and Twitter the 2fa from the app can be a welcome extra security layer.

Like mentioned before: As a developer using Identity I was surprised to see 2fa not working with external login. But I understand your explanation.

from identity.

Well what if someone added 2fa to facebook already? Do people really expect two 2fa prompts? I think 4fa might be an unwelcome shock :)

from identity.

from identity.

Maybe not a big problem.

1. They were probably logged in to Facebook earlier, and trusted their computer for a week or so.
2. 2fa might not be the same for all platforms

from identity.

The code bug is a bug, @HaoK can you look at that?

I'd respectfully disagree on the idea that "you expect 2FA to work for both local and external login", as that's not how it's designed, and to be honest it's not something I've seen in the wild, so this would be something the templates wouldn't support without you changing the flow as you have,

from identity.

from identity.

Yes, I accept its a scenario that would work for you, but templates are starter points which cover the common cases, hence saying it's not suitable for the templates themselves.

from identity.

from identity.

I suggest you change the template to:

var result = await _signInManager.ExternalLoginSignInAsync(info.LoginProvider, 
      info.ProviderKey, isPersistent: false, bypassTwoFactor: true);
if (result.Succeeded || result.RequiresTwoFactor)
{
   _logger.LogInformation("{Name} logged in with {LoginProvider} provider.", 
      info.Principal.Identity.Name, info.LoginProvider);
   return LocalRedirect(returnUrl);
}

Than nothing breaks

from identity.

Indeed it's a matter of opinion, I was merely stating my expectations.

Maybe this discussion is popping up in the future and then we can look at it with a little more statistics at hand.

For now I'm OK with this, good to see the bug is being fixed!

from identity.

@vankampenp be careful not overrate 2fa as 'prove' of a users identity. 2fa is just an extra layer. 2fa is as secure as the generated key used to set it up. If this key is leaked somehow (it is not stored encrypted in the default implementations) anybody with this key can bypass 2fa easily.

from identity.

True, if you really wanted proof you'd go for certificates, but even those aren't foolproof.

from identity.

@wijnsema thanks for the tip!. If someone gets the contents of my database, I have a whole other problem. In the end nothing is secure.

But it helps me to think that this extra layer prevents someone to guess a password, or reuse a password written on paper or something like that. At least the hacker will need access to the mobile phone as well (or my database, but than they are no longer interested in bypassing the 2fa).

from identity.

@blowdart what code bug are you specifically referring to in here, there's a lot of comments and its not clear to me what behavior is the bug

from identity.

The bit where people say there's a bug :D

var result = await _signInManager.ExternalLoginSignInAsync(info.LoginProvider, 
      info.ProviderKey, isPersistent: false, bypassTwoFactor: true);
if (result.Succeeded)
{
   _logger.LogInformation("{Name} logged in with {LoginProvider} provider.", 
      info.Principal.Identity.Name, info.LoginProvider);
   return LocalRedirect(returnUrl);
}

In the above code, result.Succeeded returns false if my app wants 2fa to be checked (cookie expired). So I think @wijnsema is correct to suggest that the code needs to check on result.RequiresTwoFactor.

from identity.

@HaoK I'm not sure this is actually a bug. Maybe @vankampenp can clarify.

From my perspective:
I wanted to change the behavior of the app to require 2fa for external login providers. I changed the ExternalLoginSignInAsync call to bypassTwoFactor: false. But then result.Succeeded is false while result.RequiresTwoFactor is true.

The code then tries to register a new user, which fails because the user already exists!

If you add

if (result.RequiresTwoFactor)
{
   return RedirectToPage("./LoginWith2fa", new { ReturnUrl = returnUrl });
}

just like in Login.cshtml.cs you are redirected to the 2fa page as expected.

If you have bypassTwoFactor = true these lines have no influence.

So yes, you could call this dead code, but it is very convenient for those who change the bypassTwoFactor while not harming those who leave it as is.

from identity.

Yeah that's what I thought, its not really a bug, if this code was still in the template, it makes more sense to change, but since users will need to scaffold and modify the code anyways, they can add that additional 2fa redirect logic after scaffolding, I'm not sure it really make sense for us to add dead code in the library.

@ajcvickers @blowdart thoughts?

from identity.

Taking one step back, it wouldn't be dead code if the default would be to have 2fa for external logins...

from identity.

@HaoK Even though the code is not in the template, given that we tell people to scaffold the code into their applications, it effectively becomes the same as code in the template. Therefore I think we should fix this. 3.0 is fine.

from identity.

Leaning towards this not being a bug, we don't want two two factor challenges. Scaffolding out and changing the code is the way to go.

from identity.