Skip to content

Make IEmailSender more easily customizable #50298

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

Closed
halter73 opened this issue Aug 23, 2023 · 5 comments · Fixed by #50301
Closed

Make IEmailSender more easily customizable #50298

halter73 opened this issue Aug 23, 2023 · 5 comments · Fixed by #50301
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-identity Includes: Identity and providers
Milestone

Comments

@halter73
Copy link
Member

halter73 commented Aug 23, 2023

Background and Motivation

Should this text be configurable eventually?

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

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.

Originally posted by @halter73 in #49981 (comment)

We now also have a customer requesting this functionality in the preview7 blogpost at https://devblogs.microsoft.com/dotnet/asp-net-core-updates-in-dotnet-8-preview-7/comment-page-2/#comment-18682

Proposed API

- // Microsoft.Extensions.Identity.Core.dll
+ // Microsoft.AspNetCore.Identity.dll (the move to the shared framework only assembly allows DIMs

namespace Microsoft.AspNetCore.Identity.UI.Services;

public interface IEmailSender
{
    Task SendEmailAsync(string email, string subject, string htmlMessage);

       // Used by MapIdentityApi and Identity UI
+    Task SendConfirmationLinkAsync<TUser>(TUser user, string email, string confirmationLink) where TUser : class
+    {
+        return SendEmailAsync(email, "Confirm your email", $"Please confirm your account by <a href='{confirmationLink}'>clicking here</a>.");
+    }

       // Used by Identity UI
+    Task SendPasswordResetLinkAsync<TUser>(TUser user, string email, string resetLink) where TUser : class
+    {
+        return SendEmailAsync(email, "Reset your password", $"Please reset your password by <a href='{resetLink}'>clicking here</a>.");
+    }

       // Used by MapIdentityApi since it does not provide a UI to enter the new password into. That's left to custom application code.
+    Task SendPasswordResetCodeAsync<TUser>(TUser user, string email, string resetCode) where TUser : class
+    {
+        return SendEmailAsync(email, "Reset your password", $"Reset your password using the following code: {resetCode}");
+    }
}

// Unchanged, but this also moves from Microsoft.Extensions.Identity.Core.dll to Microsoft.AspNetCore.Identity.dll,
// so it can still implement IEmailSender
public sealed class NoOpEmailSender : IEmailSender
{
    public Task SendEmailAsync(string email, string subject, string htmlMessage) => Task.CompletedTask;
}

Usage Examples

MyEmailSender.cs

using Azure.Identity;

namespace MyNamespace;

public class MyEmailSender : IEmailSender
{
    private readonly EmailClient _client;

    public MyEmailSender(IConfiguration config)
    {
        var credential = new ChainedTokenCredential(
            new ClientSecretCredential( 
                _config["AZURE_TENANT_ID"],
                _config["AZURE_CLIENT_ID"],
                _config["AZURE_CLIENT_SECRET"]),
            new ManagedIdentityCredential()
        )
        _client = new EmailClient(new Uri("https://my-instance.communication.azure.com/")
    }

    public Task SendEmailAsync(string email, string subject, string message)
    {
        var recipients = new EmailRecipients(new [] { new EmailAddress(email) });
        var content = new EmailContent(subject)
        {
            PlainText = message
        };

        await _client.SendAsync(new EmailMessage("[email protected]", content, recipients);
    }

    public Task SendConfirmationLinkAsync<TUser>(TUser user, string email, string confirmationLink) where TUser : class
    {
        return SendEmailAsync(email, "Confirm your email for MyWebSite", $"Please confirm your MyWebSite account by <a href='{confirmationLink}'>clicking here</a>.");
    }

    public Task SendPasswordResetLinkAsync<TUser>(TUser user, string email, string resetLink) where TUser : class
    {
        return SendEmailAsync(email, "Reset your password for MyWebSite", $"Please reset your MyWebSite password by <a href='{resetLink}'>clicking here</a>.");
    }

    public Task SendPasswordResetCodeAsync<TUser>(TUser user, string email, string resetCode) where TUser : class
    {
        return SendEmailAsync(email, "Reset your password for MyWebSite", $"Reset your MyWebSite password using the following code: {resetCode}");
    }
}

Program.cs

// ...
builder.Services.AddSingleton<IEmailSender, MyEmailSender>();
// ...

Alternative Designs

  • We could not add the DIMs and instead force customers who care to parse the subject and htmlMessage to figure out what kind of email is being sent and try to extract any links or password reset codes.
  • We could omit the TUser user parameters since it's unneeded by the default implementation, but it seems like it could be useful and it is not difficult to pass in.

Risks

In the future, we might not need to send these exact kinds of emails. Maybe the code in Identity UI or MapIdentityApi will have the need to specify more parameters than just the recipient and email confirmation link. In that case, we'd probably add more DIMs, but it would be less clear which ones are actually used.

@ghost ghost added the area-identity Includes: Identity and providers label Aug 23, 2023
@halter73 halter73 changed the title Make IEmailSender more customizable Make IEmailSender more easily customizable Aug 23, 2023
@halter73 halter73 added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 23, 2023
@ghost
Copy link

ghost commented Aug 23, 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 halter73 added this to the 8.0-rc2 milestone Aug 23, 2023
@halter73 halter73 self-assigned this Aug 23, 2023
@mkArtakMSFT mkArtakMSFT added the bug This issue describes a behavior which is not expected - a bug. label Aug 23, 2023
@halter73 halter73 removed the bug This issue describes a behavior which is not expected - a bug. label Aug 31, 2023
@halter73
Copy link
Member Author

API Review Notes:

  • Why are we making customizing the subject and body part of the IEmailSender? Could that part be separated into a new interface?
    • As an application developer, I'm going to know at development time what TUser is. Being forced to cast it is ugly and sad. It could be cleaner to have another interface like IEmailComposer<TUser> where you could register a closed concrete implementation.
    • Two phase might be nice, but returning both the subject and body for each email type adds a lot of surface area.
  • Do we still want to move assemblies if we do a separate generic interface?
    • Since the only callers are in the shared framework, lets keep it there.
  • With the two interfaces, we plan to first try to resolve IEmailSender<TUser> first and use that instead of IEmailSender if it exists. We might achieve this in practice by registering an internal generic sender that depends on IEmailSender.
  • Should the generic interface derive from the non-generic one?
    • Since it's not necessary, it's best to not imply that it is.
  • What about the Microsoft.AspNetCore.Identity.UI.Services namespace?
    • Let's move it to Microsoft.AspNetCore.Identity even though it doesn't match the non-generic version. If you implement the generic version, you don't ever have to use the non-generic one if you don't want to.

API Approved!

// Microsoft.AspNetCore.Identity.dll (the move to the shared framework only assembly allows DIMs in the future)

namespace Microsoft.AspNetCore.Identity;

public interface IEmailSender<TUser> where TUser : class
{
     // Used by MapIdentityApi and Identity UI
    Task SendConfirmationLinkAsync(TUser user, string email, string confirmationLink);

     // Used by Identity UI
    Task SendPasswordResetLinkAsync(TUser user, string email, string resetLink);

     // Used by MapIdentityApi since it does not provide a UI to enter the new password into. That's left to custom application code.
    Task SendPasswordResetCodeAsync(TUser user, string email, string resetCode);
}

@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 31, 2023
@augustevn
Copy link

augustevn commented Sep 24, 2023

I'd like an optional "CallbackUrl" since I'll likely have multiple frontends using the same Auth API.
I'm not sure if you should even attempt to form an URL for us, just parameters might be more useful, so we can form our own URLs and decide whether it goes to a frontend or backend.

If you want to see how I implemented RC1: https://youtu.be/yGYpN1hFPAg?si=SDyUdQdtUSfDHPO9
But this won't be viable for multiple frontends.

@martincostello
Copy link
Member

@augustevn Please create new issue(s) using one of the templates rather than comment on old/closed issues/pull requests.

@augustevn
Copy link

augustevn commented Oct 10, 2023

I did create an API proposal: Identity API Optional CallbackUrl

But I keep getting flabbergasted by this API, what is this method for? SendPasswordResetLinkAsync
I'm only hitting the SendPasswordResetCodeAsync & SendConfirmationLinkAsync

The code comments don't clarify the intent.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 9, 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants