diff --git a/app/Http/Controllers/Auth/NewPasswordController.php b/app/Http/Controllers/Auth/NewPasswordController.php index 264a0e97..f1f660bb 100644 --- a/app/Http/Controllers/Auth/NewPasswordController.php +++ b/app/Http/Controllers/Auth/NewPasswordController.php @@ -3,6 +3,8 @@ namespace App\Http\Controllers\Auth; use App\Http\Controllers\Controller; +use App\Modules\Identity\AuthSource; +use App\Modules\Identity\Ldap\LdapAuthenticator; use Illuminate\Auth\Events\PasswordReset; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; @@ -16,6 +18,10 @@ use Inertia\Response; class NewPasswordController extends Controller { + public function __construct( + private readonly LdapAuthenticator $ldap, + ) {} + /** * Show the password reset page. */ @@ -46,10 +52,55 @@ class NewPasswordController extends Controller $status = Password::reset( $request->only('email', 'password', 'password_confirmation', 'token'), function ($user) use ($request) { - $user->forceFill([ + // A directory account's password lives in the directory and + // the local hash is not consulted at all, which is what + // isDirectoryAccount() means. Writing one here reported + // success and changed nothing anybody could use -- including + // when the directory it points at is gone, which is exactly + // when somebody reaches for a reset. + // + // Refused here rather than where the link is asked for: that + // endpoint answers "A reset link will be sent if the account + // exists" to everybody on purpose, and a refusal there would + // tell a stranger both that an address is an account and how + // it signs in. By this point the caller holds a token that + // was emailed to the address, so the explanation reaches the + // account holder and nobody else. + // + // Throwing before the write also leaves the token unspent: + // PasswordBroker deletes it after the callback returns, so + // the link still works if an administrator converts the + // account in the meantime. + if ($this->ldap->isDirectoryAccount($user)) { + throw ValidationException::withMessages([ + 'email' => [__('This account signs in through your directory, so its password is not set here. Ask an administrator if you cannot sign in.')], + ]); + } + + $attributes = [ 'password' => Hash::make($request->password), 'remember_token' => Str::random(60), - ])->save(); + ]; + + // `social` records that the account came into existence + // without anybody choosing a password, which AuthSource + // states outright -- along with "a social account may later + // set a real password". This is that moment, and nothing + // else in the application writes it: the Connected accounts + // screen reads `auth_source === Local` as + // `has_local_password`, so without this line its refusal + // goes on asking for a password that has just been set. + // + // The two branches of this method are the same rule read + // twice: `social` is where the account came from and the + // hash here is what signs it in, so choosing one settles it; + // `ldap` is the authentication path itself, so nothing + // chosen here settles anything. + if ($user->auth_source === AuthSource::Social) { + $attributes['auth_source'] = AuthSource::Local; + } + + $user->forceFill($attributes)->save(); event(new PasswordReset($user)); } diff --git a/tests/Feature/Auth/PasswordResetTest.php b/tests/Feature/Auth/PasswordResetTest.php index 2c520cf5..49861dc7 100644 --- a/tests/Feature/Auth/PasswordResetTest.php +++ b/tests/Feature/Auth/PasswordResetTest.php @@ -3,9 +3,12 @@ namespace Tests\Feature\Auth; use App\Models\User; +use App\Modules\Identity\AuthSource; use App\Modules\Identity\Notifications\ResetPasswordNotification; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Notification; +use Illuminate\Support\Facades\Password; use Tests\TestCase; class PasswordResetTest extends TestCase @@ -109,4 +112,56 @@ class PasswordResetTest extends TestCase return true; }); } + + /** + * The reset used to report success for an account whose password does + * not live here: isDirectoryAccount() means the local hash is never + * consulted, so the password it wrote could not sign anybody in — and + * nothing said so. Nothing about the account moves either, the source + * included: taking an account off its directory is an administrator's + * decision, not a side effect of a reset. + */ + public function test_a_directory_account_is_told_where_its_password_lives() + { + $client = User::factory()->client()->create(); + $client->forceFill([ + 'auth_source' => AuthSource::Ldap, + 'ldap_dn' => 'cn=dana,dc=test', + ])->save(); + $before = $client->password; + + $this->post('/reset-password', [ + 'token' => Password::createToken($client), + 'email' => $client->email, + 'password' => 'a-password-of-her-own', + 'password_confirmation' => 'a-password-of-her-own', + ])->assertSessionHasErrors('email'); + + $client->refresh(); + + $this->assertSame($before, $client->password); + $this->assertSame(AuthSource::Ldap, $client->auth_source); + $this->assertSame('cn=dana,dc=test', $client->ldap_dn); + } + + /** + * The half that must not be refused, and the reason the check is + * isDirectoryAccount() rather than a comparison against auth_source: + * LDAP is client-only, enforced on the account, so a staff row is + * verified against the local hash whatever its source happens to say. + */ + public function test_a_staff_account_resets_whatever_its_source_says() + { + $staff = User::factory()->create(); + $staff->forceFill(['auth_source' => AuthSource::Ldap])->save(); + + $this->post('/reset-password', [ + 'token' => Password::createToken($staff), + 'email' => $staff->email, + 'password' => 'a-password-of-his-own', + 'password_confirmation' => 'a-password-of-his-own', + ])->assertSessionHasNoErrors(); + + $this->assertTrue(Hash::check('a-password-of-his-own', $staff->fresh()->password)); + } } diff --git a/tests/Feature/Identity/ConnectedAccountsTest.php b/tests/Feature/Identity/ConnectedAccountsTest.php index 9cd360eb..0cff0d84 100644 --- a/tests/Feature/Identity/ConnectedAccountsTest.php +++ b/tests/Feature/Identity/ConnectedAccountsTest.php @@ -11,6 +11,7 @@ use App\Modules\Identity\Social\SocialGateway; use App\Modules\Identity\Social\SocialIdentity; use App\Modules\Identity\Social\SocialProvider; use App\Modules\Identity\Social\SocialSettings; +use Illuminate\Support\Facades\Password; use Illuminate\Testing\TestResponse; use Tests\Support\FakeSocialGateway; @@ -147,6 +148,30 @@ test('an account created by a provider cannot remove its last connection', funct expect(SocialAccount::query()->count())->toBe(1); }); +// And the instruction inside that refusal, followed. AuthSource says a +// social account "may later set a real password"; a reset by emailed token +// is where somebody does, and nothing else in the application records it. +test('a provider account that resets its password can then disconnect', function () { + $client = User::factory()->client()->create(); + $client->forceFill(['auth_source' => AuthSource::Social])->save(); + linkRow($client, 'sub-1'); + + $this->post(route('password.store'), [ + 'token' => Password::createToken($client), + 'email' => $client->email, + 'password' => 'a-password-of-her-own', + 'password_confirmation' => 'a-password-of-her-own', + ])->assertSessionHasNoErrors(); + + expect($client->fresh()->auth_source)->toBe(AuthSource::Local); + + $this->actingAs($client->fresh()) + ->delete(route('connected-accounts.destroy', ['provider' => 'google'])) + ->assertSessionHasNoErrors(); + + expect(SocialAccount::query()->count())->toBe(0); +}); + test('it can remove one of two connections', function () { $client = User::factory()->client()->create(); $client->forceFill(['auth_source' => AuthSource::Social])->save();