From 870d8ab1dba1466e5ced0e59a9fae4989db4c2f2 Mon Sep 17 00:00:00 2001 From: Brady Vercher Date: Tue, 26 Mar 2019 16:06:08 -0700 Subject: [PATCH] Remove the handle_error() method from authentication servers. This creates a new AuthenticationException exception to transport authentication errors, including headers and status code, from the authenticate() method instead of using a handle_error() method. If the current request is a REST request, the Authentication provider will convert the exception to an error instead of allowing the exception to bubble up. --- src/Authentication/ApiKey/Server.php | 25 +--- src/Authentication/Server.php | 14 +-- src/Authentication/UnauthorizedServer.php | 24 +--- src/Exception/AuthenticationException.php | 136 ++++++++++++++++++++++ src/Exception/HttpException.php | 54 --------- src/HTTP/Response.php | 10 +- src/Provider/Authentication.php | 32 ++--- 7 files changed, 172 insertions(+), 123 deletions(-) create mode 100644 src/Exception/AuthenticationException.php diff --git a/src/Authentication/ApiKey/Server.php b/src/Authentication/ApiKey/Server.php index 2e1af1f..180aa61 100644 --- a/src/Authentication/ApiKey/Server.php +++ b/src/Authentication/ApiKey/Server.php @@ -12,7 +12,7 @@ namespace SatisPress\Authentication\ApiKey; use SatisPress\Authentication\Server as ServerInterface; -use SatisPress\Exception\HttpException; +use SatisPress\Exception\AuthenticationException; use SatisPress\HTTP\Request; use WP_Error; use WP_Http as HTTP; @@ -74,7 +74,7 @@ public function check_scheme( Request $request ): bool { * @since 0.3.0 * * @param Request $request Request instance. - * @throws HttpException If authentication fails. + * @throws AuthenticationException If authentication fails. * @return int A user ID. */ public function authenticate( Request $request ): int { @@ -82,21 +82,21 @@ public function authenticate( Request $request ): int { // Bail if an API Key wasn't provided. if ( null === $api_key_id ) { - throw HttpException::forMissingAuthorizationHeader(); + throw AuthenticationException::forMissingAuthorizationHeader(); } $api_key = $this->repository->find_by_token( $api_key_id ); // Bail if the API Key doesn't exist. if ( null === $api_key ) { - throw HttpException::forInvalidCredentials(); + throw AuthenticationException::forInvalidCredentials(); } $user = $api_key->get_user(); // Bail if the user couldn't be determined. if ( ! $this->validate_user( $user ) ) { - throw HttpException::forInvalidCredentials(); + throw AuthenticationException::forInvalidCredentials(); } $this->maybe_update_last_used_time( $api_key ); @@ -104,21 +104,6 @@ public function authenticate( Request $request ): int { return $user->ID; } - /** - * Handle errors encountered when authenticating. - * - * @since 0.3.0 - * - * @param HttpException $e HTTP exception. - */ - public function handle_error( HttpException $e ): WP_Error { - if ( HTTP::UNAUTHORIZED === $e->getStatusCode() ) { - header( 'WWW-Authenticate: Basic realm="SatisPress"' ); - } - - wp_die( wp_kses_data( $e->getMessage() ), absint( $e->getStatusCode() ) ); - } - /** * Update the last used time if it's been more than a minute. * diff --git a/src/Authentication/Server.php b/src/Authentication/Server.php index 774f337..0a92f51 100644 --- a/src/Authentication/Server.php +++ b/src/Authentication/Server.php @@ -11,7 +11,7 @@ namespace SatisPress\Authentication; -use SatisPress\Exception\HttpException; +use SatisPress\Exception\AuthenticationException; use Satispress\HTTP\Request; use WP_Error; @@ -37,18 +37,8 @@ public function check_scheme( Request $request ): bool; * @since 0.3.0 * * @param Request $request Request instance. - * @throws HttpException If authentications fails. + * @throws AuthenticationException If authentications fails. * @return int A user ID. */ public function authenticate( Request $request ): int; - - /** - * Handle errors encountered when authenticating. - * - * @since 0.4.0 - * - * @param HttpException $e HTTP exception. - * @return WP_Error - */ - public function handle_error( HttpException $e ): WP_Error; } diff --git a/src/Authentication/UnauthorizedServer.php b/src/Authentication/UnauthorizedServer.php index 88701d4..fc24e66 100644 --- a/src/Authentication/UnauthorizedServer.php +++ b/src/Authentication/UnauthorizedServer.php @@ -13,7 +13,7 @@ namespace SatisPress\Authentication; -use SatisPress\Exception\HttpException; +use SatisPress\Exception\AuthenticationException; use SatisPress\HTTP\Request; use WP_Error; use WP_Http as HTTP; @@ -42,27 +42,9 @@ public function check_scheme( Request $request ): bool { * @since 0.3.0 * * @param Request $request Request instance. - * @throws HttpException If the user has not been authenticated at this point. + * @throws AuthenticationException If the user has not been authenticated at this point. */ public function authenticate( Request $request ): int { - throw HttpException::forAuthenticationRequired(); - } - - /** - * Display an error message when authentication fails. - * - * @since 0.3.0 - * - * @param HttpException $e HTTP exception. - */ - public function handle_error( HttpException $e ): WP_Error { - header( 'WWW-Authenticate: Basic realm="SatisPress"' ); - - wp_die( - wp_kses_data( $e->getMessage() ), - esc_html__( 'Authentication Required', 'satispress' ), - // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped - [ 'response' => HTTP::UNAUTHORIZED ] - ); + throw AuthenticationException::forAuthenticationRequired(); } } diff --git a/src/Exception/AuthenticationException.php b/src/Exception/AuthenticationException.php new file mode 100644 index 0000000..128eaff --- /dev/null +++ b/src/Exception/AuthenticationException.php @@ -0,0 +1,136 @@ +code = $code; + $this->headers = $headers; + + parent::__construct( $message, $status_code, 0, $previous ); + } + + /** + * Create an exception for requests that require authentication. + * + * @since 0.4.0. + * + * @param array $headers Response headers. + * @param string $code Optional. The Exception code. + * @param Throwable $previous Optional. The previous throwable used for the exception chaining. + * @return HTTPException + */ + public static function forAuthenticationRequired( + array $headers = [], + string $code = 'invalid_request', + Throwable $previous = null + ): HttpException { + $headers = $headers ?: [ 'WWW-Authenticate' => 'Basic realm="SatisPress"' ]; + $message = 'Authentication is required for this resource.'; + + return new static( $code, $message, HTTP::UNAUTHORIZED, $headers, $previous ); + } + + /** + * Create an exception for invalid credentials. + * + * @since 0.4.0. + * + * @param array $headers Response headers. + * @param string $code Optional. The Exception code. + * @param Throwable $previous Optional. The previous throwable used for the exception chaining. + * @return HTTPException + */ + public static function forInvalidCredentials( + array $headers = [], + string $code = 'invalid_credentials', + Throwable $previous = null + ): HttpException { + $headers = $headers ?: [ 'WWW-Authenticate' => 'Basic realm="SatisPress"' ]; + $message = 'Invalid credentials.'; + + return new static( $code, $message, HTTP::UNAUTHORIZED, $headers, $previous ); + } + + /** + * Create an exception for a missing authorization header. + * + * @since 0.4.0. + * + * @param array $headers Response headers. + * @param string $code Optional. The Exception code. + * @param Throwable $previous Optional. The previous throwable used for the exception chaining. + * @return HTTPException + */ + public static function forMissingAuthorizationHeader( + array $headers = [], + string $code = 'invalid_credentials', + Throwable $previous = null + ): HttpException { + $headers = $headers ?: [ 'WWW-Authenticate' => 'Basic realm="SatisPress"' ]; + $message = 'Missing authorization header.'; + + return new static( $code, $message, HTTP::UNAUTHORIZED, $headers, $previous ); + } + + /** + * Retrieve the response headers. + * + * @since 0.4.0 + * + * @return array Map of header name to header value. + */ + public function getHeaders(): array { + return $this->headers; + } +} diff --git a/src/Exception/HttpException.php b/src/Exception/HttpException.php index 39d4dab..7932a16 100644 --- a/src/Exception/HttpException.php +++ b/src/Exception/HttpException.php @@ -51,60 +51,6 @@ public function __construct( parent::__construct( $message, $code, $previous ); } - /** - * Create an exception for requests that require authentication. - * - * @since 0.4.0. - * - * @param int $code Optional. The Exception code. - * @param Throwable $previous Optional. The previous throwable used for the exception chaining. - * @return HTTPException - */ - public static function forAuthenticationRequired( - int $code = 0, - Throwable $previous = null - ): HttpException { - $message = 'Authentication is required for this resource.'; - - return new static( $message, HTTP::UNAUTHORIZED, $code, $previous ); - } - - /** - * Create an exception for invalid credentials. - * - * @since 0.4.0. - * - * @param int $code Optional. The Exception code. - * @param Throwable $previous Optional. The previous throwable used for the exception chaining. - * @return HTTPException - */ - public static function forInvalidCredentials( - int $code = 0, - Throwable $previous = null - ): HttpException { - $message = 'Invalid credentials.'; - - return new static( $message, HTTP::UNAUTHORIZED, $code, $previous ); - } - - /** - * Create an exception for a missing authorization header. - * - * @since 0.4.0. - * - * @param int $code Optional. The Exception code. - * @param Throwable $previous Optional. The previous throwable used for the exception chaining. - * @return HTTPException - */ - public static function forMissingAuthorizationHeader( - int $code = 0, - Throwable $previous = null - ): HttpException { - $message = 'Missing authorization header.'; - - return new static( $message, HTTP::UNAUTHORIZED, $code, $previous ); - } - /** * Create an exception for a forbidden resource request. * diff --git a/src/HTTP/Response.php b/src/HTTP/Response.php index 7bb1d1b..b3a023f 100644 --- a/src/HTTP/Response.php +++ b/src/HTTP/Response.php @@ -11,6 +11,7 @@ namespace SatisPress\HTTP; +use SatisPress\Exception\AuthenticationException; use SatisPress\Exception\HttpException; use SatisPress\HTTP\ResponseBody\ErrorBody; use SatisPress\HTTP\ResponseBody\FileBody; @@ -159,6 +160,7 @@ public static function for_file( string $filename ): Response { */ public static function from_exception( \Exception $e ): Response { $status_code = 500; + $headers = []; if ( $e instanceof HttpException ) { $status_code = $e->getStatusCode(); @@ -171,9 +173,15 @@ public static function from_exception( \Exception $e ): Response { $message = 'Sorry, you cannot view this resource.'; } + if ( $e instanceof AuthenticationException ) { + $headers = $e->getHeaders(); + $message = $e->getMessage(); + } + return new static( new ErrorBody( $message, $status_code ), - $status_code + $status_code, + $headers ); } diff --git a/src/Provider/Authentication.php b/src/Provider/Authentication.php index 541ca3a..2801e14 100644 --- a/src/Provider/Authentication.php +++ b/src/Provider/Authentication.php @@ -15,7 +15,7 @@ use Pimple\ServiceIterator; use SatisPress\Authentication\Server; use SatisPress\Capabilities as Caps; -use SatisPress\Exception\HttpException; +use SatisPress\Exception\AuthenticationException; use SatisPress\HTTP\Request; use WP_Error; @@ -25,17 +25,10 @@ * @since 0.3.0 */ class Authentication extends AbstractHookProvider { - /** - * Server used for authenticating the request. - * - * @var Server - */ - protected $active_server; - /** * Errors that occurred during authentication. * - * @var HttpException Authentication exception. + * @var AuthenticationException Authentication exception. */ protected $auth_status; @@ -119,10 +112,9 @@ public function determine_current_user( $user_id ) { try { $user_id = $server->authenticate( $this->request ); - } catch ( HttpException $e ) { - $this->auth_status = $e; - $this->active_server = $server; - $user_id = false; + } catch ( AuthenticationException $e ) { + $this->auth_status = $e; + $user_id = false; add_filter( 'rest_authentication_errors', [ $this, 'get_authentication_errors' ] ); } @@ -140,6 +132,7 @@ public function determine_current_user( $user_id ) { * * @param WP_Error|mixed $value Error from another authentication handler, * null if we should handle it, or another value if not. + * @throws AuthenticationException If this isn't a REST request. * @return WP_Error|bool|null */ public function get_authentication_errors( $value ) { @@ -147,8 +140,17 @@ public function get_authentication_errors( $value ) { return $value; } - return $this->active_server - ->handle_error( $this->auth_status ); + $e = $this->auth_status; + + if ( ! defined( 'REST_REQUEST' ) || ! REST_REQUEST ) { + throw $e; + } + + return new WP_Error( + $e->getCode(), + $e->getMessage(), + [ 'status' => $e->getStatusCode() ] + ); } /**