[wp-trac] [WordPress Trac] #43936: Settings: Warn when open registration and new user default is privileged

[WordPress Trac]

#43936: Settings: Warn when open registration and new user default is privileged
-------------------------------------+-----------------------------
 Reporter:  kraftbj                  |       Owner:  SergeyBiryukov
     Type:  defect (bug)             |      Status:  reviewing
 Priority:  normal                   |   Milestone:  5.4
Component:  Users                    |     Version:
 Severity:  normal                   |  Resolution:
 Keywords:  has-patch needs-refresh  |     Focuses:  administration
-------------------------------------+-----------------------------
Changes (by jrf):

 * keywords:  has-patch => has-patch needs-refresh


Comment:

 I've read through this and all related tickets at @ottok's request.

 Great input and patches. Thanks everyone who has contributed so far.

 Based on everything I've read, I see two distinct points of entry to make
 changes:
 1. The default user role selection drop down on the Options -> General
 page (display).
 2. The `update_option()` call to update the value for `default_role`
 (saving).

 For the default user role selection drop down, I would like to suggest the
 following taking all input given into account:
 * Get the `default_role` from the database.
 * If registration is open (`users_can_register` is enabled), allow the
 "excluded roles" to be filtered. By default, set this to `administrator`
 and `editor`.
 * If registration is open, don't allow `administrator` as the default role
 *ever*. The editor role should be allowed, but only when explicitly
 removed from "excluded roles" via the filter, not as a role available by
 default.
 * If registration is open and the output of the filter would have removed
 `administrator` from the "excluded roles", add back `administrator` and
 throw a `_doing_it_wrong()`. This will allow sysadmins to pick up on this
 being attempted in their error logs.
 * Use the output of the "excluded roles" filter in the
 `wp_dropdown_roles()` function as proposed in the current patches to limit
 the roles displayed in the dropdown.
 * If the `default_role` is set to one of the "excluded roles", use
 `subscriber` instead. This will also prevent an existing default role of
 `administrator` coming from the database from being used.

 I agree that using the existing `'editable_roles'` filter which filters
 the roles which will be displayed via the `wp_dropdown_roles()` function
 is not strong enough protection as a secondary filter running after "our"
 filter could undo the removal of `administrator`. The current patches
 already take this into account.

 For the saving of the option part, we could add a filter hooked into
 `pre_update_option_default_role` to check if the value is `administrator`
 and if so and only if registration is open, revert it back to
 `subscriber`.


 I would also like to see some unit tests added for each of these
 situations to safeguard this change from accidentally being reverted in
 the future, think tests along the lines of:
 * Registration open and default role in database is `administrator` =>
 actual default role is set to `subscriber`.
 * Registration open and default role in database is something else =>
 actual default role is the same.
 * Registration closed and default role in database is `administrator` =>
 actual default role is set to `administator`.
 * Registration closed and default role in database is something else =>
 actual default role is the same.
 * Registration open and default role in database is `subscriber`, filter
 on the option changes it to `administrator` => actual default role is set
 to `subscriber`.
 * Registration open and default role in database is `subscriber`, filter
 on the option changes it to `editor` => actual default role is set to
 `editor`.
 * Registration closed and default role in database is `subscriber`, filter
 on the option changes it to `administrator` => actual default role is set
 to `administrator`.
 * Registration closed and default role in database is `subscriber`, filter
 on the option changes it to `editor` => actual default role is set to
 `editor`.
 ... etc ...

 This all could still be bypassed by unhooking the save filter and/or using
 actions before and after the dropdown display to change registration from
 open to closed and back again, but for that a malicious plugin would
 already need to be installed. As far as I c currently see, the above
 proposed process flow couldn't be bypassed just by manipulating URLs.

 Another thing to consider, but this will need further discussion: what
 about adding a check in the upgrade routine for WP 5.4 to verify the
 `default_role` and if 1) registration is open and 2) the default role is
 set to `administrator` change it `subscriber` ?
 This will break expectations for site owners which have set the default
 role to `administrator` on purpose (intranet), but would - in one go -
 make all sites where a hack has taken place which has changed this value
 without the site owner being aware, secure again.

 Either way, I hope this feedback helps.

 > Do you agree? Do you want me to write the patch? Would somebody sponsor
 putting it in then?

 @ottok I'd love to see a patch implementing this and will definitely
 support such a patch to go in.

 > Also this should be changed: https://wordpress.org/support/article
 /settings-general-screen/#new-user-default-role

 @ottok Good catch and yes, I agree.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/43936#comment:15>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform