diff --git a/inc/class-client.php b/inc/class-client.php index bc9df85..e9a8303 100644 --- a/inc/class-client.php +++ b/inc/class-client.php @@ -122,6 +122,28 @@ public function get_secret() { return get_post_meta( $this->get_post_id(), static::CLIENT_SECRET_KEY, true ); } + /** + * Check whether the client must authenticate with its secret. + * + * Clients declared as private are confidential clients in RFC 6749 terms, + * so they have to prove they hold the secret they were issued. + * + * @link https://tools.ietf.org/html/rfc6749#section-3.2.1 + * + * @return bool True if the secret must be verified, false otherwise. + */ + public function requires_secret() { + $requires_secret = ( 'private' === $this->get_type() ); + + /** + * Filter whether a client must authenticate with its secret. + * + * @param bool $requires_secret Whether the secret must be verified. + * @param Client $client Client being checked. + */ + return (bool) apply_filters( 'oauth2.client.requires_secret', $requires_secret, $this ); + } + /** * Check if the provided secret matches the client's secret. * diff --git a/inc/class-clientinterface.php b/inc/class-clientinterface.php index 9b42081..b492b97 100644 --- a/inc/class-clientinterface.php +++ b/inc/class-clientinterface.php @@ -47,6 +47,31 @@ public function get_type(); */ public function get_secret(); + /** + * Check whether the client must authenticate with its secret. + * + * @link https://tools.ietf.org/html/rfc6749#section-3.2.1 + * + * @return bool True if the secret must be verified, false otherwise. + */ + public function requires_secret(); + + /** + * Check if the provided secret matches the client's secret. + * + * @param string $secret Secret to check. + * + * @return bool True if the secret matches, false otherwise. + */ + public function check_secret( $secret ); + + /** + * Check whether the client_credentials grant is enabled for this client. + * + * @return bool True if enabled, false otherwise. + */ + public function is_client_credentials_enabled(); + /** * Get registered URI for the client. * diff --git a/inc/class-personalclient.php b/inc/class-personalclient.php index 6105983..694a13d 100644 --- a/inc/class-personalclient.php +++ b/inc/class-personalclient.php @@ -93,6 +93,35 @@ public function get_secret() { return ''; } + /** + * Check whether the client must authenticate with its secret. + * + * @return bool Always false: personal tokens have no secret to check. + */ + public function requires_secret() { + return false; + } + + /** + * Check if the provided secret matches the client's secret. + * + * @param string $secret Secret to check. + * + * @return bool Always false: personal tokens cannot authenticate as a client. + */ + public function check_secret( $secret ) { + return false; + } + + /** + * Check whether the client_credentials grant is enabled for this client. + * + * @return bool Always false: personal tokens are issued to a user, not a client. + */ + public function is_client_credentials_enabled() { + return false; + } + /** * Get registered URI for the client. * diff --git a/inc/endpoints/class-token.php b/inc/endpoints/class-token.php index b853c75..647f068 100644 --- a/inc/endpoints/class-token.php +++ b/inc/endpoints/class-token.php @@ -11,6 +11,7 @@ use WP_Http; use WP\OAuth2; use WP_REST_Request; +use WP_REST_Response; /** * Token endpoint handler. */ @@ -74,16 +75,19 @@ public function exchange_token( WP_REST_Request $request ) { // RFC 6749 section 2.3.1: a client may authenticate with HTTP Basic // instead of body parameters. Body parameters take precedence. - if ( $request->get_param( 'client_id' ) === null || $request->get_param( 'client_id' ) === '' ) { - $basic = $this->get_basic_auth_credentials( $request ); - if ( is_wp_error( $basic ) ) { - return $basic; - } - if ( null !== $basic ) { + $basic = $this->get_basic_auth_credentials( $request ); + if ( is_wp_error( $basic ) ) { + return $basic; + } + if ( null !== $basic ) { + if ( $this->is_param_empty( $request, 'client_id' ) ) { $request->set_param( 'client_id', $basic[0] ); - if ( $request->get_param( 'client_secret' ) === null || $request->get_param( 'client_secret' ) === '' ) { - $request->set_param( 'client_secret', $basic[1] ); - } + } + + // Only accept the header secret for the client it names, so a body + // client_id can still be paired with a Basic secret. + if ( $this->is_param_empty( $request, 'client_secret' ) && $basic[0] === $request->get_param( 'client_id' ) ) { + $request->set_param( 'client_secret', $basic[1] ); } } @@ -93,7 +97,7 @@ public function exchange_token( WP_REST_Request $request ) { // shape matches what WP REST API would produce at the schema layer. $missing = []; foreach ( [ 'client_id', 'code' ] as $required_param ) { - if ( $request->get_param( $required_param ) === null || $request->get_param( $required_param ) === '' ) { + if ( $this->is_param_empty( $request, $required_param ) ) { $missing[] = $required_param; } } @@ -122,6 +126,15 @@ public function exchange_token( WP_REST_Request $request ) { ); } + // RFC 6749 section 4.1.3: the server must authenticate the client when + // the client is confidential. Public clients have no secret to check. + if ( $client->requires_secret() ) { + $client_secret = (string) $request->get_param( 'client_secret' ); + if ( '' === $client_secret || ! $client->check_secret( $client_secret ) ) { + return $this->client_authentication_failed( $request ); + } + } + $auth_code = $client->get_authorization_code( $request['code'] ); if ( is_wp_error( $auth_code ) ) { return $auth_code; @@ -182,11 +195,7 @@ private function handle_client_credentials( WP_REST_Request $request ) { $grant_ok = $client && $client->is_client_credentials_enabled(); if ( ! $creds_ok || ! $grant_ok ) { - return new WP_Error( - 'oauth2.endpoints.token.invalid_client', - __( 'Client authentication failed.', 'oauth2' ), - [ 'status' => WP_Http::UNAUTHORIZED ] - ); + return $this->client_authentication_failed( $request ); } $token = OAuth2\Tokens\Access_Token::create_for_client( $client ); @@ -228,6 +237,62 @@ private function extract_client_credentials( WP_REST_Request $request ) { ); } + /** + * Check whether a request parameter is missing or empty. + * + * @param WP_REST_Request $request Request object. + * @param string $param Parameter name. + * + * @return bool True if the parameter has no usable value. + */ + private function is_param_empty( WP_REST_Request $request, $param ) { + $value = $request->get_param( $param ); + + return null === $value || '' === $value; + } + + /** + * Build the response for a failed client authentication. + * + * The reason is never given: telling "unknown client" apart from "wrong + * secret" would confirm a valid client ID and secret pair. + * + * @param WP_REST_Request $request Request object. + * + * @return WP_Error|WP_REST_Response Error, or a response carrying a Basic challenge. + */ + private function client_authentication_failed( WP_REST_Request $request ) { + $error = new WP_Error( + 'oauth2.endpoints.token.invalid_client', + __( 'Client authentication failed.', 'oauth2' ), + [ 'status' => WP_Http::UNAUTHORIZED ] + ); + + if ( ! $this->has_basic_auth_header( $request ) ) { + return $error; + } + + // RFC 6749 section 5.2: a client that authenticated with the + // Authorization header must get a matching challenge back. + $response = rest_convert_error_to_response( $error ); + $response->header( 'WWW-Authenticate', 'Basic realm="OAuth2 token endpoint"' ); + + return $response; + } + + /** + * Check whether the request carries an HTTP Basic Authorization header. + * + * @param WP_REST_Request $request Request object. + * + * @return bool True if a Basic header is present. + */ + private function has_basic_auth_header( WP_REST_Request $request ) { + $auth_header = $request->get_header( 'authorization' ); + + return ! empty( $auth_header ) && stripos( $auth_header, 'Basic ' ) === 0; + } + /** * Read client credentials from an HTTP Basic Authorization header. * @@ -236,13 +301,11 @@ private function extract_client_credentials( WP_REST_Request $request ) { * header is malformed, or null if there is no Basic header. */ private function get_basic_auth_credentials( WP_REST_Request $request ) { - $auth_header = $request->get_header( 'authorization' ); - - if ( empty( $auth_header ) || stripos( $auth_header, 'Basic ' ) !== 0 ) { + if ( ! $this->has_basic_auth_header( $request ) ) { return null; } - $encoded = substr( $auth_header, 6 ); + $encoded = substr( $request->get_header( 'authorization' ), 6 ); $decoded = base64_decode( $encoded, true ); if ( false === $decoded ) { diff --git a/tests/test-client.php b/tests/test-client.php index f63fb51..43d91a7 100644 --- a/tests/test-client.php +++ b/tests/test-client.php @@ -153,6 +153,33 @@ public function test_check_secret_false_for_wrong_secret() { $this->assertFalse( $this->client->check_secret( 'wrongsecret' ) ); } + public function test_requires_secret_true_for_private_client() { + $client = $this->create_client( [ 'type' => 'private' ] ); + $this->assertTrue( $client->requires_secret() ); + } + + public function test_requires_secret_false_for_public_client() { + $client = $this->create_client( [ 'type' => 'public' ] ); + $this->assertFalse( $client->requires_secret() ); + } + + public function test_requires_secret_false_for_other_type() { + $this->assertFalse( $this->client->requires_secret() ); + } + + public function test_requires_secret_false_without_a_stored_type() { + delete_post_meta( $this->client->get_post_id(), Client::TYPE_KEY ); + $this->assertFalse( $this->client->requires_secret() ); + } + + public function test_requires_secret_can_be_filtered() { + add_filter( 'oauth2.client.requires_secret', '__return_true' ); + $requires = $this->client->requires_secret(); + remove_filter( 'oauth2.client.requires_secret', '__return_true' ); + + $this->assertTrue( $requires ); + } + public function test_update_changes_name() { $updated = $this->client->update( [ 'name' => 'Updated Name', diff --git a/tests/test-personalclient.php b/tests/test-personalclient.php new file mode 100644 index 0000000..5431fc5 --- /dev/null +++ b/tests/test-personalclient.php @@ -0,0 +1,41 @@ +client = PersonalClient::get_instance(); + } + + public function test_requires_secret_is_false() { + $this->assertFalse( $this->client->requires_secret() ); + } + + public function test_check_secret_is_false_for_any_value() { + $this->assertFalse( $this->client->check_secret( '' ) ); + $this->assertFalse( $this->client->check_secret( 'anything' ) ); + } + + public function test_client_credentials_grant_is_disabled() { + $this->assertFalse( $this->client->is_client_credentials_enabled() ); + } +} diff --git a/tests/test-token-endpoint.php b/tests/test-token-endpoint.php index d7c7180..7d00723 100644 --- a/tests/test-token-endpoint.php +++ b/tests/test-token-endpoint.php @@ -11,6 +11,7 @@ use WP\OAuth2\Client; use WP\OAuth2\Endpoints\Token; +use WP\OAuth2\PersonalClient; use WP\OAuth2\Tokens\Authorization_Code; use WP_REST_Request; use WP_REST_Server; @@ -340,4 +341,214 @@ public function test_validate_grant_type_rejects_unknown() { $this->assertFalse( $handler->validate_grant_type( 'implicit' ) ); $this->assertFalse( $handler->validate_grant_type( '' ) ); } + + // ------------------------------------------------------------------------- + // Confidential client authentication + // ------------------------------------------------------------------------- + + /** + * Request an access token for a client, with an optional secret. + * + * @param Client $client Client to exchange a fresh code for. + * @param array $params Extra request parameters. + * @param string $basic Raw value for an Authorization header, if any. + * + * @return \WP_REST_Response Dispatched response. + */ + protected function request_token( Client $client, array $params = [], $basic = '' ) { + $user = $this->factory->user->create_and_get(); + $code = Authorization_Code::create( $client, $user ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'code', $code->get_code() ); + foreach ( $params as $key => $value ) { + $request->set_param( $key, $value ); + } + if ( '' !== $basic ) { + $request->add_header( 'Authorization', 'Basic ' . $basic ); + } + + return $this->server->dispatch( $request ); + } + + public function test_private_client_without_secret_is_rejected() { + $client = $this->create_client( [ 'type' => 'private' ] ); + + $response = $this->request_token( $client, [ 'client_id' => $client->get_id() ] ); + + $this->assertEquals( 401, $response->get_status() ); + $data = $response->get_data(); + $this->assertEquals( 'oauth2.endpoints.token.invalid_client', $data['code'] ); + } + + public function test_private_client_with_correct_secret_is_accepted() { + $client = $this->create_client( [ 'type' => 'private' ] ); + + $response = $this->request_token( + $client, + [ + 'client_id' => $client->get_id(), + 'client_secret' => $client->get_secret(), + ] + ); + + $this->assertEquals( 200, $response->get_status() ); + $this->assertArrayHasKey( 'access_token', $response->get_data() ); + } + + public function test_private_client_with_wrong_secret_is_rejected() { + $client = $this->create_client( [ 'type' => 'private' ] ); + + $response = $this->request_token( + $client, + [ + 'client_id' => $client->get_id(), + 'client_secret' => 'wrong-secret', + ] + ); + + $this->assertEquals( 401, $response->get_status() ); + } + + public function test_private_client_secret_via_basic_auth_header() { + $client = $this->create_client( [ 'type' => 'private' ] ); + $encoded = base64_encode( $client->get_id() . ':' . $client->get_secret() ); + + $response = $this->request_token( $client, [], $encoded ); + + $this->assertEquals( 200, $response->get_status() ); + $this->assertArrayHasKey( 'access_token', $response->get_data() ); + } + + public function test_private_client_basic_secret_pairs_with_body_client_id() { + $client = $this->create_client( [ 'type' => 'private' ] ); + $encoded = base64_encode( $client->get_id() . ':' . $client->get_secret() ); + + $response = $this->request_token( $client, [ 'client_id' => $client->get_id() ], $encoded ); + + $this->assertEquals( 200, $response->get_status() ); + } + + public function test_private_client_basic_secret_ignored_for_another_client() { + $client = $this->create_client( [ 'type' => 'private' ] ); + $other = $this->create_client( [ 'type' => 'private' ], 'Other Client' ); + $encoded = base64_encode( $other->get_id() . ':' . $other->get_secret() ); + + $response = $this->request_token( $client, [ 'client_id' => $client->get_id() ], $encoded ); + + $this->assertEquals( 401, $response->get_status() ); + } + + public function test_private_client_with_empty_stored_secret_is_rejected() { + $client = $this->create_client( [ 'type' => 'private' ] ); + update_post_meta( $client->get_post_id(), Client::CLIENT_SECRET_KEY, '' ); + + $response = $this->request_token( + $client, + [ + 'client_id' => $client->get_id(), + 'client_secret' => '', + ] + ); + + $this->assertEquals( 401, $response->get_status() ); + } + + public function test_failed_client_authentication_keeps_the_code() { + $client = $this->create_client( [ 'type' => 'private' ] ); + $user = $this->factory->user->create_and_get(); + $code = Authorization_Code::create( $client, $user ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + $this->server->dispatch( $request ); + + $stored = Authorization_Code::get_by_code( $client, $code->get_code() ); + $this->assertNotWPError( $stored ); + } + + public function test_failed_basic_authentication_sends_a_challenge() { + $client = $this->create_client( [ 'type' => 'private' ] ); + $encoded = base64_encode( $client->get_id() . ':wrong-secret' ); + + $response = $this->request_token( $client, [], $encoded ); + + $this->assertEquals( 401, $response->get_status() ); + $headers = $response->get_headers(); + $this->assertArrayHasKey( 'WWW-Authenticate', $headers ); + $this->assertStringStartsWith( 'Basic realm=', $headers['WWW-Authenticate'] ); + } + + public function test_failed_body_authentication_sends_no_challenge() { + $client = $this->create_client( [ 'type' => 'private' ] ); + + $response = $this->request_token( + $client, + [ + 'client_id' => $client->get_id(), + 'client_secret' => 'wrong-secret', + ] + ); + + $this->assertEquals( 401, $response->get_status() ); + $this->assertArrayNotHasKey( 'WWW-Authenticate', $response->get_headers() ); + } + + public function test_public_client_without_secret_is_accepted() { + $client = $this->create_client( [ 'type' => 'public' ] ); + + $response = $this->request_token( $client, [ 'client_id' => $client->get_id() ] ); + + $this->assertEquals( 200, $response->get_status() ); + $this->assertArrayHasKey( 'access_token', $response->get_data() ); + } + + public function test_client_without_a_stored_type_is_accepted() { + $client = $this->create_client(); + delete_post_meta( $client->get_post_id(), Client::TYPE_KEY ); + + $response = $this->request_token( $client, [ 'client_id' => $client->get_id() ] ); + + $this->assertEquals( 200, $response->get_status() ); + $this->assertArrayHasKey( 'access_token', $response->get_data() ); + } + + public function test_requires_secret_filter_can_force_authentication() { + $client = $this->create_client( [ 'type' => 'public' ] ); + add_filter( 'oauth2.client.requires_secret', '__return_true' ); + + $response = $this->request_token( $client, [ 'client_id' => $client->get_id() ] ); + + remove_filter( 'oauth2.client.requires_secret', '__return_true' ); + $this->assertEquals( 401, $response->get_status() ); + } + + public function test_personal_client_cannot_use_the_authorization_code_grant() { + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', PersonalClient::ID ); + $request->set_param( 'code', 'anycode' ); + + $response = $this->server->dispatch( $request ); + + $this->assertNotEquals( 200, $response->get_status() ); + $data = $response->get_data(); + $this->assertEquals( 'oauth2.personalclient.no_auth_code', $data['code'] ); + } + + public function test_personal_client_cannot_use_the_client_credentials_grant() { + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'client_credentials' ); + $request->set_param( 'client_id', PersonalClient::ID ); + $request->set_param( 'client_secret', 'any-secret' ); + + $response = $this->server->dispatch( $request ); + + $this->assertEquals( 401, $response->get_status() ); + $data = $response->get_data(); + $this->assertEquals( 'oauth2.endpoints.token.invalid_client', $data['code'] ); + } }