-
Notifications
You must be signed in to change notification settings - Fork 3.7k
#46744 Prevent the default role from being set to a privileged role when user registration is open #12977
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
base: trunk
Are you sure you want to change the base?
#46744 Prevent the default role from being set to a privileged role when user registration is open #12977
Changes from all commits
666257a
e705bed
1689fa8
c6af56f
cbfb5de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9415,3 +9415,43 @@ function wp_verify_fast_hash( | |
|
|
||
| return hash_equals( $hash, wp_fast_hash( $message ) ); | ||
| } | ||
|
|
||
| /** | ||
| * Filters the default role for new user registrations. | ||
| * | ||
| * This ensures that privileged roles are not made available as the default role when user registration is enabled. | ||
| * | ||
| * @since 7.2.0 | ||
| * | ||
| * @param string $default_role The default role for new user registrations. | ||
| * @return string The filtered default role for new user registrations. | ||
| */ | ||
| function filter_default_role( $default_role ) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This isn't only style. It's declared unconditionally and Also: |
||
| static $filtering = false; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why Is there a static variable in use here? It feels unnecessary |
||
|
|
||
| if ( $filtering ) { | ||
| return $default_role; | ||
| } | ||
|
|
||
| $filtering = true; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following on from @aaronjorbin's question about whether this guard is needed — whatever the answer, it currently fails open.
A No test covers this. Nothing in the suite exercises a nested or throwing call, so a broken guard would surface in production rather than CI. |
||
|
|
||
| if ( get_option( 'users_can_register' ) ) { | ||
| /** | ||
| * Filters the roles that are excluded from being available as the default role for new user registrations. | ||
| * | ||
| * @since 7.2.0 | ||
| * | ||
| * @param string[] $roles Roles that are excluded from being available. | ||
| */ | ||
| $excluded = apply_filters( 'default_role_excluded_roles', array( 'administrator', 'editor' ) ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A slug list means a custom role carrying A capability probe over Separately on the hook itself: it's currently removable, and the PR's own |
||
|
|
||
| // Don't allow a privileged default role if users can register. | ||
| if ( in_array( $default_role, $excluded, true ) ) { | ||
| $default_role = 'subscriber'; | ||
| } | ||
| } | ||
|
|
||
| $filtering = false; | ||
|
|
||
| return $default_role; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,213 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * Tests for the filtering of the `default_role` option. | ||
| * | ||
| * @group option | ||
| * @group user | ||
| * | ||
| * @covers ::filter_default_role | ||
| */ | ||
| class Tests_Functions_FilterDefaultRole extends WP_UnitTestCase { | ||
|
|
||
| /** | ||
| * Opens or closes user registration. | ||
| */ | ||
| private function set_users_can_register( bool $can_register ) { | ||
| if ( is_multisite() ) { | ||
| update_site_option( 'registration', $can_register ? 'user' : 'none' ); | ||
| } else { | ||
| update_option( 'users_can_register', $can_register ? 1 : 0 ); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Ensures privileged roles are left alone when user registration is closed. | ||
| * | ||
| * @dataProvider data_excluded_roles | ||
| */ | ||
| public function test_excluded_role_is_unchanged_when_registration_is_closed( string $role ) { | ||
| $this->set_users_can_register( false ); | ||
|
|
||
| $this->assertSame( | ||
| $role, | ||
| filter_default_role( $role ), | ||
| 'The default role was changed while user registration was closed.' | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Ensures privileged roles are replaced with the subscriber role when user registration is open. | ||
| * | ||
| * @dataProvider data_excluded_roles | ||
| */ | ||
| public function test_excluded_role_is_replaced_when_registration_is_open( string $role ) { | ||
| $this->set_users_can_register( true ); | ||
|
|
||
| $this->assertSame( | ||
| 'subscriber', | ||
| filter_default_role( $role ), | ||
| 'The privileged default role was not replaced with the subscriber role.' | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Data provider. | ||
| * | ||
| * @return array[] | ||
| */ | ||
| public static function data_excluded_roles() { | ||
| return array( | ||
| 'administrator' => array( 'administrator' ), | ||
| 'editor' => array( 'editor' ), | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Ensures roles that are not excluded are left alone when user registration is open. | ||
| * | ||
| * @dataProvider data_allowed_roles | ||
| */ | ||
| public function test_allowed_role_is_unchanged_when_registration_is_open( string $role ) { | ||
| $this->set_users_can_register( true ); | ||
|
|
||
| $this->assertSame( | ||
| $role, | ||
| filter_default_role( $role ), | ||
| 'A role which is not excluded was changed.' | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Data provider. | ||
| * | ||
| * @return array[] | ||
| */ | ||
| public static function data_allowed_roles() { | ||
| return array( | ||
| 'author' => array( 'author' ), | ||
| 'contributor' => array( 'contributor' ), | ||
| 'subscriber' => array( 'subscriber' ), | ||
| 'unknown role' => array( 'this-role-does-not-exist' ), | ||
| 'empty string' => array( '' ), | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Ensures a role can be added to the list of excluded roles. | ||
| */ | ||
| public function test_excluded_roles_can_be_added_to() { | ||
| $this->set_users_can_register( true ); | ||
|
|
||
| add_filter( | ||
| 'default_role_excluded_roles', | ||
| static function ( $excluded ) { | ||
| $excluded[] = 'author'; | ||
| return $excluded; | ||
| } | ||
| ); | ||
|
|
||
| $this->assertSame( | ||
| 'subscriber', | ||
| filter_default_role( 'author' ), | ||
| 'A role added to the excluded roles was not replaced with the subscriber role.' | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Ensures a role can be removed from the list of excluded roles. | ||
| */ | ||
| public function test_excluded_roles_can_be_removed() { | ||
| $this->set_users_can_register( true ); | ||
|
|
||
| add_filter( 'default_role_excluded_roles', '__return_empty_array' ); | ||
|
|
||
| $this->assertSame( | ||
| 'administrator', | ||
| filter_default_role( 'administrator' ), | ||
| 'A role removed from the excluded roles was replaced.' | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Ensures a new user is not assigned a privileged role when user registration is open. | ||
| */ | ||
| public function test_new_user_is_not_assigned_a_privileged_role() { | ||
| $this->set_users_can_register( true ); | ||
| update_option( 'default_role', 'administrator' ); | ||
|
|
||
| $user_id = wp_insert_user( | ||
| array( | ||
| 'user_login' => 'test_default_role', | ||
| 'user_pass' => 'password', | ||
| 'user_email' => 'test_default_role@example.org', | ||
| ) | ||
| ); | ||
|
|
||
| $this->assertNotWPError( $user_id, 'The user was not created.' ); | ||
| $this->assertSame( | ||
| array( 'subscriber' ), | ||
| get_userdata( $user_id )->roles, | ||
| 'The new user was assigned a privileged role.' | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Ensures the network registration setting governs the default role on Multisite, even when | ||
| * the `users_can_register` option of the current site is closed. | ||
| * | ||
| * @ticket 46744 | ||
| * @group ms-required | ||
| */ | ||
| public function test_network_registration_governs_when_site_option_is_closed() { | ||
| update_site_option( 'registration', 'user' ); | ||
| update_option( 'users_can_register', 0 ); | ||
|
|
||
| $this->assertSame( | ||
| 'subscriber', | ||
| filter_default_role( 'administrator' ), | ||
| 'The privileged default role was not replaced while network registration was open.' | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Ensures the network registration setting governs the default role on Multisite, even when | ||
| * the `users_can_register` option of the current site is open. | ||
| * | ||
| * @ticket 46744 | ||
| * @group ms-required | ||
| */ | ||
| public function test_network_registration_governs_when_site_option_is_open() { | ||
| update_site_option( 'registration', 'none' ); | ||
| update_option( 'users_can_register', 1 ); | ||
|
|
||
| $this->assertSame( | ||
| 'administrator', | ||
| filter_default_role( 'administrator' ), | ||
| 'The default role was changed while network registration was closed.' | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Ensures a new user is assigned the stored role when user registration is closed. | ||
| */ | ||
| public function test_new_user_is_assigned_the_stored_role_when_registration_is_closed() { | ||
| $this->set_users_can_register( false ); | ||
| update_option( 'default_role', 'editor' ); | ||
|
|
||
| $user_id = wp_insert_user( | ||
| array( | ||
| 'user_login' => 'test_default_role', | ||
| 'user_pass' => 'password', | ||
| 'user_email' => 'test_default_role@example.org', | ||
| ) | ||
| ); | ||
|
|
||
| $this->assertNotWPError( $user_id, 'The user was not created.' ); | ||
| $this->assertSame( | ||
| array( 'editor' ), | ||
| get_userdata( $user_id )->roles, | ||
| 'The new user was not assigned the stored default role.' | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filtering the option on read rather than clamping it on write has three consequences downstream that I don't think are intended. I verified each against trunk:
1. The companion Site Health check can no longer fire.
WP_Site_Health::get_test_insecure_registration()— added in 7.0.0 for this same ticket — does:It reads
get_option( 'default_role' ), which is now filtered. Its critical branch requiresusers_can_registerto be truthy, which is exactly the condition under whichfilter_default_role()has already rewritten the value tosubscriber. So the branch becomes unreachable: the one screen that warned owners about this configuration reports "good" while the database still holdsadministrator.2. An admin can no longer save a corrected value.
update_option()atoption.php:887does$old_value = get_option( $option );— the filtered value — and then returns early:With
administratorstored and registration open,get_option()returnssubscriber, so selecting "Subscriber" in Settings > General writes nothing. "Settings saved", database unchanged. The privileged value stays until someone turns registration off, at which point the clamp lifts and the site silently resumes creating administrators.3.
WP_Roles::remove_role()breaks on the same comparison —class-wp-roles.php:217usesif ( get_option( 'default_role' ) === $role )to reset the default when the role is removed, and that can no longer match the stored value.All three go away if enforcement moves to write time.
sanitize_option()already has adefault_rolecase atformatting.php:5165handling the missing-role fallback, which looks like the natural home;pre_update_option_default_rolewould also work. Stored and effective values then agree, Site Health keeps working, and the settings screen stops disagreeing with the database.