dotnet / dotnet/aspnetcore

Consider honoring two-factor on external-login sign-in in the default Identity UI (currently bypassed for 2FA-enabled accounts)

Open
#68,497 2 comments 0 reactions 1 assignee Claimed by @GrantTotinov View on GitHub
area-identity feature-identity-ui
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 6h
Merged PRs (30d)
290

Description

## Summary

The default scaffolded ASP.NET Core Identity UI completes an external-provider (OAuth/OIDC) sign-in with two-factor authentication bypassed **unconditionally**, even for accounts that have 2FA enabled. This breaks the invariant that a 2FA-enabled account is challenged for a second factor on **every** interactive sign-in path. The external-login path is currently an exception to the password path, which does honor 2FA.

## What is wrong

* The external-login callback signs the user in via `SignInManager.ExternalLoginSignInAsync(..., bypassTwoFactor: true)`, so the second factor is skipped regardless of whether the account has 2FA enabled.
* The same callback has no `result.RequiresTwoFactor` handling, so even with `bypassTwoFactor: false` the flow would not route to the two-factor page - it would fall through to the account-confirmation path.
* Net property break: `2FA enabled => a second factor is required to complete an interactive sign-in` holds on the password sign-in path but **not** on the external-login sign-in path. Two paths to the same account apply different second-factor policy.

## Why it matters (defense in depth)

* **Correctness / consistency, independent of any attacker:** a user who enables 2FA reasonably expects it to apply to all interactive sign-ins to their account. Today one sign-in path silently does not enforce it, so the account''s effective second-factor policy depends on which credential type is used.
* **Hardening:** enforcing the second factor uniformly across credential types strengthens the second-factor boundary and reduces an account''s reliance on a single external IdP''s assurance level. It aligns the external-login path with the password path and with the related account-management hardening tracked in #66865.

## This should be explicitly considered (not silently kept as-is)

This is a deliberate design decision that **shall be considered and decided on the record**, because the current default silently diverges from the common user expectation that "2FA on" applies everywhere. Even if the resolution is to keep bypass as the default, the choice should be intentional and documented rather than incidental. The pros/cons below are provided to support that decision.

### Pros of honoring 2FA on external-login sign-in

* Makes second-factor enforcement **uniform** across password and external-login paths (least surprise for users who enabled 2FA).
* Strengthens the second-factor boundary so an account is not reducible to a single external IdP''s assurance.
* Low implementation cost: the core plumbing already carries the external `loginProvider` through the two-factor round-trip; only the UI callback needs a branch that already exists on the password page.
* Consistent with the direction of the analogous account-management hardening in #66865.

### Cons / trade-offs

* **UX change:** users with local 2FA enabled would be prompted for their second factor even when signing in through an external IdP - which some deployments consider redundant when the IdP already performed MFA.
* Could surprise "external-only" accounts if applied unconditionally (mitigated by gating on `GetTwoFactorEnabledAsync`, so only accounts that opted into local 2FA are affected).
* Behavior change to a default template; scaffolded/overridden copies in existing apps will not pick it up automatically (developer-owned source), so it needs a documentation call-out.
* For servicing branches, a default-on change may be considered too behavior-changing; may warrant an opt-in option or major/minor-only rollout.

## Affected code

* src/Identity/UI/src/Areas/Identity/Pages/V5/Account/ExternalLogin.cshtml.cs:141 - external sign-in call passes `bypassTwoFactor: true`
* src/Identity/UI/src/Areas/Identity/Pages/V5/Account/ExternalLogin.cshtml.cs:142-153 - result handling has `Succeeded` and `IsLockedOut` branches but no `RequiresTwoFactor` branch (insertion point after line 149)
* src/Identity/UI/src/Areas/Identity/Pages/V4/Account/ExternalLogin.cshtml.cs:141 - V4 mirror, identical behavior and line numbers
* src/Identity/UI/src/Areas/Identity/Pages/V5/Account/Login.cshtml.cs:136-139 - the `RequiresTwoFactor` branch to mirror
* src/Identity/Core/src/SignInManager.cs:1041 - `ExternalLoginSignInAsync` overload already forwards a caller-supplied `bypassTwoFactor`
* src/Identity/Core/src/SignInManager.cs:1177,1262,839 - two-factor round-trip already preserves the external `loginProvider` (StoreTwoFactorInfo / RetrieveTwoFactorInfoAsync / DoTwoFactorSignInAsync)
* src/Identity/UI/src/Areas/Identity/Pages/V5/Account/LoginWith2fa.cshtml.cs:96,118 - pending-user retrieval already supports external logins

## Recommended fix

* **Selected approach:** In the external-login callback, do not bypass the second factor for accounts that have 2FA enabled - call `ExternalLoginSignInAsync` with `bypassTwoFactor` gated on `GetTwoFactorEnabledAsync(user)` (or simply `false`), and add the missing `RequiresTwoFactor` branch that routes to the existing two-factor page, mirroring the password page:

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

The core already preserves the external `loginProvider` across the round-trip, so no framework/API change is required - the fix is UI-only in both V4 and V5 `ExternalLogin.cshtml.cs`.

* **Alternatives considered:**
* *Unconditional `bypassTwoFactor: false`* - simplest, but challenges 2FA even for accounts that intentionally treat the external IdP as their sole factor. Gating on `GetTwoFactorEnabledAsync` avoids surprising external-only accounts.
* *Make it a configurable option* - more flexible and servicing-friendly, but adds surface; can be layered on top of the conditional default.
* *Leave as-is (rely on the external IdP''s own MFA)* - status quo; acceptable for some deployments but inconsistent with local 2FA policy and with the password path.

* **Compatibility / migration / versioning:** UX behavior change (accounts with local 2FA get prompted for a second factor on external sign-in), not a breaking API change. Gating on `GetTwoFactorEnabledAsync` minimizes impact. Applies to both V4 and V5 scaffolded pages; existing scaffolded/overridden copies will not pick it up automatically (developer-owned source) - document this. A default-on change fits a major/minor release; consider an opt-in flag for servicing branches.

## Acceptance criteria

* [ ] An account with 2FA enabled that signs in via an external provider is routed through the two-factor page and must supply the second factor before authentication completes.
* [ ] An account without 2FA enabled signs in via an external provider unchanged.
* [ ] The completed sign-in remains the external login (external `loginProvider` preserved) and the external sign-in cookie is cleaned up after the two-factor step.
* [ ] Tests lock the property for both the 2FA-enabled and 2FA-disabled paths, for V4 and V5.
* [ ] The behavior change and its scaffold-override implications are documented.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.