Bug 22343 introduced a rewrite of Koha::Email. One of the changes it introduced was that it now validates all addresses, not just the 'To' header. This immediately affected anyone with default values like root@localhost. An exchange with the library author [1] highlighted that the way we use Email::Valid is to blame for breaking on @localhost addresses. It also made it clear that this behavior can be overridden by using a trivial flag on the call. While the RFC for email systems specify that FQDN needs to be used, debugging bug 26705 highlighted we need this to allow anyone to test email stuffs. I propose to allow non-fqdn addresses. [1] https://github.com/Perl-Email-Project/Email-Valid/issues/49
Created attachment 118620 [details] [review] Bug 28017: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118621 [details] [review] Bug 28017: Allow non-FQDN (@localhost) addresses This patch makes Koha::Email support using @localhost addresses. To test: 1. Apply the regression tests 2. Run: $ kshell k$ prove t/Koha/Email.t => FAIL: Koha::Email doesn't support non-fqdn addresses 3. Apply this patch 4. Repeat 2 => SUCCESS: All tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118622 [details] [review] Bug 28017: (follow-up) Make controller scripts use -fqdn as well This patch makes the *memberentry.pl scripts use the -fqdn flag as well. This is to allow setting the @localhost addresses. I'm not sure about the need for this patch, as we shouldn't probably allow end-users to use such addresses, and for testing purposes we can set things on the DB. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118644 [details] [review] Bug 28017: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 118645 [details] [review] Bug 28017: Allow non-FQDN (@localhost) addresses This patch makes Koha::Email support using @localhost addresses. To test: 1. Apply the regression tests 2. Run: $ kshell k$ prove t/Koha/Email.t => FAIL: Koha::Email doesn't support non-fqdn addresses 3. Apply this patch 4. Repeat 2 => SUCCESS: All tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 118646 [details] [review] Bug 28017: (follow-up) Make controller scripts use -fqdn as well This patch makes the *memberentry.pl scripts use the -fqdn flag as well. This is to allow setting the @localhost addresses. I'm not sure about the need for this patch, as we shouldn't probably allow end-users to use such addresses, and for testing purposes we can set things on the DB. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signing off because it works as described.. However, I'm not entirely sure about how far it goes. 1. I think we don't need/want the final patch 2. I'm not sure we want to disable the domain checking for all the address headers.. shouldn't we be a bit more selective?
Addresses fields currently affected: * 'To' - Shouldn't we require fully compliant non-local emails here? * 'From' - This one I can understand the need for localhost * 'Reply-to' - Again, this one feels like we ought to require a fully compliant non-local email for * 'Return-path' - This one is a more difficult call.. I could imagine an IP based address being used here maybe * 'Cc' - Again, shouldn't this be a fully compliant email * 'Bcc' - As above. Just some food for thought...
(In reply to Martin Renvoize from comment #8) > Addresses fields currently affected: > > * 'To' - Shouldn't we require fully compliant non-local emails here? > * 'From' - This one I can understand the need for localhost > * 'Reply-to' - Again, this one feels like we ought to require a fully > compliant non-local email for > * 'Return-path' - This one is a more difficult call.. I could imagine an IP > based address being used here maybe > * 'Cc' - Again, shouldn't this be a fully compliant email > * 'Bcc' - As above. > > Just some food for thought... Maybe we need some syspref for enabling this. Like EmailDebug?
(In reply to Martin Renvoize from comment #8) > * 'Return-path' - This one is a more difficult call.. I could imagine an IP > based address being used here maybe Using IPs requires other option switch (-allow_ip), actually.
I spoke to our sysadmin about this.. his suggestion was to allow IP and non-fqdn addresses at this low level.. which would allow people to 'get clever' with the admin settings. But to drop the last patch here, as the more user-facing stuff is best left validated at a fairly strict level :)
Created attachment 118668 [details] [review] Bug 28017: Test IPs are supported Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118696 [details] [review] Bug 28017: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 118697 [details] [review] Bug 28017: Allow non-FQDN (@localhost) addresses This patch makes Koha::Email support using @localhost addresses. To test: 1. Apply the regression tests 2. Run: $ kshell k$ prove t/Koha/Email.t => FAIL: Koha::Email doesn't support non-fqdn addresses 3. Apply this patch 4. Repeat 2 => SUCCESS: All tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 118698 [details] [review] Bug 28017: Test IPs are supported Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 21.05, thanks to everybody involved!
Some tests failing, please fix ASAP t/db_dependent/Letters.t .. 83/84 Fake send_or_die at t/db_dependent/Letters.t line 41. # Failed test 'Exception thrown if invalid email is passed' # at t/db_dependent/Letters.t line 995. # expecting: Koha::Exceptions::BadParameter # found: normal exit # Failed test 'Invalid KohaAdminEmailAddress => status pending' # at t/db_dependent/Letters.t line 999. # got: 'sent' # expected: 'pending' Fake send_or_die at t/db_dependent/Letters.t line 41. # Failed test 'Message 1 status is unchanged' # at t/db_dependent/Letters.t line 1006. # got: 'sent' # expected: 'pending' # Looks like you planned 5 tests but ran 6. # Looks like you failed 3 tests of 6 run.
Created attachment 119208 [details] [review] Bug 28017: (follow-up) Fix Letters.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Follow-up pushed to master.
Pushed to 20.11.x for 20.11.05
Doesn't apply cleanly to 20.05. I'm guessing from comments this depends on 22343 (which is a 20.11 enhancement)? Please rebase if not dependent on 22343 and needed in 20.05.