Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Email field should not be filled with example.com domain #93

Closed
henryk opened this issue Nov 9, 2015 · 3 comments · Fixed by #94
Closed

Email field should not be filled with example.com domain #93

henryk opened this issue Nov 9, 2015 · 3 comments · Fixed by #94

Comments

@henryk
Copy link

henryk commented Nov 9, 2015

Currently, when the LDAP user entry does not have an email field value, authLdap will set a made-up e-mail address consisting of user name at example.com:

$user_info['user_email'] = $username . '@example.com';

This is both completely unexpected and rather bad from a number of perspectives. In my tests nothing seems to break when just completely removing that line. This is on WordPress 4.3.1 with authLdap 1.4.10 against OpenLDAP on Ubuntu 14.04 LTS.

Steps to reproduce

  1. Have a user foo in LDAP without email field
  2. The user logs in to WordPress with authLdap

Actual results

The user foo is created in WordPress, with email set to [email protected]

Expected results

The user foo is created in WordPress, with no email field.

@heiglandreas
Copy link
Owner

Thanks @henryk for raising this very detailed issue!

But as the Email-Address is a required field in Wordpress for the user your test works fine for the first* user without an email-address. As soon as you try to add the second user without an email-address it will fail as in https://github.com/WordPress/WordPress/blob/master/wp-includes/user-functions.php#L1393 a check is done for a unique email-address. And an empty address is a unique one - but only once. And this check does not seem to be circumventable without reinventing the wheel (rewriting the complete function).

Therefore I've decided to add a unique address by using the users username (which is unique within your wordpress-installation) and the example.com-domain which is not routed in the internet and only exists for testing-purposes. So it is rather easy to filter those addresses from the database and replace them with something different. This will only happen when the logging in user doesn't have an email-address set in the LDAP or with a misconfigured authLDAP-plugin.

To get a different email-address than the default one you can also use the pre_user_email-filter to alter the email-address to something more suitable to your specific needs like so:

add_filter('pre_user_email', function($email){

    // return ''; // will work the first time and fail after that!
    return mt_rand(0, 100000) . '@whatever.you.like';
}, 10, 1);

But this will still require to be unique within your WP-installations user-base.

@henryk
Copy link
Author

henryk commented Nov 10, 2015

I disagree. Observe:

mysql> select ID, user_email from wp_users where user_email = '';
+----+------------+
| ID | user_email |
+----+------------+
|  4 |            |
|  6 |            |
|  9 |            |
| 10 |            |
| 11 |            |
| 14 |            |
+----+------------+

The reason is here: https://github.com/WordPress/WordPress/blob/4.3.1/wp-includes/capabilities.php#L593
The path to there is as follows: wp_insert_user() has a && email_exists( $user_email ) in the if that you linked to. email_exists() calls get_user_by( 'email', … ) which calls WP_User::get_data_by( … ), which returns false if looking up by empty email.

On a different front: https://codex.wordpress.org/Function_Reference/wp_insert_user doesn't indicate a requirement to have an email address. In fact, it provides an example that does not set an email address.

@heiglandreas
Copy link
Owner

My apologies. It seems a lot has changed since the last time I've actually tested that 😉

I will test that and if adding a user without an email-address will work (which I expect) I will change that to adding an email-address only when one is supplied instead of setting a fake address!

Thanks again for raising this! Stay tuned

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants