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.
Martin, could you maybe take a look?
(In reply to Katrin Fischer from comment #1) > Martin, could you maybe take a look? Hi Matrin, Can you give us your comments.
(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.
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
(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.
> > "Matrin" made me giggle. Imagine Martin and Katrin fusing into one QA being. 2 QA beings is better than one...
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.
(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.
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"
(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.
Created attachment 107978 [details] [review] Bug 26067 Follow-up added in sysprefs.sql Bug 26067 Follow-up added in sysprefs.sql
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.
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"
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>
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.
(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.
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.
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