diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bdeb44..113172f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/Actions/SanctumTokenIssuer.php b/src/Actions/SanctumTokenIssuer.php index 1a25c37..b196d63 100644 --- a/src/Actions/SanctumTokenIssuer.php +++ b/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 */ + /** + * 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 + */ 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'); diff --git a/tests/Feature/SanctumTokenIssuerTest.php b/tests/Feature/SanctumTokenIssuerTest.php new file mode 100644 index 0000000..df4fd0d --- /dev/null +++ b/tests/Feature/SanctumTokenIssuerTest.php @@ -0,0 +1,73 @@ +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(); +});