Skip to content

feat!: make permission_callback arg required #73

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 6 commits into from
Sep 23, 2025
Merged

feat!: make permission_callback arg required #73

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b99744c
feat!: make `permission_callback` arg required
justlevine Sep 13, 2025
4d7ba35
Update tests/unit/abilities-api/wpAbilitiesRegistry.php
justlevine Sep 13, 2025
b08da61
tests: add WP_Ability::prepare_properties to coverage
justlevine Sep 13, 2025
c99eaae
Merge branch 'trunk' into feat/require-permission_callback
justlevine Sep 15, 2025
00d5099
Update includes/abilities-api/class-wp-ability.php
gziolo Sep 23, 2025
a1b27ab
Merge branch 'trunk' into feat/require-permission_callback
gziolo Sep 23, 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
8 changes: 4 additions & 4 deletions docs/3.registering-abilities.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ The `$args` array accepts the following keys:
- `execute_callback` (`callable`, **Required**): The PHP function or method to execute when this ability is called.
- The callback receives one optional argument: it can have any type as defined in the input schema (e.g., `array`, `object`, `string`, etc.).
- The callback should return the result of the ability's operation or return a `WP_Error` object on failure.
- `permission_callback` (`callable`|`null`, **Optional**): A callback function to check if the current user has permission to execute this ability.
- Should return `true` if the user has permission, or a `WP_Error` object otherwise.
- If not provided, the ability will only validate input parameters before execution.
- This defaults to `true` if not set.
- `permission_callback` (`callable`, **Required**): A callback function to check if the current user has permission to execute this ability.
- The callback receives one optional argument: it can have any type as defined in the input schema (e.g., `array`, `object`, `string`, etc.).
- The callback should return a boolean (`true` if the user has permission, `false` otherwise), or a `WP_Error` object on failure.
- If the input does not validate against the input schema, the permission callback will not be called, and a `WP_Error` will be returned instead.
- `meta` (`array`, **Optional**): An associative array for storing arbitrary additional metadata about the ability.

**Ability ID Convention**
Expand Down
4 changes: 2 additions & 2 deletions includes/abilities-api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@
* @phpstan-param array{
* label?: string,
* description?: string,
* input_schema?: array<string,mixed>,
* output_schema?: array<string,mixed>,
* execute_callback?: callable( mixed $input= ): (mixed|\WP_Error),
* permission_callback?: callable( mixed $input= ): (bool|\WP_Error),
* input_schema?: array<string,mixed>,
* output_schema?: array<string,mixed>,
* meta?: array<string,mixed>,
* ability_class?: class-string<\WP_Ability>,
* ...<string, mixed>
Expand Down
9 changes: 4 additions & 5 deletions includes/abilities-api/class-wp-abilities-registry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,17 @@ final class WP_Abilities_Registry {
* @param string $name The name of the ability. The name must be a string containing a namespace
* prefix, i.e. `my-plugin/my-ability`. It can only contain lowercase
* alphanumeric characters, dashes and the forward slash.
* @param array<string,mixed> $args An associative array of arguments for the ability. This should include
* `label`, `description`, `input_schema`, `output_schema`,
* `execute_callback`, `permission_callback`, `meta`, and ability_class.
* @param array<string,mixed> $args An associative array of arguments for the ability. See wp_register_ability() for
* details.
* @return ?\WP_Ability The registered ability instance on success, null on failure.
*
* @phpstan-param array{
* label?: string,
* description?: string,
* execute_callback?: callable( mixed $input= ): (mixed|\WP_Error),
* permission_callback?: callable( mixed $input= ): (bool|\WP_Error),
* input_schema?: array<string,mixed>,
* output_schema?: array<string,mixed>,
* execute_callback?: callable( mixed $input= ): (mixed|\WP_Error),
* permission_callback?: ?callable( mixed $input= ): (bool|\WP_Error),
* meta?: array<string,mixed>,
* ability_class?: class-string<\WP_Ability>,
* ...<string, mixed>
Expand Down
39 changes: 17 additions & 22 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 @@ -73,9 +73,9 @@ class WP_Ability {
* The optional ability permission callback.
*
* @since 0.1.0
* @var ?callable( mixed $input= ): (bool|\WP_Error)
* @var callable( mixed $input= ): (bool|\WP_Error)
*/
protected $permission_callback = null;
protected $permission_callback;

/**
* The optional ability metadata.
Expand Down Expand Up @@ -143,15 +143,16 @@ public function __construct( string $name, array $args ) {
* @phpstan-return array{
* label: string,
* description: string,
* execute_callback: callable( mixed $input= ): (mixed|\WP_Error),
* permission_callback: callable( mixed $input= ): (bool|\WP_Error),
* input_schema?: array<string,mixed>,
* output_schema?: array<string,mixed>,
* execute_callback: callable( mixed $input= ): (mixed|\WP_Error),
* permission_callback?: ?callable( mixed $input= ): (bool|\WP_Error),
* meta?: array<string,mixed>,
* ...<string, mixed>,
* } $args
*/
protected function prepare_properties( array $args ): array {
// Required args must be present and of the correct type.
if ( empty( $args['label'] ) || ! is_string( $args['label'] ) ) {
throw new \InvalidArgumentException(
esc_html__( 'The ability properties must contain a `label` string.' )
Expand All @@ -164,27 +165,28 @@ protected function prepare_properties( array $args ): array {
);
}

if ( isset( $args['input_schema'] ) && ! is_array( $args['input_schema'] ) ) {
if ( empty( $args['execute_callback'] ) || ! is_callable( $args['execute_callback'] ) ) {
throw new \InvalidArgumentException(
esc_html__( 'The ability properties should provide a valid `input_schema` definition.' )
esc_html__( 'The ability properties must contain a valid `execute_callback` function.' )
);
}

if ( isset( $args['output_schema'] ) && ! is_array( $args['output_schema'] ) ) {
if ( empty( $args['permission_callback'] ) || ! is_callable( $args['permission_callback'] ) ) {
throw new \InvalidArgumentException(
esc_html__( 'The ability properties should provide a valid `output_schema` definition.' )
esc_html__( 'The ability properties must provide a valid `permission_callback` function.' )
);
}

if ( empty( $args['execute_callback'] ) || ! is_callable( $args['execute_callback'] ) ) {
// Optional args only need to be of the correct type if they are present.
if ( isset( $args['input_schema'] ) && ! is_array( $args['input_schema'] ) ) {
throw new \InvalidArgumentException(
esc_html__( 'The ability properties must contain a valid `execute_callback` function.' )
esc_html__( 'The ability properties should provide a valid `input_schema` definition.' )
);
}

if ( isset( $args['permission_callback'] ) && ! is_callable( $args['permission_callback'] ) ) {
if ( isset( $args['output_schema'] ) && ! is_array( $args['output_schema'] ) ) {
throw new \InvalidArgumentException(
esc_html__( 'The ability properties should provide a valid `permission_callback` function.' )
esc_html__( 'The ability properties should provide a valid `output_schema` definition.' )
);
}

Expand Down Expand Up @@ -278,6 +280,7 @@ protected function validate_input( $input = null ) {
if ( null === $input ) {
return true;
}

return new \WP_Error(
'ability_missing_input_schema',
sprintf(
Expand Down Expand Up @@ -306,8 +309,8 @@ protected function validate_input( $input = null ) {

/**
* Checks whether the ability has the necessary permissions.
* If the permission callback is not set, the default behavior is to allow access
* when the input provided passes validation.
*
* The input is validated against the input schema before it is passed to to permission callback.
*
* @since 0.1.0
*
Expand All @@ -320,14 +323,6 @@ public function has_permission( $input = null ) {
return $is_valid;
}

if ( ! is_callable( $this->permission_callback ) ) {
return true;
}

if ( empty( $this->get_input_schema() ) ) {
return call_user_func( $this->permission_callback );
}
Comment on lines 327 to 329
Copy link
Contributor Author

@justlevine justlevine Sep 13, 2025

Choose a reason for hiding this comment

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

This is handled by ::validate_input()

Copy link
Member

@gziolo gziolo Sep 15, 2025

Choose a reason for hiding this comment

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

The point is that, if there is no schema, then we don't pass any input. What we agreed upon was that if someone wants to explicitly pass null, they need to provide the schema. This builds predictability, and that's why the condition is in place also here.

Copy link
Member

@gziolo gziolo Sep 15, 2025

Choose a reason for hiding this comment

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

Previously, I considered adding a method for calling these callbacks, but it felt like more code than necessary. However, having a method could help document this expected behavior better.

Copy link
Contributor Author

@justlevine justlevine Sep 15, 2025

Choose a reason for hiding this comment

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

What we agreed upon was that if someone wants to explicitly pass null, they need to provide the schema.

Can you clarify? If I understood you correctly that behavior is what this change enforces.

Previously diff, if there was no input schema, then any $input would get stripped and the execution callback would be be handled as if nothing was passed. As a result, if the schema allows an "explicit null, then the ability would still execute even if the supplied $input doesn't meet the correct shape.

With this diff, both no $input must be passed and the schema must support a null value for the ability to execute successfully.

(If you're referring to our internal ability to differentiate between null and unset, that was just one one of the concerns about going to mixed that I raised on #58 . Until core's minimum PHP supports named arguments, nothing can compete with either the dx or future-compat of a partially-sealed array, nullable or otherwise. 🤷‍♂️)

Copy link
Member

@gziolo gziolo Sep 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I follow, so let me explain it in a different way:

  • if there is no input schema declared, it means there is no input to pass, so the callback should be executed with no arguments
  • If there is an input schema provided, it means it needs to be validated. This also allows passing an explicit null as the first argument, but also any other type of data: string, boolean, array, etc.

Copy link
Contributor Author

@justlevine justlevine Sep 15, 2025

Choose a reason for hiding this comment

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

Okay now I'm sure we're not following each other, because that's literally the initial comment I left here explaining why I removed this conditional here, except you're using it to (seemingly) justify the opposite of what I am 😭

I'll add the unit tests for the above edge cases, if that doesn't clear things up (for either of us), i'll revert here and open a separate bug report that follows up #61 .

Copy link
Member

@gziolo gziolo Sep 15, 2025

Choose a reason for hiding this comment

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

Works for me.

Copy link
Member

@felixarntz felixarntz Sep 15, 2025

Choose a reason for hiding this comment

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

The key here is that it needs to be impossible to pass unvalidated input through to permission_callback (and similarly execute_callback).

  • @justlevine You're right that technically we can drop the empty( $this->get_input_schema() ) condition because if this is the case and the input isn't null, the validate_input method would return a WP_Error and this code wouldn't be reached.
  • That being said, I do think this is good to keep as a safe guard for good measure. We have the same in do_execute. Having this extra check in there ensures directly before the relevant call that we don't pass any input (i.e. enforce null) when there's no input schema.

Is it necessary? No. Is it a good measure for defensive coding and clarifying the importance of this to someone new working on this code in the future? Yes, I think so.

IMO we should just keep it. It doesn't hurt.

Copy link
Member

@gziolo gziolo Sep 22, 2025

Choose a reason for hiding this comment

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

Can we bring it back here as discussed?

if ( empty( $this->get_input_schema() ) ) {
	return call_user_func( $this->permission_callback );
}

That's the only remaining item to approve and land this PR.

Copy link
Member

@gziolo gziolo Sep 23, 2025

Choose a reason for hiding this comment

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

I addressed it with 00d5099.


return call_user_func( $this->permission_callback, $input );
}

Expand Down
26 changes: 26 additions & 0 deletions tests/unit/abilities-api/wpAbilitiesRegistry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ public function test_register_invalid_uppercase_characters_in_name() {
* Should reject ability registration without a label.
*
* @covers WP_Abilities_Registry::register
* @covers WP_Ability::prepare_properties
*
* @expectedIncorrectUsage WP_Abilities_Registry::register
*/
Expand All @@ -127,6 +128,7 @@ public function test_register_invalid_missing_label() {
* Should reject ability registration with invalid label type.
*
* @covers WP_Abilities_Registry::register
* @covers WP_Ability::prepare_properties
*
* @expectedIncorrectUsage WP_Abilities_Registry::register
*/
Expand All @@ -141,6 +143,7 @@ public function test_register_invalid_label_type() {
* Should reject ability registration without a description.
*
* @covers WP_Abilities_Registry::register
* @covers WP_Ability::prepare_properties
*
* @expectedIncorrectUsage WP_Abilities_Registry::register
*/
Expand All @@ -156,6 +159,7 @@ public function test_register_invalid_missing_description() {
* Should reject ability registration with invalid description type.
*
* @covers WP_Abilities_Registry::register
* @covers WP_Ability::prepare_properties
*
* @expectedIncorrectUsage WP_Abilities_Registry::register
*/
Expand All @@ -170,6 +174,7 @@ public function test_register_invalid_description_type() {
* Should reject ability registration without an execute callback.
*
* @covers WP_Abilities_Registry::register
* @covers WP_Ability::prepare_properties
*
* @expectedIncorrectUsage WP_Abilities_Registry::register
*/
Expand All @@ -185,6 +190,7 @@ public function test_register_invalid_missing_execute_callback() {
* Should reject ability registration if the execute callback is not a callable.
*
* @covers WP_Abilities_Registry::register
* @covers WP_Ability::prepare_properties
*
* @expectedIncorrectUsage WP_Abilities_Registry::register
*/
Expand All @@ -195,10 +201,27 @@ public function test_register_incorrect_execute_callback_type() {
$this->assertNull( $result );
}

/**
* Should reject ability registration without an execute callback.
*
* @covers WP_Abilities_Registry::register
* @covers WP_Ability::prepare_properties
*
* @expectedIncorrectUsage WP_Abilities_Registry::register
*/
public function test_register_invalid_missing_permission_callback() {
// Remove the permission_callback from the args.
unset( self::$test_ability_args['permission_callback'] );

$result = $this->registry->register( self::$test_ability_name, self::$test_ability_args );
$this->assertNull( $result );
}

/**
* Should reject ability registration if the permission callback is not a callable.
*
* @covers WP_Abilities_Registry::register
* @covers WP_Ability::prepare_properties
*
* @expectedIncorrectUsage WP_Abilities_Registry::register
*/
Expand All @@ -213,6 +236,7 @@ public function test_register_incorrect_permission_callback_type() {
* Should reject ability registration if the input schema is not an array.
*
* @covers WP_Abilities_Registry::register
* @covers WP_Ability::prepare_properties
*
* @expectedIncorrectUsage WP_Abilities_Registry::register
*/
Expand All @@ -227,6 +251,7 @@ public function test_register_incorrect_input_schema_type() {
* Should reject ability registration if the output schema is not an array.
*
* @covers WP_Abilities_Registry::register
* @covers WP_Ability::prepare_properties
*
* @expectedIncorrectUsage WP_Abilities_Registry::register
*/
Expand All @@ -241,6 +266,7 @@ public function test_register_incorrect_output_schema_type() {
* Should reject ability registration with invalid meta type.
*
* @covers WP_Abilities_Registry::register
* @covers WP_Ability::prepare_properties
*
* @expectedIncorrectUsage WP_Abilities_Registry::register
*/
Expand Down
1 change: 0 additions & 1 deletion tests/unit/abilities-api/wpRegisterAbility.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,6 @@ public function test_register_ability_custom_ability_class(): void {
);
}


/**
* Tests executing an ability with input not matching schema.
*/
Expand Down
32 changes: 0 additions & 32 deletions tests/unit/rest-api/wpRestAbilitiesRunController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,38 +703,6 @@ public function test_ability_without_type_defaults_to_tool(): void {
$this->assertEquals( 200, $post_response->get_status() );
}

/**
* Test permission check with null permission callback.
*/
public function test_permission_check_passes_when_callback_not_set(): void {
// Register ability without permission callback.
wp_register_ability(
'test/no-permission-callback',
array(
'label' => 'No Permission Callback',
'description' => 'Ability without permission callback',
'execute_callback' => static function () {
return array( 'executed' => true );
},
'meta' => array( 'type' => 'tool' ),
// No permission_callback set
)
);

wp_set_current_user( 0 ); // Not logged in

$request = new WP_REST_Request( 'POST', '/wp/v2/abilities/test/no-permission-callback/run' );
$request->set_header( 'Content-Type', 'application/json' );

$response = $this->server->dispatch( $request );

// Should succeed when no permission callback is set
$this->assertEquals( 200, $response->get_status() );

// Restore user for other tests
wp_set_current_user( self::$user_id );
}

/**
* Test edge case with empty input for both GET and POST.
*/
Expand Down