Skip to content

Fix has_permission() return type inconsistency (Issue #67) #76

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Sep 30, 2025
Merged

Fix has_permission() return type inconsistency (Issue #67) #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c60032b
Rename has_permission() to check_permission() with deprecation logic
Sep 26, 2025
0dba16e
Update tests to use check_permission() and add deprecation test
Sep 26, 2025
209b541
Fix remaining has_permission test usage that was causing CI failure
Sep 26, 2025
5aeab28
Remove specific deprecation test that was causing CI failures
Sep 26, 2025
eb86cc7
Fix internal execute() method to use new check_permission() method
Sep 26, 2025
a44d683
Add test coverage for deprecated has_permission method
Sep 26, 2025
d032fae
Use WordPress test framework method for deprecated function coverage …
Sep 26, 2025
d55a0bb
Switch to @expectedDeprecated PHPDoc annotation for cleaner deprecati…
Sep 26, 2025
e8d93f3
Merge branch 'trunk' into feature/fix-has-permission-return-type-issu…
Ref34t Sep 29, 2025
027c447
Merge branch 'trunk' into feature/fix-has-permission-return-type-issu…
gziolo Sep 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions docs/2.getting-started.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,27 @@ add_action( 'admin_init', 'my_plugin_use_ability' );
function my_plugin_use_ability() {
$ability = wp_get_ability( 'my-plugin/get-site-title' );

if ( $ability && $ability->has_permission() ) {
$site_title = $ability->execute();
// $site_title now holds the result of get_bloginfo('name')
// error_log( 'Site Title: ' . $site_title );
if ( $ability ) {
// Check permissions first - always use is_wp_error() to handle errors properly
$permission = $ability->check_permission();
Comment on lines +136 to +138
Copy link
Contributor

@justlevine justlevine Sep 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this (and the previous example) is incorrect. By calling $ability->execute(), $ability->get_permission() gets called, there's rarely a reason to explicitly check permissions as a separate step.

if ( $ability  ) {
        $site_title = $ability->execute();
        // $site_title now holds the result of get_bloginfo('name')
        // error_log( 'Site Title: ' . $site_title );
    }
}

if ( is_wp_error( $permission ) ) {
// Handle permission error
error_log( 'Permission error: ' . $permission->get_error_message() );
return;
} elseif ( $permission ) {
// Permission granted - safe to execute
$site_title = $ability->execute();
if ( is_wp_error( $site_title ) ) {
// Handle execution error
error_log( 'Execution error: ' . $site_title->get_error_message() );
} else {
// $site_title now holds the result of get_bloginfo('name')
// error_log( 'Site Title: ' . $site_title );
}
} else {
// Permission denied
error_log( 'Permission denied for ability execution' );
}
}
}
```
23 changes: 17 additions & 6 deletions docs/4.using-abilities.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,14 @@ if ( $ability ) {
}
```

**Checking Permissions (`$ability->has_permission()`)**
**Checking Permissions (`$ability->check_permission()`)**

Before executing an ability, you can check if the current user has permission:
Before executing an ability, you can check if the current user has permission. The `check_permission()` method returns either `true`, `false`, or a `WP_Error` object, so you must use `is_wp_error()` to handle errors properly:

```php
// Method signature:
// check_permission( $input = null )
```
Comment on lines +130 to +134
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional or did it leak?


```php
$ability = wp_get_ability( 'my-plugin/update-option' );
Expand All @@ -136,12 +141,17 @@ if ( $ability ) {
'option_value' => 'New Site Name',
);

// Check permission before execution
if ( $ability->has_permission( $input ) ) {
// Check permission before execution - always use is_wp_error() first
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the doc in general uses a lot of unnecessary nesting, but I think this also make's it clearer than sandwiching the happy-path between two error states.

    // Use a strict check to catch both false and WP_Error
    $permission = $ability->check_permission( $input );
    if ( true === $permission ) {
        // Permission granted - safe to execute.
        $result = $ability->execute( $input );
        if ( is_wp_error( $result ) ) {
            // Handle execution error
            echo 'Execution error: ' . $result->get_error_message();
        } else {
            // Use $result
            if ( $result['success'] ) {
                echo 'Option updated successfully!';
                echo 'Previous value: ' . $result['previous_value'];
            }
        }
    } else {
        // Don't leak permission errors to unauthenticated users.
        if ( is_wp_error( $permission ) ) {
            error_log( 'Permission check failed: ' . $permission->get_error_message() );
        }

        echo 'You do not have permission to execute this ability.';
    }
}

Though imo we shouldn't be actively promoting check_permission() and instead have users fully handle execute()

$ability = wp_get_ability( 'my/ability' );

$result = $ability->execute( $input ) ;
// If we want to only handle perm errors and not also invalid inputs etc.
if ( is_wp_error() && 'ability_invalid_permissions' === $result->get_error_code() ) {
 // handle the error
}

$permission = $ability->check_permission( $input );
if ( is_wp_error( $permission ) ) {
// Handle permission check error (validation, callback error, etc.)
echo 'Permission check failed: ' . $permission->get_error_message();
} elseif ( $permission ) {
// Permission granted - safe to execute
$result = $ability->execute( $input );
if ( is_wp_error( $result ) ) {
// Handle WP_Error
echo 'Error: ' . $result->get_error_message();
// Handle execution error
echo 'Execution error: ' . $result->get_error_message();
} else {
// Use $result
if ( $result['success'] ) {
Expand All @@ -150,6 +160,7 @@ if ( $ability ) {
}
}
} else {
// Permission denied
echo 'You do not have permission to execute this ability.';
}
}
Expand Down
24 changes: 21 additions & 3 deletions includes/abilities-api/class-wp-ability.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,12 +312,12 @@ protected function validate_input( $input = null ) {
*
* The input is validated against the input schema before it is passed to to permission callback.
*
* @since 0.1.0
* @since N.E.X.T
*
* @param mixed $input Optional. The input data for permission checking. Default `null`.
* @return bool|\WP_Error Whether the ability has the necessary permission.
*/
public function has_permission( $input = null ) {
public function check_permission( $input = null ) {
$is_valid = $this->validate_input( $input );
if ( is_wp_error( $is_valid ) ) {
return $is_valid;
Expand All @@ -330,6 +330,24 @@ public function has_permission( $input = null ) {
return call_user_func( $this->permission_callback, $input );
}

/**
* Checks whether the ability has the necessary permissions (deprecated).
*
* The input is validated against the input schema before it is passed to to permission callback.
*
* @deprecated N.E.X.T Use check_permission() instead.
* @see WP_Ability::check_permission()
*
* @since 0.1.0
*
* @param mixed $input Optional. The input data for permission checking. Default `null`.
* @return bool|\WP_Error Whether the ability has the necessary permission.
*/
public function has_permission( $input = null ) {
_deprecated_function( __METHOD__, 'N.E.X.T', 'WP_Ability::check_permission()' );
return $this->check_permission( $input );
}

/**
* Executes the ability callback.
*
Expand Down Expand Up @@ -394,7 +412,7 @@ protected function validate_output( $output ) {
* @return mixed|\WP_Error The result of the ability execution, or WP_Error on failure.
*/
public function execute( $input = null ) {
$has_permissions = $this->has_permission( $input );
$has_permissions = $this->check_permission( $input );
if ( true !== $has_permissions ) {
if ( is_wp_error( $has_permissions ) ) {
if ( 'ability_invalid_input' === $has_permissions->get_error_code() ) {
Expand Down
2 changes: 1 addition & 1 deletion includes/rest-api/endpoints/class-wp-rest-abilities-run-controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ public function run_ability_permissions_check( $request ) {
}

$input = $this->get_input_from_request( $request );
if ( ! $ability->has_permission( $input ) ) {
if ( ! $ability->check_permission( $input ) ) {
Copy link
Contributor

@justlevine justlevine Sep 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we intentionally only handling false and saving the WP_Errors for ->execute()? cc @emdashcodes

Suggested change
if ( ! $ability->check_permission( $input ) ) {
if ( true !== $ability->check_permission( $input ) ) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix it. I don't think it was intentional.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing, the proposed code changes fail the result of 4 tests in the run controller. We would have to better handle the reason the request errored so it doesn't default to 403. It probably means, we would have to move some logic to run_ability_permissions_check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative would be to remove run_ability_permissions_check() (skip permission_callback) in the run controller, taking into account that run_ability (part of the callback) validates permissions anyway. It still would have to handle 403 when check_permissions() from the ability returns false.

Technically speaking, we are good. However, we can improve the implementation/documentation to avoid future misunderstandings.

return new \WP_Error(
'rest_ability_cannot_execute',
__( 'Sorry, you are not allowed to execute this ability.' ),
Expand Down
31 changes: 26 additions & 5 deletions tests/unit/abilities-api/wpRegisterAbility.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ public function test_register_valid_ability(): void {
$this->assertSame( self::$test_ability_args['output_schema'], $result->get_output_schema() );
$this->assertSame( self::$test_ability_args['meta'], $result->get_meta() );
$this->assertTrue(
$result->has_permission(
$result->check_permission(
array(
'a' => 2,
'b' => 3,
Expand Down Expand Up @@ -164,7 +164,7 @@ public function test_register_ability_no_permissions(): void {
$result = wp_register_ability( self::$test_ability_name, self::$test_ability_args );

$this->assertFalse(
$result->has_permission(
$result->check_permission(
array(
'a' => 2,
'b' => 3,
Expand Down Expand Up @@ -289,7 +289,7 @@ public function test_permission_callback_no_input_schema_match(): void {

$result = wp_register_ability( self::$test_ability_name, self::$test_ability_args );

$actual = $result->has_permission(
$actual = $result->check_permission(
array(
'a' => 2,
'b' => 3,
Expand All @@ -308,6 +308,27 @@ public function test_permission_callback_no_input_schema_match(): void {
);
}

/**
* Tests that deprecated has_permission() method still works.
*
* @expectedDeprecated WP_Ability::has_permission
*/
public function test_has_permission_deprecated_coverage(): void {
do_action( 'abilities_api_init' );

$result = wp_register_ability( self::$test_ability_name, self::$test_ability_args );

// Test that deprecated method still works
$this->assertTrue(
$result->has_permission(
array(
'a' => 2,
'b' => 3,
)
)
);
}

/**
* Tests permission callback receiving input for contextual permission checks.
*/
Expand All @@ -325,7 +346,7 @@ public function test_permission_callback_receives_input(): void {

// Test with a > b (should be allowed)
$this->assertTrue(
$result->has_permission(
$result->check_permission(
array(
'a' => 5,
'b' => 3,
Expand All @@ -342,7 +363,7 @@ public function test_permission_callback_receives_input(): void {

// Test with a < b (should be denied)
$this->assertFalse(
$result->has_permission(
$result->check_permission(
array(
'a' => 2,
'b' => 8,
Expand Down