From 34da7232aad236787831a5a151f62cdcee392e6a Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 19 Aug 2026 11:27:23 +0200 Subject: [PATCH] fix(api-tokens): scope token access to current team (#11396) --- app/Jobs/ApiTokenExpirationWarningJob.php | 4 ++ app/Livewire/Security/ApiTokens.php | 10 +++- app/Policies/ApiTokenPolicy.php | 21 ++++++- .../Feature/ApiTokenExpirationWarningTest.php | 28 ++++++--- .../ApiTokenLivewireAuthorizationTest.php | 36 +++++++++++ tests/Unit/Policies/ApiTokenPolicyTest.php | 59 ++++++++++++++++--- 6 files changed, 135 insertions(+), 23 deletions(-) diff --git a/app/Jobs/ApiTokenExpirationWarningJob.php b/app/Jobs/ApiTokenExpirationWarningJob.php index e7b34248e..f59b1a0b1 100644 --- a/app/Jobs/ApiTokenExpirationWarningJob.php +++ b/app/Jobs/ApiTokenExpirationWarningJob.php @@ -41,6 +41,10 @@ public function handle(): void continue; } + if (! $team->members()->whereKey($token->tokenable_id)->exists()) { + continue; + } + $warningSentAt = now(); $team->notify(new ApiTokenExpiringNotification($token)); diff --git a/app/Livewire/Security/ApiTokens.php b/app/Livewire/Security/ApiTokens.php index d6bd6e54b..5a978ac84 100644 --- a/app/Livewire/Security/ApiTokens.php +++ b/app/Livewire/Security/ApiTokens.php @@ -59,7 +59,10 @@ public function mount() private function getTokens() { - $this->tokens = auth()->user()->tokens->sortByDesc('created_at'); + $this->tokens = auth()->user()->tokens() + ->where('team_id', currentTeam()->id) + ->latest() + ->get(); } public function updatedPermissions($permissionToUpdate) @@ -148,7 +151,10 @@ public function addNewToken() public function revoke(int $id) { try { - $token = auth()->user()->tokens()->where('id', $id)->firstOrFail(); + $token = auth()->user()->tokens() + ->where('team_id', currentTeam()->id) + ->where('id', $id) + ->firstOrFail(); $this->authorize('delete', $token); $token->delete(); $this->getTokens(); diff --git a/app/Policies/ApiTokenPolicy.php b/app/Policies/ApiTokenPolicy.php index ba9bade01..e9ef4d7c9 100644 --- a/app/Policies/ApiTokenPolicy.php +++ b/app/Policies/ApiTokenPolicy.php @@ -20,7 +20,7 @@ public function viewAny(User $user): bool */ public function view(User $user, PersonalAccessToken $token): bool { - return $user->id === $token->tokenable_id && $token->tokenable_type === User::class; + return $this->belongsToUserAndCurrentTeam($user, $token); } /** @@ -36,7 +36,7 @@ public function create(User $user): bool */ public function update(User $user, PersonalAccessToken $token): bool { - return $user->id === $token->tokenable_id && $token->tokenable_type === User::class; + return $this->belongsToUserAndCurrentTeam($user, $token); } /** @@ -44,7 +44,7 @@ public function update(User $user, PersonalAccessToken $token): bool */ public function delete(User $user, PersonalAccessToken $token): bool { - return $user->id === $token->tokenable_id && $token->tokenable_type === User::class; + return $this->belongsToUserAndCurrentTeam($user, $token); } /** @@ -86,4 +86,19 @@ public function useSensitivePermissions(User $user): bool { return $user->isAdmin() || $user->isOwner(); } + + private function belongsToUserAndCurrentTeam(User $user, PersonalAccessToken $token): bool + { + if ($user->id !== $token->tokenable_id || $token->tokenable_type !== User::class) { + return false; + } + + $currentTeamId = $user->currentTeam()?->id; + + if ($currentTeamId === null || $token->team_id === null) { + return false; + } + + return (string) $currentTeamId === (string) $token->team_id; + } } diff --git a/tests/Feature/ApiTokenExpirationWarningTest.php b/tests/Feature/ApiTokenExpirationWarningTest.php index beea1f126..92c207607 100644 --- a/tests/Feature/ApiTokenExpirationWarningTest.php +++ b/tests/Feature/ApiTokenExpirationWarningTest.php @@ -43,7 +43,7 @@ function createTokenExpiring(User $user, Team $team, ?Carbon $expiresAt, ?Carbon describe('ApiTokenExpirationWarningJob', function () { test('notifies team when token expires within 24h', function () { - $token = createTokenExpiring($this->user, $this->team, now()->addHours(23)); + $token = createTokenExpiring($this->user, $this->team, Carbon::now()->addHours(23)); (new ApiTokenExpirationWarningJob)->handle(); @@ -52,7 +52,7 @@ function createTokenExpiring(User $user, Team $team, ?Carbon $expiresAt, ?Carbon }); test('does not mark token as warned when notification fails', function () { - $token = createTokenExpiring($this->user, $this->team, now()->addHours(23)); + $token = createTokenExpiring($this->user, $this->team, Carbon::now()->addHours(23)); $dispatcher = Mockery::mock(Dispatcher::class); $dispatcher->shouldReceive('send') ->once() @@ -67,7 +67,7 @@ function createTokenExpiring(User $user, Team $team, ?Carbon $expiresAt, ?Carbon }); test('database marker prevents duplicate warnings on repeat runs', function () { - createTokenExpiring($this->user, $this->team, now()->addHours(12)); + createTokenExpiring($this->user, $this->team, Carbon::now()->addHours(12)); (new ApiTokenExpirationWarningJob)->handle(); (new ApiTokenExpirationWarningJob)->handle(); @@ -76,7 +76,7 @@ function createTokenExpiring(User $user, Team $team, ?Carbon $expiresAt, ?Carbon }); test('database marker prevents duplicate warnings after cache is flushed', function () { - createTokenExpiring($this->user, $this->team, now()->addHours(12)); + createTokenExpiring($this->user, $this->team, Carbon::now()->addHours(12)); (new ApiTokenExpirationWarningJob)->handle(); @@ -88,7 +88,7 @@ function createTokenExpiring(User $user, Team $team, ?Carbon $expiresAt, ?Carbon }); test('skips tokens that already have an expiration warning marker', function () { - createTokenExpiring($this->user, $this->team, now()->addHours(12), now()->subHour()); + createTokenExpiring($this->user, $this->team, Carbon::now()->addHours(12), Carbon::now()->subHour()); (new ApiTokenExpirationWarningJob)->handle(); @@ -96,8 +96,8 @@ function createTokenExpiring(User $user, Team $team, ?Carbon $expiresAt, ?Carbon }); test('notifies once for each unmarked expiring token', function () { - createTokenExpiring($this->user, $this->team, now()->addHours(12)); - createTokenExpiring($this->user, $this->team, now()->addHours(23)); + createTokenExpiring($this->user, $this->team, Carbon::now()->addHours(12)); + createTokenExpiring($this->user, $this->team, Carbon::now()->addHours(23)); (new ApiTokenExpirationWarningJob)->handle(); @@ -105,7 +105,7 @@ function createTokenExpiring(User $user, Team $team, ?Carbon $expiresAt, ?Carbon }); test('skips tokens expiring more than 24h out', function () { - createTokenExpiring($this->user, $this->team, now()->addDays(3)); + createTokenExpiring($this->user, $this->team, Carbon::now()->addDays(3)); (new ApiTokenExpirationWarningJob)->handle(); @@ -113,7 +113,7 @@ function createTokenExpiring(User $user, Team $team, ?Carbon $expiresAt, ?Carbon }); test('skips already-expired tokens', function () { - createTokenExpiring($this->user, $this->team, now()->subHour()); + createTokenExpiring($this->user, $this->team, Carbon::now()->subHour()); (new ApiTokenExpirationWarningJob)->handle(); @@ -127,4 +127,14 @@ function createTokenExpiring(User $user, Team $team, ?Carbon $expiresAt, ?Carbon Notification::assertNothingSent(); }); + + test('skips tokens whose owner is no longer a team member', function () { + $token = createTokenExpiring($this->user, $this->team, Carbon::now()->addHours(12)); + $this->team->members()->detach($this->user); + + (new ApiTokenExpirationWarningJob)->handle(); + + Notification::assertNothingSent(); + expect($token->fresh()->api_token_expiration_warning_sent_at)->toBeNull(); + }); }); diff --git a/tests/Feature/ApiTokenLivewireAuthorizationTest.php b/tests/Feature/ApiTokenLivewireAuthorizationTest.php index 2a875ca26..4497fe2b4 100644 --- a/tests/Feature/ApiTokenLivewireAuthorizationTest.php +++ b/tests/Feature/ApiTokenLivewireAuthorizationTest.php @@ -76,6 +76,42 @@ ->and($token->abilities)->toBe(['read']); }); +test('api token list only contains tokens for the current team', function () { + $user = User::factory()->create(); + $otherTeam = Team::factory()->create(); + $this->team->members()->attach($user->id, ['role' => 'admin']); + $otherTeam->members()->attach($user->id, ['role' => 'admin']); + + session(['currentTeam' => $this->team]); + $currentTeamToken = $user->createToken('current-team-token', ['read'])->accessToken; + + session(['currentTeam' => $otherTeam]); + $user->createToken('other-team-token', ['read']); + + $this->actingAs($user); + session(['currentTeam' => $this->team]); + + Livewire::test(ApiTokens::class) + ->assertSet('tokens', fn ($tokens) => $tokens->pluck('id')->all() === [$currentTeamToken->id]); +}); + +test('user cannot revoke a token from another team through the current team', function () { + $user = User::factory()->create(); + $otherTeam = Team::factory()->create(); + $this->team->members()->attach($user->id, ['role' => 'admin']); + $otherTeam->members()->attach($user->id, ['role' => 'admin']); + + session(['currentTeam' => $otherTeam]); + $otherTeamToken = $user->createToken('other-team-token', ['read'])->accessToken; + + $this->actingAs($user); + session(['currentTeam' => $this->team]); + + Livewire::test(ApiTokens::class)->call('revoke', $otherTeamToken->id); + + expect($user->tokens()->whereKey($otherTeamToken->id)->exists())->toBeTrue(); +}); + test('owner can create root token', function () { $owner = User::factory()->create(); $this->team->members()->attach($owner->id, ['role' => 'owner']); diff --git a/tests/Unit/Policies/ApiTokenPolicyTest.php b/tests/Unit/Policies/ApiTokenPolicyTest.php index 98c60aae8..055297b3c 100644 --- a/tests/Unit/Policies/ApiTokenPolicyTest.php +++ b/tests/Unit/Policies/ApiTokenPolicyTest.php @@ -1,9 +1,28 @@ makePartial(); + $token->tokenable_id = $userId; + $token->tokenable_type = User::class; + $token->team_id = $teamId; + + return $token; +} + +function apiTokenTeam(int $teamId): Team +{ + $team = new Team; + $team->id = $teamId; + + return $team; +} + it('allows any user to view any api tokens', function () { $user = Mockery::mock(User::class)->makePartial(); @@ -28,10 +47,9 @@ it('allows owner to view their own api token', function () { $user = Mockery::mock(User::class)->makePartial(); $user->id = 1; + $user->shouldReceive('currentTeam')->andReturn(apiTokenTeam(10)); - $token = Mockery::mock(PersonalAccessToken::class)->makePartial(); - $token->tokenable_id = 1; - $token->tokenable_type = User::class; + $token = apiTokenForUserAndTeam(1, 10); $policy = new ApiTokenPolicy; expect($policy->view($user, $token))->toBeTrue(); @@ -52,10 +70,9 @@ it('allows owner to update their own api token', function () { $user = Mockery::mock(User::class)->makePartial(); $user->id = 1; + $user->shouldReceive('currentTeam')->andReturn(apiTokenTeam(10)); - $token = Mockery::mock(PersonalAccessToken::class)->makePartial(); - $token->tokenable_id = 1; - $token->tokenable_type = User::class; + $token = apiTokenForUserAndTeam(1, 10); $policy = new ApiTokenPolicy; expect($policy->update($user, $token))->toBeTrue(); @@ -76,10 +93,9 @@ it('allows owner to delete their own api token', function () { $user = Mockery::mock(User::class)->makePartial(); $user->id = 1; + $user->shouldReceive('currentTeam')->andReturn(apiTokenTeam(10)); - $token = Mockery::mock(PersonalAccessToken::class)->makePartial(); - $token->tokenable_id = 1; - $token->tokenable_type = User::class; + $token = apiTokenForUserAndTeam(1, 10); $policy = new ApiTokenPolicy; expect($policy->delete($user, $token))->toBeTrue(); @@ -97,6 +113,31 @@ expect($policy->delete($user, $token))->toBeFalse(); }); +it('denies access to an owned api token from another team', function (string $ability) { + $user = Mockery::mock(User::class)->makePartial(); + $user->id = 1; + $user->shouldReceive('currentTeam')->andReturn(apiTokenTeam(10)); + + $token = apiTokenForUserAndTeam(1, 20); + + $policy = new ApiTokenPolicy; + expect($policy->{$ability}($user, $token))->toBeFalse(); +})->with(['view', 'update', 'delete']); + +it('denies access to an owned api token without team identifiers', function (string $ability) { + $user = Mockery::mock(User::class)->makePartial(); + $user->id = 1; + $user->shouldReceive('currentTeam')->andReturnNull(); + + $token = Mockery::mock(PersonalAccessToken::class)->makePartial(); + $token->tokenable_id = 1; + $token->tokenable_type = User::class; + $token->team_id = null; + + $policy = new ApiTokenPolicy; + expect($policy->{$ability}($user, $token))->toBeFalse(); +})->with(['view', 'update', 'delete']); + it('allows admin to use root permissions', function () { $user = Mockery::mock(User::class)->makePartial(); $user->shouldReceive('isAdmin')->andReturn(true);