Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support account lockout with identity endpoints #47232

Closed
Tracked by #47288
halter73 opened this issue Mar 15, 2023 · 2 comments · Fixed by #49498
Closed
Tracked by #47288

Support account lockout with identity endpoints #47232

halter73 opened this issue Mar 15, 2023 · 2 comments · Fixed by #49498
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-identity Includes: Identity and providers enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-token-identity Priority:0 Work that we can't release without
Milestone

Comments

@halter73
Copy link
Member

halter73 commented Mar 15, 2023

Background and Motivation

Add alternative authentication scheme support a number of SignInManager APIs that support features such as 2fa and lockout.

Proposed API

// Microsoft.AspNetCore.Identity.dll

namespace namespace Microsoft.AspNetCore.Identity;

public class SignInManager<TUser> where TUser : class
{
+    public string PrimaryAuthenticationScheme { get; set; } = IdentityConstants.ApplicationScheme;
}

Usage Examples

routeGroup.MapPost("/login", async Task<Results<Ok<AccessTokenResponse>, ProblemHttpResult, IResult>>
    ([FromBody] LoginRequest login, [FromQuery] bool? cookieMode, [FromQuery] bool? persistCookies, [FromServices] IServiceProvider sp) =>
{
    var signInManager = sp.GetRequiredService<SignInManager<TUser>>();

    signInManager.PrimaryAuthenticationScheme = cookieMode == true ? IdentityConstants.ApplicationScheme : IdentityConstants.BearerScheme;
    var isPersistent = persistCookies ?? true;

    var result = await signInManager.PasswordSignInAsync(login.Username, login.Password, isPersistent, lockoutOnFailure: true);

    if (result.RequiresTwoFactor)
    {
        if (!string.IsNullOrEmpty(login.TwoFactorCode))
        {
            result = await signInManager.TwoFactorAuthenticatorSignInAsync(login.TwoFactorCode, isPersistent, rememberClient: isPersistent);
        }
        else if (!string.IsNullOrEmpty(login.TwoFactorRecoveryCode))
        {
            result = await signInManager.TwoFactorRecoveryCodeSignInAsync(login.TwoFactorRecoveryCode);
        }
    }

    if (result.Succeeded)
    {
        // The signInManager already produced the needed response in the form of a cookie or bearer token.
        return TypedResults.Empty;
    }

    return TypedResults.Problem(result.ToString(), statusCode: StatusCodes.Status401Unauthorized);
});

Alternative Designs

Add half a dozen or more overloads to PasswordSignInAsync, TwoFactorAuthenticatorSignInAsync, TwoFactorRecoveryCodeSignInAsync, etc... that take the primary authentication scheme.

Maybe come up with a better name.

Risks

We have to add even more properties like this in the future and all the state becomes unweildy.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-identity Includes: Identity and providers untriaged labels Mar 15, 2023
@mkArtakMSFT mkArtakMSFT added enhancement This issue represents an ask for new feature or an enhancement to an existing one and removed untriaged labels Mar 15, 2023
@dotnet dotnet deleted a comment Mar 15, 2023
@mkArtakMSFT mkArtakMSFT added the Priority:0 Work that we can't release without label Mar 17, 2023
mkArtakMSFT pushed a commit that referenced this issue Jul 21, 2023
Backport of #49498 to release/8.0-preview7

/cc @halter73

# Add more MapIdentityApi endpoints
## Description

This adds the following new endpoints:

- GET /confirmEmail
- POST /resendConfirmationEmail
- POST /resetPassword
- GET /account/2fa
- POST /account/2fa
- GET /account/info
- POST /account/info

Additionally, the existing /login endpoint now accepts 2fa codes and 2fa recovery codes as part of the request body. These can be queried and regenerated from /account/2fa. The /login endpoint now also gives limited failure reasons in the form of application/problem+json instead of empty 401 responses with details such as "LockedOut", "RequiresTwoFactor", "NotAllowed" (usually because lack of email confirmation), and the generic "Failed" statuses.

Fixes #47232 (lockout support)
Fixes #47231 (reset password support)
Fixes #47230 (2fa support)
Fixes #47229 (change username and password)
Fixes #49404 (Removes AddIdentityBearerToken which is no longer needed)

## Customer Impact

This makes the MapIdentityApi API introduced in preview4 more usable. See https://devblogs.microsoft.com/dotnet/asp-net-core-updates-in-dotnet-8-preview-4/#auth where we promised the following.

> In addition to user registration and login, the identity API endpoints will support features like two-factor authentication and email verification in upcoming previews. You can find a list of planned features in the issues labeled [feature-token-identity](https://github.com/dotnet/aspnetcore/issues?q=is%3Aopen+label%3Afeature-token-identity+sort%3Aupdated-desc) on the ASP.NET Core GitHub repository.

This PR adds all of these features, and it's important to make this available to customers as soon as possible, so we have time to react to any feedback. It appears customers are [excited to give it a go.](https://www.reddit.com/r/programming/comments/13jxcsx/aspnet_core_updates_in_net_8_preview_4_net_blog/jki0p3g/)

## Regression?

- [ ] Yes
- [x] No

## Risk

- [ ] High
- [ ] Medium
- [x] Low

This is primarily new API with minimal changes to SignInManager that should have no impact unless used by the new MapIdentityApi endpoints.

## Verification

- [x] Manual (required)
- [x] Automated

## Packaging changes reviewed?

- [ ] Yes
- [ ] No
- [x] N/A
@halter73 halter73 added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jul 25, 2023
@ghost
Copy link

ghost commented Jul 25, 2023

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@halter73
Copy link
Member Author

API Review Notes:

  • Discussed why adding parameters to a lot of methods rather than a property could work but would be a lot noisier.
  • We don't think the "Primary" in PrimaryAuthenticationScheme is adding much. Let's just go with AuthenticationScheme instead.
  • What about SignInScheme?
    • We don't think "SignIn" adds value since this is a property on the SignInManager.

API Approved!

// Microsoft.AspNetCore.Identity.dll

namespace namespace Microsoft.AspNetCore.Identity;

public class SignInManager<TUser> where TUser : class
{
+    public string AuthenticationScheme { get; set; } = IdentityConstants.ApplicationScheme;
}

@halter73 halter73 removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 21, 2023
@halter73 halter73 added the api-approved API was approved in API review, it can be implemented label Aug 21, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-identity Includes: Identity and providers enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-token-identity Priority:0 Work that we can't release without
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@halter73 @mkArtakMSFT and others