Revoke a user's own Sanctum tokens, never another model's

CHANGELOG.md

@@ -95,6 +95,7 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Fixed
 
+- **Revoking all of a user's tokens could delete another user's.** With the token issuer registry, `SanctumTokenIssuer::revokeAll()` (called on every password reset and update) queried the token table by a morph type taken from configuration rather than the user's own, so a second token-bearing model with a colliding id (an Admin with id 5) revoked User 5's tokens and kept its own. It also queried that table for user models that hold no Sanctum tokens, failing the reset where the table does not exist. It now revokes through the user's own `tokens()` relation, and only for models that use Sanctum's `HasApiTokens`, as before the registry.
 - The user-model exception named the old package.
 
 [Unreleased]: https://github.com/laranail/authkit/compare/v0.1.0...HEAD

src/Actions/SanctumTokenIssuer.php

@@ -5,9 +5,8 @@
 namespace Simtabi\Laranail\AuthKit\Actions;
 
 use DateTimeInterface;
-use Laravel\Sanctum\Sanctum;
 use InvalidArgumentException;
-use Illuminate\Database\Eloquent\Model;
+use Laravel\Sanctum\HasApiTokens;
 use Illuminate\Contracts\Auth\Authenticatable;
 use Simtabi\Laranail\AuthKit\Support\TokenResult;
 use Simtabi\Laranail\AuthKit\Contracts\TokenIssuerInterface;
@@ -50,27 +49,32 @@ public function revokeCurrent(Authenticatable $user): void
         }
     }
 
+    /**
+     * Revoke every Sanctum token the given user holds, through the user's own `tokens()` relation.
+     *
+     * Only a model that uses Sanctum's `HasApiTokens` can hold one. Querying the token table by a
+     * morph type taken from configuration instead deleted another model's tokens whenever ids
+     * collided (an Admin with id 5 revoked User 5's tokens and kept its own), and a model without
+     * the trait made every password reset hit a token table the application may not have.
+     */
     public function revokeAll(Authenticatable $user): void
     {
-        if (! $user instanceof Model) {
+        if (! in_array(HasApiTokens::class, class_uses_recursive($user), true)) {
             return;
         }
 
-        $model = Sanctum::$personalAccessTokenModel;
-        $morphType = $user->getMorphClass();
-        $userModel = config('laranail.authkit.user_model') ?? config('auth.providers.users.model');
-
-        if (is_string($userModel) && class_exists($userModel) && is_subclass_of($userModel, Model::class)) {
-            $morphType = (new $userModel)->getMorphClass();
-        }
-
-        $model::query()
-            ->where('tokenable_type', $morphType)
-            ->where('tokenable_id', $user->getAuthIdentifier())
-            ->delete();
+        $user->tokens()->delete();
     }
 
-    /** @return array<int, string> */
+    /**
+     * Both defaults come from configuration rather than being fixed here, because both were once
+     * fixed here in the least safe way available: every token was minted with the wildcard ability
+     * `*` and no expiry. A wildcard token can do anything its owner can, so a leaked one is a full
+     * account compromise; a token with no expiry recovered from a log or an old backup never stops
+     * working, and Sanctum's own `sanctum.expiration` is null by default.
+     *
+     * @return array<int, string>
+     */
     private function defaultAbilities(): array
     {
         $abilities = config('laranail.authkit.tokens.abilities', ['*']);
@@ -82,6 +86,10 @@ private function defaultAbilities(): array
         return array_values(array_filter($abilities, is_string(...)));
     }
 
+    /**
+     * A null lifetime defers to Sanctum's own `sanctum.expiration`, which is the only way to opt out
+     * of expiry deliberately rather than silently inherit none.
+     */
     private function defaultExpiry(): ?DateTimeInterface
     {
         $minutes = config('laranail.authkit.tokens.expires_after_minutes');

tests/Feature/SanctumTokenIssuerTest.php

@@ -0,0 +1,73 @@
+<?php
+
+declare(strict_types=1);
+
+use Illuminate\Support\Str;
+use Workbench\App\Models\User;
+use Illuminate\Support\Facades\Hash;
+use Illuminate\Support\Facades\Schema;
+use Simtabi\Laranail\AuthKit\Actions\ResetUserPassword;
+use Simtabi\Laranail\AuthKit\Actions\SanctumTokenIssuer;
+use Illuminate\Foundation\Auth\User as PlainAuthenticatable;
+
+/*
+ * revokeAll() runs on every password reset and update. It must revoke exactly the tokens of the
+ * user it was given: never another model's tokens that share the id, and never fail for a user
+ * model that does not issue Sanctum tokens at all.
+ */
+
+/** A second token-bearing model over the same table, as an Admin model would be. */
+final class SanctumIssuerTestAdmin extends User
+{
+    protected $table = 'users';
+
+    public function getMorphClass(): string
+    {
+        return 'sanctum-issuer-test-admin';
+    }
+}
+
+/** A user model with no Sanctum tokens: no HasApiTokens, so no tokens() relation. */
+final class SanctumIssuerTestPlainUser extends PlainAuthenticatable
+{
+    protected $table = 'users';
+}
+
+it('revokes every token of the given user', function (): void {
+    $user = User::factory()->create();
+    $user->createToken('one');
+    $user->createToken('two');
+
+    app(SanctumTokenIssuer::class)->revokeAll($user);
+
+    expect($user->tokens()->count())->toBe(0);
+});
+
+it('leaves another model\'s tokens alone when the ids collide', function (): void {
+    $user = User::factory()->create();
+    $user->createToken('belongs-to-the-user');
+
+    $admin = SanctumIssuerTestAdmin::query()->findOrFail($user->getKey());
+    $admin->createToken('belongs-to-the-admin');
+
+    app(SanctumTokenIssuer::class)->revokeAll($admin);
+
+    expect($admin->tokens()->count())->toBe(0)
+        ->and($user->tokens()->count())->toBe(1);
+});
+
+it('resets the password of a user model without Sanctum tokens', function (): void {
+    $user = User::factory()->create();
+    Schema::drop('personal_access_tokens');
+
+    $plain = SanctumIssuerTestPlainUser::query()->findOrFail($user->getKey());
+
+    $password = Str::password(16);
+
+    app(ResetUserPassword::class)->reset($plain, [
+        'password'              => $password,
+        'password_confirmation' => $password,
+    ]);
+
+    expect(Hash::check($password, $plain->fresh()->password))->toBeTrue();
+});