Bug 26067 - Member email validation based on MX record
Summary: Member email validation based on MX record
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Amit Gupta
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-07-26 14:41 UTC by Amit Gupta
Modified: 2021-01-19 21:22 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 26067 Member email validation based on MX record. (5.12 KB, patch)
2020-08-09 16:20 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 26067 Follow-up added in sysprefs.sql (1.59 KB, patch)
2020-08-09 18:13 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 26067 Member email validation based on MX record. (6.61 KB, patch)
2020-08-10 01:34 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 26067 Member email validation based on MX record. (6.64 KB, patch)
2020-08-19 15:35 UTC, Brandon J
Details | Diff | Splinter Review
Bug 26067: Member email validation based on MX record. (8.04 KB, patch)
2020-08-22 15:41 UTC, Amit Gupta
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Amit Gupta 2020-07-26 14:41:47 UTC
Currently we are checking Email validation of member (both Staff and OPAC interface)based on the Email::Valid->address but we are not checking the MX record of the domain name,

We have encountered the issue with one of our client while doing the email validation through PatronSelfRegistration.

I am planning to add below code.

Email::Valid->address( -address => 'amitddng135@gmail.com',  -mxcheck => 1 );
instead of  Email::Valid->address('amitddng135@gmail.com');

Can you provide us your feedback. Should we do fix code through system preference for ex: "EmailValidationMXrecord" or without system preference.
Comment 1 Katrin Fischer 2020-07-27 15:15:40 UTC
Martin, could you maybe take a look?
Comment 2 Amit Gupta 2020-07-28 16:27:32 UTC
(In reply to Katrin Fischer from comment #1)
> Martin, could you maybe take a look?
Hi Matrin,

Can you give us your comments.
Comment 3 David Cook 2020-07-29 02:55:21 UTC
(In reply to Amit Gupta from comment #2)
> (In reply to Katrin Fischer from comment #1)
> > Martin, could you maybe take a look?
> Hi Matrin,

"Matrin" made me giggle. Imagine Martin and Katrin fusing into one QA being.
Comment 4 David Cook 2020-07-29 03:00:18 UTC
Using -mxcheck sounds interesting! 

My first thoughts are that it presents these problems:
1) Could add latency
2) Relies on DNS being available and up-to-date
2b) Might give false negative and make an email seem invalid even when it's valid

I'd say write a patch and let people thoroughly test it
Comment 5 Amit Gupta 2020-07-29 03:42:17 UTC
(In reply to David Cook from comment #4)
> Using -mxcheck sounds interesting! 
> 
> My first thoughts are that it presents these problems:
> 1) Could add latency
> 2) Relies on DNS being available and up-to-date
> 2b) Might give false negative and make an email seem invalid even when it's
> valid
> 
> I'd say write a patch and let people thoroughly test it

Hi David,

Thank for the feedback.

I have checked and latency is not an issue in case Koha installation is on cloud for ex: AWS or Azure.
May be problem in case of local installation of Koha.

I think we can go by system preferences yes or no to mx check.

I did some basic testing mxrecords and gives me correct result.

Will write the patch and share for the testing.
Comment 6 Katrin Fischer 2020-07-29 07:36:34 UTC
> 
> "Matrin" made me giggle. Imagine Martin and Katrin fusing into one QA being.

2 QA beings is better than one...
Comment 7 Martin Renvoize 2020-08-04 13:37:23 UTC
I think a syspref sounds optimal.. as you say, there may be issues with locally hosted installations and I'm not 100% sure on how widely supported MX lookup is.. but I do think it's a great idea to add the option.. probably enabled by default too.
Comment 8 Amit Gupta 2020-08-04 13:41:42 UTC
(In reply to Martin Renvoize from comment #7)
> I think a syspref sounds optimal.. as you say, there may be issues with
> locally hosted installations and I'm not 100% sure on how widely supported
> MX lookup is.. but I do think it's a great idea to add the option.. probably
> enabled by default too.

Thank Martin for the feedback. System preference is a good idea. 

Will make the patch on master branch and submit for review.
Comment 9 Amit Gupta 2020-08-09 16:20:09 UTC
Created attachment 107975 [details] [review]
Bug 26067 Member email validation based on MX record.

To Test
Currently we are doing testing for Primary email address from the OPAC page.
1. Enable system preference PatronSelfRegistration.
2. Set PatronSelfRegistrationDefaultCategory for ex: ST
3. Set PatronSelfRegistrationLibraryList for ex: MPL
4. Open the page /cgi-bin/koha/opac-memberentry.pl
5. Fill the form and enter email address in Primary email for ex: amit@gmail12.com you will able to submit the page and it will not give any error message.
6. Apply the patch.
7. Enable system preference PatronEmailValidationMXrecord set as Require.
8. Open the /cgi-bin/koha/opac-memberentry.pl again.
9. Fill the form and enter email address in Primary email for ex: amit@gmail12.com and click on Submit button and it will give error message "Invalid primary email address".

---------------------------------------
Currently in this patch we are checking on "Primary Email address". If this is OK we can do check for "Secondary email" and Alternate "Email address"
Comment 10 Amit Gupta 2020-08-09 16:23:39 UTC
(In reply to Amit Gupta from comment #9)
> Created attachment 107975 [details] [review] [review]
> Bug 26067 Member email validation based on MX record.
> 
> To Test
> Currently we are doing testing for Primary email address from the OPAC page.
> 1. Enable system preference PatronSelfRegistration.
> 2. Set PatronSelfRegistrationDefaultCategory for ex: ST
> 3. Set PatronSelfRegistrationLibraryList for ex: MPL
> 4. Open the page /cgi-bin/koha/opac-memberentry.pl
> 5. Fill the form and enter email address in Primary email for ex:
> amit@gmail12.com you will able to submit the page and it will not give any
> error message.
> 6. Apply the patch.
> 7. Enable system preference PatronEmailValidationMXrecord set as Require.
> 8. Open the /cgi-bin/koha/opac-memberentry.pl again.
> 9. Fill the form and enter email address in Primary email for ex:
> amit@gmail12.com and click on Submit button and it will give error message
> "Invalid primary email address".
> 
> ---------------------------------------
> Currently in this patch we are checking on "Primary Email address". If this
> is OK we can do check for "Secondary email" and Alternate "Email address"

By default system preference "PatronEmailValidationMXrecord" is don't require.
Comment 11 Amit Gupta 2020-08-09 18:13:53 UTC
Created attachment 107978 [details] [review]
Bug 26067 Follow-up added in sysprefs.sql

Bug 26067 Follow-up added in sysprefs.sql
Comment 12 Amit Gupta 2020-08-10 01:34:48 UTC
Created attachment 107979 [details] [review]
Bug 26067 Member email validation based on MX record.

To Test
Currently we are doing testing for Primary email address from the OPAC page.
1. Enable system preference PatronSelfRegistration
2. Set PatronSelfRegistrationDefaultCategory for ex: ST
3. Set PatronSelfRegistrationLibraryList for ex: MPL
4. Open the page /cgi-bin/koha/opac-memberentry.pl
5. Fill the form and enter email address in Primary email for ex:
   amit@gmail.c you will able to submit the page and it will not give any error message.
6. Apply the patch
7. Check system preference PatronEmailValidationMXrecord by default it show "Require".
8. Open the /cgi-bin/koha/opac-memberentry.pl again
9. Fill the form and enter email address in Primary email for ex:
   amit@gmail.c and click on Submit button and it will give error message "Invalid primary email address".
--------------------------
Thanks Martin 
set as default enabled "PatronEmailValidationMXrecord" as Require.
Comment 13 Amit Gupta 2020-08-10 01:35:58 UTC
Currently in this patch we are checking on "Primary Email address". If this is OK we can do check for "Secondary email" and Alternate "Email address"
Comment 14 Brandon J 2020-08-19 15:35:01 UTC
Created attachment 108660 [details] [review]
Bug 26067 Member email validation based on MX record.

To Test
Currently we are doing testing for Primary email address from the OPAC page.
1. Enable system preference PatronSelfRegistration
2. Set PatronSelfRegistrationDefaultCategory for ex: ST
3. Set PatronSelfRegistrationLibraryList for ex: MPL
4. Open the page /cgi-bin/koha/opac-memberentry.pl
5. Fill the form and enter email address in Primary email for ex:
   amit@gmail.c you will able to submit the page and it will not give any error message.
6. Apply the patch
7. Check system preference PatronEmailValidationMXrecord by default it show "Require".
8. Open the /cgi-bin/koha/opac-memberentry.pl again
9. Fill the form and enter email address in Primary email for ex:
   amit@gmail.c and click on Submit button and it will give error message "Invalid primary email address".

Signed-off-by: Brandon J <brandon.jimenez@inLibro.com>
Comment 15 Katrin Fischer 2020-08-22 13:37:17 UTC
Hi Amit,

1) sysprefs.sql 
is sorted alphabetically, can you please move the new pref line into the right spot?

2) opac.pref
Doesn't quite work yet as a sentence and there is a little typo on 'no'. Maybe:

Now:
+            - pref: PatronEmailValidationMXrecord
+              choices:
+                  yes: Require
+                  no: "Dont't require"
+            - "Mail Exchange (MX) records are DNS records that are necessary for delivering email to Patron."
+        -

Suggestion:
+            - pref: PatronEmailValidationMXrecord
+              choices:
+                  yes: Require
+                  no: "Don't require"
+            - "email address entered on patron self registration to pass Mail Exchange (MX) record check."

3) As we allow to use every patron email address as the 'to' address we sent email to (see AutoEmailPrimaryAddress), I'd suggest to make this change for all the email address fields.
Comment 16 Amit Gupta 2020-08-22 13:44:05 UTC
(In reply to Katrin Fischer from comment #15)
> Hi Amit,
> 
> 1) sysprefs.sql 
> is sorted alphabetically, can you please move the new pref line into the
> right spot?
> 
> 2) opac.pref
> Doesn't quite work yet as a sentence and there is a little typo on 'no'.
> Maybe:
> 
> Now:
> +            - pref: PatronEmailValidationMXrecord
> +              choices:
> +                  yes: Require
> +                  no: "Dont't require"
> +            - "Mail Exchange (MX) records are DNS records that are
> necessary for delivering email to Patron."
> +        -
> 
> Suggestion:
> +            - pref: PatronEmailValidationMXrecord
> +              choices:
> +                  yes: Require
> +                  no: "Don't require"
> +            - "email address entered on patron self registration to pass
> Mail Exchange (MX) record check."
> 
> 3) As we allow to use every patron email address as the 'to' address we sent
> email to (see AutoEmailPrimaryAddress), I'd suggest to make this change for
> all the email address fields.

Thank Katrin for QA will do the changes.
Comment 17 Amit Gupta 2020-08-22 15:41:40 UTC
Created attachment 108932 [details] [review]
Bug 26067: Member email validation based on MX record.

To Test
1. Enable system preference PatronSelfRegistration
2. Set PatronSelfRegistrationDefaultCategory for ex: ST
3. Set PatronSelfRegistrationLibraryList for ex: MPL
4. Open the page /cgi-bin/koha/opac-memberentry.pl
5. Fill the form and enter email address in Primary email, Secondary email and B_email for ex:
   amit@gmail.c, amit1@gmail.c, amitgupta@gmail.c you will able to submit the page and it will not give any error message.
6. Apply the patch
7. Check system preference PatronEmailValidationMXrecord by default it show "Require".
8. Open the /cgi-bin/koha/opac-memberentry.pl again
9. Fill the form and enter email address in Primary email, Secondary email and B_email for ex:
   amit@gmail.c, amit1@gmail.c, amitgupta@gmail.c and click on Submit button and it will give error message
   "Invalid primary email address", "Invalid secondary email address", "Invalid email address".
10. This patch will check email, emailpro and B_email address field.

--------------------------
Fix QA comments
This patch will check email, emailpro and B_email address field.
Comment 18 Mazen Khallaf 2021-01-19 21:22:10 UTC
Could not apply the patch in step 6.

$ git bz apply 26067
Bug 26067 - Member email validation based on MX record

108932 - Bug 26067: Member email validation based on MX record.

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 26067: Member email validation based on MX record.
.git/rebase-apply/patch:46: trailing whitespace.
            - "email address entered on patron self registration to pass Mail Exchange (MX) record check." 
.git/rebase-apply/patch:76: trailing whitespace.
            push(@invalidFields, "mxrecord_email") if ( !Email::Valid->address( -address => $borrower->{'email'}, -mxcheck => 1));    
.git/rebase-apply/patch:86: trailing whitespace.
            push(@invalidFields, "mxrecord_emailpro") if (!Email::Valid->address( -address => $borrower->{'emailpro'}, -mxcheck => 1)); 
.git/rebase-apply/patch:94: trailing whitespace.
            push(@invalidFields, "mxrecord_b_email") if (!Email::Valid->address( -address => $borrower->{'B_email'}, -mxcheck => 1)); 
warning: 4 lines add whitespace errors.
Using index info to reconstruct a base tree...
A	installer/data/mysql/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
M	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt
M	opac/opac-memberentry.pl
.git/rebase-apply/patch:46: trailing whitespace.
            - "email address entered on patron self registration to pass Mail Exchange (MX) record check." 
.git/rebase-apply/patch:76: trailing whitespace.
            push(@invalidFields, "mxrecord_email") if ( !Email::Valid->address( -address => $borrower->{'email'}, -mxcheck => 1));    
.git/rebase-apply/patch:86: trailing whitespace.
            push(@invalidFields, "mxrecord_emailpro") if (!Email::Valid->address( -address => $borrower->{'emailpro'}, -mxcheck => 1)); 
.git/rebase-apply/patch:94: trailing whitespace.
            push(@invalidFields, "mxrecord_b_email") if (!Email::Valid->address( -address => $borrower->{'B_email'}, -mxcheck => 1)); 
warning: 4 lines applied after fixing whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-memberentry.pl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging installer/data/mysql/mandatory/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/mandatory/sysprefs.sql
error: Failed to merge in the changes.
Patch failed at 0001 Bug 26067: Member email validation based on MX record.
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-26067-Member-email-validation-based-on-MX-reco-EGPSaM.patch