diff --git a/lib/Application/User/ManageUsersService.php b/lib/Application/User/ManageUsersService.php index 639eb94dbe..2352385744 100644 --- a/lib/Application/User/ManageUsersService.php +++ b/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(); + } } diff --git a/lib/Common/ServiceLocator.php b/lib/Common/ServiceLocator.php index a7ec6e52bc..acd83647de 100644 --- a/lib/Common/ServiceLocator.php +++ b/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; } } diff --git a/tests/Application/User/ManageUsersServiceTest.php b/tests/Application/User/ManageUsersServiceTest.php index 627f294636..da4b12d757 100644 --- a/tests/Application/User/ManageUsersServiceTest.php +++ b/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()) diff --git a/tests/Infrastructure/Common/ServiceLocatorTest.php b/tests/Infrastructure/Common/ServiceLocatorTest.php new file mode 100644 index 0000000000..4f7a10fd5b --- /dev/null +++ b/tests/Infrastructure/Common/ServiceLocatorTest.php @@ -0,0 +1,41 @@ +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()); + } +} diff --git a/tests/TestBase.php b/tests/TestBase.php index 569720981d..48b2852a5e 100644 --- a/tests/TestBase.php +++ b/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();