michaelryanmcneill / shibboleth

Shibboleth plugin for WordPress
https://wordpress.org/plugins/shibboleth/
19 stars 11 forks source link

Email comparisons should be case insensitive #39

Closed mrbrown8 closed 6 years ago

mrbrown8 commented 6 years ago

Centralized IT may have shibbolized email addresses differently than what a departmental web admin may have for his/her local users, even though they're both technically correct. One entity may use mixed case versus another. Comparisons of email addresses should be case-insensitive when a user tries to manually link his/her account with Shibboleth.

diff -Naur public_html/wp-content/plugins/shibboleth/options-user.php public_html_modified/wp-content/plugins/shibboleth/options-user.php
--- public_html/wp-content/plugins/shibboleth/options-user.php  2018-03-12 14:39:51.000000000 -0600
+++ public_html_modified/wp-content/plugins/shibboleth/options-user.php 2018-03-13 16:40:15.386361724 -0600
@@ -218,7 +218,7 @@
                                                                        wp_safe_redirect( get_edit_user_link() . '?shibboleth=failed' );
                                                                        exit;
                                                                }
-                                               } elseif ( $user->user_email == $email && $allowed === 'bypass' ) {
+                                               } elseif ( $user->user_email === $email && $allowed === 'bypass' ) {
                                                        update_user_meta( $user->ID, 'shibboleth_account', true );
                                                        wp_safe_redirect( get_edit_user_link() . '?shibboleth=linked' );
                                                        exit;
jrchamp commented 6 years ago

It sounds like you are asking for the check to be switched to a case insensitive check. Looking at the code you provided, I want to note that the difference between == and === is a type check, not a case insensitive check. Instead, you could try strcasecmp( $user->user_email, $email ) === 0

mrbrown8 commented 6 years ago

I /am/ asking for a case insensitive check. My php-fu is not strong. I crudely went by the advise given here:

https://stackoverflow.com/questions/3333353/string-comparison-using-vs-strcmp

My rudimentary printf-style testing indicated to me that === would help where an IdP would format an address one way and a local webmaster would format other.

Whichever is the proper forward-thinking way of doing this is fine, but I am advocating to have this kind check put in.

Thanks in advance.

mrbrown8 commented 6 years ago

Suggestions from here:

https://stackoverflow.com/questions/5473542/case-insensitive-string-comparison

indicate you could also call strtolower() on both variables before applying the == test.

michaelryanmcneill commented 6 years ago

I worry about using strcasecmp() because it has problems with multibyte characters which are allowed in emails now. Instead I think we should do strtolower() on both variables and then apply a strict match (i.e. ===). I think that it would be best to go through the code sometime and switch from loose match (i.e. ==) to strict match (i.e. ===), just to be safe, but that is for another day.

jrchamp commented 6 years ago

I had the same concerns. The question would be: what are other systems doing? Sure, email is supposed to be case-insensitive, but does that only apply to the English alphabet characters as in strtolower() or does it extend to the full Unicode alphabetic set? That might encourage us to use mb_strtolower().

As far as strict matches, I agree that everything should default to strict matches and loose matches should be the exception.

michaelryanmcneill commented 6 years ago

After looking at the docs and the RFC for SMTP, I think using mb_strtolower() is best, but we need to make sure to account for PHP which might not be compiled with --enable-mbstring. I believe most standard packages do compile with that flag, but I'd rather not introduce an incompatibility there. I'll work to get it done.

michaelryanmcneill commented 6 years ago

So, after working on this more, I decided that because of portability issues, using mb_strtolower() was more of a hassle than it was worth. I proceeded with using strtolower() which should address this use case.

mrbrown8 commented 6 years ago

Thank you both for your consideration.