From be554b9c797c53a9de301dc58b9e8bc1e8b54ae6 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 18 Jan 2020 15:03:28 +0000 Subject: [PATCH] Added configurable API throttling, Handled API errors standardly --- .env.example.complete | 5 ++- app/Config/api.php | 3 ++ app/Exceptions/Handler.php | 42 +++++++++++++++++++++ app/Http/Kernel.php | 2 +- app/Http/Middleware/ThrottleApiRequests.php | 18 +++++++++ phpunit.xml | 1 + tests/Api/ApiAuthTest.php | 25 ++++++++++++ tests/Api/ApiConfigTest.php | 11 ++++++ tests/Api/BooksApiTest.php | 20 ++++++++++ 9 files changed, 125 insertions(+), 2 deletions(-) create mode 100644 app/Http/Middleware/ThrottleApiRequests.php diff --git a/.env.example.complete b/.env.example.complete index 716d614a3..e44644f08 100644 --- a/.env.example.complete +++ b/.env.example.complete @@ -260,4 +260,7 @@ ALLOW_ROBOTS=null # The default and maximum item-counts for listing API requests. API_DEFAULT_ITEM_COUNT=100 -API_MAX_ITEM_COUNT=500 \ No newline at end of file +API_MAX_ITEM_COUNT=500 + +# The number of API requests that can be made per minute by a single user. +API_REQUESTS_PER_MIN=180 \ No newline at end of file diff --git a/app/Config/api.php b/app/Config/api.php index dd54b2906..6afea2dc8 100644 --- a/app/Config/api.php +++ b/app/Config/api.php @@ -17,4 +17,7 @@ return [ // The maximum number of items that can be returned in a listing API request. 'max_item_count' => env('API_MAX_ITEM_COUNT', 500), + // The number of API requests that can be made per minute by a single user. + 'requests_per_minute' => env('API_REQUESTS_PER_MIN', 180) + ]; diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php index 70a534975..284d731d7 100644 --- a/app/Exceptions/Handler.php +++ b/app/Exceptions/Handler.php @@ -7,6 +7,9 @@ use Illuminate\Auth\Access\AuthorizationException; use Illuminate\Auth\AuthenticationException; use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler; +use Illuminate\Http\JsonResponse; +use Illuminate\Http\Request; +use Illuminate\Http\Response; use Illuminate\Validation\ValidationException; use Symfony\Component\HttpKernel\Exception\HttpException; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; @@ -47,6 +50,10 @@ class Handler extends ExceptionHandler */ public function render($request, Exception $e) { + if ($this->isApiRequest($request)) { + return $this->renderApiException($e); + } + // Handle notify exceptions which will redirect to the // specified location then show a notification message. if ($this->isExceptionType($e, NotifyException::class)) { @@ -70,6 +77,41 @@ class Handler extends ExceptionHandler return parent::render($request, $e); } + /** + * Check if the given request is an API request. + */ + protected function isApiRequest(Request $request): bool + { + return strpos($request->path(), 'api/') === 0; + } + + /** + * Render an exception when the API is in use. + */ + protected function renderApiException(Exception $e): JsonResponse + { + $code = $e->getCode() === 0 ? 500 : $e->getCode(); + $headers = []; + if ($e instanceof HttpException) { + $code = $e->getStatusCode(); + $headers = $e->getHeaders(); + } + + $responseData = [ + 'error' => [ + 'message' => $e->getMessage(), + ] + ]; + + if ($e instanceof ValidationException) { + $responseData['error']['validation'] = $e->errors(); + $code = $e->status; + } + + $responseData['error']['code'] = $code; + return new JsonResponse($responseData, $code, $headers); + } + /** * Check the exception chain to compare against the original exception type. * @param Exception $e diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index 978583a7f..c2016281a 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -32,7 +32,7 @@ class Kernel extends HttpKernel \BookStack\Http\Middleware\GlobalViewData::class, ], 'api' => [ - 'throttle:60,1', + \BookStack\Http\Middleware\ThrottleApiRequests::class, \BookStack\Http\Middleware\EncryptCookies::class, \BookStack\Http\Middleware\StartSessionIfCookieExists::class, \BookStack\Http\Middleware\ApiAuthenticate::class, diff --git a/app/Http/Middleware/ThrottleApiRequests.php b/app/Http/Middleware/ThrottleApiRequests.php new file mode 100644 index 000000000..d08840cd1 --- /dev/null +++ b/app/Http/Middleware/ThrottleApiRequests.php @@ -0,0 +1,18 @@ + + diff --git a/tests/Api/ApiAuthTest.php b/tests/Api/ApiAuthTest.php index b6b6b72ac..2ab81814b 100644 --- a/tests/Api/ApiAuthTest.php +++ b/tests/Api/ApiAuthTest.php @@ -120,4 +120,29 @@ class ApiAuthTest extends TestCase $resp->assertJson($this->errorResponse("The email address for the account in use needs to be confirmed", 401)); } + public function test_rate_limit_headers_active_on_requests() + { + $resp = $this->actingAsApiEditor()->get($this->endpoint); + $resp->assertHeader('x-ratelimit-limit', 180); + $resp->assertHeader('x-ratelimit-remaining', 179); + $resp = $this->actingAsApiEditor()->get($this->endpoint); + $resp->assertHeader('x-ratelimit-remaining', 178); + } + + public function test_rate_limit_hit_gives_json_error() + { + config()->set(['api.requests_per_minute' => 1]); + $resp = $this->actingAsApiEditor()->get($this->endpoint); + $resp->assertStatus(200); + + $resp = $this->actingAsApiEditor()->get($this->endpoint); + $resp->assertStatus(429); + $resp->assertHeader('x-ratelimit-remaining', 0); + $resp->assertHeader('retry-after'); + $resp->assertJson([ + 'error' => [ + 'code' => 429, + ] + ]); + } } \ No newline at end of file diff --git a/tests/Api/ApiConfigTest.php b/tests/Api/ApiConfigTest.php index d9367741f..1b3da2f34 100644 --- a/tests/Api/ApiConfigTest.php +++ b/tests/Api/ApiConfigTest.php @@ -44,4 +44,15 @@ class ApiConfigTest extends TestCase $resp->assertJsonCount(2, 'data'); } + public function test_requests_per_min_alters_rate_limit() + { + $resp = $this->actingAsApiEditor()->get($this->endpoint); + $resp->assertHeader('x-ratelimit-limit', 180); + + config()->set(['api.requests_per_minute' => 10]); + + $resp = $this->actingAsApiEditor()->get($this->endpoint); + $resp->assertHeader('x-ratelimit-limit', 10); + } + } \ No newline at end of file diff --git a/tests/Api/BooksApiTest.php b/tests/Api/BooksApiTest.php index 813e34360..a40e4c93b 100644 --- a/tests/Api/BooksApiTest.php +++ b/tests/Api/BooksApiTest.php @@ -38,6 +38,26 @@ class BooksApiTest extends TestCase $this->assertActivityExists('book_create', $newItem); } + public function test_book_name_needed_to_create() + { + $this->actingAsApiEditor(); + $details = [ + 'description' => 'A book created via the API', + ]; + + $resp = $this->postJson($this->baseEndpoint, $details); + $resp->assertStatus(422); + $resp->assertJson([ + "error" => [ + "message" => "The given data was invalid.", + "validation" => [ + "name" => ["The name field is required."] + ], + "code" => 422, + ], + ]); + } + public function test_read_endpoint() { $this->actingAsApiEditor();