From 5933325855091553719a484649de03212012516b Mon Sep 17 00:00:00 2001 From: Chuck Adams Date: Thu, 6 Feb 2025 13:10:32 -0700 Subject: [PATCH] Make API token optional for user api routes (#159) * feat: make api token optional for user api routes * config: disable deprecations in production php.ini files dev should probably override this setting, but keeping it as-is for now --- .env.example | 1 - app/Http/Middleware/AuthOptional.php | 22 +++++++++++++++++++ bootstrap/app.php | 4 +++- config/app.php | 1 - docker/cli/php.ini | 4 ++-- docker/webapp/php.ini | 4 ++-- routes/api.php | 22 ++++++++----------- routes/inc/download.php | 3 --- routes/web.php | 11 ---------- .../API/WpOrg/ApiAuthenticationTest.php | 15 ++++++++----- tests/Helpers/requests.php | 21 +++++------------- 11 files changed, 52 insertions(+), 56 deletions(-) create mode 100644 app/Http/Middleware/AuthOptional.php diff --git a/.env.example b/.env.example index 5a5f9025..9cdb732a 100644 --- a/.env.example +++ b/.env.example @@ -76,7 +76,6 @@ MAILGUN_ENDPOINT=api.mailgun.net MAILGUN_SECRET= # AspirePress Related Configuration -API_AUTHENTICATION_ENABLED=true DOWNLOAD_BASE=https://api.aspiredev.org/download/ # DOWNLOAD_BASE=https://fastly.api.aspiredev.org/download/ DOWNLOAD_CACHE_SECONDS=864000 diff --git a/app/Http/Middleware/AuthOptional.php b/app/Http/Middleware/AuthOptional.php new file mode 100644 index 00000000..521f27b4 --- /dev/null +++ b/app/Http/Middleware/AuthOptional.php @@ -0,0 +1,22 @@ +bearerToken()) { + $user = auth($gate)->user() or throw new UnauthorizedHttpException('Invalid authentication token'); + auth($gate)->setUser($user); + } + return $next($request); + } +} diff --git a/bootstrap/app.php b/bootstrap/app.php index 1b436e8d..d072310b 100644 --- a/bootstrap/app.php +++ b/bootstrap/app.php @@ -52,11 +52,13 @@ // CacheApiResponse::class // replaced with laravels cache.headers in api.php ]); - // https://spatie.be/docs/laravel-permission/v6/basic-usage/middleware $middleware->alias([ + // https://spatie.be/docs/laravel-permission/v6/basic-usage/middleware 'role' => \Spatie\Permission\Middleware\RoleMiddleware::class, 'permission' => \Spatie\Permission\Middleware\PermissionMiddleware::class, 'role_or_permission' => \Spatie\Permission\Middleware\RoleOrPermissionMiddleware::class, + + 'auth.optional' => App\Http\Middleware\AuthOptional::class, ]); }) diff --git a/config/app.php b/config/app.php index 66f0e01d..7495d205 100644 --- a/config/app.php +++ b/config/app.php @@ -28,7 +28,6 @@ //// AspirePress Configuration 'aspirecloud' => [ - 'api_authentication_enable' => env('API_AUTHENTICATION_ENABLED', false), 'download' => [ 'base' => env('DOWNLOAD_BASE', env('APP_URL') . '/download/'), # must have a trailing slash! 'cache_seconds' => env('DOWNLOAD_CACHE_SECONDS', 60 * 60 * 24 * 10), diff --git a/docker/cli/php.ini b/docker/cli/php.ini index 889b14be..a4f69b65 100644 --- a/docker/cli/php.ini +++ b/docker/cli/php.ini @@ -5,8 +5,8 @@ upload_max_filesize = 20M max_file_uploads = 20 -error_reporting = E_ALL -; error_reporting = E_ALL & ~E_DEPRECATED +; error_reporting = E_ALL +error_reporting = E_ALL & ~E_DEPRECATED display_errors = Off display_startup_errors = Off diff --git a/docker/webapp/php.ini b/docker/webapp/php.ini index 889b14be..a4f69b65 100644 --- a/docker/webapp/php.ini +++ b/docker/webapp/php.ini @@ -5,8 +5,8 @@ upload_max_filesize = 20M max_file_uploads = 20 -error_reporting = E_ALL -; error_reporting = E_ALL & ~E_DEPRECATED +; error_reporting = E_ALL +error_reporting = E_ALL & ~E_DEPRECATED display_errors = Off display_startup_errors = Off diff --git a/routes/api.php b/routes/api.php index fb25eb40..4d5b5c0f 100644 --- a/routes/api.php +++ b/routes/api.php @@ -15,18 +15,12 @@ // https://codex.wordpress.org/WordPress.org_API -$middlewares = [ - NormalizeWpOrgRequest::class, - 'cache.headers:public;max_age=300,etag', // for the CDN's benefit: the WP user agent does not cache at all. -]; -$routeDefinition = Route::prefix('/'); - -if (config('app.aspirecloud.api_authentication_enable')) { - $middlewares[] = 'auth:sanctum'; -} - -$routeDefinition - ->middleware($middlewares) +Route::prefix('/') + ->middleware([ + 'auth.optional:sanctum', + NormalizeWpOrgRequest::class, + 'cache.headers:public;s_maxage=300,etag', // for the CDN's benefit: the WP user agent does not cache at all. + ]) ->group(function (Router $router) { $router->get('/secret-key/{version}', [SecretKeyController::class, 'index'])->where(['version' => '1.[01]']); $router->get('/secret-key/{version}/salt', [SecretKeyController::class, 'salt'])->where(['version' => '1.1']); @@ -52,7 +46,9 @@ $router->get('/translations/themes/{version}', CatchAllController::class)->where(['version' => '1.0']); $router->get('/themes/info/{version}', [ThemeController::class, 'info'])->where(['version' => '1.[012]']); - $router->match(['get', 'post'], '/themes/update-check/{version}', ThemeUpdatesController::class)->where(['version' => '1.[01]']); + $router->match(['get', 'post'], '/themes/update-check/{version}', ThemeUpdatesController::class)->where( + ['version' => '1.[01]'], + ); $router->get('/plugins/info/1.2', PluginInformation_1_2_Controller::class); $router->get('/plugins/info/{version}', CatchAllController::class)->where(['version' => '1.[01]']); diff --git a/routes/inc/download.php b/routes/inc/download.php index 5922d750..7d6fdc86 100644 --- a/routes/inc/download.php +++ b/routes/inc/download.php @@ -9,12 +9,9 @@ use Illuminate\Routing\Router; use Illuminate\Support\Facades\Route; -// downloads can never require api keys, they're fetched by ordinary browser UI and by WP in places we don't hook. -// $auth_middleware = config('app.aspirecloud.api_authentication_enable') ? ['auth:sanctum'] : []; $cache_seconds = config('app.aspirecloud.download.cache_seconds'); $middleware = [ "cache.headers:public;max_age=$cache_seconds", // we're streaming responses, so no etags - // ...$auth_middleware, ]; Route::prefix('/') diff --git a/routes/web.php b/routes/web.php index 463aff8e..ab26a598 100644 --- a/routes/web.php +++ b/routes/web.php @@ -1,17 +1,6 @@ Route::has('login'), -// 'canRegister' => Route::has('register'), -// 'laravelVersion' => Application::VERSION, -// 'phpVersion' => PHP_VERSION, -// ]); -// }); Route::view('/', 'home'); diff --git a/tests/Feature/API/WpOrg/ApiAuthenticationTest.php b/tests/Feature/API/WpOrg/ApiAuthenticationTest.php index 6019ba9b..c5cef2c1 100644 --- a/tests/Feature/API/WpOrg/ApiAuthenticationTest.php +++ b/tests/Feature/API/WpOrg/ApiAuthenticationTest.php @@ -1,11 +1,14 @@ getjson('/plugins/info/1.1?action=query_plugins'); + $response->assertStatus(200); +}); + +it('should return 401 when an invalid auth token is present', function () { + $response = $this->getjson('/plugins/info/1.1?action=query_plugins', ['Authorization' => 'Bearer invalid-token']); $response->assertStatus(401); -})->skip(fn() => !config('app.aspirecloud.api_authentication_enable'), 'API authentication is disabled'); +}); + +// TODO: write test for real auth token -it('should return 200 if the API authentication is disable', function () { - $response = $this->getjson('/plugins/info/1.1?action=query_plugins'); - $response->assertStatus(200); -})->skip(fn() => config('app.aspirecloud.api_authentication_enable'), 'API authentication is enabled'); diff --git a/tests/Helpers/requests.php b/tests/Helpers/requests.php index 069303a5..bbae481d 100644 --- a/tests/Helpers/requests.php +++ b/tests/Helpers/requests.php @@ -5,29 +5,18 @@ use App\Models\User; use Illuminate\Support\Str; -/** - * Helper function to make an authenticated request or not - * based on the configuration. - * @param mixed $method - * @param mixed $uri - * @param mixed $data - * @param mixed $headers - */ -function makeApiRequest($method, $uri, $data = [], $headers = []) +function makeApiRequest(string $method, string $uri, array $data = [], array $headers = []) { - $isAuthEnabled = config('app.aspirecloud.api_authentication_enable'); - $testCase = test(); + $testCase = test(); - if ($isAuthEnabled) { - $user = User::factory()->create(); - $testCase = $testCase->actingAs($user); - } + $user = User::factory()->create(); + $testCase = $testCase->actingAs($user); if (Str::lower($method) === 'post') { return $testCase->post($uri, $data, $headers); } - // sent the header on get request too + // XXX shouldn't we be setting headers unconditionally? if (!empty($headers)) { return $testCase->withHeaders($headers)->get($uri); }