From 47ca995b73bff9ef5a15b2b38de38b9da9a97627 Mon Sep 17 00:00:00 2001 From: "John L. Villalovos" Date: Fri, 9 Oct 2026 23:58:00 -0700 Subject: [PATCH 1/6] fix(auth): persist only login data when a user logs in Authentication::Login() saved the user with UserRepository::Update(), which rewrites most users columns and all preferences from the copy loaded at the start of the login. A password change, deactivation, credit adjustment or preference change made between that load and the save was silently reverted. Add IUserRepository::UpdateLoginData(), which writes only lastlogin, last_modified, the language chosen at login, and the generated public_id when the user had none. Authentication::Login() now uses it. A login that chooses no language leaves the stored language alone instead of writing back the loaded one. Add database integration tests that change the user's row between the login's load and its save, and check that the change survives. Assisted-by: Claude:claude-opus-5-5 --- Domain/Access/UserRepository.php | 25 ++ .../Authentication/Authentication.php | 12 +- lib/Database/Commands/Commands.php | 4 +- lib/Database/Commands/Queries.php | 10 +- .../Authentication/AuthenticationTest.php | 23 +- tests/Domain/User/UserRepositoryTest.php | 51 ++++ .../Database/DatabaseCommandTest.php | 21 ++ .../Database/AuthenticationDatabaseTest.php | 260 ++++++++++++++++++ tests/fakes/FakeUserRepository.php | 9 + 9 files changed, 404 insertions(+), 11 deletions(-) create mode 100644 tests/Integration/Database/AuthenticationDatabaseTest.php diff --git a/Domain/Access/UserRepository.php b/Domain/Access/UserRepository.php index c6158fc2fe..022cae6796 100644 --- a/Domain/Access/UserRepository.php +++ b/Domain/Access/UserRepository.php @@ -32,6 +32,15 @@ public function LoadByUsername($userName); */ public function Update(User $user); + /** + * Persists only the data changed by a login: last login time, the language chosen at + * login and, if the user had none, the generated public id. Leaves all other columns + * untouched. + * + * @param string|null $language language chosen at login, or null to keep the stored language + */ + public function UpdateLoginData(User $user, ?string $language): void; + /** * @param User $user * @return int @@ -551,6 +560,22 @@ public function Update(User $user) $this->_cache->Remove($userId); } + public function UpdateLoginData(User $user, ?string $language): void + { + // An existing public_id is never overwritten. If two first logins race, each may + // generate an id and only the first is stored; the other session then holds an + // id that was never persisted. The session's PublicId has no consumer that relies + // on it, so this is accepted rather than adding a read-back query to every login. + ServiceLocator::GetDatabase()->Execute(new UpdateLoginDataCommand( + userId: $user->Id(), + lastLoginTime: $user->LastLogin(), + language: $language, + publicId: $user->GetPublicId() + )); + + $this->_cache->Remove($user->Id()); + } + public function DeleteById($userId) { $deleteUserCommand = new DeleteUserCommand($userId); diff --git a/lib/Application/Authentication/Authentication.php b/lib/Application/Authentication/Authentication.php index d787f7aa3b..94b30217e6 100644 --- a/lib/Application/Authentication/Authentication.php +++ b/lib/Application/Authentication/Authentication.php @@ -117,14 +117,12 @@ public function Login($username, $loginContext) if ($user->StatusId() == AccountStatus::ACTIVE) { $loginData = $loginContext->GetData(); $loginTime = LoginTime::Now(); - $language = $user->Language(); + // Only a language chosen at login is saved. Writing back the loaded one could + // revert a language change made since the user was loaded. + $loginLanguage = empty($loginData->Language) ? null : $loginData->Language; - if (!empty($loginData->Language)) { - $language = $loginData->Language; - } - - $user->Login($loginTime, $language); - $this->userRepository->Update($user); + $user->Login($loginTime, $loginLanguage ?? $user->Language()); + $this->userRepository->UpdateLoginData(user: $user, language: $loginLanguage); $user = $this->GetFirstRegistrationStrategy()->HandleLogin($user, $this->userRepository, $this->groupRepository); diff --git a/lib/Database/Commands/Commands.php b/lib/Database/Commands/Commands.php index 75b1213272..7b1defd495 100644 --- a/lib/Database/Commands/Commands.php +++ b/lib/Database/Commands/Commands.php @@ -2664,11 +2664,13 @@ public function __construct($groupId, $groupName, $adminGroupId, $isDefault) class UpdateLoginDataCommand extends SqlCommand { - public function __construct($userId, $lastLoginTime, $language) + public function __construct($userId, $lastLoginTime, $language, $publicId) { parent::__construct(Queries::UPDATE_LOGINDATA); $this->AddParameter(new Parameter(ParameterNames::LAST_LOGIN, $lastLoginTime)); $this->AddParameter(new Parameter(ParameterNames::LANGUAGE, $language)); + $this->AddParameter(new Parameter(ParameterNames::PUBLIC_ID, $publicId)); + $this->AddParameter(new Parameter(ParameterNames::DATE_MODIFIED, Date::Now()->ToDatabase())); $this->AddParameter(new Parameter(ParameterNames::USER_ID, $userId)); } } diff --git a/lib/Database/Commands/Queries.php b/lib/Database/Commands/Queries.php index c703200520..314ba36e7e 100644 --- a/lib/Database/Commands/Queries.php +++ b/lib/Database/Commands/Queries.php @@ -1089,7 +1089,15 @@ private function __construct() SET `name` = @groupname, `admin_group_id` = @admin_group_id, `isdefault` = @isdefault WHERE `group_id` = @groupid'; - public const UPDATE_LOGINDATA = 'UPDATE `users` SET `lastlogin` = @lastlogin, `language` = @language WHERE `user_id` = @userid'; + public const UPDATE_LOGINDATA = + 'UPDATE `users` + SET + `lastlogin` = @lastlogin, + `language` = COALESCE(@language, `language`), + `public_id` = COALESCE(NULLIF(`public_id`, \'\'), @publicid), + `last_modified` = @dateModified + WHERE + `user_id` = @userid'; public const UPDATE_FUTURE_RESERVATION_INSTANCES = 'UPDATE `reservation_instances` diff --git a/tests/Application/Authentication/AuthenticationTest.php b/tests/Application/Authentication/AuthenticationTest.php index f516c154ea..5ab25b918a 100644 --- a/tests/Application/Authentication/AuthenticationTest.php +++ b/tests/Application/Authentication/AuthenticationTest.php @@ -131,6 +131,22 @@ public function testValidateChecksAgainstDB() $this->assertEquals($command, $this->db->_LastCommand); } + public function testLoginWithoutALanguageDoesNotSaveTheLoadedLanguage() + { + $this->userRepository->expects($this->once()) + ->method('LoadByUsername') + ->with($this->equalTo($this->username)) + ->willReturn($this->user); + + $this->userRepository->expects($this->once()) + ->method('UpdateLoginData') + ->with($this->equalTo($this->user), $this->isNull()); + + $actualSession = $this->auth->Login($this->username, new WebLoginContext(new LoginData())); + + $this->assertSame($this->user->Language(), $actualSession->LanguageCode); + } + public function testLoginGetsUserDataFromDatabase() { CSRFToken::$_Token = 'token'; @@ -146,8 +162,11 @@ public function testLoginGetsUserDataFromDatabase() $this->user->Login(LoginTime::Now(), $language); $this->userRepository->expects($this->once()) - ->method('Update') - ->with($this->equalTo($this->user)); + ->method('UpdateLoginData') + ->with($this->equalTo($this->user), $this->equalTo($language)); + + $this->userRepository->expects($this->never()) + ->method('Update'); $this->authorization->expects($this->once()) ->method('IsApplicationAdministrator') diff --git a/tests/Domain/User/UserRepositoryTest.php b/tests/Domain/User/UserRepositoryTest.php index 9d581b3d03..43f029d1d5 100644 --- a/tests/Domain/User/UserRepositoryTest.php +++ b/tests/Domain/User/UserRepositoryTest.php @@ -298,6 +298,57 @@ public function testUpdateSetsUserProperties() $this->assertTrue($this->db->ContainsCommand($command)); } + public function testUpdateLoginDataWritesOnlyLoginData() + { + $userId = 987; + $loginTime = '2026-10-01 12:00:00'; + $language = 'en_gb'; + + $user = new User(); + $user->WithId($userId); + $user->ChangePassword('password', 'salt'); + $user->ChangeName('f', 'l'); + $user->ChangeCurrentCredits(100); + $user->ChangePreference('pref', 'value'); + $user->Login($loginTime, $language); + + $repo = new UserRepository(); + $repo->UpdateLoginData(user: $user, language: $language); + + $expected = new UpdateLoginDataCommand( + userId: $userId, + lastLoginTime: $loginTime, + language: $language, + publicId: $user->GetPublicId() + ); + + $this->assertCount(1, $this->db->_Commands); + $this->assertEquals($expected, $this->db->_LastCommand); + $this->assertNotEmpty($user->GetPublicId(), 'Login() generates a public id when the user has none'); + } + + public function testUpdateLoginDataWithoutALanguageDoesNotWriteTheUsersLanguage() + { + $userId = 987; + $loginTime = '2026-10-01 12:00:00'; + + $user = new User(); + $user->WithId($userId); + $user->Login($loginTime, 'en_gb'); + + $repo = new UserRepository(); + $repo->UpdateLoginData(user: $user, language: null); + + $expected = new UpdateLoginDataCommand( + userId: $userId, + lastLoginTime: $loginTime, + language: null, + publicId: $user->GetPublicId() + ); + + $this->assertEquals($expected, $this->db->_LastCommand); + } + public function testAddsAndUpdatesUserPreferences() { $userId = 987; diff --git a/tests/Infrastructure/Database/DatabaseCommandTest.php b/tests/Infrastructure/Database/DatabaseCommandTest.php index 26da4e3f8f..d617321c45 100644 --- a/tests/Infrastructure/Database/DatabaseCommandTest.php +++ b/tests/Infrastructure/Database/DatabaseCommandTest.php @@ -318,4 +318,25 @@ public function testSorts() $this->assertEquals($expected, $command->GetQuery()); $this->assertEquals(new ParameterRaw('@sort_params', 'email'), $command->Parameters->Items(0)); } + + public function testUpdateLoginDataOnlySetsLoginColumns() + { + $command = new UpdateLoginDataCommand( + userId: 1, + lastLoginTime: '2026-10-01 12:00:00', + language: 'en_us', + publicId: 'public_id' + ); + + $query = $command->GetQuery(); + $this->assertSame(1, preg_match('/\bSET\b(.*)\bWHERE\b/is', $query, $setClause), 'Query should have a SET ... WHERE clause'); + preg_match_all('/`(\w+)`\s*=/', $setClause[1], $assignedColumns); + + $assigned = $assignedColumns[1]; + sort($assigned); + + // Login must never rewrite credentials, status, credits or other account data + // from a possibly stale copy of the user. + $this->assertSame(['language', 'last_modified', 'lastlogin', 'public_id'], $assigned); + } } diff --git a/tests/Integration/Database/AuthenticationDatabaseTest.php b/tests/Integration/Database/AuthenticationDatabaseTest.php new file mode 100644 index 0000000000..c4eddb4d38 --- /dev/null +++ b/tests/Integration/Database/AuthenticationDatabaseTest.php @@ -0,0 +1,260 @@ +requireLoadedSchema(); + + $this->database = $this->realDatabase(); + ServiceLocator::SetDatabase($this->database); + + $this->username = 'login-test-' . bin2hex(random_bytes(8)); + $this->userId = (int)$this->database->ExecuteInsert($this->command( + 'INSERT INTO `users` + (`fname`, `lname`, `username`, `email`, `password`, `salt`, `timezone`, `language`, `homepageid`, `date_created`, `last_modified`, `status_id`) + VALUES + (@fname, @lname, @username, @email, @password, @salt, @timezone, @language, @homepageid, @dateCreated, @lastModified, @statusid)', + [ + '@fname' => 'Login', + '@lname' => 'Test', + '@username' => $this->username, + '@email' => $this->username . '@example.com', + '@password' => self::ORIGINAL_PASSWORD, + '@salt' => self::ORIGINAL_SALT, + '@timezone' => 'UTC', + '@language' => self::ORIGINAL_LANGUAGE, + '@homepageid' => Pages::DEFAULT_HOMEPAGE_ID, + '@dateCreated' => Date::Now()->ToDatabase(), + '@lastModified' => self::ORIGINAL_LAST_MODIFIED, + '@statusid' => AccountStatus::ACTIVE, + ] + )); + } + + public function tearDown(): void + { + if ($this->userId !== null) { + $this->database->Execute($this->command('DELETE FROM `users` WHERE `user_id` = @userid', ['@userid' => $this->userId])); + } + ServiceLocator::SetDatabase(new FakeDatabase()); + + parent::tearDown(); + } + + public function testLoginStoresLoginTimeLanguageAndPublicId(): void + { + $session = $this->login(); + + $row = $this->userRow(); + $this->assertTrue($session->IsLoggedIn()); + $this->assertSame($session->LoginTime, $row['lastlogin']); + $this->assertSame(self::LOGIN_LANGUAGE, $row['language']); + $this->assertNotEmpty($row['public_id']); + $this->assertSame($session->PublicId, $row['public_id']); + } + + public function testLoginUpdatesLastModified(): void + { + $this->assertSame(self::ORIGINAL_LAST_MODIFIED, $this->userRow()['last_modified']); + + $this->login(); + + // TestBase freezes Date::Now(), so the stored value is known exactly. + $this->assertSame(Date::Now()->ToDatabase(), $this->userRow()['last_modified']); + } + + public function testLoginReplacesAnEmptyPublicId(): void + { + $this->changeUser('`public_id` = @value', ''); + + $session = $this->login(); + + $row = $this->userRow(); + $this->assertNotEmpty($row['public_id']); + $this->assertSame($session->PublicId, $row['public_id']); + } + + public function testLoginKeepsAnExistingPublicId(): void + { + $this->changeUser('`public_id` = @value', 'existing-public-id'); + + $this->login(); + + $this->assertSame('existing-public-id', $this->userRow()['public_id']); + } + + public function testLoginDoesNotReplaceAPublicIdStoredDuringTheLogin(): void + { + $this->login(function (): void { + $this->changeUser('`public_id` = @value', 'concurrent-public-id'); + }); + + $this->assertSame('concurrent-public-id', $this->userRow()['public_id']); + } + + public function testLoginDoesNotRevertAPasswordChangedDuringTheLogin(): void + { + $this->login(function (): void { + $this->changeUser('`password` = @value, `salt` = \'new-salt\'', 'new-password-hash'); + }); + + $row = $this->userRow(); + $this->assertSame('new-password-hash', $row['password']); + $this->assertSame('new-salt', $row['salt']); + } + + public function testLoginDoesNotReactivateAUserDeactivatedDuringTheLogin(): void + { + $this->login(function (): void { + $this->changeUser('`status_id` = @value', AccountStatus::INACTIVE); + }); + + $this->assertSame(AccountStatus::INACTIVE, (int)$this->userRow()['status_id']); + } + + public function testLoginDoesNotRevertCreditsChangedDuringTheLogin(): void + { + $this->login(function (): void { + $this->changeUser('`credit_count` = @value', '12.50'); + }); + + $this->assertSame('12.50', $this->userRow()['credit_count']); + } + + public function testLoginWithoutALanguageKeepsTheStoredLanguage(): void + { + $session = $this->login(language: null); + + $this->assertSame(self::ORIGINAL_LANGUAGE, $session->LanguageCode); + $this->assertSame(self::ORIGINAL_LANGUAGE, $this->userRow()['language']); + } + + public function testLoginWithoutALanguageDoesNotRevertALanguageChangedDuringTheLogin(): void + { + $this->login( + duringLogin: function (): void { + $this->changeUser('`language` = @value', self::CHANGED_LANGUAGE); + }, + language: null + ); + + $this->assertSame(self::CHANGED_LANGUAGE, $this->userRow()['language']); + } + + public function testLoginDoesNotRemoveAPreferenceAddedDuringTheLogin(): void + { + $this->login(function (): void { + $this->database->Execute($this->command( + 'INSERT INTO `user_preferences` (`user_id`, `name`, `value`) VALUES (@userid, @name, @value)', + ['@userid' => $this->userId, '@name' => 'added-during-login', '@value' => 'kept'] + )); + }); + + $reader = $this->database->Query($this->command( + 'SELECT `value` FROM `user_preferences` WHERE `user_id` = @userid AND `name` = @name', + ['@userid' => $this->userId, '@name' => 'added-during-login'] + )); + $row = $reader->GetRow(); + $this->assertIsArray($row, 'The preference added during the login should still exist'); + $this->assertSame('kept', $row['value']); + } + + /** + * Logs the test user in. $duringLogin runs after the login has loaded the + * user and before it saves, simulating a change made by another request. + * A null $language is a login that does not choose a language. + */ + private function login(?Closure $duringLogin = null, ?string $language = self::LOGIN_LANGUAGE): UserSession + { + $userRepository = new class ($duringLogin) extends UserRepository { + public function __construct(private readonly ?Closure $afterLoad) + { + parent::__construct(); + } + + public function LoadByUsername($userName) + { + $user = parent::LoadByUsername($userName); + if ($this->afterLoad !== null) { + ($this->afterLoad)(); + } + + return $user; + } + }; + + $authentication = new Authentication( + roleService: new FakeAuthorizationService(), + userRepository: $userRepository, + groupRepository: new GroupRepository() + ); + // The default strategy rewrites config/config.php when there is only one user. + $authentication->SetFirstRegistrationStrategy(new class () implements IFirstRegistrationStrategy { + public function HandleLogin(User $user, IUserRepository $userRepository, IGroupRepository $groupRepository) + { + return $user; + } + }); + + return $authentication->Login( + $this->username, + new WebLoginContext(new LoginData(false, $language)) + ); + } + + private function changeUser(string $assignment, int|string $value): void + { + $this->database->Execute($this->command( + 'UPDATE `users` SET ' . $assignment . ' WHERE `user_id` = @userid', + ['@value' => $value, '@userid' => $this->userId] + )); + } + + /** + * @return array + */ + private function userRow(): array + { + $row = $this->database->Query($this->command('SELECT * FROM `users` WHERE `user_id` = @userid', ['@userid' => $this->userId]))->GetRow(); + $this->assertIsArray($row, 'The test user should exist'); + + return $row; + } + + /** + * @param array $parameters + */ + private function command(string $sql, array $parameters): SqlCommand + { + $command = new SqlCommand($sql); + foreach ($parameters as $name => $value) { + $command->AddParameter(new Parameter($name, $value)); + } + + return $command; + } +} diff --git a/tests/fakes/FakeUserRepository.php b/tests/fakes/FakeUserRepository.php index f2107b7f2b..4ae8e885cb 100644 --- a/tests/fakes/FakeUserRepository.php +++ b/tests/fakes/FakeUserRepository.php @@ -43,6 +43,10 @@ class FakeUserRepository implements IUserRepository */ public $_AllUsers = []; public $_DeletedUserId; + /** + * @var User|null + */ + public $_LoginDataUpdatedUser = null; public function __construct() { @@ -87,6 +91,11 @@ public function Update(User $user) $this->_UpdatedUser = $user; } + public function UpdateLoginData(User $user, ?string $language): void + { + $this->_LoginDataUpdatedUser = $user; + } + /** * @param User $user * @return int From 3a2067b0dface0df822271b2a60212d4754adb42 Mon Sep 17 00:00:00 2001 From: "John L. Villalovos" Date: Sat, 10 Oct 2026 00:29:26 -0700 Subject: [PATCH 2/6] feat(security): remove remember-me login Remove the "Remember Me" option from the login page, together with the persistent login cookie behind it. The cookie could not be made safe without a redesign, so this release removes the feature rather than patching it. Users must authenticate again once their normal application session is unavailable. A persist_login cookie left over from an earlier release is never read. Logout still expires it. For authentication plugin authors: - IWebAuthentication::CookieLogin() is removed. Plugins implement IAuthentication and were never required to provide it. - IAuthentication::ShowPersistLoginPrompt() is kept so that existing plugins continue to load, and is marked deprecated. Its result is no longer used. - LoginData::$Persist and the matching constructor argument are kept and marked deprecated. The value is ignored. - FormKeys::PERSIST_LOGIN is kept and marked deprecated. A login template compiled by an earlier release still resolves it, so the login page keeps working until tpl_c is cleared. Update the plugin guide and the authentication plugin readme to match. Assisted-by: Claude:claude-opus-5-5 --- Pages/LoginPage.php | 20 --- Presenters/LoginPresenter.php | 19 +-- docs/source/CUSTOM-PLUGINS.rst | 15 ++- .../Authentication/Authentication.php | 3 + .../Authentication/IAuthentication.php | 3 +- .../Authentication/ILoginContext.php | 1 + .../Authentication/LoginCookie.php | 28 ---- .../Authentication/WebAuthentication.php | 76 ++--------- lib/Application/Authentication/namespace.php | 1 - lib/Database/Commands/Commands.php | 9 -- lib/Database/Commands/Queries.php | 4 - lib/Server/CookieKeys.php | 3 + lib/Server/FormKeys.php | 6 + plugins/Authentication/readme.txt | 9 +- .../Authentication/WebAuthenticationTest.php | 121 ++++++++---------- .../Database/DatabaseCommandTest.php | 12 -- tests/Presenters/LoginPresenterTest.php | 42 +----- tests/fakes/FakeWebAuthentication.php | 12 -- tests/lib/Server/FormKeysTest.php | 19 +++ tpl/login.tpl | 10 -- 20 files changed, 119 insertions(+), 294 deletions(-) delete mode 100644 lib/Application/Authentication/LoginCookie.php create mode 100644 tests/lib/Server/FormKeysTest.php diff --git a/Pages/LoginPage.php b/Pages/LoginPage.php index 36abfa71c6..ecc3958953 100644 --- a/Pages/LoginPage.php +++ b/Pages/LoginPage.php @@ -21,11 +21,6 @@ public function GetEmailAddress(); */ public function GetPassword(); - /** - * @return bool - */ - public function GetPersistLogin(); - public function GetShowRegisterLink(); public function SetShowRegisterLink($value); @@ -64,11 +59,6 @@ public function ShowUsernamePrompt($shouldShow); */ public function ShowPasswordPrompt($shouldShow); - /** - * @param $shouldShow bool - */ - public function ShowPersistLoginPrompt($shouldShow); - /** * @param $shouldShow bool */ @@ -171,11 +161,6 @@ public function GetPassword() return strval($this->GetRawForm(FormKeys::PASSWORD)); } - public function GetPersistLogin() - { - return $this->GetCheckbox(FormKeys::PERSIST_LOGIN); - } - public function GetShowRegisterLink() { return $this->GetVar('ShowRegisterLink'); @@ -284,11 +269,6 @@ public function ShowPasswordPrompt($shouldShow) $this->Set('ShowPasswordPrompt', $shouldShow); } - public function ShowPersistLoginPrompt($shouldShow) - { - $this->Set('ShowPersistLoginPrompt', $shouldShow); - } - public function ShowForgotPasswordPrompt($shouldShow) { $this->Set('ShowForgotPasswordPrompt', $shouldShow); diff --git a/Presenters/LoginPresenter.php b/Presenters/LoginPresenter.php index 6c170f8fb2..decf8672e2 100644 --- a/Presenters/LoginPresenter.php +++ b/Presenters/LoginPresenter.php @@ -92,16 +92,6 @@ public function PageLoad() return; } - $server = ServiceLocator::GetServer(); - $loginCookie = $server->GetCookie(CookieKeys::PERSIST_LOGIN); - - if ($this->IsCookieLogin($loginCookie)) { - if ($this->authentication->CookieLogin($loginCookie, new WebLoginContext(new LoginData(true)))) { - $this->_Redirect(); - return; - } - } - $allowRegistration = Configuration::Instance()->GetKey(ConfigKeys::REGISTRATION_ALLOW_SELF, new BooleanConverter()); $allowAnonymousSchedule = Configuration::Instance()->GetKey(ConfigKeys::PRIVACY_VIEW_SCHEDULES, new BooleanConverter()); $allowGuestBookings = Configuration::Instance()->GetKey(ConfigKeys::PRIVACY_ALLOW_GUEST_RESERVATIONS, new BooleanConverter()); @@ -114,7 +104,6 @@ public function PageLoad() $showForgot = (!$resetDisabled && $this->authentication->ShowForgotPasswordPrompt() && !$hideLogin); $showPasswordPrompt = ($this->authentication->ShowPasswordPrompt() && !$hideLogin); - $showPersistLogin = ($this->authentication->ShowPersistLoginPrompt() && !$hideLogin); $showUsernamePrompt = ($this->authentication->ShowUsernamePrompt() && !$hideLogin); $this->_page->SetShowRegisterLink($showRegisterLink); @@ -122,7 +111,6 @@ public function PageLoad() $this->_page->ShowForgotPasswordPrompt($showForgot); $this->_page->ShowPasswordPrompt($showPasswordPrompt); - $this->_page->ShowPersistLoginPrompt($showPersistLogin); $this->_page->ShowUsernamePrompt($showUsernamePrompt); $registrationUrl = $this->authentication->GetRegistrationUrl(); $this->_page->SetRegistrationUrl($showRegisterLink ? $registrationUrl : null); @@ -163,7 +151,7 @@ public function Login() } if ($isValid) { - $context = new WebLoginContext(new LoginData($this->_page->GetPersistLogin(), $this->_page->GetSelectedLanguage())); + $context = new WebLoginContext(new LoginData(language: $this->_page->GetSelectedLanguage())); $this->authentication->Login($id, $context); $this->_Redirect(); } else { @@ -238,11 +226,6 @@ private function _Redirect() LoginRedirector::Redirect($this->_page); } - private function IsCookieLogin($loginCookie) - { - return !empty($loginCookie); - } - private function SetSelectedLanguage() { $requestedLanguage = $this->_page->GetRequestedLanguage(); diff --git a/docs/source/CUSTOM-PLUGINS.rst b/docs/source/CUSTOM-PLUGINS.rst index 9284e1a041..94db522ce9 100644 --- a/docs/source/CUSTOM-PLUGINS.rst +++ b/docs/source/CUSTOM-PLUGINS.rst @@ -184,7 +184,11 @@ Important methods: ``Login($username, $loginContext)`` Loads or creates the LibreBooking user session after validation succeeds. Most plugins delegate to the base authentication service once they have determined - the LibreBooking username. + the LibreBooking username. ``$loginContext->GetData()`` returns a + ``LoginData`` object. Its ``Persist`` property and the matching first + constructor argument (``new LoginData($persist, $language)``) are deprecated: + the "Remember Me" login option was removed, so the value is ignored. Existing + code that passes it keeps working. ``Logout(UserSession $user)`` Runs when a user logs out. Use this to clear external sessions or delegate to @@ -201,8 +205,13 @@ Important methods: Control whether the login form asks for username and password. Single sign-on plugins often return ``false``. -``ShowPersistLoginPrompt()`` and ``ShowForgotPasswordPrompt()`` - Control whether the remember-me and forgot-password prompts are shown. +``ShowForgotPasswordPrompt()`` + Controls whether the forgot-password prompt is shown. + +``ShowPersistLoginPrompt()`` + Deprecated. The "Remember Me" login option was removed, so the return value + is no longer used. The method is still part of the interface, so plugins + must keep implementing it. Profile field permission methods Control which profile fields users can edit when account data is managed by diff --git a/lib/Application/Authentication/Authentication.php b/lib/Application/Authentication/Authentication.php index 94b30217e6..a50cafb3fb 100644 --- a/lib/Application/Authentication/Authentication.php +++ b/lib/Application/Authentication/Authentication.php @@ -157,6 +157,9 @@ public function ShowPasswordPrompt() return true; } + /** + * @deprecated Persistent login was removed. The result is no longer used. + */ public function ShowPersistLoginPrompt() { return true; diff --git a/lib/Application/Authentication/IAuthentication.php b/lib/Application/Authentication/IAuthentication.php index be7b6d8323..8c13270093 100644 --- a/lib/Application/Authentication/IAuthentication.php +++ b/lib/Application/Authentication/IAuthentication.php @@ -69,7 +69,8 @@ public function ShowUsernamePrompt(); public function ShowPasswordPrompt(); /** - * @abstract + * @deprecated Persistent login was removed. The result is no longer used; the method is + * kept so that existing authentication plugins continue to load. * @return bool */ public function ShowPersistLoginPrompt(); diff --git a/lib/Application/Authentication/ILoginContext.php b/lib/Application/Authentication/ILoginContext.php index be096eb32a..c7a61216d1 100644 --- a/lib/Application/Authentication/ILoginContext.php +++ b/lib/Application/Authentication/ILoginContext.php @@ -12,6 +12,7 @@ public function GetData(); class LoginData { /** + * @deprecated Persistent login was removed. The value is no longer used. * @var bool */ public $Persist; diff --git a/lib/Application/Authentication/LoginCookie.php b/lib/Application/Authentication/LoginCookie.php deleted file mode 100644 index 44f56a939a..0000000000 --- a/lib/Application/Authentication/LoginCookie.php +++ /dev/null @@ -1,28 +0,0 @@ -UserID = $userid; - $this->LastLogin = $lastLoginTime; - - parent::__construct(CookieKeys::PERSIST_LOGIN, sprintf('%s|%s', $userid, $lastLoginTime)); - } - - public static function FromValue($cookieValue) - { - $cookieParts = explode('|', $cookieValue); - - if (count($cookieParts) == 2) { - return new LoginCookie($cookieParts[0], $cookieParts[1]); - } - - return null; - } -} diff --git a/lib/Application/Authentication/WebAuthentication.php b/lib/Application/Authentication/WebAuthentication.php index df89438554..7a590c456f 100644 --- a/lib/Application/Authentication/WebAuthentication.php +++ b/lib/Application/Authentication/WebAuthentication.php @@ -30,13 +30,6 @@ public function Logout(UserSession $user); */ public function postLogout(UserSession $user); - /** - * @param string $cookieValue authentication cookie value - * @param ILoginContext $loginContext - * @return bool If the login was successful - */ - public function CookieLogin($cookieValue, $loginContext); - /** * @param ILoginPage $loginPage * @return void @@ -105,10 +98,6 @@ public function Login($username, $loginContext) { $userSession = $this->authentication->Login($username, $loginContext); $this->server->SetUserSession($userSession); - - if ($loginContext->GetData()->Persist) { - $this->SetLoginCookie($userSession->UserId, $userSession->LoginTime); - } } /** @@ -120,7 +109,7 @@ public function Logout(UserSession $userSession) $this->authentication->Logout($userSession); Log::Debug('Logout userId: %s', $userSession->UserId); - $this->DeleteLoginCookie($userSession->UserId); + $this->DeletePersistLoginCookie(); ServiceLocator::GetServer()->EndSession(SessionKeys::USER_SESSION); } @@ -130,70 +119,24 @@ public function Logout(UserSession $userSession) */ public function postLogout(UserSession $userSession) { - $this->authentication->postLogout($userSession); + // postLogout() is not part of IAuthentication; only some plugins provide it. + if (method_exists($this->authentication, 'postLogout')) { + $this->authentication->postLogout($userSession); + } Log::Debug('Logout userId: %s', $userSession->UserId); - $this->DeleteLoginCookie($userSession->UserId); + $this->DeletePersistLoginCookie(); ServiceLocator::GetServer()->EndSession(SessionKeys::USER_SESSION); } - public function CookieLogin($cookieValue, $loginContext) - { - $loginCookie = LoginCookie::FromValue($cookieValue); - $valid = false; - $this->server->SetUserSession(new NullUserSession()); - - if (!is_null($loginCookie)) { - $validEmail = $this->ValidateCookie($loginCookie); - $valid = !is_null($validEmail); - - if ($valid) { - $loginContext->GetData()->Persist = true; - $this->Login($validEmail, $loginContext); - } else { - $this->DeletePersistLoginCookie(); - } - } else { - $this->DeletePersistLoginCookie(); - } - - Log::Debug('Cookie login. IsValid: %s', $valid); - - return $valid; - } - /** - * @param int $userid - * @param string $lastLogin + * Expires a persistent login cookie left over from an earlier release. Its value is never read. */ - private function SetLoginCookie($userid, $lastLogin) - { - $cookie = new LoginCookie($userid, $lastLogin); - $this->server->SetCookie($cookie); - } - - private function DeleteLoginCookie($userid) - { - ServiceLocator::GetServer()->DeleteCookie(new LoginCookie($userid, null)); - } - - private function DeletePersistLoginCookie() + private function DeletePersistLoginCookie(): void { $this->server->DeleteCookie(new Cookie(CookieKeys::PERSIST_LOGIN, '')); } - private function ValidateCookie($loginCookie) - { - $valid = false; - $reader = ServiceLocator::GetDatabase()->Query(new CookieLoginCommand($loginCookie->UserID)); - - if ($row = $reader->GetRow()) { - $valid = $row[ColumnNames::LAST_LOGIN] == $loginCookie->LastLogin; - } - - return $valid ? $row[ColumnNames::EMAIL] : null; - } - public function HandleLoginFailure(ILoginPage $loginPage) { $this->authentication->HandleLoginFailure(new WebAuthenticationPage($loginPage)); @@ -214,6 +157,9 @@ public function ShowPasswordPrompt() return $this->authentication->ShowPasswordPrompt(); } + /** + * @deprecated Persistent login was removed. The result is no longer used. + */ public function ShowPersistLoginPrompt() { return $this->authentication->ShowPersistLoginPrompt(); diff --git a/lib/Application/Authentication/namespace.php b/lib/Application/Authentication/namespace.php index 47672f30a4..70780e6060 100644 --- a/lib/Application/Authentication/namespace.php +++ b/lib/Application/Authentication/namespace.php @@ -8,7 +8,6 @@ require_once(ROOT_DIR . 'lib/Application/Authentication/Authentication.php'); require_once(ROOT_DIR . 'lib/Application/Authentication/PasswordEncryption.php'); require_once(ROOT_DIR . 'lib/Application/Authentication/Password.php'); -require_once(ROOT_DIR . 'lib/Application/Authentication/LoginCookie.php'); require_once(ROOT_DIR . 'lib/Application/Authentication/IRegistration.php'); require_once(ROOT_DIR . 'lib/Application/Authentication/Registration.php'); require_once(ROOT_DIR . 'lib/Application/Authentication/AuthenticatedUser.php'); diff --git a/lib/Database/Commands/Commands.php b/lib/Database/Commands/Commands.php index 7b1defd495..c49ee2773f 100644 --- a/lib/Database/Commands/Commands.php +++ b/lib/Database/Commands/Commands.php @@ -824,15 +824,6 @@ public function __construct($username) } } -class CookieLoginCommand extends SqlCommand -{ - public function __construct($userId) - { - parent::__construct(Queries::COOKIE_LOGIN); - $this->AddParameter(new Parameter(ParameterNames::USER_ID, $userId)); - } -} - class DeleteAccessoryCommand extends SqlCommand { public function __construct($accessoryId) diff --git a/lib/Database/Commands/Queries.php b/lib/Database/Commands/Queries.php index 314ba36e7e..8b02f3319d 100644 --- a/lib/Database/Commands/Queries.php +++ b/lib/Database/Commands/Queries.php @@ -210,10 +210,6 @@ private function __construct() public const CLEANUP_USER_SESSIONS = 'DELETE FROM `user_session` WHERE utc_timestamp()>date_add(`last_modified`,interval 24 hour)'; - public const COOKIE_LOGIN = - 'SELECT `user_id`, `lastlogin`, `email` - FROM `users` WHERE `user_id` = @userid'; - public const DELETE_ACCESSORY = 'DELETE FROM `accessories` WHERE `accessory_id` = @accessoryid'; public const DELETE_ACCESSORY_RESOURCES = 'DELETE FROM `resource_accessories` WHERE `accessory_id` = @accessoryid'; diff --git a/lib/Server/CookieKeys.php b/lib/Server/CookieKeys.php index 6cdb77a607..a4a3ca1438 100644 --- a/lib/Server/CookieKeys.php +++ b/lib/Server/CookieKeys.php @@ -7,5 +7,8 @@ private function __construct() } public const LANGUAGE = 'language'; + /** + * No longer issued or read. Kept only so that logout can expire cookies set by earlier releases. + */ public const PERSIST_LOGIN = 'persist_login'; } diff --git a/lib/Server/FormKeys.php b/lib/Server/FormKeys.php index 8f7507a44b..536700332b 100644 --- a/lib/Server/FormKeys.php +++ b/lib/Server/FormKeys.php @@ -176,6 +176,12 @@ private function __construct() public const PEAK_BEGIN_TIME = 'PEAK_BEGIN_TIME'; public const PEAK_END_TIME = 'PEAK_END_TIME'; public const PEAK_DELETE = 'PEAK_DELETE'; + /** + * @deprecated Persistent login was removed and no form field uses this key. It is kept + * because a login template compiled by an earlier release still resolves it: with + * template caching on, that compiled copy is used until tpl_c is cleared, and a missing + * key would make the login page fail. + */ public const PERSIST_LOGIN = 'persistLogin'; public const PHONE = 'phone'; public const POSITION = 'position'; diff --git a/plugins/Authentication/readme.txt b/plugins/Authentication/readme.txt index 0f435972cb..74cdb63764 100644 --- a/plugins/Authentication/readme.txt +++ b/plugins/Authentication/readme.txt @@ -20,18 +20,17 @@ To create a new Authentication plugin: This class is intended to decorate an IAuthentication instance, so in most cases you would want to create a new Authentication() class in the constructor if one isn't supplied as a parameter. This class must support parameterless instantiation. -There are three methods which need to be implemented: +There are two methods which need to be implemented: 1) Validate() accepts $username and $password as parameters. This method should go to the credential store and validate that this user has proper access. This method returns true or false depending on if the user is authorized or not. -2) Login() accepts $username and $persist as parameters. +2) Login() accepts $username and $loginContext as parameters. This method is only called if the result of Validate() was true. Typically, this method would synchronize data between the source store and Booked Scheduler, although this is not required if the user account data already exists in LibreBooking. If synchronization is necessary, the account should be registered if it does not exist or updated if it does. The Ldap plugin illustrates how to accomplish this. The Login method should always call to the decorated Authentication classes Login() method. This ensures that any functionality required by LibreBooking is executed. -3) CookieLogin() accepts $cookieValue as a parameter. -This method will typically delegate directly to the decorated Authentication class. The intent of this method is to support persistent sign-on through cookies. -This can be overridden with a no-op to suppress this behavior. \ No newline at end of file +Persistent sign-on through cookies ("Remember Me") has been removed. Plugins no longer implement CookieLogin(), and the +Persist value of the login context is ignored. ShowPersistLoginPrompt() must still be implemented, but its result is not used. \ No newline at end of file diff --git a/tests/Application/Authentication/WebAuthenticationTest.php b/tests/Application/Authentication/WebAuthenticationTest.php index 614709a440..df68558dda 100644 --- a/tests/Application/Authentication/WebAuthenticationTest.php +++ b/tests/Application/Authentication/WebAuthenticationTest.php @@ -55,99 +55,78 @@ public function testSuccessfulLoginSetsSession() $this->assertEquals($user, $this->fakeServer->GetUserSession()); } - public function testCanPersistLoginWhenValidLogin() + public function testLoginDoesNotSetAPersistentLoginCookie() { - $id = 100; - $now = mktime(10, 11, 12, 1, 2, 2000); - LoginTime::$Now = $now; - - $hashedValue = sprintf('%s|%s', $id, $now); - - $session = new UserSession($id); - $session->LoginTime = $now; - $this->fakeAuth->_Session = $session; + $this->fakeAuth->_Session = new FakeUserSession(); + $this->assertTrue($this->loginContext->GetData()->Persist, 'The deprecated flag is set for this test'); $this->webAuth->Login($this->username, $this->loginContext); - $expectedCookie = new Cookie(CookieKeys::PERSIST_LOGIN, $hashedValue); - $this->assertEquals($expectedCookie->Value, $this->fakeServer->GetCookie(CookieKeys::PERSIST_LOGIN)); + $this->assertNull($this->fakeServer->GetCookie(CookieKeys::PERSIST_LOGIN)); + $this->assertSame([], $this->fakeServer->Cookies); } - public function testCanAutoLoginWithCookie() + public function testLogsUserOut() { - $userid = 'userid'; - $lastLogin = LoginTime::Now(); - $email = 'email@address.com'; - $cookie = new LoginCookie($userid, $lastLogin); - - $rows = [ - ColumnNames::USER_ID => $userid, - ColumnNames::LAST_LOGIN => $lastLogin, - ColumnNames::EMAIL => $email - ]; - $this->db->SetRows([$rows]); + $userSession = new FakeUserSession(); + $userSession->UserId = 100; - $user = new FakeUserSession(); - $this->fakeAuth->_Session = $user; + $this->webAuth->Logout($userSession); - $valid = $this->webAuth->CookieLogin($cookie->Value, $this->loginContext); + $this->assertPersistentLoginCookieWasExpired(); + $this->assertEquals(SessionKeys::USER_SESSION, $this->fakeServer->_EndedSession); + $this->assertTrue($this->fakeAuth->_LogoutCalled); + } - $cookieValidateCommand = new CookieLoginCommand($userid); + public function testPostLogoutExpiresThePersistentLoginCookieAndEndsTheSession() + { + $this->assertFalse(method_exists($this->fakeAuth, 'postLogout'), 'The default fake has no postLogout() hook'); - $this->assertTrue($valid, 'should be valid if cookie matches'); - $this->assertEquals($cookieValidateCommand, $this->db->_LastCommand); - $this->assertEquals($email, $this->fakeAuth->_LastLogin); - $this->assertEquals($this->loginContext, $this->fakeAuth->_LastLoginContext); - $this->assertEquals($user, $this->fakeServer->GetUserSession()); + $this->webAuth->postLogout(new FakeUserSession()); + + $this->assertPersistentLoginCookieWasExpired(); + $this->assertEquals(SessionKeys::USER_SESSION, $this->fakeServer->_EndedSession); } - public function testDoesNotAutoLoginIfLastLoginDateOnCookieDoesNotMatch() + public function testPostLogoutCallsThePluginHookWhenItExists() { - $userid = 'userid'; - $lastLogin = LoginTime::Now(); - $email = 'email@address.com'; - $cookie = new LoginCookie($userid, $lastLogin); - - $rows = [[ - ColumnNames::USER_ID => $userid, - ColumnNames::LAST_LOGIN => 'not the same thing', - ColumnNames::EMAIL => $email - ]]; - $this->db->SetRows($rows); - - $valid = $this->webAuth->CookieLogin($cookie->Value, $this->loginContext); - - $this->assertFalse($valid, 'should not be valid if cookie does not match'); - $this->assertEquals(1, count($this->db->_Commands)); - $this->assertFalse($this->fakeAuth->_LoginCalled); - $this->assertEquals(new NullUserSession(), $this->fakeServer->GetUserSession()); - $this->assertEquals(CookieKeys::PERSIST_LOGIN, $this->fakeServer->_DeletedCookie->Name); + // postLogout() is not part of IAuthentication; only some plugins provide it. + $auth = new class () extends FakeAuth { + public ?UserSession $_PostLogoutUser = null; + + public function postLogout(UserSession $user): void + { + $this->_PostLogoutUser = $user; + } + }; + $userSession = new FakeUserSession(); + $webAuth = new WebAuthentication($auth, $this->fakeServer); + + $webAuth->postLogout($userSession); + + $this->assertSame($userSession, $auth->_PostLogoutUser); + $this->assertEquals(SessionKeys::USER_SESSION, $this->fakeServer->_EndedSession); } - public function testDoesNotAutoLoginIfCookieValueIsMalformed() + public function testLogoutExpiresThePersistentLoginCookieWithoutAUserSession() { - $valid = $this->webAuth->CookieLogin('malformed-cookie-value', $this->loginContext); + $this->webAuth->Logout(new NullUserSession()); - $this->assertFalse($valid, 'should not be valid if cookie cannot be parsed'); - $this->assertEquals(0, count($this->db->_Commands)); - $this->assertFalse($this->fakeAuth->_LoginCalled); - $this->assertEquals(new NullUserSession(), $this->fakeServer->GetUserSession()); - $this->assertEquals(CookieKeys::PERSIST_LOGIN, $this->fakeServer->_DeletedCookie->Name); + $this->assertPersistentLoginCookieWasExpired(); + $this->assertEquals(SessionKeys::USER_SESSION, $this->fakeServer->_EndedSession); } - public function testLogsUserOut() + public function testLoggingOutRepeatedlyIsHarmless() { - $userId = 100; $userSession = new FakeUserSession(); - $userSession->UserId = $userId; - - $loginCookie = new LoginCookie($userId, null); $this->webAuth->Logout($userSession); + $this->webAuth->Logout($userSession); + $this->webAuth->postLogout($userSession); + $this->webAuth->postLogout($userSession); - $this->assertEquals($loginCookie, $this->fakeServer->_DeletedCookie); + $this->assertPersistentLoginCookieWasExpired(); $this->assertEquals(SessionKeys::USER_SESSION, $this->fakeServer->_EndedSession); - $this->assertTrue($this->fakeAuth->_LogoutCalled); } public function testHandlesLoginFailure() @@ -157,4 +136,12 @@ public function testHandlesLoginFailure() $this->assertTrue($this->fakeAuth->_HandleLoginFailureCalled); } + + private function assertPersistentLoginCookieWasExpired(): void + { + $deleted = $this->fakeServer->_DeletedCookie; + $this->assertInstanceOf(Cookie::class, $deleted); + $this->assertSame(CookieKeys::PERSIST_LOGIN, $deleted->Name); + $this->assertSame('', $deleted->Value); + } } diff --git a/tests/Infrastructure/Database/DatabaseCommandTest.php b/tests/Infrastructure/Database/DatabaseCommandTest.php index d617321c45..2ff4fe13ee 100644 --- a/tests/Infrastructure/Database/DatabaseCommandTest.php +++ b/tests/Infrastructure/Database/DatabaseCommandTest.php @@ -62,18 +62,6 @@ public function testMigratePasswordCommand() $this->assertEquals($salt, $par3->Value); } - public function testCookieLoginCommand() - { - $userid = 10; - $command = new CookieLoginCommand($userid); - - $this->assertEquals(Queries::COOKIE_LOGIN, $command->GetQuery()); - $this->assertEquals(1, $command->Parameters->Count()); - - $par1 = $command->Parameters->Items(0); - $this->assertEquals(new Parameter(ParameterNames::USER_ID, $userid), $par1); - } - public function testCheckUserExistenceCommand() { $username = 'username'; diff --git a/tests/Presenters/LoginPresenterTest.php b/tests/Presenters/LoginPresenterTest.php index 3f3cb3b39d..aa3b8f58a1 100644 --- a/tests/Presenters/LoginPresenterTest.php +++ b/tests/Presenters/LoginPresenterTest.php @@ -46,7 +46,6 @@ public function setUp(): void $this->page->_EmailAddress = 'nobody@localhost'; $this->page->_Password = 'somepassword'; - $this->page->_PersistLogin = true; $this->fakeServer->SetSession(SessionKeys::USER_SESSION, new UserSession(1)); @@ -70,11 +69,13 @@ public function testLoginCallsAuthValidate() public function testSuccessfulValidateCallsLogin() { $this->auth->_ValidateResult = true; + $this->page->_CurrentCode = 'en_gb'; $this->presenter->Login(); $this->assertEquals($this->page->_EmailAddress, $this->auth->_LastLogin); $data = $this->auth->_LastLoginContext->GetData(); - $this->assertEquals($this->page->_PersistLogin, $data->Persist); + $this->assertFalse($data->Persist, 'persistent login is never requested'); + $this->assertSame('en_gb', $data->Language); } public function testSuccessfulValidateCallsRedirectToNormalPageWhenNoRequestedPage() @@ -188,7 +189,6 @@ public function testPageLoadSetsVariablesCorrectly() $this->auth->_ShowUsernamePrompt = true; $this->auth->_ShowPasswordPrompt = true; - $this->auth->_ShowPersistLoginPrompt = true; $this->auth->_ShowForgotPasswordPrompt = true; $this->presenter->PageLoad(); @@ -197,7 +197,6 @@ public function testPageLoadSetsVariablesCorrectly() $this->assertEquals(true, $this->page->_ShowUsernamePrompt); $this->assertEquals(true, $this->page->_ShowPasswordPrompt); - $this->assertEquals(true, $this->page->_ShowPersistLoginPrompt); $this->assertEquals(true, $this->page->_ShowForgotPasswordPrompt); } @@ -279,29 +278,6 @@ public function testNonLdapExceptionFallsBackToGenericLoginErrorPath() $this->assertNull($this->page->_LoginErrorMessage); } - public function testAutoLoginIfCookieIsSet() - { - $this->page->_ResumeUrl = '/autologin/page/whatever.html'; - $cookie = new Cookie(CookieKeys::PERSIST_LOGIN, 'part1|part2'); - $this->fakeServer->SetCookie($cookie); - - $this->auth->_CookieValidateResult = true; - - $this->presenter->PageLoad(); - - $this->assertTrue($this->auth->_CookieLoginCalled, 'should try to auto login if persist cookie is set'); - $this->assertEquals($cookie->Value, $this->auth->_LastLoginCookie); - $this->assertEquals($this->page->_ResumeUrl, $this->page->_LastRedirect); - } - - public function testDoesNotAutoLoginIfCookieNotSet() - { - $this->page->_ResumeUrl = '/autologin/page/whatever.html'; - $this->presenter->PageLoad(); - - $this->assertFalse($this->auth->_CookieLoginCalled, 'should not try to auto login without persist cookie'); - } - public function testCanChangeToKnownLanguage() { $this->page->_requestedLanguage = 'en_gb'; @@ -341,7 +317,6 @@ class FakeLoginPage extends FakePageBase implements ILoginPage { public $_EmailAddress; public $_Password; - public $_PersistLogin = false; public $_LastRedirect; public $_ShowRegisterLink; public $_PageLoadWasCalled = false; @@ -356,7 +331,6 @@ class FakeLoginPage extends FakePageBase implements ILoginPage public $_CurrentCode = ''; public $_ShowUsernamePrompt = false; public $_ShowPasswordPrompt = false; - public $_ShowPersistLoginPrompt = false; public $_ShowForgotPasswordPrompt = false; public $_ShowScheduleLink = false; public $_Announcements; @@ -401,11 +375,6 @@ public function GetPassword() return $this->_Password; } - public function GetPersistLogin() - { - return $this->_PersistLogin; - } - public function GetShowRegisterLink() { return $this->_ShowRegisterLink; @@ -481,11 +450,6 @@ public function ShowPasswordPrompt($shouldShow) $this->_ShowPasswordPrompt = $shouldShow; } - public function ShowPersistLoginPrompt($shouldShow) - { - $this->_ShowPersistLoginPrompt = $shouldShow; - } - public function ShowForgotPasswordPrompt($shouldShow) { $this->_ShowForgotPasswordPrompt = $shouldShow; diff --git a/tests/fakes/FakeWebAuthentication.php b/tests/fakes/FakeWebAuthentication.php index c0fb5ac2af..691d9e70ba 100644 --- a/tests/fakes/FakeWebAuthentication.php +++ b/tests/fakes/FakeWebAuthentication.php @@ -155,9 +155,6 @@ class FakeWebAuthentication implements IWebAuthentication */ public $_LastLoginContext; public $_LastLoginId; - public $_CookieLoginCalled = false; - public $_LastLoginCookie; - public $_CookieValidateResult = false; public $_LoginCalled = false; public $_ValidateResult = false; @@ -199,15 +196,6 @@ public function Logout(UserSession $user) $this->_LogoutCalled = true; } - public function CookieLogin($cookie, $context) - { - $this->_CookieLoginCalled = true; - $this->_LastLoginCookie = $cookie; - $this->_LastLoginContext = $context; - - return $this->_CookieValidateResult; - } - public function AreCredentialsKnown() { return $this->_AreCredentialsKnown; diff --git a/tests/lib/Server/FormKeysTest.php b/tests/lib/Server/FormKeysTest.php new file mode 100644 index 0000000000..91177c9b01 --- /dev/null +++ b/tests/lib/Server/FormKeysTest.php @@ -0,0 +1,19 @@ +assertSame(FormKeys::PHONE, FormKeys::Evaluate('PHONE')); + $this->assertSame(FormKeys::PHONE, FormKeys::Evaluate('phone')); + } + + public function testPersistLoginKeyStillResolvesForLoginTemplatesCompiledByEarlierReleases(): void + { + $this->assertSame('persistLogin', FormKeys::Evaluate('PERSIST_LOGIN')); + } +} diff --git a/tpl/login.tpl b/tpl/login.tpl index 7d95280194..08e26a652e 100644 --- a/tpl/login.tpl +++ b/tpl/login.tpl @@ -73,16 +73,6 @@ {/if}
- {if $ShowUsernamePrompt && $ShowPasswordPrompt} -
-
- - -
-
- {/if} - {if $ShowRegisterLink}
{translate key="FirstTimeUser?"} From 102e9be5359bd4b104411dd414f6df851d4d64a9 Mon Sep 17 00:00:00 2001 From: "John L. Villalovos" Date: Sat, 10 Oct 2026 00:46:08 -0700 Subject: [PATCH 3/6] fix(auth): stop the resource display from logging users in Booking from the resource display identifies the user by the email address typed there. To find that user the display called the login routine, which recorded a login for the account even though nobody had authenticated as that user. Look the account up read-only instead and build the booking session without logging in. An account that exists but is not active is now refused: it is neither logged in nor replaced by a new guest account. An unknown email address still creates a guest account as before. The booking is attributed to the same user, with the same roles, as before. Assisted-by: Claude:claude-opus-5-5 --- Pages/ResourceDisplayPage.php | 10 +- Presenters/ResourceDisplayPresenter.php | 9 +- .../Authentication/GuestUserService.php | 46 +++++- .../Authentication/GuestUserServiceTest.php | 126 ++++++++++++++-- .../ResourceDisplayPresenterTest.php | 137 +++++++++++++++++- 5 files changed, 300 insertions(+), 28 deletions(-) diff --git a/Pages/ResourceDisplayPage.php b/Pages/ResourceDisplayPage.php index 337ce45b95..692a6c9baa 100644 --- a/Pages/ResourceDisplayPage.php +++ b/Pages/ResourceDisplayPage.php @@ -2,6 +2,8 @@ require_once(ROOT_DIR . 'Presenters/ResourceDisplayPresenter.php'); +use LibreBooking\Application\Authentication\UserSessionBuilder; + interface IResourceDisplayPage extends IPage, IActionPage { public function DisplayLogin(); @@ -154,13 +156,15 @@ public function __construct() new ScheduleRepository(), new DailyLayoutFactory(), new GuestUserService( - PluginManager::Instance()->LoadAuthentication(), - new GuestRegistration( + authentication: PluginManager::Instance()->LoadAuthentication(), + registration: new GuestRegistration( new PasswordEncryption(), new UserRepository(), new GuestRegistrationNotificationStrategy(), new GuestReservationPermissionStrategy($this) - ) + ), + userRepository: new UserRepository(), + userSessionBuilder: new UserSessionBuilder(roleService: PluginManager::Instance()->LoadAuthorization()) ), new AttributeService(new AttributeRepository(), new GuestPermissionService()), new ReservationRepository(), diff --git a/Presenters/ResourceDisplayPresenter.php b/Presenters/ResourceDisplayPresenter.php index 3b1422e9d3..0424e7c121 100644 --- a/Presenters/ResourceDisplayPresenter.php +++ b/Presenters/ResourceDisplayPresenter.php @@ -263,8 +263,15 @@ public function Reserve() $resultCollector->SetErrors(['Unauthorized']); $success = false; } else { - $userSession = $this->guestUserService->CreateOrLoad($email); + + if (!$userSession->IsLoggedIn()) { + $resultCollector->SetSaveSuccessfulMessage(false); + $resultCollector->SetErrors([Resources::GetInstance()->GetString('RegisteredAccountRequired')]); + $this->page->SetReservationSaveResults(false, $resultCollector); + return; + } + $resource = $this->resourceRepository->LoadById($resourceId); $series = ReservationSeries::Create($userSession->UserId, $resource, Resources::GetInstance()->GetString('AdHocMeeting'), Resources::GetInstance()->GetString('AdHocMeeting'), $date, new RepeatNone(), $userSession); diff --git a/lib/Application/Authentication/GuestUserService.php b/lib/Application/Authentication/GuestUserService.php index fba285b998..a4527ac613 100644 --- a/lib/Application/Authentication/GuestUserService.php +++ b/lib/Application/Authentication/GuestUserService.php @@ -1,8 +1,14 @@ authentication = $authentication; $this->registration = $registration; } public function CreateOrLoad($email) { - $user = $this->authentication->Login($email, new WebLoginContext(new LoginData())); - if ($user->IsLoggedIn()) { + // An existing account is only read here. The caller has not authenticated as that + // user, so this must not log them in or change their account in any way. + $user = $this->userRepository->LoadByUsername($email); + + if ($this->Exists($user)) { + if (!$this->IsActive($user)) { + Log::Debug('Account is not active, not creating a guest account %s', $email); + + return new NullUserSession(); + } + Log::Debug('User already has account, skipping guest creation %s', $email); - return $user; + return $this->userSessionBuilder->buildUserSession(user: $user, loginTime: LoginTime::Now()); } Log::Debug('Email address was not found, creating guest account %s', $email); @@ -51,7 +70,18 @@ public function CreateOrLoad($email) public function EmailExists($email) { - $user = $this->authentication->Login($email, new WebLoginContext(new LoginData())); - return $user->IsLoggedIn(); + $user = $this->userRepository->LoadByUsername($email); + + return $this->Exists($user) && $this->IsActive($user); + } + + private function Exists(User $user): bool + { + return !empty($user->Id()); + } + + private function IsActive(User $user): bool + { + return $user->StatusId() == AccountStatus::ACTIVE; } } diff --git a/tests/Application/Authentication/GuestUserServiceTest.php b/tests/Application/Authentication/GuestUserServiceTest.php index 8630c762c1..bba7d3bb9e 100644 --- a/tests/Application/Authentication/GuestUserServiceTest.php +++ b/tests/Application/Authentication/GuestUserServiceTest.php @@ -2,14 +2,17 @@ declare(strict_types=1); -require_once(ROOT_DIR . 'lib/Application/Authentication/GuestUserService.php'); +use LibreBooking\Application\Authentication\UserSessionBuilder; +use PHPUnit\Framework\MockObject\MockObject; + +require_once(ROOT_DIR . 'lib/Application/Authentication/namespace.php'); class GuestUserServiceTest extends TestBase { - /** - * @var FakeAuthentication - */ - private $authentication; + private const EMAIL = 'test@email.com'; + private const USER_ID = 123; + + private IAuthentication&MockObject $authentication; /** * @var GuestUserService @@ -21,33 +24,126 @@ class GuestUserServiceTest extends TestBase */ private $registration; + /** + * @var FakeUserRepository + */ + private $userRepository; + + /** + * @var FakeAuthorizationService + */ + private $roleService; + + private FakeUser $user; + public function setUp(): void { - $this->authentication = new FakeAuthentication(); - $this->registration = new FakeRegistration(); - $this->service = new GuestUserService($this->authentication, $this->registration); parent::setup(); + + $this->authentication = $this->createMock(IAuthentication::class); + $this->registration = new FakeRegistration(); + $this->userRepository = new FakeUserRepository(); + $this->user = new FakeUser(self::USER_ID, self::EMAIL); + $this->userRepository->_User = $this->user; + $this->roleService = new FakeAuthorizationService(); + + $this->service = new GuestUserService( + authentication: $this->authentication, + registration: $this->registration, + userRepository: $this->userRepository, + userSessionBuilder: new UserSessionBuilder(roleService: $this->roleService) + ); } public function testWhenUserAlreadyExists() { - $email = 'test@email.com'; - $session = new FakeUserSession(); - $this->authentication->_UserSession = $session; + $this->authentication->expects($this->never())->method('Login'); + + $session = $this->service->CreateOrLoad(self::EMAIL); + + $this->assertTrue($session->IsLoggedIn()); + $this->assertSame(self::USER_ID, $session->UserId); + $this->assertSame(self::EMAIL, $session->Email); + $this->assertFalse($this->registration->_RegisterCalled); + $this->assertUserWasNotChanged(); + } + + public function testExistingUserSessionKeepsTheUsersRoles() + { + $this->roleService->_IsApplicationAdministrator = true; + $this->roleService->_IsResourceAdministrator = true; - $user = $this->service->CreateOrLoad($email); + $session = $this->service->CreateOrLoad(self::EMAIL); - $this->assertEquals($session, $user); + $this->assertTrue($session->IsAdmin); + $this->assertTrue($session->IsResourceAdmin); + $this->assertFalse($session->IsGroupAdmin); } public function testWhenUserDoesntExistCreateGuest() { $email = 'guest@user.com'; - $this->authentication->_UserSession = new NullUserSession(); + $guestSession = new FakeUserSession(); + $this->userRepository->_User = User::Null(); + $this->fakeResources->CurrentLanguage = 'en_gb'; + $this->authentication->expects($this->once()) + ->method('Login') + ->with( + $this->equalTo($email), + $this->callback(fn (ILoginContext $context) => $context->GetData()->Language === 'en_gb') + ) + ->willReturn($guestSession); - $this->service->CreateOrLoad($email); + $session = $this->service->CreateOrLoad($email); + $this->assertSame($guestSession, $session); $this->assertEquals($email, $this->registration->_Login); $this->assertEquals($email, $this->registration->_Email); } + + public function testInactiveAccountIsRefusedAndLeftUntouched() + { + $this->user->SetStatus(AccountStatus::INACTIVE); + // A decorating authentication plugin may activate the account in Login(), so it must not be called. + $this->authentication->expects($this->never())->method('Login'); + + $session = $this->service->CreateOrLoad(self::EMAIL); + + $this->assertFalse($session->IsLoggedIn()); + $this->assertFalse($this->registration->_RegisterCalled, 'must not register a guest over an existing account'); + $this->assertSame(AccountStatus::INACTIVE, $this->user->StatusId()); + $this->assertUserWasNotChanged(); + } + + public function testEmailExistsForAnActiveAccountWithoutLoggingIn() + { + $this->authentication->expects($this->never())->method('Login'); + + $this->assertTrue($this->service->EmailExists(self::EMAIL)); + $this->assertUserWasNotChanged(); + } + + public function testEmailDoesNotExistForAnInactiveAccount() + { + $this->user->SetStatus(AccountStatus::INACTIVE); + $this->authentication->expects($this->never())->method('Login'); + + $this->assertFalse($this->service->EmailExists(self::EMAIL)); + $this->assertUserWasNotChanged(); + } + + public function testEmailDoesNotExistForAnUnknownAddress() + { + $this->userRepository->_User = User::Null(); + $this->authentication->expects($this->never())->method('Login'); + + $this->assertFalse($this->service->EmailExists('nobody@user.com')); + } + + private function assertUserWasNotChanged(): void + { + $this->assertNull($this->userRepository->_UpdatedUser); + $this->assertNull($this->userRepository->_LoginDataUpdatedUser); + $this->assertNull($this->userRepository->_AddedUser); + } } diff --git a/tests/Presenters/ResourceDisplayPresenterTest.php b/tests/Presenters/ResourceDisplayPresenterTest.php index c186704cf6..0fad6ca56b 100644 --- a/tests/Presenters/ResourceDisplayPresenterTest.php +++ b/tests/Presenters/ResourceDisplayPresenterTest.php @@ -2,6 +2,8 @@ declare(strict_types=1); +use LibreBooking\Application\Authentication\UserSessionBuilder; + require_once(ROOT_DIR . 'Presenters/ResourceDisplayPresenter.php'); require_once(ROOT_DIR . 'Pages/ResourceDisplayPage.php'); @@ -332,6 +334,119 @@ public function testWhenReservationFails() $this->assertEquals($this->reservationCreateHandler->_Errors, $this->page->_ResultCollector->Errors); } + public function testWhenTheAccountCannotBookNothingIsReserved() + { + Date::_SetNow(Date::Parse('2016-03-11 13:46')); + $this->page->_Email = 'inactive@user.com'; + $this->page->_BeginTime = '08:00'; + $this->page->_EndTime = '17:00'; + $this->page->_BeginDate = '2016-03-11'; + $this->page->_Timezone = 'America/New_York'; + + $this->guestUserService->_UserSession = new NullUserSession(); + $this->presenter->reservationCreateHandler = $this->reservationCreateHandler; + + $this->presenter->Reserve(); + + $this->assertFalse($this->page->_ReservationCreatedSuccessfully); + $this->assertFalse($this->page->_ResultCollector->Succeeded); + $this->assertNotEmpty($this->page->_ResultCollector->Errors); + $this->assertNull($this->reservationCreateHandler->_LastSeries, 'no reservation should be attempted'); + } + + public function testReservingAsAnExistingAccountDoesNotLogItInOrChangeItEvenWhenTheBookingFails() + { + $email = 'some@user.com'; + $userId = 123; + $authentication = $this->createMock(IAuthentication::class); + $authentication->expects($this->never())->method('Login'); + $userRepository = new FakeUserRepository(); + $userRepository->_User = new FakeUser($userId, $email); + $registration = new FakeRegistration(); + $presenter = $this->reservePresenterWithRealGuestUserService($email, $authentication, $userRepository, $registration); + $this->reservationCreateHandler->_Success = false; + $this->reservationCreateHandler->_Errors = ['booking refused']; + + $presenter->ProcessAction(); + + $this->assertTrue($this->page->_Validators['guestdenied']->IsValid()); + $this->assertFalse($this->page->_ReservationCreatedSuccessfully); + $this->assertEquals(['booking refused'], $this->page->_ResultCollector->Errors); + $this->assertSame($userId, $this->reservationCreateHandler->_LastSeries->UserId(), 'the booking is still attributed to the named user'); + $this->assertFalse($registration->_RegisterCalled); + $this->assertNull($userRepository->_UpdatedUser); + $this->assertNull($userRepository->_LoginDataUpdatedUser); + } + + public function testWhenTheRegisteredAccountCheckFailsNothingIsReservedAndNoAccountIsTouched() + { + $email = 'inactive@user.com'; + $authentication = $this->createMock(IAuthentication::class); + $authentication->expects($this->never())->method('Login'); + $user = new FakeUser(123, $email); + $user->SetStatus(AccountStatus::INACTIVE); + $userRepository = new FakeUserRepository(); + $userRepository->_User = $user; + $registration = new FakeRegistration(); + $presenter = $this->reservePresenterWithRealGuestUserService($email, $authentication, $userRepository, $registration); + + $presenter->ProcessAction(); + + $this->assertFalse($this->page->_Validators['guestdenied']->IsValid()); + $this->assertNull($this->page->_ReservationCreatedSuccessfully, 'Reserve() must not run when validation fails'); + $this->assertNull($this->reservationCreateHandler->_LastSeries); + $this->assertFalse($registration->_RegisterCalled); + $this->assertSame(AccountStatus::INACTIVE, $user->StatusId()); + $this->assertNull($userRepository->_UpdatedUser); + $this->assertNull($userRepository->_LoginDataUpdatedUser); + } + + /** + * A presenter for the 'reserve' action that uses the real GuestUserService, with guest + * reservations disabled so the registered-account validator runs, and a page that runs + * its validators before the action as the real page does. + */ + private function reservePresenterWithRealGuestUserService( + string $email, + IAuthentication $authentication, + FakeUserRepository $userRepository, + FakeRegistration $registration + ): ResourceDisplayPresenter { + Date::_SetNow(Date::Parse('2016-03-11 13:46')); + $this->page->_Action = 'reserve'; + $this->page->_RunValidators = true; + // The test page returns $_Username from GetEmail(). + $this->page->_Username = $email; + $this->page->_BeginTime = '08:00'; + $this->page->_EndTime = '17:00'; + $this->page->_BeginDate = '2016-03-11'; + $this->page->_Timezone = 'America/New_York'; + $this->fakeConfig->SetKey(ConfigKeys::TABLET_VIEW_ALLOW_GUEST_RESERVATIONS, 'false'); + $this->resourceRepository->_Resource = new FakeBookableResource(122); + + $presenter = new ResourceDisplayPresenter( + $this->page, + $this->resourceRepository, + $this->reservationService, + $this->authorizationService, + $this->authentication, + $this->scheduleRepository, + $this->dailyLayoutFactory, + new GuestUserService( + authentication: $authentication, + registration: $registration, + userRepository: $userRepository, + userSessionBuilder: new UserSessionBuilder(roleService: $this->authorizationService) + ), + $this->attributeService, + $this->reservationRepository, + $this->termsOfServiceRepository + ); + $presenter->reservationCreateHandler = $this->reservationCreateHandler; + + return $presenter; + } + private function setupStandardExpectations($requiresCheckin = false, $checkinMin = 0) { $timezone = 'America/Chicago'; @@ -412,6 +527,26 @@ class TestResourceDisplayPage extends FakePageBase implements IResourceDisplayPa public $_RequiresCheckIn = false; public $_CheckinReferenceNumber; public $_TermsOfService; + public $_Action; + public $_RunValidators = false; + + /** + * Runs the registered validators, as the real page does, when $_RunValidators is set. + */ + public function IsValid() + { + if (!$this->_RunValidators) { + return parent::IsValid(); + } + + $isValid = true; + foreach ($this->_Validators as $validator) { + $validator->Validate(); + $isValid = $isValid && $validator->IsValid(); + } + + return $isValid; + } public function TakingAction() { @@ -420,7 +555,7 @@ public function TakingAction() public function GetAction() { - // TODO: Implement GetAction() method. + return $this->_Action; } public function RequestingData() From 42e0604d768b98783772b37d35e1b934f77770f0 Mon Sep 17 00:00:00 2001 From: "John L. Villalovos" Date: Sun, 11 Oct 2026 09:38:47 -0700 Subject: [PATCH 4/6] fix(security): only book display-enabled resources from the tablet view A reservation made through the resource display took the resource's internal id straight from the request and never checked that the resource was enabled for display. Any resource could be booked that way, not only the one shown on the tablet. Identify the resource by its public id, the same way the display does, and refuse the reservation unless that resource is enabled for display. The check runs before any guest account or reservation is created, so a request naming any other resource has no effect. The authenticated setup actions keep using the internal id and are unchanged. Assisted-by: Claude:claude-opus-4-8 --- Pages/ResourceDisplayPage.php | 13 ++ Presenters/ResourceDisplayPresenter.php | 76 +++++++--- lib/Server/FormKeys.php | 1 + .../ResourceDisplayPresenterTest.php | 133 +++++++++++++++++- tests/fakes/FakeGuestUserService.php | 6 + tests/fakes/FakeResourceRepository.php | 10 ++ .../resource-display-resource.tpl | 2 +- 7 files changed, 212 insertions(+), 29 deletions(-) diff --git a/Pages/ResourceDisplayPage.php b/Pages/ResourceDisplayPage.php index 692a6c9baa..ec238b08a2 100644 --- a/Pages/ResourceDisplayPage.php +++ b/Pages/ResourceDisplayPage.php @@ -18,6 +18,13 @@ public function GetResourceId(); */ public function GetPublicResourceId(); + /** + * The public id of the resource being booked, posted by the reservation form. + * + * @return string + */ + public function GetReservationResourcePublicId(); + /** * @return string */ @@ -244,6 +251,11 @@ public function GetPublicResourceId() return $this->GetQuerystring(QueryStringKeys::RESOURCE_ID); } + public function GetReservationResourcePublicId() + { + return $this->GetForm(FormKeys::PUBLIC_RESOURCE_ID); + } + public function GetStartDate() { $userTimezone = ServiceLocator::GetServer()->GetUserSession()->Timezone; @@ -260,6 +272,7 @@ public function BindResource(BookableResource $resource) { $this->Set('ResourceName', $resource->GetName()); $this->Set('ResourceId', $resource->GetId()); + $this->Set('PublicResourceId', $resource->GetPublicId()); } public function DisplayAvailability(IDailyLayout $dailyLayout, Date $today, Date $reservationDate, $current, $next, $upcoming, $requiresCheckin, $checkinReferenceNumber) diff --git a/Presenters/ResourceDisplayPresenter.php b/Presenters/ResourceDisplayPresenter.php index 0424e7c121..1909a95c08 100644 --- a/Presenters/ResourceDisplayPresenter.php +++ b/Presenters/ResourceDisplayPresenter.php @@ -243,8 +243,21 @@ public function Reserve() return; } + $resultCollector = new ReservationResultCollector(); + + // The resource is identified by its public id and must be enabled for display, the + // same requirement as viewing it here. This is checked before any guest account or + // reservation is created, so a request for any other resource has no effect. An + // internal resource id from the request is never used. + $resource = $this->LoadDisplayEnabledResource($this->page->GetReservationResourcePublicId()); + if ($resource == null) { + $resultCollector->SetSaveSuccessfulMessage(false); + $resultCollector->SetErrors(['Unauthorized']); + $this->page->SetReservationSaveResults(false, $resultCollector); + return; + } + $timezone = $this->page->GetTimezone(); - $resourceId = $this->page->GetResourceId(); $email = $this->page->GetEmail(); $reservationDate = Date::Parse($this->page->GetBeginDate(), $timezone)->Format('Y-m-d '); @@ -256,38 +269,59 @@ public function Reserve() } $maxDate = Date::Now()->ToTimezone($timezone)->AddDays($maxFutureDays + 1)->GetDate(); - $resultCollector = new ReservationResultCollector(); - if ($date->GetBegin()->GreaterThan($maxDate)) { $resultCollector->SetSaveSuccessfulMessage(false); $resultCollector->SetErrors(['Unauthorized']); - $success = false; - } else { - $userSession = $this->guestUserService->CreateOrLoad($email); - - if (!$userSession->IsLoggedIn()) { - $resultCollector->SetSaveSuccessfulMessage(false); - $resultCollector->SetErrors([Resources::GetInstance()->GetString('RegisteredAccountRequired')]); - $this->page->SetReservationSaveResults(false, $resultCollector); - return; - } + $this->page->SetReservationSaveResults(false, $resultCollector); + return; + } - $resource = $this->resourceRepository->LoadById($resourceId); + $userSession = $this->guestUserService->CreateOrLoad($email); - $series = ReservationSeries::Create($userSession->UserId, $resource, Resources::GetInstance()->GetString('AdHocMeeting'), Resources::GetInstance()->GetString('AdHocMeeting'), $date, new RepeatNone(), $userSession); + if (!$userSession->IsLoggedIn()) { + $resultCollector->SetSaveSuccessfulMessage(false); + $resultCollector->SetErrors([Resources::GetInstance()->GetString('RegisteredAccountRequired')]); + $this->page->SetReservationSaveResults(false, $resultCollector); + return; + } - $series->AcceptTerms($this->page->GetTermsOfServiceAcknowledgement()); + $series = ReservationSeries::Create($userSession->UserId, $resource, Resources::GetInstance()->GetString('AdHocMeeting'), Resources::GetInstance()->GetString('AdHocMeeting'), $date, new RepeatNone(), $userSession); - $attributes = $this->page->GetAttributes(); - foreach ($attributes as $attribute) { - $series->AddAttributeValue(new AttributeValue($attribute->Id, $attribute->Value)); - } + $series->AcceptTerms($this->page->GetTermsOfServiceAcknowledgement()); - $success = $this->GetHandler($userSession)->Handle($series, $resultCollector); + $attributes = $this->page->GetAttributes(); + foreach ($attributes as $attribute) { + $series->AddAttributeValue(new AttributeValue($attribute->Id, $attribute->Value)); } + + $success = $this->GetHandler($userSession)->Handle($series, $resultCollector); $this->page->SetReservationSaveResults($success, $resultCollector); } + /** + * Loads the resource to book from the tablet view by its public id. Returns null when the + * public id is missing or unknown, or the resource is not enabled for display. Enablement + * is the same test DisplayResource() applies, so only a resource the tablet can show can + * be booked from it; an internal resource id is never accepted. + * + * @param string|null $publicResourceId + * @return BookableResource|null + */ + private function LoadDisplayEnabledResource($publicResourceId) + { + if (empty($publicResourceId)) { + return null; + } + + $resource = $this->resourceRepository->LoadByPublicId($publicResourceId); + + if (empty($resource->GetId()) || !$resource->GetIsCalendarSubscriptionAllowed()) { + return null; + } + + return $resource; + } + public function Checkin() { $resultCollector = new ReservationResultCollector(); diff --git a/lib/Server/FormKeys.php b/lib/Server/FormKeys.php index 536700332b..b8c0623e73 100644 --- a/lib/Server/FormKeys.php +++ b/lib/Server/FormKeys.php @@ -225,6 +225,7 @@ private function __construct() public const RESOURCE_CONTACT = 'resourceContact'; public const RESOURCE_DESCRIPTION = 'resourceDescription'; public const RESOURCE_ID = 'resourceId'; + public const PUBLIC_RESOURCE_ID = 'publicResourceId'; public const RESOURCE_IMAGE = 'resourceImage'; public const RESOURCE_IMPORT_FILE = 'resourceImportFile'; public const RESOURCE_LOCATION = 'resourceLocation'; diff --git a/tests/Presenters/ResourceDisplayPresenterTest.php b/tests/Presenters/ResourceDisplayPresenterTest.php index 0fad6ca56b..cc7fdc496c 100644 --- a/tests/Presenters/ResourceDisplayPresenterTest.php +++ b/tests/Presenters/ResourceDisplayPresenterTest.php @@ -284,14 +284,13 @@ public function testWhenBookingSucceeds() $this->page->_EndTime = '17:00'; $this->page->_BeginDate = '2016-03-11'; $this->page->_Timezone = 'America/New_York'; - $this->page->_ResourceId = 292; $userAccount = new FakeUserSession(123); $this->guestUserService->_UserSession = $userAccount; $this->reservationCreateHandler->_Success = true; - $resource = new FakeBookableResource(122); - $this->resourceRepository->_Resource = $resource; + $resource = $this->displayEnabledResource(122, 'pub-122'); + $this->page->_ReservationResourcePublicId = 'pub-122'; $begin = Date::Parse('2016-03-11 08:00', $this->page->_Timezone); $end = Date::Parse('2016-03-11 17:00', $this->page->_Timezone); @@ -316,15 +315,14 @@ public function testWhenReservationFails() $this->page->_EndTime = '17:00'; $this->page->_BeginDate = '2016-03-11'; $this->page->_Timezone = 'America/New_York'; - $this->page->_ResourceId = 292; $userAccount = new FakeUserSession(123); $this->guestUserService->_UserSession = $userAccount; $this->reservationCreateHandler->_Success = false; $this->reservationCreateHandler->_Errors = ['one', 'two']; - $resource = new FakeBookableResource(122); - $this->resourceRepository->_Resource = $resource; + $this->displayEnabledResource(122, 'pub-122'); + $this->page->_ReservationResourcePublicId = 'pub-122'; $this->presenter->reservationCreateHandler = $this->reservationCreateHandler; @@ -343,6 +341,11 @@ public function testWhenTheAccountCannotBookNothingIsReserved() $this->page->_BeginDate = '2016-03-11'; $this->page->_Timezone = 'America/New_York'; + // A display-enabled resource, so the request is refused by the account check, not the + // resource check. + $this->displayEnabledResource(122, 'pub-122'); + $this->page->_ReservationResourcePublicId = 'pub-122'; + $this->guestUserService->_UserSession = new NullUserSession(); $this->presenter->reservationCreateHandler = $this->reservationCreateHandler; @@ -401,6 +404,115 @@ public function testWhenTheRegisteredAccountCheckFailsNothingIsReservedAndNoAcco $this->assertNull($userRepository->_LoginDataUpdatedUser); } + public function testReserveRejectsAnythingNotDisplayEnabled() + { + Date::_SetNow(Date::Parse('2016-03-11 13:46')); + $this->page->_Email = 'some@user.com'; + $this->page->_BeginTime = '08:00'; + $this->page->_EndTime = '17:00'; + $this->page->_BeginDate = '2016-03-11'; + $this->page->_Timezone = 'America/New_York'; + // A session that would book successfully if the resource gate let the request through. + $this->guestUserService->_UserSession = new FakeUserSession(123); + + // One resource is display-enabled under its public id; another exists but is not. + $this->displayEnabledResource(122, 'pub-122'); + $this->resourceRepository->_ResourcesByPublicId['not-enabled'] = new FakeBookableResource(133); + + // Missing, unknown, an internal id in place of the public id, and a resource that + // exists but is not display-enabled are all refused, with no reservation created. + foreach (['', 'unknown-public-id', '122', 'not-enabled'] as $publicId) { + $handler = new FakeReservationHandler(); + $handler->_Success = true; + $this->presenter->reservationCreateHandler = $handler; + $this->guestUserService->_CreateOrLoadCalled = false; + $this->page->_ReservationCreatedSuccessfully = null; + $this->page->_ReservationResourcePublicId = $publicId; + + $this->presenter->Reserve(); + + $this->assertFalse($this->page->_ReservationCreatedSuccessfully, "public id [$publicId] must be rejected"); + $this->assertNull($handler->_LastSeries, "public id [$publicId] must create no reservation"); + $this->assertFalse($this->guestUserService->_CreateOrLoadCalled, "public id [$publicId] must not resolve or create an account"); + } + } + + public function testReserveForANonDisplayResourceCreatesNoGuestAccountAndNoReservation() + { + $email = 'new-guest@user.com'; + $authentication = $this->createMock(IAuthentication::class); + $authentication->expects($this->never())->method('Login'); + $userRepository = new FakeUserRepository(); + $userRepository->_User = User::Null(); + $registration = new FakeRegistration(); + + Date::_SetNow(Date::Parse('2016-03-11 13:46')); + // The test page returns $_Username from GetEmail(), which is what Reserve() reads. + $this->page->_Username = $email; + $this->page->_BeginTime = '08:00'; + $this->page->_EndTime = '17:00'; + $this->page->_BeginDate = '2016-03-11'; + $this->page->_Timezone = 'America/New_York'; + // A display-enabled resource exists, but under a different public id; the requested + // one is unknown, so the repository returns a Null resource. + $this->displayEnabledResource(122, 'pub-122'); + $this->page->_ReservationResourcePublicId = 'unknown-public-id'; + + $presenter = new ResourceDisplayPresenter( + $this->page, + $this->resourceRepository, + $this->reservationService, + $this->authorizationService, + $this->authentication, + $this->scheduleRepository, + $this->dailyLayoutFactory, + new GuestUserService( + authentication: $authentication, + registration: $registration, + userRepository: $userRepository, + userSessionBuilder: new UserSessionBuilder(roleService: $this->authorizationService) + ), + $this->attributeService, + $this->reservationRepository, + $this->termsOfServiceRepository + ); + $presenter->reservationCreateHandler = $this->reservationCreateHandler; + + $presenter->Reserve(); + + $this->assertFalse($this->page->_ReservationCreatedSuccessfully); + $this->assertNull($this->reservationCreateHandler->_LastSeries); + $this->assertFalse($registration->_RegisterCalled, 'no guest account may be created for a non-display resource'); + } + + public function testActivateStillUsesTheInternalResourceIdAndEnablesDisplay() + { + $resource = new FakeBookableResource(55); + $this->resourceRepository->_Resource = $resource; + $this->page->_ResourceId = 55; + $this->authorizationService->_CanEditForResource = true; + + $this->presenter->Activate(); + + $this->assertTrue($resource->GetIsDisplayEnabled(), 'the setup flow still enables display by internal id'); + $this->assertEquals($resource, $this->resourceRepository->_UpdatedResource); + $this->assertEquals($resource->GetPublicId(), $this->page->_ActivatedResourceId); + } + + /** + * Builds a resource that is enabled for display (has a public id and allows calendar + * subscription) and registers it with the repository fake under that public id. + */ + private function displayEnabledResource($resourceId, $publicId): FakeBookableResource + { + $resource = new FakeBookableResource($resourceId); + $resource->WithPublicId($publicId); + $resource->EnableSubscription(); + $this->resourceRepository->_ResourcesByPublicId[$publicId] = $resource; + + return $resource; + } + /** * A presenter for the 'reserve' action that uses the real GuestUserService, with guest * reservations disabled so the registered-account validator runs, and a page that runs @@ -422,7 +534,8 @@ private function reservePresenterWithRealGuestUserService( $this->page->_BeginDate = '2016-03-11'; $this->page->_Timezone = 'America/New_York'; $this->fakeConfig->SetKey(ConfigKeys::TABLET_VIEW_ALLOW_GUEST_RESERVATIONS, 'false'); - $this->resourceRepository->_Resource = new FakeBookableResource(122); + $this->displayEnabledResource(122, 'pub-122'); + $this->page->_ReservationResourcePublicId = 'pub-122'; $presenter = new ResourceDisplayPresenter( $this->page, @@ -492,6 +605,7 @@ class TestResourceDisplayPage extends FakePageBase implements IResourceDisplayPa public $_ResourceId; public $_ActivatedResourceId; public $_PublicResourceId; + public $_ReservationResourcePublicId; public $_DailyLayout; public $_BoundResource; public $_AvailableNow = false; @@ -618,6 +732,11 @@ public function GetPublicResourceId() return $this->_PublicResourceId; } + public function GetReservationResourcePublicId() + { + return $this->_ReservationResourcePublicId; + } + public function DisplayAvailability(IDailyLayout $dailyLayout, Date $today, Date $reservationDate, $current, $next, $upcoming, $requiresCheckin, $checkinReferenceNumber) { $this->_DailyLayout = $dailyLayout; diff --git a/tests/fakes/FakeGuestUserService.php b/tests/fakes/FakeGuestUserService.php index 26dc0177de..bd61b6a198 100644 --- a/tests/fakes/FakeGuestUserService.php +++ b/tests/fakes/FakeGuestUserService.php @@ -15,8 +15,14 @@ class FakeGuestUserService implements IGuestUserService */ private $_EmailExists = false; + public $_CreateOrLoadCalled = false; + public $_LastCreateOrLoadEmail = null; + public function CreateOrLoad($email) { + $this->_CreateOrLoadCalled = true; + $this->_LastCreateOrLoadEmail = $email; + return $this->_UserSession; } diff --git a/tests/fakes/FakeResourceRepository.php b/tests/fakes/FakeResourceRepository.php index f3d278aaae..ac557c903d 100644 --- a/tests/fakes/FakeResourceRepository.php +++ b/tests/fakes/FakeResourceRepository.php @@ -19,6 +19,13 @@ class FakeResourceRepository implements IResourceRepository * @var FakeBookableResource */ public $_Resource; + /** + * Resources keyed by public id. When non-empty, LoadByPublicId() uses this map strictly + * and returns a Null resource for an id that is not present. + * + * @var BookableResource[] + */ + public $_ResourcesByPublicId = []; /** * @var BookableResource|FakeBookableResource @@ -80,6 +87,9 @@ public function LoadById($resourceId) public function LoadByPublicId($publicId) { + if (!empty($this->_ResourcesByPublicId)) { + return $this->_ResourcesByPublicId[$publicId] ?? BookableResource::Null(); + } return $this->_Resource; } diff --git a/tpl/ResourceDisplay/resource-display-resource.tpl b/tpl/ResourceDisplay/resource-display-resource.tpl index 60c692031c..26e25e9c6a 100644 --- a/tpl/ResourceDisplay/resource-display-resource.tpl +++ b/tpl/ResourceDisplay/resource-display-resource.tpl @@ -240,7 +240,7 @@ data-bs-dismiss="modal" id="reserveCancel">{translate key=Cancel}
- + From f864b8235f260f43f7b537454d2912224fbec18d Mon Sep 17 00:00:00 2001 From: "John L. Villalovos" Date: Mon, 12 Oct 2026 16:28:39 -0700 Subject: [PATCH 5/6] fix(security): remove admin exemptions from kiosk bookings Use a restricted copy of the booking session and prevent stored roles from restoring administrative permissions or exemptions. Preserve ownership, ordinary permissions, and authenticated sessions. Add regression coverage across all four administrative roles. Assisted-by: Codex:GPT-6 --- Domain/Access/ScheduleUserRepository.php | 14 +- Presenters/ResourceDisplayPresenter.php | 6 +- .../CustomAttributeValidationRule.php | 8 +- .../Validation/PermissionValidationRule.php | 9 + lib/Server/UserSession.php | 19 ++ .../Reservation/PreReservationFactoryTest.php | 257 ++++++++++++++++++ .../ResourceDisplayPresenterTest.php | 86 +++++- 7 files changed, 390 insertions(+), 9 deletions(-) create mode 100644 tests/Application/Reservation/PreReservationFactoryTest.php diff --git a/Domain/Access/ScheduleUserRepository.php b/Domain/Access/ScheduleUserRepository.php index b411246b91..bdf2782310 100644 --- a/Domain/Access/ScheduleUserRepository.php +++ b/Domain/Access/ScheduleUserRepository.php @@ -13,15 +13,19 @@ public function GetUser($userId); class ScheduleUserRepository implements IScheduleUserRepository { + public function __construct(private readonly bool $includeAdminPermissions = true) + { + } + public function GetUser($userId) { $userPermissions = $this->GetUserPermissions($userId); return new ScheduleUser( - $userId, - $userPermissions['full'], - $userPermissions['view'], - $this->GetGroupPermissions($userId), - $this->GetGroupAdminPermissions($userId) + userId: $userId, + userPermissionsFull: $userPermissions['full'], + userPermissionsView: $userPermissions['view'], + groupPermissions: $this->GetGroupPermissions($userId), + groupAdminPermissions: $this->includeAdminPermissions ? $this->GetGroupAdminPermissions($userId) : [] ); } diff --git a/Presenters/ResourceDisplayPresenter.php b/Presenters/ResourceDisplayPresenter.php index 1909a95c08..e922f509cc 100644 --- a/Presenters/ResourceDisplayPresenter.php +++ b/Presenters/ResourceDisplayPresenter.php @@ -45,7 +45,7 @@ class ResourceDisplayPresenter extends ActionPresenter private $guestUserService; /** - * @var IReservationHandler + * @var IReservationHandler|null */ public $reservationCreateHandler; @@ -285,6 +285,10 @@ public function Reserve() return; } + // An email identifies the owner; it does not authenticate their admin privileges. + // Restrict both existing accounts and sessions returned after guest registration. + $userSession = $userSession->WithoutAdminPrivileges(); + $series = ReservationSeries::Create($userSession->UserId, $resource, Resources::GetInstance()->GetString('AdHocMeeting'), Resources::GetInstance()->GetString('AdHocMeeting'), $date, new RepeatNone(), $userSession); $series->AcceptTerms($this->page->GetTermsOfServiceAcknowledgement()); diff --git a/lib/Application/Reservation/Validation/CustomAttributeValidationRule.php b/lib/Application/Reservation/Validation/CustomAttributeValidationRule.php index ec5382d992..dfd5bff7f7 100644 --- a/lib/Application/Reservation/Validation/CustomAttributeValidationRule.php +++ b/lib/Application/Reservation/Validation/CustomAttributeValidationRule.php @@ -25,8 +25,12 @@ public function Validate($reservationSeries, $retryParameters) $user = $this->userRepository->LoadById($reservationSeries->UserId()); $bookedBy = $this->userRepository->LoadById($reservationSeries->BookedBy()->UserId); - $isResourceAdmin = $bookedBy->IsResourceAdminForOneOf($reservationSeries->AllResources()); - $isAdminUser = $bookedBy->IsAdminFor($user) || $isResourceAdmin; + // Repository roles must not restore privileges removed from the booking context. + $isAdminUser = false; + if ($reservationSeries->BookedBy()->AllowAdminPrivileges) { + $isAdminUser = $bookedBy->IsAdminFor($user) + || $bookedBy->IsResourceAdminForOneOf($reservationSeries->AllResources()); + } $result = $this->attributeService->Validate(CustomAttributeCategory::RESERVATION, $reservationSeries->AttributeValues(), [], false, $isAdminUser); diff --git a/lib/Application/Reservation/Validation/PermissionValidationRule.php b/lib/Application/Reservation/Validation/PermissionValidationRule.php index 838cda9129..280db97488 100644 --- a/lib/Application/Reservation/Validation/PermissionValidationRule.php +++ b/lib/Application/Reservation/Validation/PermissionValidationRule.php @@ -25,10 +25,19 @@ public function Validate($reservationSeries, $retryParameters) $reservationSeries->UserId(); $permissionService = $this->permissionServiceFactory->GetPermissionService(); + $session = $reservationSeries->BookedBy(); + // The usual permission store also includes resources administered by this user. + // Restricted bookings require ordinary user/group permission as well as any + // checks imposed by the configured permission plugin. + $ordinaryPermissions = $session->AllowAdminPrivileges ? null + : new PermissionService(new ResourcePermissionStore(new ScheduleUserRepository(includeAdminPermissions: false))); $resourceIds = $reservationSeries->AllResourceIds(); foreach ($resourceIds as $resourceId) { + if ($ordinaryPermissions !== null && !$ordinaryPermissions->CanBookResource(new ReservationResource($resourceId), $session)) { + return new ReservationRuleResult(false, Resources::GetInstance()->GetString('NoResourcePermission')); + } if (!$permissionService->CanBookResource(new ReservationResource($resourceId), $reservationSeries->BookedBy())) { return new ReservationRuleResult(false, Resources::GetInstance()->GetString('NoResourcePermission')); } diff --git a/lib/Server/UserSession.php b/lib/Server/UserSession.php index 3f56355ab3..a20f63786d 100644 --- a/lib/Server/UserSession.php +++ b/lib/Server/UserSession.php @@ -19,6 +19,8 @@ class UserSession public $Groups = []; public $AdminGroups = []; public $CSRFToken = ''; + // Also prevents services from deriving administrative permissions from stored roles. + public bool $AllowAdminPrivileges = true; public function __construct($id) { @@ -35,6 +37,23 @@ public function IsGuest() return false; } + /** + * Keeps booking ownership and ordinary permissions without granting admin exemptions. + * Returns a copy so a kiosk cannot change an authenticated browser session. + */ + public function WithoutAdminPrivileges(): UserSession + { + $session = clone $this; + $session->IsAdmin = false; + $session->IsGroupAdmin = false; + $session->IsResourceAdmin = false; + $session->IsScheduleAdmin = false; + $session->AdminGroups = []; + $session->AllowAdminPrivileges = false; + + return $session; + } + public function IsAdminForGroup($groupIds = []) { if (!is_array($groupIds)) { diff --git a/tests/Application/Reservation/PreReservationFactoryTest.php b/tests/Application/Reservation/PreReservationFactoryTest.php new file mode 100644 index 0000000000..5e0a8b3716 --- /dev/null +++ b/tests/Application/Reservation/PreReservationFactoryTest.php @@ -0,0 +1,257 @@ +fakeConfig->SetKey(ConfigKeys::RESERVATION_START_TIME_CONSTRAINT, ReservationStartTimeConstraint::FUTURE); + $this->schedule = new Schedule(self::SCHEDULE_ID, 'Kiosk', false, DayOfWeek::SUNDAY, 7, 'UTC'); + $this->layout = new ScheduleLayout('UTC'); + $this->layout->AppendPeriod(Time::Parse('10:00'), Time::Parse('11:00')); + $this->layout->AppendPeriod(Time::Parse('11:00'), Time::Parse('12:00')); + $schedules = $this->createMock(ScheduleRepository::class); + $schedules->method('LoadById')->willReturn($this->schedule); + $schedules->method('GetLayout')->willReturn($this->layout); + $this->resource = new FakeBookableResource(self::RESOURCE_ID, 'Room'); + $this->resource->SetScheduleId(self::SCHEDULE_ID); + $this->resource->SetAllowMultiday(true); + $this->users = new FakeUserRepository(); + $this->users->_User = new FakeUser(self::USER_ID); + // The persisted account remains an administrator throughout these tests. + $this->users->_User->_IsAdminForUser = true; + $this->users->_User->_IsResourceAdmin = true; + $this->reservations = new FakeReservationViewRepository(); + $this->factory = new class ($schedules, $this->users, $this->reservations) extends PreReservationFactory { + public function __construct($schedules, $users, $reservations) + { + parent::__construct(); + $this->scheduleRepository = $schedules; + $this->userRepository = $users; + $this->reservationRepository = $reservations; + } + }; + $plugins = $this->getMockBuilder(FakePluginManager::class)->onlyMethods(['LoadAuthorization'])->getMock(); + $plugins->method('LoadAuthorization')->willReturn(new AuthorizationService($this->users)); + PluginManager::SetInstance($plugins); + // Real permission/attribute/quota repositories, with rows selected by command, + // so the test does not depend on the number or ordering of database reads. + $db = $this->getMockBuilder(FakeDatabase::class)->onlyMethods(['Query'])->getMock(); + $db->method('Query')->willReturnCallback(fn (ISqlCommand $command) => new FakeReader($this->rows[get_class($command)] ?? [])); + ServiceLocator::SetDatabase($db); + $permission = [ColumnNames::RESOURCE_ID => self::RESOURCE_ID, ColumnNames::RESOURCE_NAME => 'Room', ColumnNames::PERMISSION_TYPE => ResourcePermissionType::Full]; + $this->rows[GetUserPermissionsCommand::class] = [$permission]; + $this->rows[SelectUserGroupResourceAdminPermissions::class] = [$permission]; + } + + public static function roles(): array + { + return [['IsAdmin'], ['IsGroupAdmin'], ['IsResourceAdmin'], ['IsScheduleAdmin']]; + } + + public static function violations(): iterable + { + $cases = [ + 'permission' => 'NoResourcePermission', + 'start' => 'StartIsInPast', + 'minimum notice' => 'MinNoticeError', + 'maximum notice' => 'MaxNoticeError', + 'title' => 'TitleRequiredRule', + 'description' => 'DescriptionRequiredRule', + 'minimum duration' => 'MinDurationError', + 'maximum duration' => 'MaxDurationError', + 'cross day' => 'MultiDayRule', + 'resource count' => 'InvalidNumberOfResourcesError', + 'quota' => 'QuotaConfiguration', + 'attribute' => 'CustomAttributeErrors', + 'credits' => 'CreditsRule', + 'concurrent' => 'ScheduleTotalReservationsError', + 'participation' => 'MaxParticipantsError', + 'buffer' => 'ConflictingReservationDates', + ]; + foreach (self::roles() as [$role]) { + foreach ($cases as $case => $error) { + yield "$role: $case" => [$role, $case, $error]; + } + } + } + + #[DataProvider('violations')] + public function testRestrictedAdminMustFollowOrdinaryRules(string $role, string $case, string $error): void + { + $session = $this->adminSession($role)->WithoutAdminPrivileges(); + $start = Date::Parse('2026-10-11 10:00', 'UTC'); + $end = $start->AddHours(1); + $title = 'Meeting'; + $description = 'Meeting'; + switch ($case) { + case 'permission': + // Admin-only resource access remains in the database, but is insufficient. + $this->rows[GetUserPermissionsCommand::class] = []; + break; + case 'start': + Date::_SetNow($end); + break; + case 'minimum notice': + $this->resource->SetMinNoticeAdd('4h'); + break; + case 'maximum notice': + $this->resource->SetMaxNotice('1h'); + break; + case 'title': + $this->fakeConfig->SetKey(ConfigKeys::RESERVATION_TITLE_REQUIRED, true); + $title = ''; + break; + case 'description': + $this->fakeConfig->SetKey(ConfigKeys::RESERVATION_DESCRIPTION_REQUIRED, true); + $description = ''; + break; + case 'minimum duration': + $this->resource->SetMinLength('2h'); + break; + case 'maximum duration': + $this->resource->SetMaxLength('30m'); + break; + case 'cross day': + $this->resource->SetAllowMultiday(false); + $end = $end->AddDays(1); + break; + case 'resource count': + $this->schedule->SetMaxResourcesPerReservation(1); + $this->rows[GetUserPermissionsCommand::class][] = [ColumnNames::RESOURCE_ID => self::OTHER_RESOURCE_ID, ColumnNames::RESOURCE_NAME => 'Other', ColumnNames::PERMISSION_TYPE => ResourcePermissionType::Full]; + break; + case 'quota': + $this->rows[GetAllQuotasCommand::class] = [[ + ColumnNames::QUOTA_ID => 1, ColumnNames::QUOTA_LIMIT => 0, + ColumnNames::QUOTA_UNIT => QuotaUnit::Reservations, ColumnNames::QUOTA_DURATION => QuotaDuration::Day, + ColumnNames::RESOURCE_ID => null, ColumnNames::GROUP_ID => null, ColumnNames::SCHEDULE_ID => null, + ColumnNames::ENFORCED_START_TIME => null, ColumnNames::ENFORCED_END_TIME => null, ColumnNames::ENFORCED_DAYS => null, + ColumnNames::QUOTA_SCOPE => QuotaScope::IncludeCompleted, + 'group_name' => '', 'resource_name' => '', 'schedule_name' => '', + ]]; + break; + case 'attribute': + $this->rows[GetAttributesByCategoryCommand::class] = [[ + ColumnNames::ATTRIBUTE_ID => 1, ColumnNames::ATTRIBUTE_LABEL => 'Required field', + ColumnNames::ATTRIBUTE_TYPE => CustomAttributeTypes::SINGLE_LINE_TEXTBOX, + ColumnNames::ATTRIBUTE_CATEGORY => CustomAttributeCategory::RESERVATION, + ColumnNames::ATTRIBUTE_CONSTRAINT => '', ColumnNames::ATTRIBUTE_REQUIRED => true, + ColumnNames::ATTRIBUTE_POSSIBLE_VALUES => '', ColumnNames::ATTRIBUTE_SORT_ORDER => 0, + ColumnNames::ATTRIBUTE_ADMIN_ONLY => false, + ]]; + break; + case 'credits': + $this->fakeConfig->SetKey(ConfigKeys::CREDITS_ENABLED, true); + $this->resource->SetCreditsPerSlot(1); + break; + case 'concurrent': + $this->schedule->SetTotalConcurrentReservations(1); + $this->reservations->_Reservations = [new TestReservationItemView(1, $start, $end, self::OTHER_RESOURCE_ID)]; + break; + case 'participation': + $this->resource->SetMaxParticipants(1); + break; + case 'buffer': + $this->resource->SetBufferTime('30m'); + $conflict = new TestReservationItemView(1, $end, $end->AddHours(1), self::RESOURCE_ID); + $conflict->WithBufferTime(30 * 60); + $this->reservations->_Reservations = [$conflict]; + break; + } + $series = ReservationSeries::Create(self::USER_ID, $this->resource, $title, $description, new DateRange($start, $end), new RepeatNone(), $session); + $series->AcceptTerms(true); + if ($case === 'resource count') { + $series->AddResource(new FakeBookableResource(self::OTHER_RESOURCE_ID)); + } + if ($case === 'participation') { + $series->ChangeParticipants([self::USER_ID, self::USER_ID + 1]); + } + if ($case === 'credits') { + $series->CalculateCredits($this->layout); + $this->assertGreaterThan(0, $series->GetCreditsRequired()); + } + $result = $this->factory->CreatePreAddService($session)->Validate($series); + $this->assertFalse($result->CanBeSaved()); + $this->assertStringContainsString($error, implode(' ', $result->GetErrors())); + + if (in_array($case, ['permission', 'start', 'minimum notice', 'maximum notice', 'title', 'description', 'attribute'], true)) { + $authenticated = $this->adminSession($role); + $normalBooking = ReservationSeries::Create(self::USER_ID, $this->resource, $title, $description, new DateRange($start, $end), new RepeatNone(), $authenticated); + $normalResult = $this->factory->CreatePreAddService($authenticated)->Validate($normalBooking); + $this->assertTrue($normalResult->CanBeSaved(), 'Authenticated admin exemptions must remain available: ' . implode(' ', $normalResult->GetErrors())); + } + } + + #[DataProvider('roles')] + public function testRestrictedBookingsRequireApprovalButAuthenticatedAdminsKeepExemptions(string $role): void + { + $source = $this->adminSession($role); + $this->resource->RequiresApproval(true); + $restricted = $source->WithoutAdminPrivileges(); + foreach ([$restricted, $source] as $session) { + $series = ReservationSeries::Create(self::USER_ID, $this->resource, 'Meeting', 'Meeting', DateRange::Create('2026-10-11 10:00', '2026-10-11 11:00', 'UTC'), new RepeatNone(), $session); + $result = $this->factory->CreatePreAddService($session)->Validate($series); + $this->assertTrue($result->CanBeSaved(), implode(' ', $result->GetErrors())); + $this->assertSame($session === $restricted, $series->RequiresApproval()); + $this->assertSame(self::USER_ID, $series->UserId()); + } + $this->assertTrue($source->$role); + $this->assertNull($this->users->_UpdatedUser); + $this->assertNull($this->users->_LoginDataUpdatedUser); + } + + public function testOrdinaryGroupPermissionStillAllowsRestrictedBooking(): void + { + $this->rows[SelectUserGroupPermissions::class] = [array_merge($this->rows[GetUserPermissionsCommand::class][0], [ColumnNames::GROUP_ID => self::GROUP_ID])]; + $this->rows[GetUserPermissionsCommand::class] = []; + $session = $this->adminSession('IsAdmin')->WithoutAdminPrivileges(); + $series = ReservationSeries::Create(self::USER_ID, $this->resource, 'Meeting', 'Meeting', DateRange::Create('2026-10-11 10:00', '2026-10-11 11:00', 'UTC'), new RepeatNone(), $session); + $result = $this->factory->CreatePreAddService($session)->Validate($series); + $this->assertTrue($result->CanBeSaved(), implode(' ', $result->GetErrors())); + } + + public function testConfiguredPermissionPluginCanStillDenyRestrictedBooking(): void + { + $permission = $this->createMock(IPermissionService::class); + $permission->expects($this->once())->method('CanBookResource')->willReturn(false); + $plugins = $this->getMockBuilder(FakePluginManager::class)->onlyMethods(['LoadPermission'])->getMock(); + $plugins->method('LoadPermission')->willReturn($permission); + PluginManager::SetInstance($plugins); + $session = $this->adminSession('IsAdmin')->WithoutAdminPrivileges(); + $series = ReservationSeries::Create(self::USER_ID, $this->resource, 'Meeting', 'Meeting', DateRange::Create('2026-10-11 10:00', '2026-10-11 11:00', 'UTC'), new RepeatNone(), $session); + $result = $this->factory->CreatePreAddService($session)->Validate($series); + $this->assertFalse($result->CanBeSaved()); + $this->assertSame(['NoResourcePermission'], $result->GetErrors()); + } + + private function adminSession(string $role): UserSession + { + $session = new UserSession(self::USER_ID); + $session->Timezone = 'UTC'; + $session->$role = true; + $session->Groups = [self::GROUP_ID]; + $session->AdminGroups = [self::GROUP_ID]; + return $session; + } +} diff --git a/tests/Presenters/ResourceDisplayPresenterTest.php b/tests/Presenters/ResourceDisplayPresenterTest.php index cc7fdc496c..e59f158775 100644 --- a/tests/Presenters/ResourceDisplayPresenterTest.php +++ b/tests/Presenters/ResourceDisplayPresenterTest.php @@ -302,7 +302,8 @@ public function testWhenBookingSucceeds() $this->assertEquals(true, $this->page->_ReservationCreatedSuccessfully); $this->assertEquals($resource, $this->reservationCreateHandler->_LastSeries->Resource()); $this->assertEquals($userAccount->UserId, $this->reservationCreateHandler->_LastSeries->UserId()); - $this->assertEquals($userAccount, $this->reservationCreateHandler->_LastSeries->BookedBy()); + $this->assertNotSame($userAccount, $this->reservationCreateHandler->_LastSeries->BookedBy()); + $this->assertEquals($userAccount->WithoutAdminPrivileges(), $this->reservationCreateHandler->_LastSeries->BookedBy()); $this->assertEquals($begin, $this->reservationCreateHandler->_LastSeries->CurrentInstance()->StartDate()); $this->assertEquals($end, $this->reservationCreateHandler->_LastSeries->CurrentInstance()->EndDate()); } @@ -381,6 +382,89 @@ public function testReservingAsAnExistingAccountDoesNotLogItInOrChangeItEvenWhen $this->assertNull($userRepository->_LoginDataUpdatedUser); } + public function testKioskPassesRestrictedCopyToRealHandlerWithoutChangingBrowserSession(): void + { + $source = new FakeUserSession(true); + $source->IsGroupAdmin = true; + $source->IsResourceAdmin = true; + $source->IsScheduleAdmin = true; + $source->Groups = [42]; + $source->AdminGroups = [42]; + $before = clone $source; + $this->fakeServer->UserSession = $source; + $this->guestUserService->_UserSession = $source; + Date::_SetNow(Date::Parse('2026-10-11 08:00', 'UTC')); + $this->page->_Username = $source->Email; + $this->page->_BeginTime = '10:00'; + $this->page->_EndTime = '11:00'; + $this->page->_BeginDate = '2026-10-11'; + $this->page->_Timezone = 'UTC'; + $this->displayEnabledResource(122, 'pub-122'); + $this->page->_ReservationResourcePublicId = 'pub-122'; + $this->fakeConfig->SetKey(ConfigKeys::RESERVATION_START_TIME_CONSTRAINT, ReservationStartTimeConstraint::FUTURE); + $this->useRealHandlerExpectingRestrictedSession($source); + + $this->presenter->Reserve(); + + $this->assertFalse($this->page->_ReservationCreatedSuccessfully); + $this->assertSame(['NoResourcePermission'], $this->page->_ResultCollector->Errors); + $this->assertNull($this->reservationRepository->_LastAddedReservation); + $this->assertSame($source, $this->fakeServer->UserSession); + $this->assertEquals($before, $source); + } + + public function testNewGuestProviderSessionIsRestrictedBeforeBooking(): void + { + $source = new FakeUserSession(true); + $source->IsGroupAdmin = true; + $source->IsResourceAdmin = true; + $source->IsScheduleAdmin = true; + $before = clone $source; + $authentication = $this->createMock(IAuthentication::class); + $authentication->expects($this->once())->method('Login')->willReturn($source); + $users = new FakeUserRepository(); + $users->_User = User::Null(); + $registration = new FakeRegistration(); + $presenter = $this->reservePresenterWithRealGuestUserService($source->Email, $authentication, $users, $registration); + $presenter->reservationCreateHandler = null; + $this->fakeConfig->SetKey(ConfigKeys::TABLET_VIEW_ALLOW_GUEST_RESERVATIONS, true); + $this->fakeConfig->SetKey(ConfigKeys::RESERVATION_START_TIME_CONSTRAINT, ReservationStartTimeConstraint::NONE); + $this->useRealHandlerExpectingRestrictedSession($source); + + $presenter->ProcessAction(); + + $this->assertTrue($registration->_RegisterCalled); + $this->assertFalse($this->page->_ReservationCreatedSuccessfully); + $this->assertSame(['NoResourcePermission'], $this->page->_ResultCollector->Errors); + $this->assertNull($this->reservationRepository->_LastAddedReservation); + $this->assertEquals($before, $source); + } + + private function useRealHandlerExpectingRestrictedSession(UserSession $source): void + { + $factory = $this->createMock(IPreReservationFactory::class); + $factory->expects($this->once())->method('CreatePreAddService') + ->willReturnCallback(function (UserSession $session) use ($source) { + $this->assertNotSame($source, $session); + $this->assertSame($source->UserId, $session->UserId); + $this->assertSame($source->Groups, $session->Groups); + $this->assertSame($source->Email, $session->Email); + $this->assertSame($source->Timezone, $session->Timezone); + $this->assertSame($source->LanguageCode, $session->LanguageCode); + $this->assertFalse($session->IsAdmin); + $this->assertFalse($session->IsGroupAdmin); + $this->assertFalse($session->IsResourceAdmin); + $this->assertFalse($session->IsScheduleAdmin); + $this->assertFalse($session->AllowAdminPrivileges); + $this->assertSame([], $session->AdminGroups); + return (new PreReservationFactory())->CreatePreAddService($session); + }); + $this->fakePluginManager->preResPlugin = $factory; + $notifications = $this->createMock(IPostReservationFactory::class); + $notifications->method('CreatePostAddService')->willReturn(new NullReservationNotificationService()); + $this->fakePluginManager->postResPlugin = $notifications; + } + public function testWhenTheRegisteredAccountCheckFailsNothingIsReservedAndNoAccountIsTouched() { $email = 'inactive@user.com'; From 7c271a968a957b67478a0276f6bf8fc3c60ab200 Mon Sep 17 00:00:00 2001 From: "John L. Villalovos" Date: Tue, 13 Oct 2026 08:45:01 -0700 Subject: [PATCH 6/6] docs: replace upcoming security notice with 7.1.0 upgrade notice The README announced a security release for 13 October 2026 without saying what it fixed. That release is LibreBooking 7.1.0, so replace the announcement with the upgrade notice. The notice tells administrators to upgrade to 7.1.0 or later, says that the release removes the "Remember Me" option, gives the workaround for installations that cannot upgrade right away, and links to security advisory GHSA-mvjx-72w3-wqhp. The notice for the earlier 7.0.0 security release is unchanged. Assisted-by: Claude:claude-opus-5-5 Closes: #1779 --- README.md | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index fcf8c59d26..3d1943598d 100644 --- a/README.md +++ b/README.md @@ -2,21 +2,26 @@ # Librebooking > [!WARNING] -> We are planning on doing a security release for a high-impact security -> vulnerability. The release is planned to happen on [Tuesday 13-October-2026 @ -> 9AM PDT (Pacific Daylight Time)](https://www.timeanddate.com/worldclock/meetingdetails.html?year=2026&month=10&day=13&hour=16&min=0&sec=0&p1=5306). +> **Security: upgrade to LibreBooking 7.1.0 or later immediately.** > -> You can convert to your local time zone here: -> [https://www.timeanddate.com/worldclock/meetingdetails.html?year=2026&month=10&day=13&hour=16&min=0&sec=0&p1=5306](https://www.timeanddate.com/worldclock/meetingdetails.html?year=2026&month=10&day=13&hour=16&min=0&sec=0&p1=5306) +> All versions before 7.1.0 contain a critical vulnerability that lets an +> unauthenticated attacker sign in as another user, including an +> administrator, by forging the "Remember Me" login cookie. Security fixes +> are not backported, so no fix will be released for 7.0.x or earlier. > -> If you have questions/comments feel free to join the -> [Discord channel](https://discord.gg/4TGThPtmX8) or comment on the -> [related issue](https://github.com/LibreBooking/librebooking/issues/1779) -> Though we will not disclose what the issue is until the release. +> LibreBooking 7.1.0 removes the "Remember Me" option. Users sign in again +> once their session ends. > -> Regular backups are also recommended in case of any issues. +> If you cannot upgrade right away, configure your web server or reverse +> proxy to remove the `persist_login` cookie from incoming requests. See the +> [security advisory](https://github.com/LibreBooking/librebooking/security/advisories/GHSA-mvjx-72w3-wqhp) +> for details and for how to check whether an installation was compromised, +> and get the +> [latest release](https://github.com/LibreBooking/librebooking/releases/latest). > -> As always we invite people to join the [Discord channel](https://discord.gg/4TGThPtmX8) +> If you have questions, join the +> [Discord channel](https://discord.gg/4TGThPtmX8) or comment on the +> [related issue](https://github.com/LibreBooking/librebooking/issues/1779). Previous security update