Expand allowedUsers email field to support comma-separated and domains #9
Conversation
Also, this breaks out a parseLine() function which can be easily tested.
This ignores a trailing comma
main.go
Outdated
| if err != nil { | ||
| return smtpd.Error{Code: 451, Message: "Bad sender address"} | ||
| } | ||
|
|
||
| if email != "" && strings.ToLower(addr) != strings.ToLower(email) { | ||
| if !addrAllowed(addr, allowedAddrs) { |
There was a problem hiding this comment.
You've lost the strings.ToLower(addr) case but i think this should be done in the new addrAllowed()
There was a problem hiding this comment.
Good catch. This was a result of the refactor. I've fixed that, and added a test (cabf848).
main.go
Outdated
| return true | ||
| } | ||
|
|
||
| domidx := strings.LastIndex(addr, "@") |
There was a problem hiding this comment.
Since addr is the sender email it shoud be valid that the address does not contain a domain and @ yet.
There was a problem hiding this comment.
Per below, I was unaware that "local" addresses could reach smtprelay. Updated (see below).
| return false | ||
| } | ||
| domain := strings.ToLower(addr[domidx+1:]) | ||
|
|
There was a problem hiding this comment.
| addr = strings.ToLower(addr) |
There was a problem hiding this comment.
Applied (earlier in the function, so domain is lowercased, too).
main.go
Outdated
| for _, allowedAddr := range allowedAddrs { | ||
| allowedAddr = strings.ToLower(allowedAddr) | ||
|
|
||
| if strings.Index(allowedAddr, "@") == -1 { |
There was a problem hiding this comment.
This is now a conflict because the absence of @ can be because it's a domain or a local address without domain yet.
There was a problem hiding this comment.
Per below, I was unaware that "local" addresses could reach smtprelay. Updated (see below).
|
In two places in In that case, when would the Anyway, with that fact, you are correct: my I think there is an easy fix: To specify an entire (sub)domain, you simply prefix with an Means:
Does that sound reasonable? |
This disambiguates a local user ('john.smith') from a domain ('example.com')
|
Commit c980683 addresses the problems by changing the anyone-at-domain specification to require an |
|
That looks good now. Thanks a lot for your contribution! |
|
Thanks for quickly reviewing and merging! Do you think it would make sense to tag/release Also, I noticed that you seem to have used the "Squash and merge" feature when you merged this. Would you prefer I squash future contributions into a single commit? |
|
Yes, the plan is to release 1.5.0 quite soon. If possible please keep it as separate commits since that is a lot easier to review but in that case the commits were all about one feature so it made sense to squash them. |
👍 As someone who spends much of their day reviewing code, I completely agree. Having clear, concise commits makes code review so much easier.
Great, thanks. I've made all of the changes I'd been considering, and Thanks for |
This closes #8 and implements Option 1.
In addition to the unit tests, this has been tested in production with the following sample:
Then I attempted to send mail (
sendmail):rootLinux usersendmail -r [email protected] [email protected]✔️sendmail [email protected](effective address:[email protected]) ✔️sendmail -r [email protected] [email protected]❌ (failed, as expected)jreinhartLinux user:sendmail [email protected](effective address:[email protected]) ✔️