FOUR-34196 Review the issue Unauthorized

ProcessMaker/Http/Middleware/EnsureAccountAllowsAccess.php

@@ -3,6 +3,7 @@
 namespace ProcessMaker\Http\Middleware;
 
 use Closure;
+use Illuminate\Contracts\Auth\StatefulGuard;
 use Illuminate\Http\Request;
 use Illuminate\Support\Facades\Auth;
 use ProcessMaker\Models\User;
@@ -16,12 +17,15 @@ class EnsureAccountAllowsAccess
     private const DENIED_STATUSES = ['BLOCKED', 'INACTIVE'];
 
     /**
-     * If any active guard has a blocked/inactive user, log out and return the appropriate response.
-     * Used by the web middleware group and by ProcessMakerAuthenticate (covers all auth:api routes, including packages).
+     * If an authenticated user for one of the supplied guards is blocked/inactive, log out and return the appropriate response.
+     *
+     * @param array<string|null> $guards
      */
-    public static function blockingResponseForRequest(Request $request): ?Response
+    public static function blockingResponseForRequest(Request $request, array $guards = []): ?Response
     {
-        foreach (['web', 'api'] as $guard) {
+        foreach ($guards ?: [config('auth.defaults.guard')] as $guard) {
+            $guard = $guard ?: config('auth.defaults.guard');
+
             if (!Auth::guard($guard)->check()) {
                 continue;
             }
@@ -40,7 +44,7 @@ public static function blockingResponseForRequest(Request $request): ?Response
 
     public function handle(Request $request, Closure $next): Response
     {
-        $blocked = self::blockingResponseForRequest($request);
+        $blocked = self::blockingResponseForRequest($request, ['web']);
         if ($blocked !== null) {
             return $blocked;
         }
@@ -50,11 +54,15 @@ public function handle(Request $request, Closure $next): Response
 
     public static function denyAccess(Request $request, string $guard, User $user): Response
     {
-        Auth::guard($guard)->logout();
+        $authGuard = Auth::guard($guard);
+
+        if ($authGuard instanceof StatefulGuard) {
+            $authGuard->logout();
 
-        if ($request->hasSession()) {
-            $request->session()->invalidate();
-            $request->session()->regenerateToken();
+            if ($request->hasSession()) {
+                $request->session()->invalidate();
+                $request->session()->regenerateToken();
+            }
         }
 
         if ($guard === 'api' || $request->expectsJson()) {

ProcessMaker/Http/Middleware/ProcessMakerAuthenticate.php

@@ -18,7 +18,7 @@ public function handle($request, Closure $next, ...$guards)
     {
         $this->authenticate($request, $guards);
 
-        if ($blocked = EnsureAccountAllowsAccess::blockingResponseForRequest($request)) {
+        if ($blocked = EnsureAccountAllowsAccess::blockingResponseForRequest($request, $guards)) {
             return $blocked;
         }
 

tests/Feature/Http/Middleware/EnsureAccountAllowsAccessTest.php

@@ -0,0 +1,136 @@
+<?php
+
+namespace Tests\Feature\Http\Middleware;
+
+use Illuminate\Contracts\Auth\Guard;
+use Illuminate\Contracts\Auth\StatefulGuard;
+use Illuminate\Http\Request;
+use Illuminate\Support\Facades\Auth;
+use Mockery;
+use PHPUnit\Framework\Attributes\DataProvider;
+use ProcessMaker\Http\Middleware\EnsureAccountAllowsAccess;
+use ProcessMaker\Http\Middleware\ProcessMakerAuthenticate;
+use ProcessMaker\Models\User;
+use Tests\TestCase;
+
+class EnsureAccountAllowsAccessTest extends TestCase
+{
+    public function test_web_middleware_does_not_resolve_the_api_guard_for_a_malformed_bearer_token(): void
+    {
+        $user = new User(['status' => 'ACTIVE']);
+        $webGuard = Mockery::mock(StatefulGuard::class);
+        $webGuard->shouldReceive('check')->once()->andReturn(true);
+        $webGuard->shouldReceive('user')->once()->andReturn($user);
+
+        Auth::shouldReceive('guard')->with('web')->andReturn($webGuard);
+        Auth::shouldReceive('guard')->never()->with('api');
+
+        $request = Request::create('/', 'GET', [], [], [], [
+            'HTTP_AUTHORIZATION' => 'Bearer undefined',
+        ]);
+
+        $response = (new EnsureAccountAllowsAccess())->handle($request, fn () => response('allowed'));
+
+        $this->assertSame(200, $response->getStatusCode());
+        $this->assertSame('allowed', $response->getContent());
+    }
+
+    #[DataProvider('deniedWebStatuses')]
+    public function test_web_middleware_denies_users_with_a_disallowed_status(string $status): void
+    {
+        $user = new User(['status' => $status]);
+        $webGuard = Mockery::mock(StatefulGuard::class);
+        $webGuard->shouldReceive('check')->once()->andReturn(true);
+        $webGuard->shouldReceive('user')->once()->andReturn($user);
+        $webGuard->shouldReceive('logout')->once();
+
+        Auth::shouldReceive('guard')->with('web')->andReturn($webGuard);
+        Auth::shouldReceive('guard')->never()->with('api');
+
+        $response = (new EnsureAccountAllowsAccess())->handle(Request::create('/', 'GET'), fn () => response('allowed'));
+
+        $this->assertSame(302, $response->getStatusCode());
+        $this->assertStringEndsWith('/login', $response->headers->get('Location'));
+    }
+
+    public static function deniedWebStatuses(): array
+    {
+        return [
+            'blocked' => ['BLOCKED'],
+            'inactive' => ['INACTIVE'],
+        ];
+    }
+
+    public function test_api_account_check_does_not_resolve_the_web_guard_for_an_active_user(): void
+    {
+        $user = new User(['status' => 'ACTIVE']);
+        $apiGuard = Mockery::mock(Guard::class);
+        $apiGuard->shouldReceive('check')->once()->andReturn(true);
+        $apiGuard->shouldReceive('user')->once()->andReturn($user);
+
+        Auth::shouldReceive('guard')->never()->with('web');
+        Auth::shouldReceive('guard')->with('api')->andReturn($apiGuard);
+
+        $response = EnsureAccountAllowsAccess::blockingResponseForRequest(
+            Request::create('/api/test', 'GET'),
+            ['api']
+        );
+
+        $this->assertNull($response);
+    }
+
+    #[DataProvider('deniedApiStatuses')]
+    public function test_api_account_check_denies_users_with_a_disallowed_status(string $status, string $message): void
+    {
+        $user = new User(['status' => $status]);
+        $apiGuard = Mockery::mock(Guard::class);
+        $apiGuard->shouldReceive('check')->once()->andReturn(true);
+        $apiGuard->shouldReceive('user')->once()->andReturn($user);
+
+        Auth::shouldReceive('guard')->never()->with('web');
+        Auth::shouldReceive('guard')->with('api')->andReturn($apiGuard);
+
+        $response = EnsureAccountAllowsAccess::blockingResponseForRequest(
+            Request::create('/api/test', 'GET', [], [], [], ['HTTP_ACCEPT' => 'application/json']),
+            ['api']
+        );
+
+        $this->assertSame(401, $response->getStatusCode());
+        $this->assertSame(['message' => $message], json_decode($response->getContent(), true));
+    }
+
+    public static function deniedApiStatuses(): array
+    {
+        return [
+            'blocked' => ['BLOCKED', 'Account locked after too many failed attempts. Contact administrator.'],
+            'inactive' => ['INACTIVE', 'Unauthorized'],
+        ];
+    }
+
+    public function test_processmaker_authenticate_checks_only_the_authenticated_api_guard(): void
+    {
+        $user = new User(['status' => 'INACTIVE']);
+        $apiGuard = Mockery::mock(Guard::class);
+        $apiGuard->shouldReceive('check')->once()->andReturn(true);
+        $apiGuard->shouldReceive('user')->once()->andReturn($user);
+
+        Auth::shouldReceive('guard')->never()->with('web');
+        Auth::shouldReceive('guard')->with('api')->andReturn($apiGuard);
+
+        $middleware = new class(app('auth')) extends ProcessMakerAuthenticate {
+            protected function authenticate($request, array $guards)
+            {
+                // Authentication is outside the scope of this guard-selection test.
+            }
+        };
+
+        $response = $middleware->handle(
+            Request::create('/api/test', 'GET', [], [], [], ['HTTP_ACCEPT' => 'application/json']),
+            fn () => response('allowed'),
+            'api'
+        );
+
+        $this->assertSame(401, $response->getStatusCode());
+        $this->assertSame(['message' => 'Unauthorized'], json_decode($response->getContent(), true));
+    }
+}

tests/k6/etag/README.md

@@ -0,0 +1,20 @@
+# ETag k6 tests
+
+The ETag tests require a valid ProcessMaker OAuth bearer token. They do not run with a missing, `null`, `undefined`, or `FAKE_JWT` token.
+
+Set the target URL and token through environment variables:
+
+```bash
+PM_K6_BASE_URL=https://performance2-qa.processmaker.net \
+PM_K6_TOKEN='<access-token>' \
+k6 run tests/k6/etag/performance-test.js
+```
+
+Use the same variables for the other tests:
+
+```bash
+k6 run tests/k6/etag/long-duration-test.js
+k6 run tests/k6/etag/detect-dynamic-endpoints.js
+```
+
+Do not set an `Authorization` header outside these scripts when the token is unavailable. Test setup must fail instead of sending `Bearer null` or `Bearer undefined`.

tests/k6/etag/config.js

@@ -1,11 +1,28 @@
+const baseUrl = (__ENV.PM_K6_BASE_URL || '').replace(/\/$/, '');
+
 export const config = {
-  token: 'FAKE_JWT', // Replace with a valid token.
+  token: __ENV.PM_K6_TOKEN || '',
   endpoints: {
-    startProcesses: 'https://processmaker.test/api/1.0/start_processes?page=1&per_page=15&filter=&order_by=category.name%2Cname&order_direction=asc%2Casc&include=events%2Ccategories&without_event_definitions=true',
+    startProcesses: `${baseUrl}/api/1.0/start_processes?page=1&per_page=15&filter=&order_by=category.name%2Cname&order_direction=asc%2Casc&include=events%2Ccategories&without_event_definitions=true`,
   },
 };
 
-export const headers = {
-  'Content-Type': 'application/json',
-  Authorization: `Bearer ${config.token}`,
-};
+export function validateConfiguration() {
+  if (!baseUrl) {
+    throw new Error('PM_K6_BASE_URL is required.');
+  }
+
+  const token = config.token.trim();
+  if (!token || ['null', 'undefined', 'fake_jwt'].includes(token.toLowerCase())) {
+    throw new Error('PM_K6_TOKEN must contain a valid bearer token.');
+  }
+}
+
+export function getHeaders() {
+  validateConfiguration();
+
+  return {
+    'Content-Type': 'application/json',
+    Authorization: `Bearer ${config.token}`,
+  };
+}

tests/k6/etag/detect-dynamic-endpoints.js

@@ -1,5 +1,5 @@
 import http from 'k6/http';
-import { config, headers } from './config.js';
+import { config, getHeaders, validateConfiguration } from './config.js';
 
 export let options = {
   vus: 1,
@@ -16,7 +16,13 @@ const etagHistory = {};
 // Limit to determine when an endpoint is considered dynamic.
 const ETAG_HISTORY_LIMIT = 5;
 
+export function setup() {
+  validateConfiguration();
+}
+
 export default function () {
+  const headers = getHeaders();
+
   endpoints.forEach((url) => {
     const res1 = http.get(url, { headers });
     const etag = res1.headers['Etag'];
@@ -54,4 +60,4 @@ export default function () {
       console.log(`Dynamic endpoint detected: ${url}`);
     }
   });
-}
+}

tests/k6/etag/long-duration-test.js

@@ -1,7 +1,7 @@
 import http from 'k6/http';
 import { check, sleep } from 'k6';
 import { Trend } from 'k6/metrics';
-import { config, headers } from './config.js';
+import { config, getHeaders, validateConfiguration } from './config.js';
 
 // Metrics to track response durations.
 let duration200 = new Trend('duration_200');
@@ -18,8 +18,13 @@ export let options = {
   },
 };
 
+export function setup() {
+  validateConfiguration();
+}
+
 export default function () {
   const url = config.endpoints.startProcesses;
+  const headers = getHeaders();
   let res = http.get(url, { headers });
   duration200.add(res.timings.duration);
   check(res, {
@@ -40,4 +45,4 @@ export default function () {
   }
 
   sleep(1);
-}
+}

tests/k6/etag/performance-test.js

@@ -1,7 +1,7 @@
 import http from 'k6/http';
 import { check, sleep, group } from 'k6';
 import { Trend } from 'k6/metrics';
-import { config, headers } from './config.js';
+import { config, getHeaders, validateConfiguration } from './config.js';
 
 // Custom metrics to track request durations.
 let duration200 = new Trend('duration_200');
@@ -15,8 +15,13 @@ export let options = {
   ],
 };
 
+export function setup() {
+  validateConfiguration();
+}
+
 export default function () {
   const baseUrl = config.endpoints.startProcesses;
+  const headers = getHeaders();
   group('ETag Performance', () => {
     // Add the duration of the 200 response to the custom metric.
     let res = http.get(baseUrl, { headers });
@@ -43,4 +48,4 @@ export default function () {
 
     sleep(1);
   });
-}
+}