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

Add support for Azure Email Communication Services #14699

Closed
BenedekFarkas opened this issue Nov 15, 2023 · 17 comments · Fixed by #15254
Closed

Add support for Azure Email Communication Services #14699

BenedekFarkas opened this issue Nov 15, 2023 · 17 comments · Fixed by #15254
Assignees
Milestone

Comments

@BenedekFarkas
Copy link
Member

BenedekFarkas commented Nov 15, 2023

Is your feature request related to a problem? Please describe.

Azure now has a new set of resources under the Communication Services (ACS) umbrella, Email Communication Services being one of them. This is a cheaper, Azure-native alternative to SendGrid.

Describe the solution you'd like

The primary way to use it is not through traditional SMTP-server configuration, but a connection string (of two components, endpoint and access key) provided the ACS resource, which allows access to the specific communication service resources attached to it (for example an Email Communication Services instance). This requires a custom implementation to send emails using the corresponding SDK.
OrchardCore.Email doesn't seem to support this scenario at its current state and needs a bit of refactoring (can take inspiration from the structure in O1 for better extensibility). Initial ideas for implementation:

  • Rename ISmtpService to IEmailService for clarity.
  • Move some of the logic (mostly related to validation) out of SmtpService to make it reusable. EmailService base class?
  • Implement email sending through Azure Email Communication Services in OrchardCore.Email.Azure (new).
  • As for the settings screen, add "Azure Communication Services" as a new "Delivery method" (although the existing two options are wired in via SmtpDeliveryMethod and SmtpService in its current state will throw an error for any other delivery method) and "Azure delivery options" needs only two fields (endpoint and access key). Also make it possible to read these from app configuration.

Describe alternatives you've considered

Traditional SMTP-servers, like SendGrid are still available and ACS also supports sending emails through SMTP, but the setup for that is very tedious.

@Piedone
Copy link
Member

Piedone commented Nov 16, 2023

After having the current ISmtpService as IEmailService, I'd move the whole of SendOnlineMessageAsync() to a new ISmtpService implementation. That would then be the SMTP provider, where the default implementation would be the current one, and a new one can be added for ACS. So, the actual e-mail sending operation would be easily pluggable, with the generic e-mail preparation remaining in one place.

@hishamco
Copy link
Member

I was thinking about both email and SMS azure services

I might handle those if no one planned

@sebastienros sebastienros added this to the 1.x milestone Nov 16, 2023
@BenedekFarkas
Copy link
Member Author

BenedekFarkas commented Nov 17, 2023

I'd rather keep this issue focused on the email functionality, but keeping further features and extensibility in mind is definitely a plus. On a related note, the base functionality (settings, configuration) should go to a separate OrchardCore.Azure.Communications module or something like that.

I might handle those if no one planned

Sounds great! :)

@hishamco hishamco self-assigned this Nov 17, 2023
@Piedone
Copy link
Member

Piedone commented Nov 19, 2023

One more thing: for this feature, we'd need IShellConfiguration-based configuration, like it is possible for all the other Azure services. I don't think we need site settings; if you're using anything more complex than a simple SMTP server, than you're already involved with hosting your app, and at that point, environmental configuration is better.

@hishamco
Copy link
Member

That's what I thought about

@BenedekFarkas
Copy link
Member Author

Sounds good as the same applies to Azure Blob Storage too.

There needs to be a clear distinction that this feature is in effect, so the current (SMTP-based) email functionality should not be enabled for this to work (or indicate that it's behavior is overridden) and make sure that SMTP-based email functionality can remain operational alongside ACS, not force an update on users, e.g., if someone wants to use ACS for SMS 2FA, but wants to stick to their current SMTP-settings for email sending.

@hishamco
Copy link
Member

hishamco commented Nov 20, 2023

Sounds good as the same applies to Azure Blob Storage too.

Exactly

There needs to be a clear distinction that this feature is in effect, so the current (SMTP-based) email functionality should not be enabled for this to work (or indicate that it's behavior is overridden) and make sure that SMTP-based email functionality can remain operational alongside ACS, not force an update on users, e.g., if someone wants to use ACS for SMS 2FA, but wants to stick to their current SMTP-settings for email sending.

I will think about it. @Piedone can we focus on supporting emailing using ACS or shall we consider the integration with other modules such as 2FA?

@BenedekFarkas
Copy link
Member Author

Please keep it focused on the basic integration and email functionality for now, but with the possibility of adding further ACS-based features in the future.

@Piedone
Copy link
Member

Piedone commented Nov 20, 2023

Yeah.

And as for the UX, I imagine exactly the same as with Azure Media Storage: We have the basic (current) SMTP functionality enabled by the Email module, and if you enable the ACS, then it'll override that, with a similar indication than this:

image

@hishamco
Copy link
Member

What I already did is something similar, whereas the ACS settings come from appsettings.json.

If we look closely into AECS the new thing should be added is the connection string, so the sender will be shared for both modules, and while the new should works independently either we have a dependency on OC.Email module to utilize the same sender or add the ability to override it from within the OC.Email.Azure module

@hishamco
Copy link
Member

@BenedekFarkas @Piedone What I already did in the draft PR is separate the basic email settings (Default Sender) - we could add a display name later if we want - from the SMTP settings, this way AECS does not depend on the SMTP feature at all but email feature.

Once AECS is enabled it shows the default send from the email feature but we could override it from within the appsettings.json

Please let me know if we need to change anything else

@Piedone
Copy link
Member

Piedone commented Nov 24, 2023

Sounds good!

@hishamco
Copy link
Member

The PR is almost ready I just need to add few tests

@hishamco
Copy link
Member

I tested the AzureEmailService using ECS and works as expected, can we do an initial review

@Piedone
Copy link
Member

Piedone commented Nov 25, 2023

Is the code as far as you're concerned complete?

@hishamco
Copy link
Member

Yep, I got some comments from that might change the implementation from what we thought about

Please have a look and let me know your comments

@Piedone
Copy link
Member

Piedone commented Mar 13, 2024

Done in #15254.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment