fix(login): prevent user enumeration via differential password reset UI

[DeepDiver1975]

Summary

• showLoginForm() showed different UI for LDAP users (no "Reset it?" link) vs non-existent users, creating an unauthenticated oracle to enumerate LDAP users
• Fix removes the canChangePassword() backend check; canResetPassword is always true unless lost_password_link=disabled
• Response is now identical for LDAP users, standard users, and non-existent users

Security Impact

Low — user enumeration limited to backends without password-change support (e.g. LDAP); requires login attempts

Note

This PR touches the same files as security/fix-login-brute-force — merge that one first.

Test plan

• testShowLoginFormCanResetPasswordUniformForNonExistentUser and updated existing tests assert canResetPassword=true regardless of backend type; fail without fix
• Run make test TEST_PHP_SUITE=core/Controller

🤖 Generated with Claude Code

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[DeepDiver1975]

Code Review — fix(login): prevent user enumeration via differential password reset UI

Overview: Removes the per-user canChangePassword() check that caused the "Reset it?" link to appear only for valid LDAP accounts, leaking user existence to unauthenticated callers. The reset link now shows unconditionally (unless lost_password_link is set to disabled).

Correctness

The fix is correct and complete. The key change is removing the if ($userObj instanceof IUser) { $parameters['canResetPassword'] = $userObj->canChangePassword(); } block, so canResetPassword defaults to true for all users regardless of existence or backend type. This collapses the differential response.

The loginName logic refactor is also correct: setting $parameters['loginName'] = $user unconditionally first and then overriding it only when $u->getUserName() returns a non-empty string is cleaner and logically equivalent to the previous two-step check.

Tests

Two new tests are well-targeted:

• testShowLoginFormWithPasswordResetOptionAlwaysTrueWhenNoLostPasswordLink — verifies canChangePassword is never called and canResetPassword is always true for a valid user
• testShowLoginFormCanResetPasswordUniformForNonExistentUser — explicitly tests the enumeration case (non-existent user must yield same canResetPassword=true as a valid user)

Both testShowLoginFormForUserNamedNull updates are correct (same fix applied).

One minor note

The $parameters['user_autofocus'] = false line remains inside the if ($u !== null && ...) branch. This means user_autofocus is only set to false when a valid user with a non-empty username is found; for non-existent users it falls through to the else branch where user_autofocus is not set. This was the pre-existing behaviour and is not changed by this PR — not an issue introduced here.

Summary

Aspect	Assessment
Security fix	✅ Correct — differential response eliminated
Tests	✅ Both enumeration cases explicitly covered
Regression risk	Low — only the reset-link visibility changes

Verdict: Ready to merge.

[DeepDiver1975]

Code Review — fix(login): prevent user enumeration via differential password reset UI (updated)

Overview: This is a significant rewrite of the PR compared to the previously-reviewed version. It now includes a full diff of LoginController.php with the user-enumeration fix and also cleans up a duplicate loginName assignment block.

User enumeration fix

Before: canResetPassword was set to false when a valid user existed on a backend where canChangePassword() returned false (e.g. LDAP). This made login-form responses distinguishable: LDAP user → no reset link, non-existent user → no reset link but for different reasons. In practice the original code set canResetPassword based on $userObj->canChangePassword(), which leaked the existence of a user on backends that don't support password change.

After: canResetPassword is unconditionally true unless lost_password_link === 'disabled'. The backend capability check is removed entirely. ✅

loginName resolution cleanup

The PR also removes a duplicated loginName/user_autofocus assignment block that appeared after the alt_login section and was clobbering the LDAP username resolution:

// Removed — this block was overwriting $parameters['loginName'] back to
// the raw $user value after the UUID→displayName resolution above.
if ($user !== null && $user !== '') {
    $parameters['loginName'] = $user;
    ...
}

The earlier block correctly resolves $u->getUserName() when the user object exists; the later redundant block was resetting it. Removal is correct. ✅

Test changes

testShowLoginFormWithPasswordResetOptionAlwaysTrueWhenNoLostPasswordLink:

• Asserts canChangePassword is never called ✅
• Asserts userManager->get() called exactly once (not twice as before) ✅
• Asserts canResetPassword is always true ✅

testShowLoginFormLdapUsernameResolutionNotClobbered:

• New test: passes a UUID internal username, asserts loginName resolves to getUserName() ('john.doe'), not the raw UUID ✅
• Directly tests the removed duplicate-block regression ✅

testShowLoginFormCanResetPasswordUniformForNonExistentUser:

• New test: userManager->get() returns null (non-existent user), asserts canResetPassword === true ✅
• This is the key regression guard for the user enumeration fix ✅

testShowLoginFormForUserNamedNull:

• Updated: canChangePassword no longer called, canResetPassword now true ✅

Summary

Aspect	Assessment
User enumeration closed	✅ Backend capability no longer consulted
LDAP name resolution	✅ Duplicate clobbering block removed
Non-existent user indistinguishable	✅ Both paths → canResetPassword=true
Tests	✅ Three new/updated tests directly assert the fix
Changelog	✅ Present

Verdict: Ready to merge.

[DeepDiver1975]

Code Review

Overview: Security fix for user enumeration via the login form's password-reset link, plus a bonus fix for LDAP display-name resolution being clobbered by a duplicate code block.

Security fix — canResetPassword always true when no custom link configured:

Previously, showLoginForm() called $userObj->canChangePassword() to decide whether to show the "Reset password?" link. This meant:

• Existing LDAP user with canChangePassword() = false → no reset link
• Non-existent user → no reset link
• Local user → reset link shown

An attacker could probe usernames and distinguish "LDAP user exists" from "user does not exist" because both produced no reset link — but a local user did. More precisely, the differential was exploitable against any backend where canChangePassword() returns false, enabling account enumeration without authentication.

The fix: when lost_password_link is not set, canResetPassword is unconditionally true. The reset link is always shown, making the UI indistinguishable regardless of whether the user exists or which backend they're on. When lost_password_link is 'disabled', it remains false as before.

LDAP display-name clobbering fix:

A duplicate loginName assignment block at the bottom of showLoginForm() (after the alt_login section) was overwriting the getUserName()-resolved display name with the raw input. This meant LDAP users with internal UUID usernames had the UUID reapplied after the resolution. The duplicate block is removed.

Test changes:

• testShowLoginFormWithPasswordResetOption — refactored from a data-provider test (true/false) to a single test asserting canResetPassword is always true; canChangePassword is asserted never() called
• testShowLoginFormLdapUsernameResolutionNotClobbered — new regression test for the display-name fix
• testShowLoginFormCanResetPasswordUniformForNonExistentUser — new test asserting null-user path also yields canResetPassword=true
• testShowLoginFormForUserNamedNull — updated to reflect canChangePassword no longer called, canResetPassword now true

Test coverage is thorough and the assertions directly guard both the security property and the regression fix. No concerns.