Require confidential clients to authenticate on the authorization_code grant

inc/class-client.php

@@ -123,6 +123,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.
 	 *

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.
 	 *

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.
 	 *

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 );
@@ -235,6 +244,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.
 	 *
@@ -243,13 +308,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 ) {

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',

tests/test-personalclient.php

@@ -0,0 +1,41 @@
+<?php
+/**
+ * Tests for the PersonalClient class.
+ *
+ * @package WP\OAuth2\Tests
+ */
+
+namespace WP\OAuth2\Tests;
+
+require_once __DIR__ . '/class-test-case.php';
+
+use WP\OAuth2\PersonalClient;
+
+/**
+ * Test cases for the internal client backing personal access tokens.
+ */
+class Test_PersonalClient extends Test_Case {
+
+	/**
+	 * @var PersonalClient
+	 */
+	protected $client;
+
+	public function set_up() {
+		parent::set_up();
+		$this->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() );
+	}
+}