From c0484085cf94178b7c270d10c3217a1c17309e41 Mon Sep 17 00:00:00 2001 From: "Marco A. Nina Mena" Date: Mon, 15 Jun 2026 11:05:49 -0400 Subject: [PATCH] refactor: streamline client lookup and enhance logging in ClientController - Replaced direct client lookup with a new method `findActiveClient` to improve readability and maintainability. - Added logging for failed client lookups to aid in debugging. - Updated tests to use constants for OAuth client URLs, improving code clarity and reducing duplication. --- .../Controllers/Auth/ClientController.php | 23 +++++++-- tests/Feature/Api/OAuthTest.php | 51 +++++++++++++++---- 2 files changed, 60 insertions(+), 14 deletions(-) diff --git a/ProcessMaker/Http/Controllers/Auth/ClientController.php b/ProcessMaker/Http/Controllers/Auth/ClientController.php index 780fbe81dd..2eadeb9f6d 100644 --- a/ProcessMaker/Http/Controllers/Auth/ClientController.php +++ b/ProcessMaker/Http/Controllers/Auth/ClientController.php @@ -4,6 +4,7 @@ use Illuminate\Http\Request; use Illuminate\Http\Response; +use Illuminate\Support\Facades\Log; use Laravel\Passport\ClientRepository; use ProcessMaker\Events\AuthClientCreated; use ProcessMaker\Events\AuthClientDeleted; @@ -27,7 +28,7 @@ public function index(Request $request) public function show(Request $request, $clientId) { - $client = $this->clients->findForUser($clientId, $request->user()); + $client = $this->findActiveClient($request, $clientId); if (!$client) { return new Response('', 404); @@ -72,7 +73,7 @@ public function store(Request $request) public function update(Request $request, $clientId) { - $client = $this->clients->findForUser($clientId, $request->user()); + $client = $this->findActiveClient($request, $clientId); if (!$client) { return new Response('', 404); @@ -101,7 +102,7 @@ public function update(Request $request, $clientId) public function destroy(Request $request, $clientId) { - $client = $this->clients->findForUser($clientId, $request->user()); + $client = $this->findActiveClient($request, $clientId); if (!$client) { return new Response('', 404); @@ -114,6 +115,22 @@ public function destroy(Request $request, $clientId) return new Response('', 204); } + private function findActiveClient(Request $request, $clientId) + { + $client = $this->clients->findActive($clientId); + + if (!$client) { + Log::warning('OAuth client lookup failed.', [ + 'client_id' => $clientId, + 'user_id' => $request->user()?->getAuthIdentifier(), + 'route' => $request->route()?->getName(), + 'method' => $request->method(), + ]); + } + + return $client; + } + private function validate($request) { $rules = [ diff --git a/tests/Feature/Api/OAuthTest.php b/tests/Feature/Api/OAuthTest.php index 52b1331310..491609ee30 100644 --- a/tests/Feature/Api/OAuthTest.php +++ b/tests/Feature/Api/OAuthTest.php @@ -10,28 +10,31 @@ class OAuthTest extends TestCase { use RequestHelper; + private const OAUTH_CLIENTS_URL = '/oauth/clients'; + private const OAUTH_CLIENT_URL = '/oauth/clients/'; + public $json = null; public function withUserSetup() { $response = $this->actingAs($this->user) - ->json('POST', '/oauth/clients', []); + ->json('POST', self::OAUTH_CLIENTS_URL, []); $this->assertEquals('The Name field is required.', $response->json()['errors']['name'][0]); $this->assertEquals('The Types field is required.', $response->json()['errors']['types'][0]); $response = $this->actingAs($this->user) - ->json('POST', '/oauth/clients', ['name' => 'foo', 'types' => []]); + ->json('POST', self::OAUTH_CLIENTS_URL, ['name' => 'foo', 'types' => []]); $this->assertEquals('The Auth-Client must have at least 1 item chosen.', $response->json()['errors']['types'][0]); $response = $this->actingAs($this->user) - ->json('POST', '/oauth/clients', ['name' => 'foo', 'types' => ['authorization_code_grant']]); + ->json('POST', self::OAUTH_CLIENTS_URL, ['name' => 'foo', 'types' => ['authorization_code_grant']]); $this->assertEquals('The Redirect field is required.', $response->json()['errors']['redirect'][0]); $response = $this->actingAs($this->user) - ->json('POST', '/oauth/clients', ['name' => 'test', 'redirect' => 'http://test.com', 'types' => ['authorization_code_grant']]); + ->json('POST', self::OAUTH_CLIENTS_URL, ['name' => 'test', 'redirect' => 'http://test.com', 'types' => ['authorization_code_grant']]); $response->assertStatus(201); $this->json = $response->json(); @@ -45,7 +48,7 @@ public function withUserSetup() public function testCreateAndList() { $response = $this->actingAs($this->user) - ->json('GET', '/oauth/clients'); + ->json('GET', self::OAUTH_CLIENTS_URL); $response->assertStatus(200); $json = $response->json()['data']; @@ -66,7 +69,7 @@ public function testEdit() $response = $this->actingAs($this->user) ->json( 'PUT', - '/oauth/clients/' . $this->json['id'], + self::OAUTH_CLIENT_URL . $this->json['id'], [ 'name' => 'test123', 'redirect' => 'http://test.com/foo', @@ -76,7 +79,7 @@ public function testEdit() $response->assertStatus(200); $response = $this->actingAs($this->user) - ->json('GET', '/oauth/clients'); + ->json('GET', self::OAUTH_CLIENTS_URL); $json = $response->json()['data']; $this->assertEquals($this->json['id'], $json[0]['id']); @@ -95,30 +98,56 @@ public function testEdit() public function testDelete() { $this->actingAs($this->user) - ->json('POST', '/oauth/clients', [ + ->json('POST', self::OAUTH_CLIENTS_URL, [ 'name' => 'other', 'redirect' => 'http://other.net', 'types' => ['authorization_code_grant'], ]); $response = $this->actingAs($this->user) - ->json('GET', '/oauth/clients'); + ->json('GET', self::OAUTH_CLIENTS_URL); $this->assertCount(2, $response->json()['data']); $response = $this->actingAs($this->user) ->json( 'DELETE', - '/oauth/clients/' . $this->json['id'] + self::OAUTH_CLIENT_URL . $this->json['id'] ); $response->assertStatus(Response::HTTP_NO_CONTENT); $response = $this->actingAs($this->user) - ->json('GET', '/oauth/clients'); + ->json('GET', self::OAUTH_CLIENTS_URL); $json = $response->json()['data']; $this->assertCount(1, $json); $this->assertEquals('other', $json[0]['name']); $this->assertEquals('http://other.net', $json[0]['redirect']); } + + public function testEditAndDeleteGlobalClient() + { + $response = $this->actingAs($this->user) + ->json('POST', self::OAUTH_CLIENTS_URL, [ + 'name' => 'global password client', + 'types' => ['password_client'], + ]); + + $response->assertStatus(Response::HTTP_CREATED); + $clientId = $response->json('id'); + + $response = $this->actingAs($this->user) + ->json('PUT', self::OAUTH_CLIENT_URL . $clientId, [ + 'name' => 'updated global password client', + 'types' => ['password_client'], + ]); + + $response->assertStatus(Response::HTTP_OK); + $this->assertEquals('updated global password client', $response->json('name')); + + $response = $this->actingAs($this->user) + ->json('DELETE', self::OAUTH_CLIENT_URL . $clientId); + + $response->assertStatus(Response::HTTP_NO_CONTENT); + } }