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 for SMTP OAuth authentication through easier IEmailSenderClient implementation #17484

Merged
merged 10 commits into from
Nov 19, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
using Umbraco.Cms.Infrastructure.HostedServices;
using Umbraco.Cms.Infrastructure.Install;
using Umbraco.Cms.Infrastructure.Mail;
using Umbraco.Cms.Infrastructure.Mail.Interfaces;
using Umbraco.Cms.Infrastructure.Manifest;
using Umbraco.Cms.Infrastructure.Migrations;
using Umbraco.Cms.Infrastructure.Migrations.Install;
Expand Down Expand Up @@ -172,14 +173,18 @@

builder.Services.AddSingleton<IContentLastChanceFinder, ContentFinderByConfigured404>();

builder.Services.AddTransient<IEmailSenderClient, BasicSmtpEmailSenderClient>();

// replace
builder.Services.AddSingleton<IEmailSender, EmailSender>(
services => new EmailSender(
services.GetRequiredService<ILogger<EmailSender>>(),
services.GetRequiredService<IOptionsMonitor<GlobalSettings>>(),
services.GetRequiredService<IEventAggregator>(),
services.GetRequiredService<IEmailSenderClient>(),
services.GetService<INotificationHandler<SendEmailNotification>>(),
services.GetService<INotificationAsyncHandler<SendEmailNotification>>()));
services.GetService<INotificationAsyncHandler<SendEmailNotification>>()
));

Check warning on line 187 in src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (contrib)

❌ Getting worse: Large Method

AddCoreInitialServices increases from 111 to 114 lines of code, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
builder.Services.AddTransient<IUserInviteSender, EmailUserInviteSender>();
builder.Services.AddTransient<IUserForgotPasswordSender, EmailUserForgotPasswordSender>();

Expand Down
50 changes: 50 additions & 0 deletions src/Umbraco.Infrastructure/Mail/BasicSmtpEmailSenderClient.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
using System.Net.Mail;
using Microsoft.Extensions.Options;
using Umbraco.Cms.Core.Configuration.Models;
using Umbraco.Cms.Core.Models.Email;
using Umbraco.Cms.Infrastructure.Extensions;
using Umbraco.Cms.Infrastructure.Mail.Interfaces;
using SecureSocketOptions = MailKit.Security.SecureSocketOptions;
using SmtpClient = MailKit.Net.Smtp.SmtpClient;

namespace Umbraco.Cms.Infrastructure.Mail
{
/// <summary>
/// A basic SMTP email sender client using MailKits SMTP client.
/// </summary>
public class BasicSmtpEmailSenderClient : IEmailSenderClient
{
private readonly GlobalSettings _globalSettings;
public BasicSmtpEmailSenderClient(IOptionsMonitor<GlobalSettings> globalSettings)
{
_globalSettings = globalSettings.CurrentValue;
}

public async Task SendAsync(EmailMessage message)
{
using var client = new SmtpClient();

await client.ConnectAsync(
_globalSettings.Smtp!.Host,
_globalSettings.Smtp.Port,
(SecureSocketOptions)(int)_globalSettings.Smtp.SecureSocketOptions);

if (!string.IsNullOrWhiteSpace(_globalSettings.Smtp.Username) &&
!string.IsNullOrWhiteSpace(_globalSettings.Smtp.Password))
{
await client.AuthenticateAsync(_globalSettings.Smtp.Username, _globalSettings.Smtp.Password);
}

var mimeMessage = message.ToMimeMessage(_globalSettings.Smtp!.From);

if (_globalSettings.Smtp.DeliveryMethod == SmtpDeliveryMethod.Network)
{
await client.SendAsync(mimeMessage);
}
else
{
client.Send(mimeMessage);
}
}
}
}
39 changes: 9 additions & 30 deletions src/Umbraco.Infrastructure/Mail/EmailSender.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Copyright (c) Umbraco.
// See LICENSE for more details.

using System.Net.Mail;
using MailKit.Net.Smtp;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
Expand All @@ -13,8 +12,7 @@
using Umbraco.Cms.Core.Models.Email;
using Umbraco.Cms.Core.Notifications;
using Umbraco.Cms.Infrastructure.Extensions;
using SecureSocketOptions = MailKit.Security.SecureSocketOptions;
using SmtpClient = MailKit.Net.Smtp.SmtpClient;
using Umbraco.Cms.Infrastructure.Mail.Interfaces;

namespace Umbraco.Cms.Infrastructure.Mail;

Expand All @@ -28,26 +26,29 @@
private readonly ILogger<EmailSender> _logger;
private readonly bool _notificationHandlerRegistered;
private GlobalSettings _globalSettings;

private readonly IEmailSenderClient _emailSenderClient;
public EmailSender(
ILogger<EmailSender> logger,
IOptionsMonitor<GlobalSettings> globalSettings,
IEventAggregator eventAggregator)
: this(logger, globalSettings, eventAggregator, null, null)
IEventAggregator eventAggregator,
IEmailSenderClient emailSenderClient)
: this(logger, globalSettings, eventAggregator, emailSenderClient,null, null)
Copy link
Member

Choose a reason for hiding this comment

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

Besides the breaking changes it looks good now.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

{
}

public EmailSender(
ILogger<EmailSender> logger,
IOptionsMonitor<GlobalSettings> globalSettings,
IEventAggregator eventAggregator,
IEmailSenderClient emailSenderClient,
INotificationHandler<SendEmailNotification>? handler1,
INotificationAsyncHandler<SendEmailNotification>? handler2)
{
_logger = logger;
_eventAggregator = eventAggregator;
_globalSettings = globalSettings.CurrentValue;
_notificationHandlerRegistered = handler1 is not null || handler2 is not null;
_emailSenderClient = emailSenderClient;

Check warning on line 51 in src/Umbraco.Infrastructure/Mail/EmailSender.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (contrib)

❌ New issue: Constructor Over-Injection

EmailSender has 6 arguments, threshold = 5. This constructor has too many arguments, indicating an object with low cohesion or missing function argument abstraction. Avoid adding more arguments.
globalSettings.OnChange(x => _globalSettings = x);
}

Expand Down Expand Up @@ -152,29 +153,7 @@
while (true);
}

using var client = new SmtpClient();

await client.ConnectAsync(
_globalSettings.Smtp!.Host,
_globalSettings.Smtp.Port,
(SecureSocketOptions)(int)_globalSettings.Smtp.SecureSocketOptions);

if (!string.IsNullOrWhiteSpace(_globalSettings.Smtp.Username) &&
!string.IsNullOrWhiteSpace(_globalSettings.Smtp.Password))
{
await client.AuthenticateAsync(_globalSettings.Smtp.Username, _globalSettings.Smtp.Password);
}

var mailMessage = message.ToMimeMessage(_globalSettings.Smtp.From);
if (_globalSettings.Smtp.DeliveryMethod == SmtpDeliveryMethod.Network)
{
await client.SendAsync(mailMessage);
}
else
{
client.Send(mailMessage);
}

await client.DisconnectAsync(true);
await _emailSenderClient.SendAsync(message);

Check notice on line 156 in src/Umbraco.Infrastructure/Mail/EmailSender.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (contrib)

✅ Getting better: Complex Method

SendAsyncInternal decreases in cyclomatic complexity from 16 to 13, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
}

}
17 changes: 17 additions & 0 deletions src/Umbraco.Infrastructure/Mail/Interfaces/IEmailSenderClient.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
using Umbraco.Cms.Core.Models.Email;

namespace Umbraco.Cms.Infrastructure.Mail.Interfaces
{
/// <summary>
/// Client for sending an email from a MimeMessage
/// </summary>
public interface IEmailSenderClient
{
/// <summary>
/// Sends the email message
/// </summary>
/// <param name="message"></param>
/// <returns></returns>
public Task SendAsync(EmailMessage message);
}
}
3 changes: 2 additions & 1 deletion tests/Umbraco.Tests.UnitTests/TestHelpers/TestHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
using Umbraco.Cms.Core.Serialization;
using Umbraco.Cms.Core.Strings;
using Umbraco.Cms.Infrastructure.Mail;
using Umbraco.Cms.Infrastructure.Mail.Interfaces;
using Umbraco.Cms.Infrastructure.Persistence;
using Umbraco.Cms.Infrastructure.Persistence.Mappers;
using Umbraco.Cms.Persistence.SqlServer.Services;
Expand Down Expand Up @@ -80,7 +81,7 @@ public static class TestHelper

public static UriUtility UriUtility => s_testHelperInternal.UriUtility;

public static IEmailSender EmailSender { get; } = new EmailSender(new NullLogger<EmailSender>(), new TestOptionsMonitor<GlobalSettings>(new GlobalSettings()), Mock.Of<IEventAggregator>());
public static IEmailSender EmailSender { get; } = new EmailSender(new NullLogger<EmailSender>(), new TestOptionsMonitor<GlobalSettings>(new GlobalSettings()), Mock.Of<IEventAggregator>(), Mock.Of<IEmailSenderClient>(), null,null);

public static ITypeFinder GetTypeFinder() => s_testHelperInternal.GetTypeFinder();

Expand Down
Loading