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

AddIdentityBearerToken should not be generic #49404

Closed
davidfowl opened this issue Jul 14, 2023 · 5 comments · Fixed by #49498
Closed

AddIdentityBearerToken should not be generic #49404

davidfowl opened this issue Jul 14, 2023 · 5 comments · Fixed by #49498
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-identity Includes: Identity and providers breaking-change This issue / pr will introduce a breaking change, when resolved / merged. enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-token-identity
Milestone

Comments

@davidfowl
Copy link
Member

After playing with the APIs introduced in #47228, I realized that making the API generic is pretty pointless. The TUser isn't used to discriminate any options so it seems superfluous.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-identity Includes: Identity and providers label Jul 14, 2023
@halter73
Copy link
Member

We can remove the generic parameter. The original version used the TUser to register an OnSigningIn callback with the auth handler for the security stamp validation. See bd5ffbe (#48595)

After @Tratcher's PR feedback, we wound up moving the refresh token validation logic into the API endpoint directly. The auth handler now issues the refresh token but never validates it. If the API endpoint determines the refresh token is valid, it just does a normal "SignIn" again like it would after a normal username/password login.

I considered removing the generic parameter, but I figured we might need it in the future. Note that this only affects the Identity APIs which are often generic. AddBearerToken itself is not generic. But you're right that it's not necessary right now.

@davidfowl
Copy link
Member Author

I think we should remove it.

@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Jul 18, 2023
@mkArtakMSFT mkArtakMSFT added this to the 8.0-preview7 milestone Jul 18, 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
@mkArtakMSFT mkArtakMSFT added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Jul 24, 2023
@halter73 halter73 reopened this Jul 24, 2023
@halter73
Copy link
Member

Background and Motivation

Remove unnecessary code.

Proposed API

// Microsoft.AspNetCore.Identity.dll

namespace Microsoft.AspNetCore.Identity;

- public static class IdentityAuthenticationBuilderExtensions
- {
-     public static AuthenticationBuilder AddIdentityBearerToken<TUser>(this AuthenticationBuilder builder)
-         where TUser : class, new()
-    public static AuthenticationBuilder AddIdentityBearerToken<TUser>(this AuthenticationBuilder builder, Action<BearerTokenOptions> configure)
-        where TUser : class, new()
- }

Usage Examples

No instead of calling AddIdentityBearerToken to add bearer token auth for use with AddIdentityCore:

// ...
builder.Services.AddAuthentication()
    .AddIdentityBearerToken<ApplicationUser>();
// ... 

You'd call the normal AddBearerToken method with the correct Identity Constant.

builder.Services.AddAuthentication()
    .AddBearerToken(IdentityConstants.BearerScheme)

Alternative Designs

Just remove the generic parameter, but keep AddIdentityBearerToken.

Risks

It's a breaking change, but the workaround is easy and it's in a preview.

@halter73 halter73 added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jul 24, 2023
@ghost
Copy link

ghost commented Jul 24, 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

API Review Notes:

  • We are not deleting any public API that shipped in .NET 7. This was only in earlier previews.
  • We think AddIdentityBearerToken without a generic parameter doesn't add much value when you can just call .AddBearerToken(IdentityConstants.BearerScheme).

API Approved!

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 21, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 20, 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 breaking-change This issue / pr will introduce a breaking change, when resolved / merged. enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-token-identity
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@halter73 @davidfowl @mkArtakMSFT and others