43251 add a check for editable roles #6808

ideag · 2024-06-13T12:16:20Z

This adds a check to the create/add user to blog screen to match and allow only the roles that are displayed in the UI.

Trac ticket: https://core.trac.wordpress.org/ticket/43251

…blog in a multisite

github-actions · 2024-06-13T12:16:41Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props ideag.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-06-13T12:29:12Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

peterwilsoncc

I've added a few suggestions inline.

My one other thought is that it might be preferable to move the new function to be available on single sites and use it in src/wp-admin/users.php too. (@spacedmonkey may have some thoughts here)

peterwilsoncc · 2024-07-31T01:15:09Z

src/wp-admin/includes/ms.php

+		wp_die(
+			'<h1>' . __( 'You need a higher level of permission.' ) . '</h1>' .
+			'<p>' . __( 'Sorry, you are not allowed to assign users to this role.' ) . '</p>',
+			403
+		);


As the filter prevents users from adding the role, the level of permission heading is misleading.

I've modified the message to match the one for single sites. WordPress aims to reuse strings where possible to avoid the need for similar strings to be translated.

Suggested change

wp_die(

'<h1>' . __( 'You need a higher level of permission.' ) . '</h1>' .

'<p>' . __( 'Sorry, you are not allowed to assign users to this role.' ) . '</p>',

403

);

wp_die( __( 'Sorry, you are not allowed to give users that role.' ), 403 );

peterwilsoncc · 2024-07-31T01:18:10Z

src/wp-admin/includes/ms.php

+/**
+ * Makes sure the passed $role is part of editable_roles
+ *
+ * @since 6.7.0
+ *
+ * @param string $role - name of the role
+ * @return void
+ */
+
+function ensure_editable_role( $role ) {


The main change is to rename the function to use the wp_ prefix

Suggested change

/**

* Makes sure the passed $role is part of editable_roles

*

* @since 6.7.0

*

* @param string $role - name of the role

* @return void

*/

function ensure_editable_role( $role ) {

/**

* Stop execution if the role can not be assigned by the current user.

*

* @since 6.7.0

*

* @param string $role Role the user is attempting to assign.

*/

function wp_ensure_editable_role( $role ) {

peterwilsoncc · 2024-07-31T01:18:31Z

src/wp-admin/user-new.php

@@ -67,6 +67,9 @@
 		$redirect = add_query_arg( array( 'update' => 'addexisting' ), 'user-new.php' );
 	} else {
 		if ( isset( $_POST['noconfirmation'] ) && current_user_can( 'manage_network_users' ) ) {
+
+			ensure_editable_role( $_REQUEST['role'] );


Suggested change

ensure_editable_role( $_REQUEST['role'] );

wp_ensure_editable_role( $_REQUEST['role'] );

peterwilsoncc · 2024-07-31T01:18:41Z

src/wp-admin/user-new.php

@@ -218,6 +221,9 @@
 				add_filter( 'wpmu_signup_user_notification', '__return_false' );  // Disable confirmation email.
 				add_filter( 'wpmu_welcome_user_notification', '__return_false' ); // Disable welcome email.
 			}
+
+			ensure_editable_role( $_REQUEST['role'] );


Suggested change

ensure_editable_role( $_REQUEST['role'] );

wp_ensure_editable_role( $_REQUEST['role'] );

peterwilsoncc · 2024-07-31T01:38:00Z

tests/phpunit/tests/multisite/wpmuValidateUserSignup.php

+		public function test_ensure_editable_role() {
+			$exception = null;
+			try {
+				ensure_editable_role( 'editor' );
+			} catch ( WPDieException $e ) {
+				$exception = $e;
+			}
+			$this->assertNull( $exception );
+
+			$exception = null;
+			try {
+				ensure_editable_role( 'non-existant' );
+			} catch ( WPDieException $e ) {
+				$exception = $e;
+			}
+			$this->assertNotNull( $exception );
+			$this->assertStringContainsString( 'Sorry, you are not allowed to assign users to this role.', $exception->getMessage() );
+
+			$exception = null;
+			try {
+				add_filter(
+					'editable_roles',
+					function ( $roles ) {
+						unset( $roles['administrator'] );
+						return $roles;
+					}
+				);
+
+				ensure_editable_role( 'administrator' );
+
+				remove_filter(
+					'editable_roles',
+					function ( $roles ) {
+						unset( $roles['administrator'] );
+						return $roles;
+					}
+				);
+			} catch ( WPDieException $e ) {
+				$exception = $e;
+			}
+			$this->assertNotNull( $exception );
+			$this->assertStringContainsString( 'Sorry, you are not allowed to assign users to this role.', $exception->getMessage() );
+		}


There's a few things happening here:

I've added docblocks and the ticket number to the tests so they can be run as their own group

I've split the tests up in to the three cases: editable role, unknown role, excluded role

I've used $this->expectException( 'WPDieException' ); to test for the wp_die() call to remove the need for try...catch

I've used the name change suggested above.

The WordPress test suite resets filters between tests so the remove_filter() isn't needed.

Suggested change

public function test_ensure_editable_role() {

$exception = null;

try {

ensure_editable_role( 'editor' );

} catch ( WPDieException $e ) {

$exception = $e;

}

$this->assertNull( $exception );

$exception = null;

try {

ensure_editable_role( 'non-existant' );

} catch ( WPDieException $e ) {

$exception = $e;

}

$this->assertNotNull( $exception );

$this->assertStringContainsString( 'Sorry, you are not allowed to assign users to this role.', $exception->getMessage() );

$exception = null;

try {

add_filter(

'editable_roles',

function ( $roles ) {

unset( $roles['administrator'] );

return $roles;

}

);

ensure_editable_role( 'administrator' );

remove_filter(

'editable_roles',

function ( $roles ) {

unset( $roles['administrator'] );

return $roles;

}

);

} catch ( WPDieException $e ) {

$exception = $e;

}

$this->assertNotNull( $exception );

$this->assertStringContainsString( 'Sorry, you are not allowed to assign users to this role.', $exception->getMessage() );

}

/**

* Ensure that wp_ensure_editable_role does not throw an exception when the role is editable.

*

* @ticket 43251

*

* @covers ::wp_ensure_editable_role

*/

public function test_wp_ensure_editable_role_allows_editable_roles() {

$role = get_role( 'editor' );

$this->assertInstanceOf( 'WP_Role', $role, 'The editor role should exist.' );

$this->assertNull( wp_ensure_editable_role( 'editor' ), 'The editor role should be editable.' );

}

/**

* Ensure that wp_ensure_editable_role throws an exception for non-existent roles.

*

* @ticket 43251

*

* @covers ::wp_ensure_editable_role

*/

public function test_wp_ensure_editable_role_does_not_allow_non_existent_role() {

$this->expectException( 'WPDieException' );

$role = get_role( 'non-existent-role' );

$this->assertNotInstanceOf( 'WP_Role', $role, 'The non-existent-role role should not exist.' );

wp_ensure_editable_role( 'non-existent-role' );

}

/**

* Ensure that wp_ensure_editable_role throws an exception for roles that are not editable.

*

* @ticket 43251

*

* @covers ::wp_ensure_editable_role

*/

public function test_wp_ensure_editable_role_does_not_allow_uneditable_roles() {

add_filter(

'editable_roles',

function ( $roles ) {

unset( $roles['editor'] );

return $roles;

}

);

$this->expectException( 'WPDieException' );

$role = get_role( 'editor' );

$this->assertInstanceOf( 'WP_Role', $role, 'The editor role should exist.' );

wp_ensure_editable_role( 'editor' );

}

peterwilsoncc · 2024-07-31T01:39:59Z

src/wp-admin/includes/ms.php

}
+

Removes an additional line break.

Suggested change

}

}

43251 add a check for editable roles when creating/assigning user to …

5667786

…blog in a multisite

ideag added 4 commits June 13, 2024 16:32

43251 make the change more testable

3fdce98

43251 add unit tests

528f314

43251 update code style

782fe07

43251 fix formatting

8d30374

peterwilsoncc reviewed Jul 31, 2024

View reviewed changes

Sukhendu2002 mentioned this pull request Dec 12, 2024

Fix: Improve editable_roles validation in multisite environments #7999

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

43251 add a check for editable roles #6808

43251 add a check for editable roles #6808

ideag commented Jun 13, 2024

github-actions bot commented Jun 13, 2024

github-actions bot commented Jun 13, 2024

peterwilsoncc left a comment

peterwilsoncc Jul 31, 2024

peterwilsoncc Jul 31, 2024

peterwilsoncc Jul 31, 2024

peterwilsoncc Jul 31, 2024

peterwilsoncc Jul 31, 2024

peterwilsoncc Jul 31, 2024

	ensure_editable_role( $_REQUEST['role'] );
	wp_ensure_editable_role( $_REQUEST['role'] );

43251 add a check for editable roles #6808

Are you sure you want to change the base?

43251 add a check for editable roles #6808

Conversation

ideag commented Jun 13, 2024

github-actions bot commented Jun 13, 2024

github-actions bot commented Jun 13, 2024

Test using WordPress Playground

Some things to be aware of

peterwilsoncc left a comment

Choose a reason for hiding this comment

peterwilsoncc Jul 31, 2024

Choose a reason for hiding this comment

peterwilsoncc Jul 31, 2024

Choose a reason for hiding this comment

peterwilsoncc Jul 31, 2024

Choose a reason for hiding this comment

peterwilsoncc Jul 31, 2024

Choose a reason for hiding this comment

peterwilsoncc Jul 31, 2024

Choose a reason for hiding this comment

peterwilsoncc Jul 31, 2024

Choose a reason for hiding this comment