Builds the login result from the synchronized access

[AltamashShaikh]

Refs #AS-730.

Problem

Base::synchronizeLdapUser() caches the user row returned by identity synchronization, then runs access synchronization, and never reads the row back:

$this->userForLogin = $this->userSynchronizer->synchronizeLdapUser($this->login, $ldapUser);
$syncedLogin = !empty($this->userForLogin['login']) ? $this->userForLogin['login'] : $this->login;
$this->userSynchronizer->synchronizePiwikAccessFromLdap($syncedLogin, $ldapUser);

UserSynchronizer::synchronizeLdapUser() ends with return $userModel->getUser($syncLogin), which is a fresh read but taken before access synchronization. synchronizePiwikAccessFromLdap() then clears the user's access and applies what LDAP grants now. getUserForLogin() short-circuits on the cached value, so makeSuccessLogin() builds the result from the access the row held beforehand rather than the access synchronization just applied.

Access::reloadAccess() takes the request's authority from that result, so for one request the two disagree.

Change

Read the row back after access synchronization, keyed on $syncedLogin rather than $this->login — UserMapper::getExpectedLdapUsername() can append the configured email suffix, so the synchronized login is not always the asserted one.

If the user has been deleted meanwhile the re-read returns empty and makeSuccessLogin() throws User couldn't be found, which fails the login. That is the safe direction.

Scope

Not specific to web server auth. synchronizeLdapUser() is also reached from authenticateByLdap(), which LdapAuth and SynchronizedAuth both call before makeSuccessLogin($this->getUserForLogin()), so all three auth implementations are affected by the one change. The relevant setting is enable_synchronize_access_from_ldap, which defaults to 0; synchronize_users_after_login defaults to 1.

Tests

tests/Unit/WebServerAuthAccessSyncTest.php drives the real authenticate() with an access-synchronization stub that changes the flag, and asserts the result reflects the value synchronization left rather than the one read before it, in both directions. I confirmed the first case fails without the change and passes with it.

Plugin unit suite: 155 tests, 357 assertions, OK. PHPCS clean.

Note

#487 is open against the same file. It touches getUserForLogin(), isSameLogin() and makeAuthFailure(), not synchronizeLdapUser(), so the two should merge without conflict. Worth landing #487 first since it is further along.

Longer term the seam is worth closing rather than patching: Base::synchronizeLdapUser() assigning $this->userForLogin directly is also what let a synchronized row skip the login comparison in #487. Having one synchronization operation return the final post-access row would remove both hazards.

Checklist

• [✔] Tested locally or on demo2/demo3?
• [✔] New test case added/updated?
• [✔] Are all newly added texts included via translation?
• [NA] Are text sanitized properly? (Eg use of v-text v/s v-html for vue)
• [✔] Version bumped?
• [✔] I have understood, reviewed, and tested all AI outputs before use
• [✔] All AI instructions respect security, IP, and privacy rules
• [✖] Documentation updated?

[AltamashShaikh]

Fixed in f3214e5. Two separate things in that CI output:

The skipped test is unrelated. SynchronizeUsersTest::test_CommandReportsUsersThatAreNotSynchronized_WhenUserMissing_AndUserInfoBrokenInLdap calls markTestSkipped() unconditionally on its first line, and has since 69e2d9f in 2014. PHPUnit reports skips separately from failures, so it is not a check failure and nothing on this branch caused it.

The real failure was a test fixture of mine, not the product. WebServerAuthLoginResolutionTest::test_authenticate_Succeeds_OnEveryRequest_IfTheUserWasProvisionedFromADifferentlyCasedLogin came in with #487 and stubbed UserModel::getUser() with a fixed return value. In the provisioning case that value is the empty array, so the stub kept reporting "no such user" even after synchronization had created one. Reading the row back after access synchronization then found nothing and makeSuccessLogin() threw User couldn't be found.

Real synchronization creates the Matomo user, so getUser() finds it from that point on. The stub now models that: it reports no user until synchronizeLdapUser() runs and the row afterwards.

It only showed up in CI because this branch was cut before #487 merged, so my local runs did not include that test. Rebased onto the current 5.x-dev and reproduced it locally before fixing.

I checked the change did not weaken either test's coverage — reinstating the byte-exact comparison still fails 2 tests in WebServerAuthLoginResolutionTest, and removing the re-read from Base::synchronizeLdapUser() still fails WebServerAuthAccessSyncTest.

181 tests, 412 assertions locally. PHPCS clean.

[snake14]

Looks good and no issues found during functional testing. 👍