From 3e24ccd42f50e13547bc99850dfdd9fd77fd8cc6 Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Sat, 29 Aug 2026 00:10:59 +0200 Subject: [PATCH] Let the confirm-password screen ask where the password lives ConfirmablePasswordController checked the local hash and nothing else: Auth::guard('web')->validate(['email' => ..., 'password' => ...]) An account provisioned from a directory has no local password. It holds a Str::password(64) generated at provisioning time that nobody has ever seen, and the application knows this -- LdapAuthenticator::isDirectoryAccount() is the question, and the sign-in form asks it before deciding what to check. This screen did not, so it refused those accounts the only password they have. That is not a cosmetic refusal. `password.confirm` stands in front of enrolling in two-factor, so a directory-provisioned client could not enrol at all. Set TwoFactorEnforcement to `clients` or `all` and EnforceTwoFactor redirects every request they make to two-factor.show -- a screen whose "enable" button leads to a door they cannot open. PR #1708 fixed the routing half of that ("Let an enforced user reach the far side of the confirm-password screen"); this is the credential half. The rule now lives in one place. PasswordVerification is the sibling of SignIn on the other side of the line SignIn draws -- SignIn is everything after a credential checks out, this is the one question asked before it -- and it exists for the reason SignIn gives for existing: "the way they get broken is by being written twice". LoginRequest keeps its ordering, its provisioning and its rate limiting, and delegates the check itself. Behaviour preserved exactly on the sign-in path: local hash first so an account that answers locally generates no directory traffic, directory only for accounts whose credentials live there, the stale-hash re-hash on the local branch only, and the ldap_dn stamp on the directory branch. All 23 existing LDAP sign-in tests pass unchanged. One thing this closes on the way past. Because the old check went straight to the local hash, a directory account's placeholder *would* have confirmed if anybody ever learned it -- a door the sign-in form does not have, since it skips the local branch for those accounts. It now behaves the same on both screens; there is a test. **What this does not fix, and should be read as a limitation.** Accounts provisioned by a social provider are in the same position -- a random local password nobody holds -- and they are not directory accounts, so this changes nothing for them. Their route to a local password is the password reset, which #1748 made work end to end by moving auth_source to Local when the reset completes. A social account that has never done that still cannot confirm a password, and so still cannot enrol in two-factor. Tests: three fail against the unfixed pair, including the placeholder case above. Two more pin what must not change -- a wrong directory password is still refused, and a local account with LDAP switched on still confirms against its own hash. --- .../Auth/ConfirmablePasswordController.php | 17 ++-- app/Http/Requests/Auth/LoginRequest.php | 74 ++------------- app/Modules/Identity/PasswordVerification.php | 94 +++++++++++++++++++ tests/Feature/Auth/LdapAuthenticationTest.php | 93 ++++++++++++++++++ 4 files changed, 205 insertions(+), 73 deletions(-) create mode 100644 app/Modules/Identity/PasswordVerification.php diff --git a/app/Http/Controllers/Auth/ConfirmablePasswordController.php b/app/Http/Controllers/Auth/ConfirmablePasswordController.php index bd2db422..dbeb4efb 100644 --- a/app/Http/Controllers/Auth/ConfirmablePasswordController.php +++ b/app/Http/Controllers/Auth/ConfirmablePasswordController.php @@ -3,9 +3,9 @@ namespace App\Http\Controllers\Auth; use App\Http\Controllers\Controller; +use App\Modules\Identity\PasswordVerification; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; -use Illuminate\Support\Facades\Auth; use Illuminate\Validation\ValidationException; use Inertia\Inertia; use Inertia\Response; @@ -22,16 +22,21 @@ class ConfirmablePasswordController extends Controller /** * Confirm the user's password. + * + * Through PasswordVerification, so this asks the same question the + * sign-in form asks: is this the account's password, from wherever + * that account's password lives. Checking only the local hash refused + * every directory-provisioned account the password it actually has -- + * their local hash is a Str::password(64) nobody has ever seen -- and + * this screen stands in front of enrolling in two-factor, so those + * accounts could not enrol at all. */ - public function store(Request $request): RedirectResponse + public function store(Request $request, PasswordVerification $passwords): RedirectResponse { $user = $request->user(); assert($user !== null); - if (! Auth::guard('web')->validate([ - 'email' => $user->email, - 'password' => $request->password, - ])) { + if (! $passwords->verify($user, (string) $request->string('password'))) { throw ValidationException::withMessages([ 'password' => __('auth.password'), ]); diff --git a/app/Http/Requests/Auth/LoginRequest.php b/app/Http/Requests/Auth/LoginRequest.php index 3e6b1e87..dc5876d2 100644 --- a/app/Http/Requests/Auth/LoginRequest.php +++ b/app/Http/Requests/Auth/LoginRequest.php @@ -3,13 +3,12 @@ namespace App\Http\Requests\Auth; use App\Models\User; -use App\Modules\Identity\Ldap\LdapAuthenticator; use App\Modules\Identity\Ldap\LdapProvisioner; +use App\Modules\Identity\PasswordVerification; use App\Modules\Identity\SignIn; use App\Modules\Platform\Captcha\CaptchaForm; use App\Support\Rules; use Illuminate\Auth\Events\Lockout; -use Illuminate\Auth\SessionGuard; use Illuminate\Contracts\Validation\ValidationRule; use Illuminate\Foundation\Http\FormRequest; use Illuminate\Support\Facades\Auth; @@ -115,10 +114,9 @@ class LoginRequest extends FormRequest /** * The account whose password checks out, or null. * - * The local hash is tried first and the directory only on failure, so - * a login that succeeds locally never generates directory traffic. - * The exception is an account whose credentials are known to live in - * the directory, where the local hash is a placeholder nobody holds. + * The rule itself -- local hash first, directory when the credentials + * live there -- is PasswordVerification's, because this is no longer + * the only screen that has to ask it. See that class. */ private function verifyCredentials(?User $user): ?User { @@ -126,67 +124,9 @@ class LoginRequest extends FormRequest return null; } - $ldap = app(LdapAuthenticator::class); - - if (! $ldap->isDirectoryAccount($user) - && Auth::validate($this->only('email', 'password'))) { - $this->upgradeHashIfStale($user); - - return $user; - } - - $identity = $ldap->attempt( - (string) $this->string('email'), - (string) $this->string('password'), - $user, - ); - - if ($identity === null) { - return null; - } - - $ldap->stamp($user, $identity); - - return $user; - } - - /** - * Re-hash a password stored under weaker settings than this - * installation now uses. - * - * Laravel does this for you inside SessionGuard::attempt(), but this - * form does not use attempt() — it verifies with Auth::validate() and - * hands the account to SignIn, which calls Auth::login(). Neither - * re-hashes, so without this an account keeps whatever cost it was - * created under forever, and raising BCRYPT_ROUNDS would quietly - * apply to new accounts only. - * - * That is not hypothetical: every account the v1 migration carries - * across arrives as `$2y$08$…`, because v1 hashed at cost 8, and - * would otherwise stay four times cheaper to attack than an account - * created here. - * - * **Only ever called on the local branch.** On the directory branch - * the submitted plaintext is the *LDAP* password and the local hash - * is a `Str::password(64)` placeholder nobody holds; writing the - * directory credential into it would mint a second way into the - * account that keeps working after LDAP is switched off. - */ - private function upgradeHashIfStale(User $user): void - { - $guard = Auth::guard('web'); - - // getProvider() is on SessionGuard rather than on the StatefulGuard - // contract. This guard is a SessionGuard in every configuration this - // application ships; the check is here so a custom driver degrades - // to "no re-hash" instead of a fatal on the login path. - if (! $guard instanceof SessionGuard) { - return; - } - - // No-ops unless the hasher says the stored digest needs it, so - // this costs an already-current account nothing. - $guard->getProvider()->rehashPasswordIfRequired($user, $this->only('password')); + return app(PasswordVerification::class)->verify($user, (string) $this->string('password')) + ? $user + : null; } /** diff --git a/app/Modules/Identity/PasswordVerification.php b/app/Modules/Identity/PasswordVerification.php new file mode 100644 index 00000000..4625f04c --- /dev/null +++ b/app/Modules/Identity/PasswordVerification.php @@ -0,0 +1,94 @@ +ldap->isDirectoryAccount($user) + && Auth::guard('web')->validate(['email' => $user->email, 'password' => $password])) { + $this->rehashIfStale($user, $password); + + return true; + } + + $identity = $this->ldap->attempt($user->email, $password, $user); + + if ($identity === null) { + return false; + } + + $this->ldap->stamp($user, $identity); + + return true; + } + + /** + * Re-hash a password stored under weaker settings than this + * installation now uses. + * + * Laravel does this inside SessionGuard::attempt(), which neither + * caller uses -- they verify and then hand the account to SignIn, + * which calls Auth::login(). Neither re-hashes, so without this an + * account keeps whatever cost it was created under forever, and + * raising BCRYPT_ROUNDS would quietly apply to new accounts only. + * + * That is not hypothetical: every account the v1 migration carries + * across arrives as `$2y$08$…`, because v1 hashed at cost 8, and would + * otherwise stay four times cheaper to attack than an account created + * here. + * + * **Only ever called on the local branch.** On the directory branch the + * submitted plaintext is the *LDAP* password and the local hash is a + * placeholder nobody holds; writing the directory credential into it + * would mint a second way into the account that keeps working after + * LDAP is switched off. + */ + private function rehashIfStale(User $user, string $password): void + { + $guard = Auth::guard('web'); + + // getProvider() is on SessionGuard rather than on the StatefulGuard + // contract. This guard is a SessionGuard in every configuration this + // application ships; the check is here so a custom driver degrades + // to "no re-hash" instead of a fatal on the login path. + if (! $guard instanceof SessionGuard) { + return; + } + + // No-ops unless the hasher says the stored digest needs it, so this + // costs an already-current account nothing. + $guard->getProvider()->rehashPasswordIfRequired($user, ['password' => $password]); + } +} diff --git a/tests/Feature/Auth/LdapAuthenticationTest.php b/tests/Feature/Auth/LdapAuthenticationTest.php index 845041ec..315113d7 100644 --- a/tests/Feature/Auth/LdapAuthenticationTest.php +++ b/tests/Feature/Auth/LdapAuthenticationTest.php @@ -15,6 +15,7 @@ use App\Modules\Platform\Capabilities\Edition; use App\Modules\Platform\Settings\Setting; use App\Modules\Platform\Settings\Settings; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\Hash; use Tests\Support\FakeLdapDirectory; beforeEach(function () { @@ -419,3 +420,95 @@ test('settings with no host are treated as switched off', function () { expect($fake->calls)->toBe(0); }); + +/* +|-------------------------------------------------------------------------- +| Re-proving the password, once signed in +|-------------------------------------------------------------------------- +| The confirm-password screen asked the local hash and nothing else. A +| directory account's local hash is a Str::password(64) nobody has ever +| seen, so it refused them the only password they have -- and that screen +| stands in front of enrolling in two-factor. +*/ + +test('a directory account can confirm its password with its directory password', function () { + fakeDirectory(['someone@example.test' => ['password' => 'directory-pass']]); + enableLdap(); + + $client = User::factory()->client()->create([ + 'email' => 'someone@example.test', + 'auth_source' => AuthSource::Ldap, + ]); + + $this->actingAs($client) + ->post('/confirm-password', ['password' => 'directory-pass']) + ->assertRedirect() + ->assertSessionHasNoErrors(); + + expect(session()->has('auth.password_confirmed_at'))->toBeTrue(); +}); + +test('and can therefore enrol in two-factor at all', function () { + // The consequence that matters. Under TwoFactorEnforcement this is not + // a missing convenience -- EnforceTwoFactor redirects every request to + // two-factor.show, and the enrolment behind password.confirm could + // never be reached, so the account had nowhere to go. + fakeDirectory(['someone@example.test' => ['password' => 'directory-pass']]); + enableLdap(); + + $client = User::factory()->client()->create([ + 'email' => 'someone@example.test', + 'auth_source' => AuthSource::Ldap, + ]); + + $this->actingAs($client)->post('/confirm-password', ['password' => 'directory-pass']); + $this->actingAs($client)->post('/settings/two-factor')->assertRedirect(); + + expect($client->refresh()->two_factor_secret)->not->toBeNull(); +}); + +test('the wrong directory password is still refused', function () { + fakeDirectory(['someone@example.test' => ['password' => 'directory-pass']]); + enableLdap(); + + $client = User::factory()->client()->create([ + 'email' => 'someone@example.test', + 'auth_source' => AuthSource::Ldap, + ]); + + $this->actingAs($client) + ->post('/confirm-password', ['password' => 'not-the-directory-pass']) + ->assertSessionHasErrors('password'); + + expect(session()->has('auth.password_confirmed_at'))->toBeFalse(); +}); + +test('the local hash of a directory account is still not a way in', function () { + // The placeholder must stay unusable: if somebody knew it, confirming + // with it would be a second door that keeps working after LDAP is + // switched off. + fakeDirectory(['someone@example.test' => ['password' => 'directory-pass']]); + enableLdap(); + + $client = User::factory()->client()->create([ + 'email' => 'someone@example.test', + 'auth_source' => AuthSource::Ldap, + 'password' => Hash::make('the-local-placeholder'), + ]); + + $this->actingAs($client) + ->post('/confirm-password', ['password' => 'the-local-placeholder']) + ->assertSessionHasErrors('password'); +}); + +test('a local account still confirms against its own hash, with LDAP on', function () { + fakeDirectory([]); + enableLdap(); + + $staff = User::factory()->create(); + + $this->actingAs($staff) + ->post('/confirm-password', ['password' => 'password']) + ->assertRedirect() + ->assertSessionHasNoErrors(); +});