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(); +});