umbraco / Umbraco.Forms.Issues

Public issue tracker for Umbraco Forms
29 stars 0 forks source link

Prevent users defining an email workflow that allows the form's sender email to be defined as that entered by the user #1210

Closed RachBreeze closed 2 weeks ago

RachBreeze commented 2 months ago

We have clients which add an email field on the form eg emailAddress and then set this as the Sender Email in the email workflow, an example is shown below

image

Whilst this makes sense from a content manager's perspective (they can just reply to the email the form's workflow sends).

The website is actually spoofing the email address and this means some commonly used email clients are rejecting the email from being sent.

The correct fix is when someone defines the email workflow, prevent them from entering any fields used in the form definition to be used as a value in the Sender Email field is this something you could look at please?

AndyButland commented 2 months ago

Are you able to use this configuration feature to achieve this end? In particular, hiding the "Sender Email" field such that it'll use the site wide configured value.

RachBreeze commented 2 months ago

Hi @AndyButland

Thanks for getting back so quickly. This wouldn't allow the content manager to use different sender email addresses (there are instances for this too), for example from "events@client.com" and from "jobs@client.com"

AndyButland commented 2 months ago

OK, will have a think about it. Not 100% sure it should be default behaviour as I suppose it could be that people would want to use the feature your editors are employing, but it could be a configuration option.

I had a look into the existing validation notifications thinking they could be used, but it's not ideal given that the UI allows you to save forms and workflows together, but the underlying services are separate. This means the WorkflowSavingNotification and FormSavingNotification only know about the workflow and form respectively, and not each other.

As some sort of a solution currently you could hook into the WorkflowSavingNotification, check for the specific workflow and setting and empty the value if it contains something starting with { and ending with }. That would at least stop the situation happening and the workflow would fall back to using the Umbraco-wide configured sender address. But that would be a "silent" change as I don't see a way currently to bubble that up to the UI (only cancellations of FormSavingNotifications do this).

AndyButland commented 2 months ago

Something like this:

using Umbraco.Cms.Core.Events;
using Umbraco.Forms.Core.Models;
using Umbraco.Forms.Core.Providers;
using Umbraco.Forms.Core.Services.Notifications;

namespace Umbraco.Forms.Testsite;

public class WorkflowSavingNotificationHandler : INotificationHandler<WorkflowSavingNotification>
{
    private readonly WorkflowCollection _workflowCollection;

    public WorkflowSavingNotificationHandler(WorkflowCollection workflowCollection) => _workflowCollection = workflowCollection;

    public void Handle(WorkflowSavingNotification notification)
    {
        foreach (Workflow workflow in notification.SavedEntities)
        {
            Core.WorkflowType workflowTYpe = _workflowCollection[workflow.WorkflowTypeId];
            if (workflowTYpe.Id == Guid.Parse(Core.Constants.WorkflowTypes.SendRazorEmail))
            {
                var senderEmailValue = workflow.Settings["SenderEmail"];
                if (senderEmailValue.StartsWith("{") && senderEmailValue.EndsWith("}"))
                {
                    workflow.Settings["SenderEmail"] = string.Empty;
                }
            }
        }
    }
}
RachBreeze commented 2 months ago

Hi @AndyButland Thanks for the quick response.

I understand the issue re validation (now you've explained it :-) ) but also this will be impacting a lot of Umbraco Forms users, as SMTP servers tighten their policies around spoofing and it would be really good if it could be resolved in Umbraco Forms rather than handled as a "Training issue" or a package/ agency implementation

AndyButland commented 2 weeks ago

Makes sense, thanks.... I've added the validation now so we'll have this in the next releases.

image

RachBreeze commented 2 weeks ago

Hi @AndyButland

That's perfect thank you