fix(webservice): judge admin user operations by the API session

lib/Application/User/ManageUsersService.php

@@ -134,7 +134,7 @@ public function AddUser(
         $extraAttributes,
         $customAttributes
     ) {
-        $currentUser = ServiceLocator::GetServer()->GetUserSession();
+        $currentUser = $this->CurrentUser();
         if (!$currentUser->IsAdmin) {
             // only application administrators may create accounts
             Log::Error('AddUser denied. UserId=%s is not an application administrator', $currentUser->UserId);
@@ -159,7 +159,7 @@ public function AddUser(
 
     public function ChangeAttribute($userId, $attributeValue)
     {
-        $currentUser = ServiceLocator::GetServer()->GetUserSession();
+        $currentUser = $this->CurrentUser();
         if (!$currentUser->IsAdmin) {
             // only application administrators may change user attributes
             Log::Error('ChangeAttribute denied. UserId=%s is not an application administrator. Target UserId=%s', $currentUser->UserId, $userId);
@@ -173,7 +173,7 @@ public function ChangeAttribute($userId, $attributeValue)
 
     public function ChangeAttributes($userId, $attributes)
     {
-        $currentUser = ServiceLocator::GetServer()->GetUserSession();
+        $currentUser = $this->CurrentUser();
         if (!$currentUser->IsAdmin) {
             // only application administrators may change user attributes
             Log::Error('ChangeAttributes denied. UserId=%s is not an application administrator. Target UserId=%s', $currentUser->UserId, $userId);
@@ -189,7 +189,7 @@ public function ChangeAttributes($userId, $attributes)
 
     public function DeleteUser($userId, $notify = true)
     {
-        $currentUser = ServiceLocator::GetServer()->GetUserSession();
+        $currentUser = $this->CurrentUser();
         if (!$currentUser->IsAdmin) {
             // only application administrators may delete accounts
             Log::Error('DeleteUser denied. UserId=%s is not an application administrator. Target UserId=%s', $currentUser->UserId, $userId);
@@ -220,7 +220,7 @@ public function DeleteUser($userId, $notify = true)
 
     public function UpdateUser($userId, $username, $email, $firstName, $lastName, $timezone, $extraAttributes, $customAttributes)
     {
-        $currentUser = ServiceLocator::GetServer()->GetUserSession();
+        $currentUser = $this->CurrentUser();
         if (!$currentUser->IsAdmin) {
             // only application administrators may update accounts
             Log::Error('UpdateUser denied. UserId=%s is not an application administrator. Target UserId=%s', $currentUser->UserId, $userId);
@@ -279,7 +279,7 @@ public function ChangeGroups($user, $groupIds)
 
     public function UpdatePassword($userId, $password)
     {
-        $currentUser = ServiceLocator::GetServer()->GetUserSession();
+        $currentUser = $this->CurrentUser();
         if (!$currentUser->IsAdmin) {
             // only application administrators may change passwords
             Log::Error('UpdatePassword denied. UserId=%s is not an application administrator. Target UserId=%s', $currentUser->UserId, $userId);
@@ -299,4 +299,13 @@ public function LoadUser($email)
     {
         return $this->userRepository->LoadByUsername($email);
     }
+
+    /**
+     * The acting user: the API session in a web service request (ServiceLocator::GetServer() holds only the browser
+     * session, which an API request does not have), else the browser session.
+     */
+    private function CurrentUser(): UserSession
+    {
+        return ServiceLocator::GetUserSession() ?? new NullUserSession();
+    }
 }

lib/Common/ServiceLocator.php

@@ -53,7 +53,7 @@ public static function GetApiServer(): IRestServer|null
         return self::$_apiServer;
     }
 
-    public static function SetApiServer(IRestServer $apiServer)
+    public static function SetApiServer(?IRestServer $apiServer)
     {
         self::$_apiServer = $apiServer;
     }
@@ -120,18 +120,25 @@ public static function SetFileSystem(\Booked\IFileSystem $fileSystem)
         self::$_fileSystem = $fileSystem;
     }
 
+    /**
+     * The session of the current user. In a web service request, the session authenticated by the API token comes
+     * first: a browser session cookie sent along with the request must not decide what the API caller may do.
+     */
     public static function GetUserSession(): UserSession|null
     {
+        if (!is_null(self::$_apiServer)) {
+            $apiSession = self::$_apiServer->GetSession();
+            if (!is_null($apiSession)) {
+                return $apiSession;
+            }
+        }
         if (!is_null(self::$_server)) {
             $userSession = self::$_server->GetUserSession();
             if (!$userSession instanceof NullUserSession) {
                 return $userSession;
             }
         }
-        if (is_null(self::$_apiServer)) {
-            return null;
-        }
-        return self::$_apiServer->GetSession();
+        return null;
     }
 
 }

tests/Application/User/ManageUsersServiceTest.php

@@ -261,6 +261,52 @@ public function testAddUserIsDeniedWhenNotApplicationAdmin()
         $this->assertNull($actualUser);
     }
 
+    public function testAddsUserForApiAdministratorWithoutBrowserSession()
+    {
+        $this->fakeServer->SetUserSession(new NullUserSession());
+        $apiServer = new FakeRestServer();
+        $apiServer->session->IsAdmin = true;
+        ServiceLocator::SetApiServer($apiServer);
+
+        $this->registration->expects($this->once())
+                           ->method('Register')
+                           ->willReturn(new FakeUser(99889));
+
+        $actualUser = $this->service->AddUser('un', 'e@mail.com', 'f', 'l', 'pw', 'America/Chicago', 'en_us', 1, [], []);
+
+        $this->assertEquals(99889, $actualUser->Id());
+    }
+
+    public function testApiRequestIsJudgedByApiSessionNotBrowserSession()
+    {
+        $this->fakeUser->IsAdmin = true;
+        $apiServer = new FakeRestServer();
+        $apiServer->session->IsAdmin = false;
+        ServiceLocator::SetApiServer($apiServer);
+
+        $this->registration->expects($this->never())
+                           ->method('Register');
+
+        $actualUser = $this->service->AddUser('un', 'e@mail.com', 'f', 'l', 'pw', 'America/Chicago', 'en_us', 1, [], []);
+
+        $this->assertNull($actualUser);
+    }
+
+    public function testDeleteUserForApiAdministratorWithoutBrowserSession()
+    {
+        $this->fakeServer->SetUserSession(new NullUserSession());
+        $apiServer = new FakeRestServer();
+        $apiServer->session->IsAdmin = true;
+        ServiceLocator::SetApiServer($apiServer);
+
+        $userId = 809;
+        $this->userRepo->_User = new FakeUser($userId);
+
+        $this->service->DeleteUser($userId, false);
+
+        $this->assertEquals($userId, $this->userRepo->_DeletedUserId);
+    }
+
     public function testChangeGroupsIgnoresNullUser()
     {
         $this->groupRepo->expects($this->never())

tests/Infrastructure/Common/ServiceLocatorTest.php

@@ -0,0 +1,41 @@
+<?php
+
+declare(strict_types=1);
+
+require_once(ROOT_DIR . 'lib/WebService/namespace.php');
+
+class ServiceLocatorTest extends TestBase
+{
+    public function testGetUserSessionReturnsBrowserSessionOutsideApiRequests()
+    {
+        $this->assertSame($this->fakeUser, ServiceLocator::GetUserSession());
+    }
+
+    public function testGetUserSessionPrefersAuthenticatedApiSession()
+    {
+        $apiServer = new FakeRestServer();
+        ServiceLocator::SetApiServer($apiServer);
+
+        $this->assertSame($apiServer->session, ServiceLocator::GetUserSession());
+    }
+
+    public function testGetUserSessionFallsBackToBrowserSessionWhenApiRequestIsNotAuthenticated()
+    {
+        $apiServer = new class () extends FakeRestServer {
+            public function GetSession()
+            {
+                return null;
+            }
+        };
+        ServiceLocator::SetApiServer($apiServer);
+
+        $this->assertSame($this->fakeUser, ServiceLocator::GetUserSession());
+    }
+
+    public function testGetUserSessionIsNullWithoutAnySession()
+    {
+        $this->fakeServer->SetUserSession(new NullUserSession());
+
+        $this->assertNull(ServiceLocator::GetUserSession());
+    }
+}

tests/TestBase.php

@@ -146,6 +146,7 @@ protected function assertLogMessage(array $logs, string $expectedPattern, string
 
     public function teardown(): void
     {
+        ServiceLocator::SetApiServer(null);
         Configuration::SetInstance(null);
         PluginManager::SetInstance(null);
         Date::_ResetNow();