Use email.utils to parse the from domain(s)

[adam-iris]

This probably isn't ready to merge!

I added a unit test, and it seems outside the scope of this fix but the test message returns dkim=fail. I probably did something wrong in how I generated the signature (although the body b= key looks right) but I could be missing something deeper.

[adam-iris]

Sorry I got pulled off this project. I think the code is ready, the only hiccup was that I couldn’t add a working unit test because I couldn’t generate a proper key to test against. That’s really outside the scope of the fix, though, so it doesn’t seem critical.

…

On Apr 2, 2020, at 9:38 PM, niftylettuce ***@***.***> wrote:  Is this ready yet? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[kitterma]

Please do add tests. If you look in the authheaders/test directory there is a sample key you can use there. Have a look at the other tests to see how it's used.

[adam-iris]

You can see the (failing) unit test in the history, the issue was that I had to create a new message for the test, but I wasn't able to generate a DKIM signature for it that validated. That validation is really outside the scope of the change -- which was mainly about switching to a standard package for parsing incoming email addresses -- so I wound up removing it.

I hadn't thought about it this way before, but I'd say this doesn't really require a separate unit test, because it already gets tested by the existing ones. That is, all the existing unit tests use the new code for parsing email addresses, so they adequately test whether this code is calling the library correctly. So all my test was adding was whether the library itself properly parses some edge cases, which isn't really relevant here.

TL;DR: I think this is ready to go

[niftylettuce]

Would love to see this added!

[shaunwarman]

+1

[niftylettuce]

I tested this and it's broken by the way, see my comments here #5 and #13, even with the latest recommended fixes (ref: https://github.com/forwardemail/authentication-headers).

[niftylettuce]

@adam-iris I have numerous other fixes in my branch, which you might want to sync up with yours and add to your pull request to make it easy for the ValiMail repo owners to merge.

e.g. #13 (comment)

See the commit history here:

https://github.com/forwardemail/authentication-headers/commits/master

[niftylettuce]

I have published another fix for yet another issue, forwardemail/authentication-headers@1046cd7 (see #17).

[niftylettuce]

Another fix has been published, see #18 and forwardemail/authentication-headers@b123501.

[niftylettuce]

Another fix can be found here: forwardemail/authentication-headers@54d1bc2

[kitterma]

OK, so over a year later, I'm back looking at this (sorry). I see the point of what you're trying to do and I agree it makes sense. I'll see about setting up the test case.