Skip to content

Combine email and username in MapIdentityApi #49981

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

Merged
merged 7 commits into from
Aug 11, 2023
Merged

Combine email and username in MapIdentityApi #49981

merged 7 commits into from
Aug 11, 2023

Conversation

halter73
Copy link
Member

This makes MapIdentityApi more like Identity UI. The main difference is we're now forcing the user's email to match their username internally therefore we expose just the Email in MapIdentityApi requests and responses. There are no longer any Username parameters. This makes MapIdentityApi more compatible with Identity UI and simplifies threat modeling them both.

The main differences from before are:

  • Email is now the username
  • Email always goes through basic validation by EmailAddressAttribute even if RequireUniqueEmail = false.
  • The GET /2fa endpoint is now removed. You must at least send an empty JSON object ({}) in a POST body to see 2fa details like the shared key.
  • Split the /forgotPassword and /resetPassword endpoints like Identity UI.
  • The BearerTokenHandler no longer supports reconfiguring ExpiresUtc like other non-cookie auth handlers. It never supported adjusting the refresh token expiration per-signin either. And it never supported IssuedUtc or any other AuthenticationProperties.

Contributes to #48433

@halter73 halter73 requested a review from BrennanConroy August 10, 2023 02:49
@halter73 halter73 requested a review from Tratcher as a code owner August 10, 2023 02:49
@ghost ghost added the area-identity Includes: Identity and providers label Aug 10, 2023
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idk much about the design, just reviewing the code changes 😃

code = WebEncoders.Base64UrlEncode(Encoding.UTF8.GetBytes(code));

await emailSender.SendEmailAsync(resetRequest.Email, "Reset your password",
$"Reset your password using the following code: {HtmlEncoder.Default.Encode(code)}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this text be configurable eventually?

I'm reminded of the "We will never ask you for this code" story we heard recently 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. It's not configurable with Identity UI, but you can more easily override razor pages than these endpoints. And you can scaffold them out. You could parse out the code in your IEmailSender and do whatever, but that'd obviously be very fragile.

I think in the future we might have a higher-level abstraction with methods like SendEmailConfirmationAsync and SendPasswordResetAsync and a TUser parameter. The default implementation could then just resolve the IEmailSender and use the existing strings. That's not something we need to do immediately though.

Copy link

@augustevn augustevn Sep 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like been developed on a friday evening ...
the confirmation tag was close enough, too bad for the bug with the absolute url.
But this? Couldn't you at least follow a similar approach as with the confirmation htmlMessage.

A configurable link would've been nice so we can make it either go to our backend or frontend to get the code from the query param. Expecting an app user to copy it correctly into a form field, is completely ridiculous. Most can't even paste an image URL, they copy their website address and hope for a wonder.

@halter73 halter73 merged commit 6241625 into main Aug 11, 2023
@halter73 halter73 deleted the halter73/48433 branch August 11, 2023 23:15
@ghost ghost added this to the 8.0-rc1 milestone Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-identity Includes: Identity and providers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants