diff --git a/Auth/Base.php b/Auth/Base.php index 58c22305..ec54a0a9 100644 --- a/Auth/Base.php +++ b/Auth/Base.php @@ -289,7 +289,17 @@ protected function getUserForLogin() if (!empty($this->login)) { $user = $this->usersModel->getUser($this->login); - if (!empty($user) && !$this->isSameLogin($this->login, $user['login'])) { + if (!empty($user) && !UserIdentity::isSameLogin($this->login, $user['login'])) { + $this->logger->warning( + "Auth\\Base::{func}: refusing to authenticate '{assertedLogin}': it resolves to the " + . "existing Matomo user '{storedLogin}', which is a different login.", + array( + 'func' => __FUNCTION__, + 'assertedLogin' => $this->login, + 'storedLogin' => $user['login'], + ) + ); + throw new Exception(sprintf( "Refusing to authenticate: asserted login '%s' resolved to the different existing user '%s'.", $this->login, @@ -307,21 +317,6 @@ protected function getUserForLogin() return $this->userForLogin; } - /** - * Returns whether the asserted login and the login of the user row it resolved to identify the same user. - * - * Implementations that verify no credential against the returned row have to override this and require an - * exact match. - * - * @param string $assertedLogin - * @param string $storedLogin - * @return bool - */ - protected function isSameLogin(string $assertedLogin, string $storedLogin): bool - { - return UserIdentity::isSameLogin($assertedLogin, $storedLogin); - } - protected function tryFallbackAuth($onlySuperUsers = true, ?Auth $auth = null) { if (empty($auth)) { @@ -387,7 +382,7 @@ protected function makeSuccessLogin($userInfo) protected function makeAuthFailure() { - return new AuthResult(AuthResult::FAILURE, $this->login, null); + return new AuthResult(AuthResult::FAILURE, $this->login, ''); } protected function authenticateByLdap() diff --git a/Auth/WebServerAuth.php b/Auth/WebServerAuth.php index d8afa1fe..fa6c8352 100644 --- a/Auth/WebServerAuth.php +++ b/Auth/WebServerAuth.php @@ -17,7 +17,6 @@ use Piwik\Plugins\LoginLdap\Ldap\Exceptions\ConnectionException; use Piwik\Plugins\LoginLdap\LdapInterop\UserSynchronizer; use Piwik\Plugins\LoginLdap\Model\LdapUsers; -use Piwik\Plugins\LoginLdap\UserIdentity; use Piwik\Plugins\UsersManager\API as UsersManagerAPI; use Piwik\Plugins\UsersManager\Model as UserModel; use Piwik\Session; @@ -153,8 +152,15 @@ private static function getAlreadyAuthenticatedLogin() } /** - * Returns the login the web server authenticated for this request, normalized the way - * {@link self::authenticate()} normalizes it, or null when the web server authenticated nobody. + * Returns the login the web server authenticated for this request, or null when it authenticated nobody. + * + * The single answer to "who does the web server say this is", so that authentication, the session guard + * and {@link self::isCurrentRequestWebServerAuthenticated()} cannot disagree about it. + * + * The value is returned as the web server gave it. Trimming it here would let "ironman " authenticate as + * "ironman", which is a row the login column's collation returns for it and which the login comparison + * exists to refuse. Trimming decides only whether anybody was asserted at all, so that a REMOTE_USER of + * " ", or one that strips to nothing such as "SHIELD\\", names nobody rather than the empty login. * * @return string|null */ @@ -167,29 +173,26 @@ public static function getAssertedLogin(): ?string } if (Config::getStripDomainFromWebAuth()) { - return preg_replace('/(.*?\\\\)|(@.*)/', '', $webServerAuthUser); + $webServerAuthUser = preg_replace('/(.*?\\\\)|(@.*)/', '', $webServerAuthUser); } - return $webServerAuthUser; - } - - public static function isCurrentRequestWebServerAuthenticated(): bool - { - $auth = StaticContainer::get('Piwik\Auth'); - return $auth instanceof WebServerAuth && !empty($_SERVER['REMOTE_USER']); + return trim($webServerAuthUser) === '' ? null : $webServerAuthUser; } /** - * No password, password hash or token auth is verified against the row the asserted login resolves to, so - * unlike the other auth implementations this one requires the stored login to match it exactly. + * Returns whether the web server authenticated somebody for this request. + * + * Callers use this to skip Matomo's own password confirmation, so it has to agree with + * {@link self::getAssertedLogin()}: an assertion naming nobody authenticates nobody, and must not skip + * anything. * - * @param string $assertedLogin - * @param string $storedLogin * @return bool */ - protected function isSameLogin(string $assertedLogin, string $storedLogin): bool + public static function isCurrentRequestWebServerAuthenticated(): bool { - return UserIdentity::isSameLoginExact($assertedLogin, $storedLogin); + $auth = StaticContainer::get('Piwik\Auth'); + + return $auth instanceof WebServerAuth && self::getAssertedLogin() !== null; } private function synchronizeLoggedInUser() diff --git a/Auth/WebServerSessionAuth.php b/Auth/WebServerSessionAuth.php index f449580e..77dd0d24 100644 --- a/Auth/WebServerSessionAuth.php +++ b/Auth/WebServerSessionAuth.php @@ -14,7 +14,6 @@ use Piwik\Log\LoggerInterface; use Piwik\Plugins\LoginLdap\Config; use Piwik\Plugins\LoginLdap\UserIdentity; -use Piwik\Session; use Piwik\Session\SessionAuth; use Piwik\Session\SessionFingerprint; @@ -72,7 +71,7 @@ public function authenticate() $this->endSession(); - return new AuthResult(AuthResult::FAILURE, null, null); + return new AuthResult(AuthResult::FAILURE, '', ''); } /** @@ -92,14 +91,18 @@ private function getSessionUserToEndFor(): ?string $assertedLogin = WebServerAuth::getAssertedLogin(); // an absent REMOTE_USER is not a mismatch: setups that require web server authentication on only some - // paths would otherwise log people out at random + // paths would otherwise log people out at random. getAssertedLogin() also reports one that names + // nobody as absent, so this and WebServerAuth agree on every value. + // + // A REMOTE_USER that only differs from the session's user by surrounding whitespace is a mismatch and + // ends the session, as it would be for any other login WebServerAuth refuses to authenticate. if ($assertedLogin === null) { return null; } $sessionUser = (new SessionFingerprint())->getUser(); - if (empty($sessionUser) || UserIdentity::isSameLoginExact($assertedLogin, $sessionUser)) { + if (empty($sessionUser) || UserIdentity::isSameLogin($assertedLogin, $sessionUser)) { return null; } @@ -114,9 +117,9 @@ private function getSessionUserToEndFor(): ?string */ private function endSession(): void { - if (Session::isSessionStarted()) { - $_SESSION = array(); - } + // not gated on Session::isSessionStarted(): Session::start() returns before setting that flag when a + // session is already active, so the flag can be false while $_SESSION holds the previous user's data + $_SESSION = array(); $this->destroyCurrentSession(new SessionFingerprint()); } diff --git a/CHANGELOG.md b/CHANGELOG.md index 14a52094..a9def2a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,8 @@ # LoginLdap Changelog -#### LoginLdap 5.2.7- 2026-09-14 +#### LoginLdap 5.2.7 - 2026-09-14 - Fixed the LDAP instance identifier comparison, so an access value naming a different Matomo instance is no longer applied to this one - Fixed the login comparison so that logins the database collation treats as equal, but which are different users, are no longer accepted -- Added an exact login match requirement when authenticating through the web server - Added termination of a Matomo session when the web server starts authenticating a different user #### LoginLdap 5.2.6 - 2026-08-24 diff --git a/LoginLdap.php b/LoginLdap.php index 0a1d096a..11ea665a 100644 --- a/LoginLdap.php +++ b/LoginLdap.php @@ -275,11 +275,10 @@ public function onApiRequestDispatch(&$parameters, $pluginName, $methodName) return; } - // Only a web-server-authenticated request bypasses the password. When REMOTE_USER is - // absent, WebServerAuth delegates to its password-validating fallback, so token - // creation stays safe and must remain allowed. - $auth = StaticContainer::get('Piwik\Auth'); - if ($auth instanceof WebServerAuth && !empty($_SERVER['REMOTE_USER'])) { + // Only a web-server-authenticated request bypasses the password. When the web server authenticated + // nobody, WebServerAuth delegates to its password-validating fallback, so token creation stays safe + // and must remain allowed. + if (WebServerAuth::isCurrentRequestWebServerAuthenticated()) { throw new Exception(Piwik::translate('LoginLdap_CreateAppSpecificTokenAuthBlocked')); } } diff --git a/UserIdentity.php b/UserIdentity.php index 99f132a9..d37f46e3 100644 --- a/UserIdentity.php +++ b/UserIdentity.php @@ -32,21 +32,6 @@ public static function isSameLogin(string $assertedLogin, string $storedLogin): return self::asciiLower($assertedLogin) === self::asciiLower($storedLogin); } - /** - * Returns true if the asserted login and the stored login are byte for byte identical. - * - * Used where the login is the only thing binding a request to an account, so that not even ASCII case - * differences are tolerated. - * - * @param string $assertedLogin The login the web server asserted. - * @param string $storedLogin The login of the user row, or of the session, it was matched against. - * @return bool - */ - public static function isSameLoginExact(string $assertedLogin, string $storedLogin): bool - { - return $assertedLogin === $storedLogin; - } - /** * Lowercases the ASCII letters in $value and leaves every other byte untouched. * diff --git a/tests/Integration/WebServerAuthTest.php b/tests/Integration/WebServerAuthTest.php index cb1da86e..0169fd5a 100644 --- a/tests/Integration/WebServerAuthTest.php +++ b/tests/Integration/WebServerAuthTest.php @@ -91,11 +91,39 @@ public function test_WebServerAuth_Fails_IfAssertedLoginResolvesToDifferentExist public function getLoginsResolvingToTheSuperUser() { return array( - 'ascii case difference' => array(ucfirst(self::TEST_SUPERUSER_LOGIN)), 'accented character' => array(substr(self::TEST_SUPERUSER_LOGIN, 0, -1) . "\xc3\xa1"), ); } + public function test_WebServerAuth_Works_IfAssertedLoginDiffersFromTheStoredOneOnlyByAsciiCase() + { + Config::getInstance()->LoginLdap['use_webserver_auth'] = 1; + + $_SERVER['REMOTE_USER'] = strtoupper(self::TEST_SUPERUSER_LOGIN); + + $ldapAuth = WebServerAuth::makeConfigured(); + $authResult = $ldapAuth->authenticate(); + + $this->assertEquals(AuthResult::SUCCESS_SUPERUSER_AUTH_CODE, $authResult->getCode()); + $this->assertEquals(self::TEST_SUPERUSER_LOGIN, $authResult->getIdentity()); + } + + public function test_WebServerAuth_Works_OnEveryRequest_IfTheUserIsProvisionedFromADifferentlyCasedLogin() + { + Config::getInstance()->LoginLdap['use_webserver_auth'] = 1; + + // the Matomo user does not exist yet, so it is created from the LDAP entry's own casing. Every later + // request asserts the login the web server has, which must keep resolving to that user. + $_SERVER['REMOTE_USER'] = strtoupper(self::TEST_LOGIN); + + foreach (array('first', 'second') as $request) { + $authResult = WebServerAuth::makeConfigured()->authenticate(); + + $this->assertEquals(AuthResult::SUCCESS, $authResult->getCode(), "failed on the {$request} request"); + $this->assertEquals(self::TEST_LOGIN, $authResult->getIdentity()); + } + } + public function test_WebServerAuth_Fails_IfUserIsNotPartOfRequiredGroup() { Config::getInstance()->LoginLdap['use_webserver_auth'] = 1; diff --git a/tests/Unit/WebServerAuthAssertionTest.php b/tests/Unit/WebServerAuthAssertionTest.php new file mode 100644 index 00000000..bbfb4210 --- /dev/null +++ b/tests/Unit/WebServerAuthAssertionTest.php @@ -0,0 +1,118 @@ +configBackup = Config::getInstance()->LoginLdap; + $this->authBackup = StaticContainer::get('Piwik\Auth'); + + Config::getInstance()->LoginLdap = array( + 'use_webserver_auth' => 1, + 'strip_domain_from_web_auth' => 1, + ); + + StaticContainer::getContainer()->set('Piwik\Auth', new WebServerAuth($this->createMock(LoggerInterface::class))); + } + + public function tearDown(): void + { + unset($_SERVER['REMOTE_USER']); + StaticContainer::getContainer()->set('Piwik\Auth', $this->authBackup); + Config::getInstance()->LoginLdap = $this->configBackup; + + parent::tearDown(); + } + + /** + * @dataProvider getAssertionsNamingNobody + */ + public function test_getAssertedLogin_ReturnsNull_IfTheAssertionNamesNobody($remoteUser) + { + $_SERVER['REMOTE_USER'] = $remoteUser; + + $this->assertNull(WebServerAuth::getAssertedLogin()); + } + + /** + * Callers skip Matomo's password confirmation on the strength of this, so an assertion that authenticates + * nobody must not report the request as web server authenticated. + * + * @dataProvider getAssertionsNamingNobody + */ + public function test_isCurrentRequestWebServerAuthenticated_IsFalse_IfTheAssertionNamesNobody($remoteUser) + { + $_SERVER['REMOTE_USER'] = $remoteUser; + + $this->assertFalse(WebServerAuth::isCurrentRequestWebServerAuthenticated()); + } + + public function getAssertionsNamingNobody() + { + return array( + 'absent' => array(null), + 'empty' => array(''), + 'whitespace only' => array(' '), + 'domain only' => array('SHIELD\\'), + 'at sign only' => array('@shield.org'), + ); + } + + /** + * @dataProvider getAssertionsNamingSomebody + */ + public function test_getAssertedLogin_ReturnsTheLoginUntrimmed($expected, $remoteUser) + { + $_SERVER['REMOTE_USER'] = $remoteUser; + + $this->assertSame($expected, WebServerAuth::getAssertedLogin()); + $this->assertTrue(WebServerAuth::isCurrentRequestWebServerAuthenticated()); + } + + public function getAssertionsNamingSomebody() + { + return array( + 'plain' => array('ironman', 'ironman'), + 'domain stripped' => array('ironman', 'SHIELD\\ironman'), + 'suffix stripped' => array('ironman', 'ironman@shield.org'), + + // surrounding whitespace is part of the asserted identity, not noise to be cleaned up: the login + // column's collation returns 'ironman' for 'ironman ', and trimming would authenticate that user + 'trailing space kept' => array('ironman ', 'ironman '), + 'leading space kept' => array(' ironman', ' ironman'), + ); + } +} diff --git a/tests/Unit/WebServerAuthLoginResolutionTest.php b/tests/Unit/WebServerAuthLoginResolutionTest.php new file mode 100644 index 00000000..203feb4e --- /dev/null +++ b/tests/Unit/WebServerAuthLoginResolutionTest.php @@ -0,0 +1,143 @@ +configBackup = Config::getInstance()->LoginLdap; + Config::getInstance()->LoginLdap = array( + 'use_webserver_auth' => 1, + 'strip_domain_from_web_auth' => 0, + ); + } + + public function tearDown(): void + { + unset($_SERVER['REMOTE_USER']); + Config::getInstance()->LoginLdap = $this->configBackup; + + parent::tearDown(); + } + + /** + * The Matomo user does not exist yet, so synchronization creates it from the LDAP entry's own casing. + * Every later request asserts whatever the web server has, which has to keep resolving to that user. + */ + public function test_authenticate_Succeeds_OnEveryRequest_IfTheUserWasProvisionedFromADifferentlyCasedLogin() + { + $_SERVER['REMOTE_USER'] = strtoupper(self::LDAP_LOGIN); + + $provisioning = $this->makeAuth($existingUser = array()); + $this->assertEquals(AuthResult::SUCCESS, $provisioning->authenticate()->getCode()); + + $laterRequest = $this->makeAuth($this->makeUserRow()); + $result = $laterRequest->authenticate(); + + $this->assertEquals(AuthResult::SUCCESS, $result->getCode()); + $this->assertEquals(self::LDAP_LOGIN, $result->getIdentity()); + } + + public function test_authenticate_Succeeds_IfAssertedLoginDiffersFromTheStoredOneOnlyByAsciiCase() + { + $_SERVER['REMOTE_USER'] = 'IronMan'; + + $result = $this->makeAuth($this->makeUserRow())->authenticate(); + + $this->assertEquals(AuthResult::SUCCESS, $result->getCode()); + $this->assertEquals(self::LDAP_LOGIN, $result->getIdentity()); + } + + /** + * @dataProvider getLoginsResolvingToADifferentUser + */ + public function test_authenticate_Fails_IfAssertedLoginResolvesToADifferentUser($remoteUser) + { + $_SERVER['REMOTE_USER'] = $remoteUser; + + $result = $this->makeAuth($this->makeUserRow())->authenticate(); + + $this->assertEquals(AuthResult::FAILURE, $result->getCode()); + } + + public function getLoginsResolvingToADifferentUser() + { + // the login column's collation returns the stored user for each of these, but they are not it + return array( + 'accented character' => array("ironm\xc3\xa1n"), + 'kelvin sign' => array("\xe2\x84\xaaironman"), + 'a different user' => array('thanos'), + + // PAD SPACE collations return the stored user for these, so they must not be trimmed into it + 'trailing space' => array(self::LDAP_LOGIN . ' '), + 'leading space' => array(' ' . self::LDAP_LOGIN), + ); + } + + private function makeUserRow() + { + return array('login' => self::LDAP_LOGIN, 'superuser_access' => 0, 'password' => 'whatever'); + } + + /** + * @param array $existingUser what UserModel::getUser() returns for the asserted login + */ + private function makeAuth($existingUser) + { + $auth = new WebServerAuth($this->createMock(LoggerInterface::class)); + + $usersModel = $this->getMockBuilder(UserModel::class) + ->onlyMethods(array('getUser', 'generateRandomTokenAuth')) + ->getMock(); + $usersModel->method('getUser')->willReturn($existingUser); + $usersModel->method('generateRandomTokenAuth')->willReturn('atoken'); + $auth->setUsersModel($usersModel); + + $ldapUsers = $this->getMockBuilder(LdapUsers::class) + ->disableOriginalConstructor() + ->onlyMethods(array('getUser')) + ->getMock(); + $ldapUsers->method('getUser')->willReturn(array('uid' => array(self::LDAP_LOGIN))); + $auth->setLdapUsers($ldapUsers); + + $synchronizer = $this->getMockBuilder(UserSynchronizer::class) + ->onlyMethods(array('synchronizeLdapUser', 'synchronizePiwikAccessFromLdap')) + ->getMock(); + $synchronizer->method('synchronizeLdapUser')->willReturn($this->makeUserRow()); + $auth->setUserSynchronizer($synchronizer); + + return $auth; + } +} diff --git a/tests/Unit/WebServerSessionAuthTest.php b/tests/Unit/WebServerSessionAuthTest.php index 55db424e..dea9a0d3 100644 --- a/tests/Unit/WebServerSessionAuthTest.php +++ b/tests/Unit/WebServerSessionAuthTest.php @@ -26,6 +26,7 @@ class WebServerSessionAuthTest extends TestCase { private const SESSION_USER = 'karen'; + private const OTHER_SESSION_NAMESPACE = 'Piwik_Login'; /** * @var array @@ -49,7 +50,12 @@ public function setUp(): void 'strip_domain_from_web_auth' => 0, ); - $_SESSION = array(SessionFingerprint::USER_NAME_SESSION_VAR_NAME => self::SESSION_USER); + $_SESSION = array( + SessionFingerprint::USER_NAME_SESSION_VAR_NAME => self::SESSION_USER, + // a namespace belonging to the session's user that SessionFingerprint::clear() does not touch, + // so that the tests can tell the wipe in endSession() from clear() emptying the array by itself + self::OTHER_SESSION_NAMESPACE => array('redirectParams' => 'whatever'), + ); } public function tearDown(): void @@ -86,6 +92,7 @@ public function test_authenticate_UsesTheSessionAuth_IfTheWebServerAssertsNobody public function test_authenticate_UsesTheSessionAuth_IfThereIsNoSessionYet() { $_SESSION = array(); + $_SERVER['REMOTE_USER'] = 'someoneelse'; $result = $this->makeAuth($this->makeWrappedAuth($isCalled = true))->authenticate(); @@ -115,7 +122,7 @@ public function test_authenticate_EndsTheSession_IfTheWebServerAssertsSomebodyEl $result = $this->makeAuth($this->makeWrappedAuth($isCalled = false))->authenticate(); $this->assertEquals(AuthResult::FAILURE, $result->getCode()); - $this->assertNull($result->getIdentity()); + $this->assertSame('', $result->getIdentity()); $this->assertSessionWasEnded(); } @@ -124,10 +131,50 @@ public function getLoginsThatAreNotTheSessionUser() return array( 'a different user' => array('bob'), - // the session guard uses the same exact comparison WebServerAuth uses - 'ascii case difference' => array('Karen'), + // the session guard uses the same comparison WebServerAuth uses, so a session it keeps is always + // one WebServerAuth would authenticate 'accented character' => array("\xc3\xa1aren"), 'kelvin sign' => array("\xe2\x84\xaaaren"), + + // not trimmed into the session's user, for the same reason WebServerAuth will not authenticate it + 'surrounding whitespace' => array(' ' . self::SESSION_USER . ' '), + ); + } + + public function test_authenticate_UsesTheSessionAuth_IfTheAssertedLoginDiffersOnlyByAsciiCase() + { + $_SERVER['REMOTE_USER'] = ucfirst(self::SESSION_USER); + + $result = $this->makeAuth($this->makeWrappedAuth($isCalled = true))->authenticate(); + + $this->assertEquals(AuthResult::SUCCESS, $result->getCode()); + $this->assertSessionWasKept(); + } + + /** + * @dataProvider getRemoteUsersThatStripToNothing + */ + public function test_authenticate_UsesTheSessionAuth_IfTheAssertedLoginStripsToNothing($remoteUser) + { + Config::getInstance()->LoginLdap = array( + 'use_webserver_auth' => 1, + 'strip_domain_from_web_auth' => 1, + ); + + $_SERVER['REMOTE_USER'] = $remoteUser; + + $result = $this->makeAuth($this->makeWrappedAuth($isCalled = true))->authenticate(); + + $this->assertEquals(AuthResult::SUCCESS, $result->getCode()); + $this->assertSessionWasKept(); + } + + public function getRemoteUsersThatStripToNothing() + { + return array( + 'domain only' => array('SHIELD\\'), + 'at sign only' => array('@shield.org'), + 'whitespace only' => array(' '), ); } @@ -159,11 +206,13 @@ public function test_authenticate_EndsTheSession_IfTheDomainIsNotStripped() private function assertSessionWasKept() { $this->assertEquals(self::SESSION_USER, (new SessionFingerprint())->getUser()); + $this->assertArrayHasKey(self::OTHER_SESSION_NAMESPACE, $_SESSION); } private function assertSessionWasEnded() { $this->assertNull((new SessionFingerprint())->getUser()); + $this->assertArrayNotHasKey(self::OTHER_SESSION_NAMESPACE, $_SESSION); $this->assertEquals(array(), $_SESSION); }