Support for SMTP OAuth authentication through easier IEmailSenderClient implementation

[kasparboelkjeldsen]

Prerequisites

• [x ] I have added steps to test this contribution in the description below

This pull request is a response to the following discussion Support for SMTP OAuth authentication

Description

This pull-request separates til SmtpClient out from EmailSender.cs to it's own abstraction of IEmailSenderClient/BasicSmtpEmailSenderClient to make it easier for package-authors and umbraco developers to support more ways of sending out e-mails without Umbraco having to take on supporting every variation of OAuth authentication imaginable.

The linked discussion contains more details as to why and actual OAuth2 implementation for SMTP doesn't seem feasible.

The pull request will allow developers to replace the client like so (microsoft.graph example)

public class MicrosoftGraphEmailClientComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.Services.AddTransient<IEmailSenderClient, MicrosoftGraphEmailClient>();
    }
}
public class MicrosoftGraphEmailClient : IEmailSenderClient
{
    private ClientSecretCredential? credentials;
    public MicrosoftGraphEmailClient()
    {
        var tenantId = "<tenant-id>";

        // Values from app registration
        var clientId = "<client-id>";
        var clientSecret = "<client-secret>";

        // using Azure.Identity;
        var options = new TokenCredentialOptions
        {
            AuthorityHost = AzureAuthorityHosts.AzurePublicCloud
        };

        credentials = new ClientSecretCredential(
            tenantId, clientId, clientSecret, options);

        if (credentials == null)
        {
            throw new ArgumentNullException(nameof(credentials));
        }
    }

    public async Task SendAsync(MimeMessage? mimeMessage)
    {
        if (mimeMessage == null)
        {
            throw new ArgumentNullException(nameof(mimeMessage));
        }

        var scopes = new[] { "https://graph.microsoft.com/.default" };

        var graphClient = new GraphServiceClient(credentials, scopes);

        var stream = new MemoryStream();
        mimeMessage.WriteTo(stream);

        var base64content = Convert.ToBase64String(stream.ToArray());
        var ri = graphClient.Users[mimeMessage.From.ToString()].SendMail.ToPostRequestInformation(new SendMailPostRequestBody());

        ri.Headers.Clear();// replace the json content header
        ri.SetStreamContent(new MemoryStream(Encoding.UTF8.GetBytes(base64content)), "text/plain");

        var result = await graphClient.RequestAdapter.SendAsync<Message>(ri, Message.CreateFromDiscriminatorValue);
    }
}

Test

Ideally -nothing- should have changed in Umbraco except it now exposes IEmailSenderClient as an easier integration point.

This leaves Umbraco's logic for building the messag intact but allows developers to take on the responsibility of sending it out.

[github-actions]

Hi there @kasparboelkjeldsen, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[emmagarland]

Hi @kasparboelkjeldsen !

Firstly, thanks for implementing the support for SMTP OAuth implementation as per the discussion.

One of the core collabs team will review soon, although this might also be one we need to run past HQ (I see @bergmania was on the discussion before).

Thanks again!

Emma

[bergmania]

Fundamentally I do not like the fact that we expose a third party class in this public interface.

I would prefer it to be our own model, and I'm not sure why it is nullable?

[kasparboelkjeldsen]

@bergmania nullability must have been a brainfart.

We can expose your model for the interface, but then I think we should make your extension to turn it into a mimemessage public, as the whole exercise is to make it easier to extend the emailclient without having to re-implement too much of your logic.

I've updated the pull-request with a non-nullable "EmailMessage" model and moved the MimeMessage part into the implementation as well as exposed its extention from internal to public

[bergmania]

But the issue with exposing third party classes is that we cannot replace this underlying framework without braking our contracts/interfaces. - Ideally we should be able to implement our contracts with all mail client libraries.

[kasparboelkjeldsen]

I think moving the interface to use your model honors the ability to implement contracts with all client libraries.

I understand the logic behind the extension method in principle, but also just see how it's just going to lead to us on the other side of the fence having to write the exact same code anyway.

But if that's a hard no, I'll update the pull request and steal your class for my implementation and package.

[bergmania]

Just talked with a few people from the team, and we all agree it's better in this case if you copy that code.
As I mentioned, exposing the underlying third party types in our methods, will make it impossible for us to release that thrid-party package, without making breaking changes.

If you copy that code, preferable you should take a dependency yourself and that can continue to work, even if we change to another library

[kasparboelkjeldsen]

Noted. I've updated the extension class to be internal again.

[bergmania]

Besides the breaking changes it looks good now.

Please add the original constructor signatures and use StaticServiceProvider.Instance.GetRequiredService<IEmailSenderClient>() where needed

[kasparboelkjeldsen]

Gotcha - it's back to normal now and using StaticServiceProvider instead

[bergmania]

Sorry, I was not clear enough.

We usally make the constructors as we want them, but call the new constructor from the old, and input missing dependencies using the static service provider and obsolete the old one. These can be obsoleted and removed in Umbraco 17, now that they will not be part of Umbraco 15.0.0

Example here
https://github.com/umbraco/Umbraco-CMS/blob/contrib/src/Umbraco.Cms.Api.Delivery/Controllers/Security/MemberController.cs#L37-L58

[kasparboelkjeldsen]

@bergmania - no I'm sorry, I've actually had to do that in another pull request so I should have guessed it.

Here is another attempt, though speaking of constructors, I'm a bit perplexed by the shorthanded constructor

public EmailSender(
    ILogger<EmailSender> logger,
    IOptionsMonitor<GlobalSettings> globalSettings,
    IEventAggregator eventAggregator)
    : this(logger, globalSettings, eventAggregator,null, null)
{
}

as it has no references to it in the entire code base? If it's there for backwards compatibility, I suppose it should also be obsolete ? That's what I've done anyway. Hope I'm right.

[bergmania]

Sadly I cannot push to your branch, so I cannot fix that warning that reports as error. You will need to build in release mode to get the error.

Otherwise, we are ready to merge