fix(oauth2): scope Native client custom-scheme lock to payloads that touch URI fields

ClientService::create()/update() acquired the TOCTOU lock
(NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK) whenever application_type was
Native, regardless of whether the payload touched redirect_uris,
allowed_origins, or post_logout_redirect_uris. A plain app_name rename
on one Native client therefore contended with an unrelated tenant's
scheme registration, producing spurious "please retry" 412s.

Extract NATIVE_CUSTOM_SCHEME_URI_FIELDS as the single source of truth
for which fields matter, shared by assertNativeCustomSchemesAllowed()
and the new payloadTouchesNativeCustomSchemeFields() gate. The lock is
now taken only when the payload actually registers a custom scheme -
matching what the SHORTCUT comment on the lock constant already
claimed.

Found during /review-pr-deep review of PR #147.

Author: smarcet

app/Services/OAuth2/ClientService.php

@@ -65,6 +65,14 @@ final class ClientService extends AbstractService implements IClientService
      */
     const NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK_LIFETIME = 30;
 
+    /**
+     * Single source of truth for which payload fields carry Native-client custom URI schemes - shared by
+     * assertNativeCustomSchemesAllowed() (what to validate) and payloadTouchesNativeCustomSchemeFields()
+     * (whether the TOCTOU lock is worth taking). Keeping both reads from one list means they can't drift:
+     * a field that stops needing validation automatically stops needing the lock too.
+     */
+    const NATIVE_CUSTOM_SCHEME_URI_FIELDS = ['redirect_uris', 'allowed_origins', 'post_logout_redirect_uris'];
+
     /**
      * @var IAuthService
      */
@@ -154,6 +162,23 @@ private function withNativeCustomSchemeLock(Closure $fn): IEntity
         }
     }
 
+    /**
+     * Gates the TOCTOU lock to payloads that actually register a custom URI scheme. A Native client
+     * write that doesn't touch redirect_uris/allowed_origins/post_logout_redirect_uris (e.g. renaming
+     * app_name) never calls hasCustomSchemeRegisteredOnAnotherClientThan(), so there is no race to close
+     * and no reason to serialize it behind every other tenant's scheme registration.
+     *
+     * @param array $payload
+     * @return bool
+     */
+    private function payloadTouchesNativeCustomSchemeFields(array $payload): bool
+    {
+        foreach (self::NATIVE_CUSTOM_SCHEME_URI_FIELDS as $field) {
+            if (!empty($payload[$field])) return true;
+        }
+        return false;
+    }
+
 
     /**
      * Clients in possession of a client password MAY use the HTTP Basic
@@ -286,7 +311,7 @@ public function getCurrentClientAuthInfo()
      */
     private function assertNativeCustomSchemesAllowed(array $payload, int $exclude_client_id = -1): void
     {
-        foreach (['redirect_uris', 'allowed_origins', 'post_logout_redirect_uris'] as $field) {
+        foreach (self::NATIVE_CUSTOM_SCHEME_URI_FIELDS as $field) {
             if (empty($payload[$field])) continue;
             foreach (explode(',', $payload[$field]) as $uri) {
                 $parts = @parse_url(trim($uri));
@@ -351,7 +376,8 @@ public function create(array $payload):IEntity
             });
         };
 
-        if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) {
+        if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native
+            && $this->payloadTouchesNativeCustomSchemeFields($payload)) {
             return $this->withNativeCustomSchemeLock($do_create);
         }
 
@@ -450,7 +476,8 @@ public function update(int $id, array $payload):IEntity
             });
         };
 
-        if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) {
+        if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native
+            && $this->payloadTouchesNativeCustomSchemeFields($payload)) {
             return $this->withNativeCustomSchemeLock($do_update);
         }
 

tests/ClientApiTest.php

@@ -445,4 +445,33 @@ public function testUpdateNativeClientRejectsCustomSchemeRegistrationWhenAnother
         }
     }
 
+    public function testUpdateNativeClientNotTouchingUriFieldsIgnoresLockContention(){
+
+        // The TOCTOU lock only protects hasCustomSchemeRegisteredOnAnotherClientThan(), which only runs
+        // when the payload touches redirect_uris/allowed_origins/post_logout_redirect_uris. A Native
+        // client update that doesn't touch any of those (e.g. renaming app_name) must not contend with
+        // an unrelated tenant's in-progress scheme registration - unlike the sibling test above, this
+        // must succeed even while the lock is held.
+        $lock_manager = App::make(ILockManagerService::class);
+        $lock_manager->acquireLock(ClientService::NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK, 5);
+
+        try {
+            $client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']);
+
+            $response = $this->action("PUT", "Api\\ClientApiController@update",
+                array(
+                    'id'                => $client->id,
+                    'application_type'  => IClient::ApplicationType_Native,
+                    'app_description'   => 'updated description while another registration is in progress',
+                ),
+                [],
+                [],
+                []);
+
+            $this->assertResponseStatus(201);
+        } finally {
+            $lock_manager->releaseLock(ClientService::NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK);
+        }
+    }
+
 }