From 8f90882875850c21fe8fedba02c405707e155ddb Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:38:50 +0200 Subject: [PATCH] fix(teams): make membership and source deletions atomic Wrap team membership changes, invitation revocation, and GitHub source deletion in transactions, with coverage for rollback failures. --- app/Livewire/Team/Invitations.php | 13 +++--- app/Livewire/Team/Member.php | 25 ++++++++---- app/Models/GithubApp.php | 6 +++ app/Models/GitlabApp.php | 1 + .../ApiTokenTeamLifecycleSecurityTest.php | 40 +++++++++++++++++++ tests/Feature/SourceDeletionAtomicityTest.php | 39 ++++++++++++++++++ tests/Feature/TeamInvitationUiTest.php | 29 ++++++++++++++ 7 files changed, 140 insertions(+), 13 deletions(-) create mode 100644 tests/Feature/SourceDeletionAtomicityTest.php diff --git a/app/Livewire/Team/Invitations.php b/app/Livewire/Team/Invitations.php index 523f640b96..8ecafc417c 100644 --- a/app/Livewire/Team/Invitations.php +++ b/app/Livewire/Team/Invitations.php @@ -5,6 +5,7 @@ namespace App\Livewire\Team; use App\Models\TeamInvitation; use App\Models\User; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; +use Illuminate\Support\Facades\DB; use Livewire\Component; class Invitations extends Component @@ -21,12 +22,14 @@ class Invitations extends Component $this->authorize('manageInvitations', currentTeam()); $invitation = TeamInvitation::ownedByCurrentTeam()->findOrFail($invitation_id); - $user = User::whereEmail($invitation->email)->first(); - if (filled($user)) { - $user->deleteIfNotVerifiedAndForcePasswordReset(); - } + DB::transaction(function () use ($invitation): void { + $user = User::whereEmail($invitation->email)->first(); + if (filled($user)) { + $user->deleteIfNotVerifiedAndForcePasswordReset(); + } - $invitation->delete(); + $invitation->delete(); + }); $this->refreshInvitations(); $this->dispatch('success', 'Invitation revoked.'); } catch (\Exception) { diff --git a/app/Livewire/Team/Member.php b/app/Livewire/Team/Member.php index 97d492d700..38c932c39d 100644 --- a/app/Livewire/Team/Member.php +++ b/app/Livewire/Team/Member.php @@ -7,6 +7,7 @@ use App\Enums\Role; use App\Models\User; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\DB; use Livewire\Component; class Member extends Component @@ -25,8 +26,10 @@ class Member extends Component throw new \Exception('You are not authorized to perform this action.'); } $teamId = currentTeam()->id; - $this->member->teams()->updateExistingPivot($teamId, ['role' => Role::ADMIN->value]); - RevokeUserTeamTokens::forUserTeam($this->member, $teamId); + DB::transaction(function () use ($teamId): void { + $this->member->teams()->updateExistingPivot($teamId, ['role' => Role::ADMIN->value]); + RevokeUserTeamTokens::forUserTeam($this->member, $teamId); + }); $this->dispatch('reloadWindow'); } catch (\Exception $e) { $this->dispatch('error', $e->getMessage()); @@ -43,8 +46,10 @@ class Member extends Component throw new \Exception('You are not authorized to perform this action.'); } $teamId = currentTeam()->id; - $this->member->teams()->updateExistingPivot($teamId, ['role' => Role::OWNER->value]); - RevokeUserTeamTokens::forUserTeam($this->member, $teamId); + DB::transaction(function () use ($teamId): void { + $this->member->teams()->updateExistingPivot($teamId, ['role' => Role::OWNER->value]); + RevokeUserTeamTokens::forUserTeam($this->member, $teamId); + }); $this->dispatch('reloadWindow'); } catch (\Exception $e) { $this->dispatch('error', $e->getMessage()); @@ -61,8 +66,10 @@ class Member extends Component throw new \Exception('You are not authorized to perform this action.'); } $teamId = currentTeam()->id; - $this->member->teams()->updateExistingPivot($teamId, ['role' => Role::MEMBER->value]); - RevokeUserTeamTokens::forUserTeam($this->member, $teamId); + DB::transaction(function () use ($teamId): void { + $this->member->teams()->updateExistingPivot($teamId, ['role' => Role::MEMBER->value]); + RevokeUserTeamTokens::forUserTeam($this->member, $teamId); + }); $this->dispatch('reloadWindow'); } catch (\Exception $e) { $this->dispatch('error', $e->getMessage()); @@ -79,8 +86,10 @@ class Member extends Component throw new \Exception('You are not authorized to perform this action.'); } $teamId = currentTeam()->id; - $this->member->teams()->detach(currentTeam()); - RevokeUserTeamTokens::forUserTeam($this->member, $teamId); + DB::transaction(function () use ($teamId): void { + $this->member->teams()->detach($teamId); + RevokeUserTeamTokens::forUserTeam($this->member, $teamId); + }); // Clear cache for the removed user - both old and new key formats Cache::forget("team:{$this->member->id}"); Cache::forget("user:{$this->member->id}:team:{$teamId}"); diff --git a/app/Models/GithubApp.php b/app/Models/GithubApp.php index 7c2f8c0628..564fbcf6a4 100644 --- a/app/Models/GithubApp.php +++ b/app/Models/GithubApp.php @@ -3,9 +3,15 @@ namespace App\Models; use Illuminate\Database\Eloquent\Casts\Attribute; +use Illuminate\Support\Facades\DB; class GithubApp extends BaseModel { + public function delete(): ?bool + { + return DB::transaction(fn () => parent::delete()); + } + protected $fillable = [ 'team_id', 'private_key_id', diff --git a/app/Models/GitlabApp.php b/app/Models/GitlabApp.php index 09a48e8b91..c6c2b84095 100644 --- a/app/Models/GitlabApp.php +++ b/app/Models/GitlabApp.php @@ -100,6 +100,7 @@ class GitlabApp extends BaseModel if ($gitlabApp->applications()->count() > 0) { throw new \RuntimeException('This source is being used by an application. Please delete all applications first.'); } + }); } diff --git a/tests/Feature/ApiTokenTeamLifecycleSecurityTest.php b/tests/Feature/ApiTokenTeamLifecycleSecurityTest.php index c254a26c3d..a6698833dc 100644 --- a/tests/Feature/ApiTokenTeamLifecycleSecurityTest.php +++ b/tests/Feature/ApiTokenTeamLifecycleSecurityTest.php @@ -6,9 +6,11 @@ use App\Models\InstanceSettings; use App\Models\Project; use App\Models\Team; use App\Models\User; +use Illuminate\Database\Schema\Blueprint; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Hash; +use Illuminate\Support\Facades\Schema; use Livewire\Livewire; uses(RefreshDatabase::class); @@ -96,6 +98,44 @@ test('role downgrade through team member component revokes team tokens', functio expect(DB::table('personal_access_tokens')->where('id', $token->id)->exists())->toBeFalse(); }); +test('member removal rolls back when token revocation fails', function () { + $owner = User::factory()->create(); + $this->team->members()->attach($owner->id, ['role' => 'owner']); + $token = $this->user->createToken('protected-token', ['read'])->accessToken; + Schema::create('protected_personal_access_tokens', function (Blueprint $table): void { + $table->foreignId('token_id')->constrained('personal_access_tokens'); + }); + DB::table('protected_personal_access_tokens')->insert(['token_id' => $token->id]); + + $this->actingAs($owner); + session(['currentTeam' => $this->team]); + + Livewire::test(Member::class, ['member' => $this->user]) + ->call('remove') + ->assertDispatched('error'); + + expect($this->team->members()->whereKey($this->user->id)->exists())->toBeTrue(); +}); + +test('role change rolls back when token revocation fails', function () { + $owner = User::factory()->create(); + $this->team->members()->attach($owner->id, ['role' => 'owner']); + $token = $this->user->createToken('protected-token', ['write'])->accessToken; + Schema::create('protected_personal_access_tokens', function (Blueprint $table): void { + $table->foreignId('token_id')->constrained('personal_access_tokens'); + }); + DB::table('protected_personal_access_tokens')->insert(['token_id' => $token->id]); + + $this->actingAs($owner); + session(['currentTeam' => $this->team]); + + Livewire::test(Member::class, ['member' => $this->user]) + ->call('makeReadonly') + ->assertDispatched('error'); + + expect($this->user->fresh()->teams()->findOrFail($this->team->id)->pivot->role)->toBe('admin'); +}); + test('member cannot create write token through livewire token form', function () { $this->team->members()->updateExistingPivot($this->user->id, ['role' => 'member']); diff --git a/tests/Feature/SourceDeletionAtomicityTest.php b/tests/Feature/SourceDeletionAtomicityTest.php new file mode 100644 index 0000000000..46b12757fa --- /dev/null +++ b/tests/Feature/SourceDeletionAtomicityTest.php @@ -0,0 +1,39 @@ +create([ + 'team_id' => $team->id, + 'is_git_related' => true, + ]); +} + +test('github source deletion rolls back unused private key deletion when source deletion fails', function () { + $team = Team::factory()->create(); + $privateKey = sourcePrivateKey($team); + $githubApp = GithubApp::create([ + 'name' => 'GitHub', + 'api_url' => 'https://api.github.com', + 'html_url' => 'https://github.com', + 'private_key_id' => $privateKey->id, + 'team_id' => $team->id, + ]); + + GithubApp::deleting(function (GithubApp $deletingApp) use ($githubApp): void { + if ($deletingApp->is($githubApp)) { + throw new RuntimeException('Source deletion failed.'); + } + }); + + expect(fn () => $githubApp->delete())->toThrow(RuntimeException::class, 'Source deletion failed.'); + + $this->assertModelExists($githubApp); + $this->assertModelExists($privateKey); +}); diff --git a/tests/Feature/TeamInvitationUiTest.php b/tests/Feature/TeamInvitationUiTest.php index 80a4a146df..13b6de23e5 100644 --- a/tests/Feature/TeamInvitationUiTest.php +++ b/tests/Feature/TeamInvitationUiTest.php @@ -73,3 +73,32 @@ it('exposes a resilient global copyToClipboard helper', function () { ->toContain('document.execCommand(\'copy\')') ->toContain('window.isSecureContext'); }); + +it('preserves a provisional user when revoking their invitation fails', function () { + $provisionalUser = User::factory()->create([ + 'email' => 'provisional@example.com', + 'email_verified_at' => null, + 'force_password_reset' => true, + ]); + $invitation = TeamInvitation::create([ + 'team_id' => $this->team->id, + 'uuid' => 'failing-invitation-delete', + 'email' => $provisionalUser->email, + 'role' => 'member', + 'link' => 'http://example.test/invitations/failing-invitation-delete', + 'via' => 'link', + ]); + + TeamInvitation::deleting(function (): void { + throw new RuntimeException('Invitation deletion failed.'); + }); + + Livewire::test(Invitations::class, [ + 'invitations' => collect([$invitation]), + ]) + ->call('deleteInvitation', $invitation->id) + ->assertDispatched('error'); + + $this->assertDatabaseHas('users', ['id' => $provisionalUser->id]); + $this->assertDatabaseHas('team_invitations', ['id' => $invitation->id]); +});