SPF validation

[ciihla]

I just wanted to ask why you added the SPF validation to the adapter? I think it is mandrill's responsiility to filter SPF. And if Mandrill sends the email, then griddle would not have to validate it.

Maybe Im wrong but at least it could have been optional or configurable, right? Can I do a MR with configurable this SPF validation?

[wingrunr21]

@arunthampi needed SPF validation and Mandrill wasn't doing it. They submitted a PR (#22) and I merged it.

I'm not against a configuration option, but do want to ask what about SPF validation is causing you issues?

[ciihla]

We have few domains that dont have SPF record but are valid email servers. And since version 1.1.3 we are not able to deliver emails to them. We had to downgrade to version 1.1.2. Or maybe there is missing a condition || event[:spf][:result] == 'none' ? since Mandrill sends within these emails:

"spf":{"result":"none","detail":""}

Thanks in advance!

[arunthampi]

hmm i'd rather have the option to check for spf than using event[:spf][:result] == 'none'. I confirmed that the webhook does get called for spoofed emails and that's pretty dangerous imo because almost all applications use email as a way of identifying users.

My preference would be to have strong security defaults and let users override it at their own risk.

[wingrunr21]

In hindsight, I should have bumped this change to v1.2.x instead of leaving it on v1.1.x. My apologies for that.

I too feel that strong security defaults are preferential. However, I want to make sure the adapter is still usable in non-SPF environments. Will you get a 'none' result for a spoofed email? Maybe we should be looking for positive identification of spoofing (instead of negative)?

[amika]

However, I want to make sure the adapter is still usable in non-SPF environments.

That's what @ciihla mean - now we are not able to receive emails from domains that doesn't have SPF record.

[wingrunr21]

I know...that's why I said I want to support the use case.

[amika]

Great. :)

[wingrunr21]

If we want to make this configurable, we could add something like adapter_config to Griddler::Configuration or just expose our own config object (seems a bit silly though).

From there, we could add something like spf_filter. The default could be [ 'pass', 'neutral'] so that we get our strong defaults, but people who need changes can do something like setting that to nil or changing the whitelisted result strings.

[ciihla]

That would be ideal. Regarding the question whether to use Griddler::Configuration or own config:
I would go with own config. Because otherwise we would force other adapters to implement the exact SPF check method. What do you think?

Shall I prepare a PR? Or will you do it? Thanks

[wingrunr21]

We wouldn't force anything. Adding the key to Griddler::Configuration would happen on the griddler side, but it would just be an arbitrary hash. The individual adapters would then just look for whatever they want within that key.

I can prepare the PR.

[ciihla]

I just thought that functionality of one gem would depend on configuration of other gem, but maybe I understand it wrong and its okay.. Anyway thank you for your willingness!

[wingrunr21]

This gem already depends on the functionality/configuration of griddler so I don't really see this option as being worse than what is already being done. To me it seems strange to have a config object for a dependency of the parent. IMO you should be doing all config through the parent.

I'm also considering that the other adapters may have a use case for this.

[ciihla]

If you pointed that its a de-facto a parent, you are right. It makes sense to me too now. Although both options make sense to me :)

On the other hand: if griddle changes the DSL how it creates an adapter or passes options to them then it would be needed to change all adapters now, right?

[wingrunr21]

Depends. You can check the arity of new and conditionally pass things to the method. Griddler can also bump their version to reflect the API change and the adapters can depend on the lower version until they make their changes.

[ciihla]

Thats good point too. You are right..