Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#34065 closed defect (bug) (fixed)

Upgrading from single site to multisite fails when the Network Admin Email doesn't match an existing user.

Reported by: jjeaton's profile jjeaton Owned by: jeremyfelt's profile jeremyfelt
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.4
Component: Networks and Sites Keywords: has-patch
Focuses: administration, multisite Cc:

Description

While the Network Admin Email is a freeform input field, it appears to only work if used with the email of an existing user. In the current state, the network's main site isn't configured correctly if set up with an email that doesn't exist for a user.

The function populate_network is using the network admin email to retrieve a user and with that user it's setting:

  • The super admins for the main site
  • The admin_email
  • The admin_user_id
  • Setting the 'source_domain' in that user's meta.
  • Setting the 'primary_blog' in that user's meta.

All of those except admin_email require an existing user. To fix we should check whether the email exists for an existing user, if not, bail with an error. This is similar to what happens if the field is submitted with something that doesn't pass email validation.

We could also consider changing the input field to use autocomplete, and only allow selecting of email addresses for users that exist, as it appears that on initial upgrade, Network Admin Email should be an email for an existing user. We could also update the field to indicate that requirement.

Steps to reproduce:

  1. Install WordPress.
  2. Add define( 'WP_ALLOW_MULTISITE', true ); to wp-config.php
  3. Navigate to /wp-admin/network.php
  4. Leave the defaults, but change the 'Network Admin Email' to an email address that isn't used for any user already in the database.
  5. Click 'Install'.

Actual Results

The page reloads with 6 notices in schema.php, see screenshot below. Also because of how the $site_user data is used, the sitemeta for the main site becomes invalid.

https://cldup.com/Srsks9TLID-1200x1200.png

Questions:

  • Even after the initial upgrade, is Network Admin Email required to be associated with an existing user?
  • If not, is there another way to upgrade to multisite and not require the Network Admin Email to be associated with a user? Is that something worth pursuing or should we just fix the messaging and enforce it?

Attachments (2)

34065.diff (532 bytes) - added by jjeaton 9 years ago.
34065.2.diff (1.8 KB) - added by jjeaton 9 years ago.
refreshed to use submitted email as admin_email

Download all attachments as: .zip

Change History (12)

@jjeaton
9 years ago

#1 @jjeaton
9 years ago

  • Keywords has-patch added

Added patch 34065.diff which bails out of the multisite upgrade process if the network admin email doesn't exist for an existing user.

#2 @jeremyfelt
9 years ago

  • Milestone changed from Awaiting Review to 4.4

Hey @jjeaton, nice catch.

Even if the email address is invalid, we still add the user performing the network setup as a super admin. So the only thing that really gets broken here is the site meta. At least there's that. :)

I like 34065.diff in that it cuts straight to the chase and prevents the installation. It would probably be nice for that screen to only allow the selection of an existing user one day—or really just not show that info and use the current user. We do pre-fill the email address, which is good.

I'd be okay adding the catch for 4.4, but if anybody has other ideas, let's discuss.

Also amusing to note that we check is_email() after we've already attempted to get a user with it.

#3 follow-up: @jjeaton
9 years ago

@jeremyfelt I like this option:

or really just not show that info and use the current user

While maybe there would be a case where one admin was upgrading the site to multisite and wanted another admin account to be the admin_email user, I'm not sure how common that would be.

In addition to the existing patch, would we want to add a description to the field that mentions it should correspond to an existing user?

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


9 years ago

#5 in reply to: ↑ 3 @jeremyfelt
9 years ago

  • Keywords needs-patch added; has-patch removed

Replying to jjeaton:

While maybe there would be a case where one admin was upgrading the site to multisite and wanted another admin account to be the admin_email user, I'm not sure how common that would be.

It turns out this is much more common than I thought. This email is often a catch-all box for notifications and acts as a way to separate the admin's email from that attached to emails that users get when registering.

We came up with this during office hours yesterday as a good start:

  • Check for valid $site_user based on the entered email.
  • If not valid, assign the provided email to admin_email, but ensure the current user is set for everything else.
  • Update messaging on the initial screen to match. Related: #34293.

@jjeaton
9 years ago

refreshed to use submitted email as admin_email

#6 @jjeaton
9 years ago

  • Keywords has-patch added; needs-patch removed

Refreshed patch 34065.2.diff addresses all three items.

  • The is_email() check now happens before attempting to get a user based on the email.
  • The provided email is always used for admin_email, and if it doesn't match an existing user, $site_user is set to the current user.
  • The field description on network.php has been updated to read: "This address is used for admin purposes, like site notifications. Registration emails will be sent from this address."

This ticket was mentioned in Slack in #core-multisite by jjeaton. View the logs.


9 years ago

#8 @jeremyfelt
9 years ago

  • Owner set to jeremyfelt
  • Status changed from new to reviewing

#9 @jeremyfelt
9 years ago

This looks great and 34065.2.diff is sane and works for me in testing. I'm going to defer the string change to #34293 as an enhancement for 4.5 as it deserves a bit more discussion.

#10 @jeremyfelt
9 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 35575:

MS: Allow "Network Admin Email" to be a non-user during network setup.

The network admin email setting for a network is often used as a catch-all or notification email separate from the actual user ID set as the owner of the new network. If a non-user email address is set during network installation, we can defer to the current user as the actual network admin and apply the entered email as the address to which general notifications are sent and emails are sent from.

In the future, we'll want to update the messaging around "Network Admin Email" to reflect its reality. See #34293.

Props jjeaton.
Fixes #34065.

Note: See TracTickets for help on using tickets.