Merge pull request #1748 from denkfabrik-li/fix/password-reset-credential-source

Two accounts reach the same reset with opposite needs, and it answered both by writing a hash and hoping.

A provider account is asked for something it cannot do. The Connected accounts screen refuses to release an account's last provider -- "Set a password first, then disconnect Google" -- and nothing set auth_source back to Local, so the screen went on asking for what had just been done, with no way out from inside the application. AuthSource already states the rule that closes it, for this case by name: a social account may later set a real password, and social only means the account came into existence without anybody choosing one. A reset by emailed token is where somebody chooses one, and the prop the screen reads is literally auth_source === Local under the name has_local_password.

A directory account is told something untrue. isDirectoryAccount() means the local hash is not consulted at all, so the same reset wrote a password that could never sign anybody in and reported success -- including when the directory it points at is gone, which is exactly the situation that sends somebody to a reset.

The reset now asks where the account's credentials live. social becomes Local, because the new password is the credential now. A directory account is refused, with the reason, and nothing about it moves -- writing Local there would not record something that had happened, it would take the account off its directory as a side effect of a password reset, which is an administrator's decision and already lives in AccountConversion with the password requirement and activity entry that belong to it. Everything else is byte for byte as before.

Verified before merging: 20 passed on the trial-merge, 2 failed / 18 passed with app/ reset, and the wider suites green -- tests/Feature/Auth 96 passed, tests/Feature/Identity 302 passed. Four properties were checked in the framework rather than argued. PasswordBroker::reset() calls validateReset() before the callback, so the refusal only reaches somebody holding a token emailed to that address and nothing is enumerable. It deletes the token after the callback, so a throw leaves the link usable. Every use of AuthSource::Local is in ConnectedAccountsController -- the has_local_password prop and the last-provider guard -- so the social-to-Local flip grants exactly the ability the screen instructs the user to obtain and nothing else, and no new login capability at all, since password login already worked for social accounts. And the check is isDirectoryAccount() rather than an auth_source comparison because LDAP is client-only, so staff are not refused; the test for that is green either way.

One new string is English only for now: "This account signs in through your directory, so its password is not set here."

Reported and fixed by @denkfabrik-li.

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

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

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