From 7fa5523a7d1743a8563e828970950727e2842c8d Mon Sep 17 00:00:00 2001 From: "J. King" Date: Tue, 1 Dec 2020 11:06:29 -0500 Subject: [PATCH] Simplify handling of invalid paths and methods --- lib/REST/Exception404.php | 10 ---------- lib/REST/Exception405.php | 10 ---------- lib/REST/Miniflux/V1.php | 19 ++++++++----------- lib/REST/NextcloudNews/V1_2.php | 25 +++++++++---------------- tests/cases/REST/Miniflux/TestV1.php | 20 +++++--------------- 5 files changed, 22 insertions(+), 62 deletions(-) delete mode 100644 lib/REST/Exception404.php delete mode 100644 lib/REST/Exception405.php diff --git a/lib/REST/Exception404.php b/lib/REST/Exception404.php deleted file mode 100644 index 8bee192..0000000 --- a/lib/REST/Exception404.php +++ /dev/null @@ -1,10 +0,0 @@ -handleHTTPOptions($target); } $func = $this->chooseCall($target, $method); + if ($func instanceof ResponseInterface) { + return $func; + } if ($func === "opmlImport") { if (!HTTP::matchType($req, "", ...[self::ACCEPTED_TYPES_OPML])) { return new ErrorResponse("", 415, ['Accept' => implode(", ", self::ACCEPTED_TYPES_OPML)]); @@ -149,7 +149,7 @@ class V1 extends \JKingWeb\Arsse\REST\AbstractHandler { } } - protected function chooseCall(string $url, string $method): string { + protected function chooseCall(string $url, string $method) { // // normalize the URL path: change any IDs to 1 for easier comparison $url = $this->normalizePathIds($url); // normalize the HTTP method to uppercase @@ -160,18 +160,15 @@ class V1 extends \JKingWeb\Arsse\REST\AbstractHandler { // if the path is supported, make sure the method is allowed if (isset($this->paths[$url][$method])) { // if it is allowed, return the object method to run, assuming the method exists - if (method_exists($this, $this->paths[$url][$method])) { - return $this->paths[$url][$method]; - } else { - throw new Exception501(); // @codeCoverageIgnore - } + assert(method_exists($this, $this->paths[$url][$method]), new \Exception("Method is not implemented")); + return $this->paths[$url][$method]; } else { // otherwise return 405 - throw new Exception405(implode(", ", array_keys($this->paths[$url]))); + return new EmptyResponse(405, ['Allow' => implode(", ", array_keys($this->paths[$url]))]); } } else { // if the path is not supported, return 404 - throw new Exception404(); + return new EmptyResponse(404); } } diff --git a/lib/REST/NextcloudNews/V1_2.php b/lib/REST/NextcloudNews/V1_2.php index 4741e83..7cefe13 100644 --- a/lib/REST/NextcloudNews/V1_2.php +++ b/lib/REST/NextcloudNews/V1_2.php @@ -15,9 +15,6 @@ use JKingWeb\Arsse\Db\ExceptionInput; use JKingWeb\Arsse\Feed\Exception as FeedException; use JKingWeb\Arsse\Misc\HTTP; use JKingWeb\Arsse\REST\Exception; -use JKingWeb\Arsse\REST\Exception404; -use JKingWeb\Arsse\REST\Exception405; -use JKingWeb\Arsse\REST\Exception501; use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Message\ResponseInterface; use Laminas\Diactoros\Response\JsonResponse as Response; @@ -110,15 +107,14 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler { // merge GET and POST data, and normalize it. POST parameters are preferred over GET parameters $data = $this->normalizeInput(array_merge($req->getQueryParams(), $data), $this->validInput, "unix"); // check to make sure the requested function is implemented + $func = $this->chooseCall($target, $req->getMethod()); + if ($func instanceof ResponseInterface) { + return $func; + } // dispatch try { - $func = $this->chooseCall($target, $req->getMethod()); $path = explode("/", ltrim($target, "/")); return $this->$func($path, $data); - } catch (Exception404 $e) { - return new EmptyResponse(404); - } catch (Exception405 $e) { - return new EmptyResponse(405, ['Allow' => $e->getMessage()]); // @codeCoverageIgnoreStart } catch (Exception $e) { // if there was a REST exception return 400 @@ -141,7 +137,7 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler { return implode("/", $path); } - protected function chooseCall(string $url, string $method): string { + protected function chooseCall(string $url, string $method) { // // normalize the URL path: change any IDs to 1 for easier comparison $url = $this->normalizePathIds($url); // normalize the HTTP method to uppercase @@ -152,18 +148,15 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler { // if the path is supported, make sure the method is allowed if (isset($this->paths[$url][$method])) { // if it is allowed, return the object method to run, assuming the method exists - if (method_exists($this, $this->paths[$url][$method])) { - return $this->paths[$url][$method]; - } else { - throw new Exception501(); // @codeCoverageIgnore - } + assert(method_exists($this, $this->paths[$url][$method]), new \Exception("Method is not implemented")); + return $this->paths[$url][$method]; } else { // otherwise return 405 - throw new Exception405(implode(", ", array_keys($this->paths[$url]))); + return new EmptyResponse(405, ['Allow' => implode(", ", array_keys($this->paths[$url]))]); } } else { // if the path is not supported, return 404 - throw new Exception404(); + return new EmptyResponse(404); } } diff --git a/tests/cases/REST/Miniflux/TestV1.php b/tests/cases/REST/Miniflux/TestV1.php index de35c27..c62f0c0 100644 --- a/tests/cases/REST/Miniflux/TestV1.php +++ b/tests/cases/REST/Miniflux/TestV1.php @@ -11,7 +11,6 @@ use JKingWeb\Arsse\User; use JKingWeb\Arsse\Database; use JKingWeb\Arsse\Db\Transaction; use JKingWeb\Arsse\Db\ExceptionInput; -use JKingWeb\Arsse\REST\Exception404; use JKingWeb\Arsse\REST\Miniflux\V1; use JKingWeb\Arsse\REST\Miniflux\ErrorResponse; use Psr\Http\Message\ResponseInterface; @@ -61,7 +60,7 @@ class TestV1 extends \JKingWeb\Arsse\Test\AbstractTest { /** @dataProvider provideAuthResponses */ public function testAuthenticateAUser($token, bool $auth, bool $success): void { - $exp = new ErrorResponse("401", 401); + $exp = $success ? new EmptyResponse(404) : new ErrorResponse("401", 401); $user = "john.doe@example.com"; if ($token !== null) { $headers = ['X-Auth-Token' => $token]; @@ -71,17 +70,8 @@ class TestV1 extends \JKingWeb\Arsse\Test\AbstractTest { Arsse::$user->id = null; \Phake::when(Arsse::$db)->tokenLookup->thenThrow(new ExceptionInput("subjectMissing")); \Phake::when(Arsse::$db)->tokenLookup("miniflux.login", $this->token)->thenReturn(['user' => $user]); - if ($success) { - $this->expectExceptionObject(new Exception404); - try { - $this->req("GET", "/", "", $headers, $auth); - } finally { - $this->assertSame($user, Arsse::$user->id); - } - } else { - $this->assertMessage($exp, $this->req("GET", "/", "", $headers, $auth)); - $this->assertNull(Arsse::$user->id); - } + $this->assertMessage($exp, $this->req("GET", "/", "", $headers, $auth)); + $this->assertSame($success ? $user : null, Arsse::$user->id); } public function provideAuthResponses(): iterable { @@ -100,7 +90,7 @@ class TestV1 extends \JKingWeb\Arsse\Test\AbstractTest { } /** @dataProvider provideInvalidPaths */ - public function xtestRespondToInvalidPaths($path, $method, $code, $allow = null): void { + public function testRespondToInvalidPaths($path, $method, $code, $allow = null): void { $exp = new EmptyResponse($code, $allow ? ['Allow' => $allow] : []); $this->assertMessage($exp, $this->req($method, $path)); } @@ -108,7 +98,7 @@ class TestV1 extends \JKingWeb\Arsse\Test\AbstractTest { public function provideInvalidPaths(): array { return [ ["/", "GET", 404], - ["/version", "POST", 405, "GET"], + ["/me", "POST", 405, "GET"], ]; }