diff --git a/inc/admin/class-listtable.php b/inc/admin/class-listtable.php index 83331b9..ae25fec 100644 --- a/inc/admin/class-listtable.php +++ b/inc/admin/class-listtable.php @@ -49,6 +49,7 @@ public function get_columns() { $c = [ 'cb' => '', 'name' => __( 'Name', 'oauth2' ), + 'approved' => __( 'Approved', 'oauth2' ), 'description' => __( 'Description', 'oauth2' ), ]; @@ -103,17 +104,8 @@ protected function column_name( $item ) { ]; $post_type_object = get_post_type_object( $item->post_type ); - if ( current_user_can( $post_type_object->cap->publish_posts ) && 'publish' !== $item->post_status ) { - $publish_link = add_query_arg( - [ - 'page' => 'rest-oauth2-apps', - 'action' => 'approve', - 'id' => $item->ID, - ], - admin_url( 'users.php' ) - ); - - $publish_link = wp_nonce_url( $publish_link, 'rest-oauth2-approve:' . $item->ID ); + if ( current_user_can( $post_type_object->cap->publish_posts ) && in_array( $item->post_status, Client::UNAPPROVED_STATUSES, true ) ) { + $publish_link = get_approve_url( $item ); $actions['app-approve'] = sprintf( '%s', esc_url( $publish_link ), @@ -128,7 +120,7 @@ protected function column_name( $item ) { _post_states( $item ); $title = sprintf( '%s%s', - $edit_link, + esc_url( $edit_link ), $title, ob_get_clean() ); @@ -136,6 +128,19 @@ protected function column_name( $item ) { return $title . ' ' . $action_html; } + /** + * Print whether the client is approved. + * + * @param \WP_Post $item Post object. + */ + public function column_approved( $item ) { + if ( 'publish' === $item->post_status ) { + esc_html_e( 'Yes', 'oauth2' ); + } else { + esc_html_e( 'No', 'oauth2' ); + } + } + /** * @param \WP_Post $item Post object. * @return string Content of the column. diff --git a/inc/admin/namespace.php b/inc/admin/namespace.php index ea5192b..bbcabff 100644 --- a/inc/admin/namespace.php +++ b/inc/admin/namespace.php @@ -9,6 +9,7 @@ use WP\OAuth2\Client; use WP_Error; +use WP_Post; const BASE_SLUG = 'rest-oauth2-apps'; @@ -46,6 +47,26 @@ function get_url( $params = [] ) { return add_query_arg( urlencode_deep( $params ), $url ); } +/** + * Get the URL that approves a client. + * + * @param WP_Post|int $client_post Client post or post ID. + * @param array|string $params Extra query parameters, such as `return=edit`. + * + * @return string Approve URL including the nonce. + */ +function get_approve_url( $client_post, $params = [] ) { + $id = $client_post instanceof WP_Post ? $client_post->ID : absint( $client_post ); + $url = get_url( + [ + 'action' => 'approve', + 'id' => $id, + ] + wp_parse_args( $params ) + ); + + return wp_nonce_url( $url, 'rest-oauth2-approve:' . $id ); +} + /** * Get the current page action. * @@ -273,6 +294,41 @@ function handle_edit_submit( ?Client $consumer = null ) { exit; } +/** + * Output the approval status notice for a client, with an approve button where allowed. + * + * @param WP_Post|int $client_post Client post or post ID. + */ +function render_approval_status( $client_post ) { + $client_post = get_post( $client_post ); + if ( ! $client_post ) { + return; + } + + if ( 'publish' === $client_post->post_status ) { + echo '

' . esc_html__( 'This application is approved.', 'oauth2' ) . '

'; + return; + } + + if ( ! in_array( $client_post->post_status, Client::UNAPPROVED_STATUSES, true ) ) { + return; + } + + $post_type_object = get_post_type_object( Client::POST_TYPE ); + ?> +
+

+

+ cap->publish_posts ) ) : ?> + + + + +

+
+ + get_post_id() ) ); + } + ?> +
@@ -589,6 +651,11 @@ function handle_approve() { ); } + $post = get_post( $id ); + if ( ! $post || Client::POST_TYPE !== $post->post_type || ! in_array( $post->post_status, Client::UNAPPROVED_STATUSES, true ) ) { + wp_die( esc_html__( 'This application cannot be approved.', 'oauth2' ), '', [ 'response' => 400 ] ); + } + $client = Client::get_by_post_id( $id ); if ( is_wp_error( $client ) ) { wp_die( $client ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped @@ -599,6 +666,18 @@ function handle_approve() { wp_die( $did_approve ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped } + if ( isset( $_GET['return'] ) && 'edit' === $_GET['return'] ) { + wp_safe_redirect( + get_url( + [ + 'action' => 'edit', + 'id' => $id, + ] + ) + ); + exit; + } + wp_safe_redirect( get_url( 'approved=1' ) ); exit; } diff --git a/inc/class-client.php b/inc/class-client.php index 093a305..5ecd57f 100644 --- a/inc/class-client.php +++ b/inc/class-client.php @@ -16,6 +16,7 @@ class Client implements ClientInterface { const POST_TYPE = 'oauth2_client'; + const UNAPPROVED_STATUSES = [ 'draft', 'pending' ]; const CLIENT_SECRET_KEY = '_oauth2_client_secret'; const TYPE_KEY = '_oauth2_client_type'; const REDIRECT_URI_KEY = '_oauth2_redirect_uri'; @@ -61,6 +62,15 @@ public function get_post_id() { return $this->post->ID; } + /** + * Check whether the client is approved. + * + * @return bool True if the client is published. + */ + public function is_approved() { + return 'publish' === $this->post->post_status; + } + /** * Get the client's name. * @@ -310,14 +320,20 @@ public function issue_token( WP_User $user, $meta = [] ) { /** * Get a client by ID. * - * @param string $id Client ID. + * @param string $id Client ID. + * @param bool $include_unapproved Whether to also match draft and pending clients. Trashed clients never match. * - * @return static|null Token if ID is found, null otherwise. + * @return static|null Client if ID is found, null otherwise. */ - public static function get_by_id( $id ) { + public static function get_by_id( $id, $include_unapproved = false ) { + $statuses = [ 'publish' ]; + if ( $include_unapproved ) { + $statuses = array_merge( $statuses, static::UNAPPROVED_STATUSES ); + } + $args = [ 'post_type' => static::POST_TYPE, - 'post_status' => 'publish', + 'post_status' => $statuses, 'posts_per_page' => 1, 'no_found_rows' => true, @@ -455,8 +471,13 @@ public function approve() { 'post_status' => 'publish', ]; $result = wp_update_post( wp_slash( $data ), true ); + if ( is_wp_error( $result ) ) { + return $result; + } - return is_wp_error( $result ) ? $result : true; + $this->post = get_post( $this->get_post_id() ); + + return true; } /** diff --git a/inc/endpoints/class-authorization.php b/inc/endpoints/class-authorization.php index ee031df..98c4e44 100644 --- a/inc/endpoints/class-authorization.php +++ b/inc/endpoints/class-authorization.php @@ -51,9 +51,40 @@ public function handle_request() { } if ( is_wp_error( $result ) ) { - // TODO: Handle it. - wp_die( esc_html( $result->get_error_message() ) ); + $this->render_error( $result ); } exit; } + + /** + * Get the HTTP status for an error. + * + * @param WP_Error $error Error to read the `status` error data from. + * @return int Status code, 400 if the error does not set one. + */ + public function get_error_status( WP_Error $error ) { + $data = $error->get_error_data(); + + return is_array( $data ) && ! empty( $data['status'] ) ? (int) $data['status'] : 400; + } + + /** + * Render an authorization error on the login screen, then exit. + * + * @param WP_Error $error Error to display. The `status` error data sets the HTTP status. + */ + protected function render_error( WP_Error $error ) { + // login_header() prints error messages as HTML, so escape them first. + $escaped = new WP_Error(); + foreach ( $error->get_error_codes() as $code ) { + foreach ( $error->get_error_messages( $code ) as $message ) { + $escaped->add( $code, esc_html( $message ) ); + } + } + + status_header( $this->get_error_status( $error ) ); + login_header( __( 'Authorization error', 'oauth2' ), '', $escaped ); + login_footer(); + exit; + } } diff --git a/inc/types/class-base.php b/inc/types/class-base.php index a35cf13..a217303 100644 --- a/inc/types/class-base.php +++ b/inc/types/class-base.php @@ -47,7 +47,7 @@ public function handle_authorisation() { // State is opaque to the server and must be returned to the client unchanged. $state = isset( $_GET['state'] ) && is_string( $_GET['state'] ) ? wp_unslash( $_GET['state'] ) : null; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized - $client = Client::get_by_id( $client_id ); + $client = Client::get_by_id( $client_id, true ); if ( empty( $client ) ) { return new WP_Error( 'oauth2.types.authorization_code.handle_authorisation.invalid_client_id', @@ -60,6 +60,17 @@ public function handle_authorisation() { ); } + if ( ! $client->is_approved() ) { + return new WP_Error( + 'oauth2.types.authorization_code.handle_authorisation.unapproved_client', + __( 'This application is not currently approved. Ask the site administrator to approve it.', 'oauth2' ), + [ + 'status' => WP_Http::FORBIDDEN, + 'client_id' => $client_id, + ] + ); + } + // Validate the redirection URI. $redirect_uri = $this->validate_redirect_uri( $client, $redirect_uri ); if ( is_wp_error( $redirect_uri ) ) { diff --git a/tests/class-test-case.php b/tests/class-test-case.php index 9035d85..4b9e372 100644 --- a/tests/class-test-case.php +++ b/tests/class-test-case.php @@ -16,14 +16,15 @@ abstract class Test_Case extends WP_UnitTestCase { /** - * Create and approve a test client. + * Create and optionally approve a test client. * * @param array $meta_overrides Optional overrides for the client meta. * @param string $name Client name. + * @param bool $approve Whether to approve the client. * * @return Client */ - protected function create_client( array $meta_overrides = [], $name = 'Test Client' ) { + protected function create_client( array $meta_overrides = [], $name = 'Test Client', $approve = true ) { $data = [ 'name' => $name, 'description' => 'Test client description.', @@ -39,7 +40,9 @@ protected function create_client( array $meta_overrides = [], $name = 'Test Clie $client = Client::create( $data ); $this->assertInstanceOf( Client::class, $client, 'create_client helper: Client::create() must return a Client instance.' ); - $client->approve(); + if ( $approve ) { + $client->approve(); + } return $client; } diff --git a/tests/test-admin.php b/tests/test-admin.php index 0e085bb..f259654 100644 --- a/tests/test-admin.php +++ b/tests/test-admin.php @@ -9,6 +9,9 @@ require_once __DIR__ . '/class-test-case.php'; +use WP\OAuth2\Admin\ListTable; +use function WP\OAuth2\Admin\handle_approve; +use function WP\OAuth2\Admin\render_approval_status; use function WP\OAuth2\Admin\validate_parameters; /** @@ -16,6 +19,12 @@ */ class Test_Admin extends Test_Case { + public function tear_down() { + $_GET = []; + $_REQUEST = []; + parent::tear_down(); + } + /** * Build a valid set of submitted client parameters. * @@ -77,4 +86,141 @@ public function test_validate_parameters_rejects_negative_ttl() { $this->assertWPError( $result ); $this->assertEquals( 'rest_oauth2_invalid_ttl', $result->get_error_code() ); } + + /** + * Get a list table instance. + * + * @return ListTable + */ + protected function get_list_table() { + set_current_screen( 'users_page_rest-oauth2-apps' ); + require_once ABSPATH . 'wp-admin/includes/class-wp-list-table.php'; + require_once dirname( __DIR__ ) . '/inc/admin/class-listtable.php'; + + return new ListTable( [ 'screen' => 'users_page_rest-oauth2-apps' ] ); + } + + /** + * Capture the output of the approved column for a client. + * + * @param \WP\OAuth2\Client $client Client to print the column for. + * + * @return string Column output. + */ + protected function get_approved_column( $client ) { + ob_start(); + $this->get_list_table()->column_approved( get_post( $client->get_post_id() ) ); + + return ob_get_clean(); + } + + /** + * Capture the output of the approval status for a client. + * + * @param \WP\OAuth2\Client $client Client to print the status for. + * + * @return string Notice markup. + */ + protected function get_approval_status( $client ) { + ob_start(); + render_approval_status( get_post( $client->get_post_id() ) ); + + return ob_get_clean(); + } + + public function test_list_table_has_approved_column() { + $this->assertArrayHasKey( 'approved', $this->get_list_table()->get_columns() ); + } + + public function test_approved_column_prints_yes_for_published_client() { + $this->assertSame( 'Yes', $this->get_approved_column( $this->create_client() ) ); + } + + public function test_approved_column_prints_no_for_unapproved_client() { + $this->assertSame( 'No', $this->get_approved_column( $this->create_client( [], 'Test Client', false ) ) ); + } + + public function test_approval_status_shows_warning_and_button_for_admin() { + $user_id = self::factory()->user->create( [ 'role' => 'administrator' ] ); + if ( is_multisite() ) { + grant_super_admin( $user_id ); + } + wp_set_current_user( $user_id ); + + $output = $this->get_approval_status( $this->create_client( [], 'Test Client', false ) ); + + $this->assertStringContainsString( 'notice-warning', $output ); + $this->assertStringContainsString( 'This application is not approved yet.', $output ); + $this->assertStringContainsString( 'button button-primary', $output ); + $this->assertStringContainsString( 'action=approve', $output ); + $this->assertStringContainsString( 'return=edit', $output ); + $this->assertStringNotContainsString( 'Ask an administrator', $output ); + } + + public function test_approval_status_asks_for_administrator_without_capability() { + wp_set_current_user( self::factory()->user->create( [ 'role' => 'subscriber' ] ) ); + + $output = $this->get_approval_status( $this->create_client( [], 'Test Client', false ) ); + + $this->assertStringContainsString( 'notice-warning', $output ); + $this->assertStringContainsString( 'Ask an administrator to approve it.', $output ); + $this->assertStringNotContainsString( 'button-primary', $output ); + } + + public function test_approval_status_shows_success_for_published_client() { + wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $output = $this->get_approval_status( $this->create_client() ); + + $this->assertStringContainsString( 'notice-success', $output ); + $this->assertStringContainsString( 'This application is approved.', $output ); + $this->assertStringNotContainsString( 'button-primary', $output ); + } + + public function test_approval_status_prints_nothing_for_trashed_client() { + $user_id = self::factory()->user->create( [ 'role' => 'administrator' ] ); + if ( is_multisite() ) { + grant_super_admin( $user_id ); + } + wp_set_current_user( $user_id ); + + $client = $this->create_client( [], 'Test Client', false ); + wp_update_post( + [ + 'ID' => $client->get_post_id(), + 'post_status' => 'trash', + ] + ); + + $this->assertSame( '', trim( $this->get_approval_status( $client ) ) ); + } + + public function test_handle_approve_refuses_trashed_client() { + $user_id = self::factory()->user->create( [ 'role' => 'administrator' ] ); + if ( is_multisite() ) { + grant_super_admin( $user_id ); + } + wp_set_current_user( $user_id ); + + $client = $this->create_client( [], 'Test Client', false ); + $id = $client->get_post_id(); + wp_update_post( + [ + 'ID' => $id, + 'post_status' => 'trash', + ] + ); + + $nonce = wp_create_nonce( 'rest-oauth2-approve:' . $id ); + $_GET['id'] = $id; + $_GET['_wpnonce'] = $nonce; + $_REQUEST['_wpnonce'] = $nonce; + + $this->expectException( \WPDieException::class ); + try { + handle_approve(); + } finally { + $this->assertSame( 'trash', get_post_status( $id ) ); + } + } } diff --git a/tests/test-authorize-errors.php b/tests/test-authorize-errors.php new file mode 100644 index 0000000..bd205da --- /dev/null +++ b/tests/test-authorize-errors.php @@ -0,0 +1,58 @@ +create_client( [], 'Test Client', false ); + $_GET = [ 'client_id' => $client->get_id() ]; + + $result = ( new Authorization_Code() )->handle_authorisation(); + + $this->assertWPError( $result ); + $this->assertSame( 'oauth2.types.authorization_code.handle_authorisation.unapproved_client', $result->get_error_code() ); + $this->assertSame( 403, $result->get_error_data()['status'] ); + } + + public function test_unknown_client_still_returns_invalid_client_id() { + $_GET = [ 'client_id' => 'nonexistent-client-id' ]; + + $result = ( new Authorization_Code() )->handle_authorisation(); + + $this->assertWPError( $result ); + $this->assertSame( 'oauth2.types.authorization_code.handle_authorisation.invalid_client_id', $result->get_error_code() ); + $this->assertSame( 400, $result->get_error_data()['status'] ); + } + + public function test_error_status_is_forbidden_for_unapproved_client() { + $client = $this->create_client( [], 'Test Client', false ); + $_GET = [ 'client_id' => $client->get_id() ]; + $result = ( new Authorization_Code() )->handle_authorisation(); + + $this->assertSame( 403, ( new Authorization() )->get_error_status( $result ) ); + } + + public function test_error_status_defaults_to_bad_request() { + $this->assertSame( 400, ( new Authorization() )->get_error_status( new WP_Error( 'some_code', 'Message.' ) ) ); + } +} diff --git a/tests/test-client.php b/tests/test-client.php index fb3618c..db37e2a 100644 --- a/tests/test-client.php +++ b/tests/test-client.php @@ -97,6 +97,35 @@ public function test_get_by_id_returns_null_for_unapproved() { $this->assertNull( $result ); } + public function test_get_by_id_with_unapproved_finds_draft_client() { + $draft_client = $this->create_client( [], 'Test Client', false ); + + $found = Client::get_by_id( $draft_client->get_id(), true ); + $this->assertInstanceOf( Client::class, $found ); + $this->assertEquals( $draft_client->get_id(), $found->get_id() ); + } + + public function test_get_by_id_with_unapproved_returns_null_for_trashed_client() { + $draft_client = $this->create_client( [], 'Test Client', false ); + wp_update_post( + [ + 'ID' => $draft_client->get_post_id(), + 'post_status' => 'trash', + ] + ); + + $this->assertSame( 'trash', get_post_status( $draft_client->get_post_id() ) ); + $this->assertNull( Client::get_by_id( $draft_client->get_id(), true ) ); + } + + public function test_is_approved_is_true_for_published_client() { + $this->assertTrue( $this->client->is_approved() ); + } + + public function test_is_approved_is_false_for_draft_client() { + $this->assertFalse( $this->create_client( [], 'Test Client', false )->is_approved() ); + } + public function test_get_by_post_id_finds_client() { $found = Client::get_by_post_id( $this->client->get_post_id() ); $this->assertInstanceOf( Client::class, $found );