|
Description
Tomás Cohen Arazi (tcohen)
2021-03-22 17:27:17 UTC
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. |