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

@@ -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

app/Http/Middleware/AuthOptional.php

@@ -0,0 +1,22 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Http\Middleware;
+
+use Closure;
+use Illuminate\Http\Request;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpKernel\Exception\UnauthorizedHttpException;
+
+class AuthOptional
+{
+    public function handle(Request $request, Closure $next, string $gate): Response
+    {
+        if ($request->bearerToken()) {
+            $user = auth($gate)->user() or throw new UnauthorizedHttpException('Invalid authentication token');
+            auth($gate)->setUser($user);
+        }
+        return $next($request);
+    }
+}

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,
         ]);
 
     })

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

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

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

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]']);

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('/')

routes/web.php

@@ -1,17 +1,6 @@
 <?php
 
-use Illuminate\Foundation\Application;
 use Illuminate\Support\Facades\Route;
-use Inertia\Inertia;
-
-// Route::get('/', function () {
-//     return Inertia::render('Welcome', [
-//         'canLogin' => Route::has('login'),
-//         'canRegister' => Route::has('register'),
-//         'laravelVersion' => Application::VERSION,
-//         'phpVersion' => PHP_VERSION,
-//     ]);
-// });
 
 Route::view('/', 'home');
 

tests/Feature/API/WpOrg/ApiAuthenticationTest.php

@@ -1,11 +1,14 @@
 <?php
 
-it('should return 401 if the API authentication is enable', function () {
+it('should return 200 when no auth token is present', function () {
     $response = $this->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');

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);
     }