From 680071dca8b7fb18e54abeb07f64de7fa2491873 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:32:32 +0200 Subject: [PATCH] fix(teams): restrict deletion to owners and backfill team ownership --- app/Actions/Team/DeleteTeam.php | 65 +++++++++++ app/Livewire/NavbarDeleteTeam.php | 21 +--- app/Livewire/Team/Create.php | 2 +- app/Livewire/Team/DangerZone.php | 27 ++--- app/Policies/TeamPolicy.php | 2 +- ...rst_team_member_when_team_has_no_owner.php | 52 +++++++++ .../livewire/navbar-delete-team.blade.php | 2 + .../views/livewire/team/danger-zone.blade.php | 73 ++++++++---- .../Authorization/TeamAuthorizationTest.php | 17 +-- tests/Feature/Team/TeamDeletionTest.php | 108 ++++++++++++++++++ .../Feature/TeamCreateSetsMcpDefaultTest.php | 3 +- .../TeamOwnerBackfillMigrationTest.php | 42 +++++++ tests/Feature/TeamSettingsNavigationTest.php | 11 +- tests/Unit/Policies/TeamPolicyTest.php | 14 ++- 14 files changed, 364 insertions(+), 75 deletions(-) create mode 100644 app/Actions/Team/DeleteTeam.php create mode 100644 database/migrations/2026_08_20_070831_promote_first_team_member_when_team_has_no_owner.php create mode 100644 tests/Feature/TeamOwnerBackfillMigrationTest.php diff --git a/app/Actions/Team/DeleteTeam.php b/app/Actions/Team/DeleteTeam.php new file mode 100644 index 0000000000..be880b7e78 --- /dev/null +++ b/app/Actions/Team/DeleteTeam.php @@ -0,0 +1,65 @@ +lockForUpdate()->findOrFail($team->id); + + $role = DB::table('team_user') + ->where('team_id', $team->id) + ->where('user_id', $user->id) + ->lockForUpdate() + ->value('role'); + + if ($role !== 'owner') { + throw new AuthorizationException('Only team owners can delete a team.'); + } + + $hasRunningApplications = Application::query() + ->whereHas('environment.project', fn ($query) => $query->where('team_id', $team->id)) + ->lockForUpdate() + ->get(['id', 'status']) + ->contains(fn (Application $application): bool => $application->isRunning()); + + if ($hasRunningApplications) { + throw new RuntimeException('Stop all running applications before deleting this team.'); + } + + if ($team->servers()->lockForUpdate()->get(['servers.id'])->isNotEmpty()) { + throw new RuntimeException('Delete all team servers before deleting this team.'); + } + + if (! $team->isEmpty()) { + throw new RuntimeException('Delete all team resources before deleting this team.'); + } + + $team->members() + ->where('users.id', '!=', $user->id) + ->get() + ->each(function (User $member) use ($team): void { + $member->teams()->detach($team); + DB::table('sessions')->where('user_id', $member->id)->delete(); + }); + + $team->delete(); + + return $user->teams()->first(); + }); + + Cache::forget("user:{$user->id}:team:{$team->id}"); + + return $newTeam; + } +} diff --git a/app/Livewire/NavbarDeleteTeam.php b/app/Livewire/NavbarDeleteTeam.php index 52e4460add..e28cefba48 100644 --- a/app/Livewire/NavbarDeleteTeam.php +++ b/app/Livewire/NavbarDeleteTeam.php @@ -2,10 +2,8 @@ namespace App\Livewire; +use App\Actions\Team\DeleteTeam; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; -use Illuminate\Support\Facades\Auth; -use Illuminate\Support\Facades\Cache; -use Illuminate\Support\Facades\DB; use Livewire\Component; class NavbarDeleteTeam extends Component @@ -28,22 +26,7 @@ class NavbarDeleteTeam extends Component $currentTeam = currentTeam(); $this->authorize('delete', $currentTeam); - - $currentTeam->members->each(function ($user) use ($currentTeam) { - if ($user->id === Auth::id()) { - return; - } - $user->teams()->detach($currentTeam); - $session = DB::table('sessions')->where('user_id', $user->id)->first(); - if ($session) { - DB::table('sessions')->where('id', $session->id)->delete(); - } - }); - - Cache::forget('user:'.Auth::id().':team:'.$currentTeam->id); - $currentTeam->delete(); - - $newTeam = Auth::user()->teams()->first(); + $newTeam = app(DeleteTeam::class)->handle($currentTeam, auth()->user()); refreshSession($newTeam); return redirect()->route('team.index'); diff --git a/app/Livewire/Team/Create.php b/app/Livewire/Team/Create.php index c459626482..1f191ec6b1 100644 --- a/app/Livewire/Team/Create.php +++ b/app/Livewire/Team/Create.php @@ -35,7 +35,7 @@ class Create extends Component 'personal_team' => false, 'is_mcp_server_enabled' => true, ]); - auth()->user()->teams()->attach($team, ['role' => 'admin']); + auth()->user()->teams()->attach($team, ['role' => 'owner']); refreshSession($team); return redirectRoute($this, 'team.index'); diff --git a/app/Livewire/Team/DangerZone.php b/app/Livewire/Team/DangerZone.php index a3f73a44f1..74cf3d7ef8 100644 --- a/app/Livewire/Team/DangerZone.php +++ b/app/Livewire/Team/DangerZone.php @@ -2,11 +2,9 @@ namespace App\Livewire\Team; +use App\Actions\Team\DeleteTeam; use App\Models\Team; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; -use Illuminate\Support\Facades\Auth; -use Illuminate\Support\Facades\Cache; -use Illuminate\Support\Facades\DB; use Livewire\Component; class DangerZone extends Component @@ -25,22 +23,7 @@ class DangerZone extends Component try { $currentTeam = currentTeam(); $this->authorize('delete', $currentTeam); - $currentTeam->members->each(function ($user) use ($currentTeam): void { - if ($user->id === Auth::id()) { - return; - } - - $user->teams()->detach($currentTeam); - $session = DB::table('sessions')->where('user_id', $user->id)->first(); - if ($session) { - DB::table('sessions')->where('id', $session->id)->delete(); - } - }); - - Cache::forget('user:'.Auth::id().':team:'.$currentTeam->id); - $currentTeam->delete(); - - $newTeam = Auth::user()->teams()->first(); + $newTeam = app(DeleteTeam::class)->handle($currentTeam, auth()->user()); refreshSession($newTeam); return redirect()->route('team.index'); @@ -49,6 +32,12 @@ class DangerZone extends Component } } + public function refreshResources(): void + { + $this->team = Team::query()->findOrFail($this->team->id); + refreshSession($this->team); + } + public function render(): mixed { return view('livewire.team.danger-zone'); diff --git a/app/Policies/TeamPolicy.php b/app/Policies/TeamPolicy.php index cc7745b648..5b97927601 100644 --- a/app/Policies/TeamPolicy.php +++ b/app/Policies/TeamPolicy.php @@ -53,7 +53,7 @@ class TeamPolicy return false; } - return $user->isAdminOfTeam($team->id); + return $user->roleInTeam($team->id) === 'owner'; } /** diff --git a/database/migrations/2026_08_20_070831_promote_first_team_member_when_team_has_no_owner.php b/database/migrations/2026_08_20_070831_promote_first_team_member_when_team_has_no_owner.php new file mode 100644 index 0000000000..1d6d5694d4 --- /dev/null +++ b/database/migrations/2026_08_20_070831_promote_first_team_member_when_team_has_no_owner.php @@ -0,0 +1,52 @@ +select('teams.id') + ->whereNotExists(function ($query): void { + $query->selectRaw('1') + ->from('team_user as owners') + ->whereColumn('owners.team_id', 'teams.id') + ->where('owners.role', 'owner'); + }) + ->orderBy('teams.id') + ->chunkById(100, function ($teams): void { + foreach ($teams as $team) { + $firstMember = DB::table('team_user') + ->where('team_id', $team->id) + ->orderByRaw("CASE WHEN role = 'admin' THEN 0 ELSE 1 END") + ->orderBy('created_at') + ->orderBy('id') + ->first(); + + if ($firstMember === null) { + continue; + } + + DB::table('team_user') + ->where('id', $firstMember->id) + ->update([ + 'role' => 'owner', + 'updated_at' => now(), + ]); + } + }, 'teams.id', 'id'); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + // This data migration cannot identify which owners were promoted safely. + } +}; diff --git a/resources/views/livewire/navbar-delete-team.blade.php b/resources/views/livewire/navbar-delete-team.blade.php index 55ce72a330..c02830c2c6 100644 --- a/resources/views/livewire/navbar-delete-team.blade.php +++ b/resources/views/livewire/navbar-delete-team.blade.php @@ -1,4 +1,5 @@
+ @if (auth()->user()->roleInTeam(currentTeam()->id) === 'owner') + @endif
diff --git a/resources/views/livewire/team/danger-zone.blade.php b/resources/views/livewire/team/danger-zone.blade.php index ade512a6f4..8d0c022b12 100644 --- a/resources/views/livewire/team/danger-zone.blade.php +++ b/resources/views/livewire/team/danger-zone.blade.php @@ -16,7 +16,11 @@ - @if (session('currentTeam.id') === 0) + @if (auth()->user()->roleInTeam(currentTeam()->id) !== 'owner') +

+ Only team owners can delete this team. +

+ @elseif (session('currentTeam.id') === 0)

The default team cannot be deleted.

@@ -49,6 +53,7 @@
@if ( session('currentTeam.id') !== 0 && + auth()->user()->roleInTeam(currentTeam()->id) === 'owner' && auth()->user()->teams()->count() > 1 && !auth()->user()->currentTeam()->personal_team && !currentTeam()->subscription && @@ -69,27 +74,51 @@
- @if (session('currentTeam.id') !== 0 && !currentTeam()->subscription && !currentTeam()->isEmpty()) -
- @foreach ([ - 'Projects' => currentTeam()->projects, - 'Servers' => currentTeam()->servers, - 'Private keys' => currentTeam()->privateKeys, - 'Sources' => currentTeam()->sources(), - ] as $label => $resources) - @if ($resources->isNotEmpty()) -
-

- {{ $label }} -

-
    - @foreach ($resources as $resource) -
  • {{ $resource->name }}
  • - @endforeach -
-
- @endif - @endforeach + @if (session('currentTeam.id') !== 0 && !currentTeam()->subscription && (currentTeam()->projects->isNotEmpty() || currentTeam()->servers->isNotEmpty())) +
+
+
Resources
+ + + Refresh + +
+ + + + + + + + + @foreach (currentTeam()->projects as $project) + + + + + @endforeach + @foreach (currentTeam()->servers as $server) + + + + + @endforeach + +
ResourceName
+ Project + + {{ $project->name }} +
+ Server + + {{ $server->name }} +
@endif diff --git a/tests/Feature/Authorization/TeamAuthorizationTest.php b/tests/Feature/Authorization/TeamAuthorizationTest.php index 63d5d3d264..eaae106ef3 100644 --- a/tests/Feature/Authorization/TeamAuthorizationTest.php +++ b/tests/Feature/Authorization/TeamAuthorizationTest.php @@ -1,5 +1,6 @@ user()->can('delete', $this->team))->toBeTrue(); }); -test('admin can delete team', function () { +test('admin cannot delete team', function () { $this->actingAs($this->admin); session(['currentTeam' => $this->team]); - expect(auth()->user()->can('delete', $this->team))->toBeTrue(); + expect(auth()->user()->can('delete', $this->team))->toBeFalse(); }); test('member cannot delete team', function () { @@ -156,24 +157,24 @@ test('team index mounts when is_mcp_server_enabled is null on the session team', ->assertSet('is_mcp_server_enabled', true); }); -// --- Team Index Livewire: delete --- +// --- Team Danger Zone Livewire: delete --- -test('member cannot delete team via index', function () { +test('member cannot delete team via danger zone', function () { $this->actingAs($this->member); session(['currentTeam' => $this->team]); - Livewire::test(TeamIndex::class) - ->call('delete', 'password') + Livewire::test(DangerZone::class) + ->call('delete') ->assertDispatched('error'); expect(Team::find($this->team->id))->not->toBeNull(); }); -test('admin can delete team via policy', function () { +test('admin cannot delete team via policy', function () { $this->actingAs($this->admin); session(['currentTeam' => $this->team]); - expect(auth()->user()->can('delete', $this->team))->toBeTrue(); + expect(auth()->user()->can('delete', $this->team))->toBeFalse(); }); // --- Team Member Livewire: role changes --- diff --git a/tests/Feature/Team/TeamDeletionTest.php b/tests/Feature/Team/TeamDeletionTest.php index 6bdcaf4e8a..1c11d13802 100644 --- a/tests/Feature/Team/TeamDeletionTest.php +++ b/tests/Feature/Team/TeamDeletionTest.php @@ -1,11 +1,19 @@ and($sessionTeam->id)->toBe($this->personalTeam->id); }); +test('the danger zone resource list can be refreshed', function () { + $this->actingAs($this->owner); + session(['currentTeam' => $this->teamToDelete]); + + Livewire::test(DangerZone::class) + ->call('refreshResources') + ->assertSuccessful(); +}); + +test('a team with a running application cannot be deleted', function () { + $server = Server::factory()->create(['team_id' => $this->teamToDelete->id]); + $destination = StandaloneDocker::query()->where('server_id', $server->id)->firstOrFail(); + $project = Project::factory()->create(['team_id' => $this->teamToDelete->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + Application::factory()->create([ + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => $destination->getMorphClass(), + 'status' => 'running:healthy', + ]); + + $this->actingAs($this->owner); + session(['currentTeam' => $this->teamToDelete]); + + Livewire::test(DangerZone::class) + ->call('delete') + ->assertDispatched('error'); + + expect(Team::find($this->teamToDelete->id))->not->toBeNull(); +}); + +test('a team with a server cannot be deleted', function () { + Server::factory()->create(['team_id' => $this->teamToDelete->id]); + + $this->actingAs($this->owner); + session(['currentTeam' => $this->teamToDelete]); + + Livewire::test(DangerZone::class) + ->call('delete') + ->assertDispatched('error', fn (string $event, array $params): bool => $params[0] === 'Delete all team servers before deleting this team.'); + + expect(Team::find($this->teamToDelete->id))->not->toBeNull(); +}); + +test('a team with a project but no servers cannot be deleted', function () { + Project::factory()->create(['team_id' => $this->teamToDelete->id]); + + $member = User::factory()->create(); + $this->teamToDelete->members()->attach($member->id, ['role' => 'member']); + $privateKey = PrivateKey::factory()->create(['team_id' => $this->teamToDelete->id]); + + $this->actingAs($this->owner); + session(['currentTeam' => $this->teamToDelete]); + + Livewire::test(DangerZone::class) + ->call('delete') + ->assertDispatched('error', fn (string $event, array $params): bool => $params[0] === 'Delete all team resources before deleting this team.'); + + expect(Team::find($this->teamToDelete->id))->not->toBeNull() + ->and(PrivateKey::find($privateKey->id))->not->toBeNull() + ->and($this->teamToDelete->members()->whereKey($member->id)->exists())->toBeTrue(); +}); + +test('an admin cannot delete a team through the deletion action', function () { + $admin = User::factory()->create(); + $this->teamToDelete->members()->attach($admin->id, ['role' => 'admin']); + + expect(fn () => app(DeleteTeam::class)->handle($this->teamToDelete, $admin)) + ->toThrow(AuthorizationException::class); + + expect(Team::find($this->teamToDelete->id))->not->toBeNull(); +}); + +test('a stale owner relationship cannot authorize team deletion', function () { + $this->owner->teams; + $this->owner->teams()->updateExistingPivot($this->teamToDelete->id, ['role' => 'admin']); + + expect(fn () => app(DeleteTeam::class)->handle($this->teamToDelete, $this->owner)) + ->toThrow(AuthorizationException::class); + + expect(Team::find($this->teamToDelete->id))->not->toBeNull(); +}); + test('refreshSession clears session when no team exists', function () { $user = User::factory()->create(); // Detach all teams so user has none @@ -78,3 +169,20 @@ test('deleting a team deletes github and gitlab sources with the same primary ke expect(GithubApp::find($githubApp->id))->toBeNull() ->and(GitlabApp::find($gitlabApp->id))->toBeNull(); }); + +test('team deletion rolls back all database changes when an operation fails', function () { + $member = User::factory()->create(); + $this->teamToDelete->members()->attach($member->id, ['role' => 'member']); + + Team::deleting(function (Team $deletingTeam): void { + if ($deletingTeam->id === $this->teamToDelete->id) { + throw new RuntimeException('Simulated deletion failure.'); + } + }); + + expect(fn () => app(DeleteTeam::class)->handle($this->teamToDelete, $this->owner)) + ->toThrow(RuntimeException::class, 'Simulated deletion failure.'); + + expect(Team::find($this->teamToDelete->id))->not->toBeNull() + ->and($this->teamToDelete->members()->whereKey($member->id)->exists())->toBeTrue(); +}); diff --git a/tests/Feature/TeamCreateSetsMcpDefaultTest.php b/tests/Feature/TeamCreateSetsMcpDefaultTest.php index 6f6c99c49e..b816c0609d 100644 --- a/tests/Feature/TeamCreateSetsMcpDefaultTest.php +++ b/tests/Feature/TeamCreateSetsMcpDefaultTest.php @@ -33,5 +33,6 @@ test('creating a team sets is_mcp_server_enabled to true on the model', function $created = Team::query()->where('name', 'MCP Safe Team')->first(); expect($created)->not->toBeNull() - ->and($created->is_mcp_server_enabled)->toBeTrue(); + ->and($created->is_mcp_server_enabled)->toBeTrue() + ->and($this->user->fresh()->roleInTeam($created->id))->toBe('owner'); }); diff --git a/tests/Feature/TeamOwnerBackfillMigrationTest.php b/tests/Feature/TeamOwnerBackfillMigrationTest.php new file mode 100644 index 0000000000..88e0a7e919 --- /dev/null +++ b/tests/Feature/TeamOwnerBackfillMigrationTest.php @@ -0,0 +1,42 @@ +create(['personal_team' => false]); + $firstMember = User::factory()->create(); + $firstAdmin = User::factory()->create(); + $teamWithoutOwner->members()->attach($firstMember, [ + 'role' => 'member', + 'created_at' => now()->subMinute(), + 'updated_at' => now()->subMinute(), + ]); + $teamWithoutOwner->members()->attach($firstAdmin, [ + 'role' => 'admin', + 'created_at' => now(), + 'updated_at' => now(), + ]); + + $teamWithoutAdmin = Team::factory()->create(['personal_team' => false]); + $fallbackMember = User::factory()->create(); + $teamWithoutAdmin->members()->attach($fallbackMember, ['role' => 'member']); + + $teamWithOwner = Team::factory()->create(['personal_team' => false]); + $existingOwner = User::factory()->create(); + $existingAdmin = User::factory()->create(); + $teamWithOwner->members()->attach($existingOwner, ['role' => 'owner']); + $teamWithOwner->members()->attach($existingAdmin, ['role' => 'admin']); + + $migration = require database_path('migrations/2026_08_20_070831_promote_first_team_member_when_team_has_no_owner.php'); + $migration->up(); + + expect($teamWithoutOwner->members()->find($firstMember->id)->pivot->role)->toBe('member') + ->and($teamWithoutOwner->members()->find($firstAdmin->id)->pivot->role)->toBe('owner') + ->and($teamWithoutAdmin->members()->find($fallbackMember->id)->pivot->role)->toBe('owner') + ->and($teamWithOwner->members()->find($existingOwner->id)->pivot->role)->toBe('owner') + ->and($teamWithOwner->members()->find($existingAdmin->id)->pivot->role)->toBe('admin'); +}); diff --git a/tests/Feature/TeamSettingsNavigationTest.php b/tests/Feature/TeamSettingsNavigationTest.php index d6b6f72cf2..da959b9d18 100644 --- a/tests/Feature/TeamSettingsNavigationTest.php +++ b/tests/Feature/TeamSettingsNavigationTest.php @@ -29,8 +29,15 @@ it('uses shared sidebar navigation for every team settings page', function () { ->toContain('Delete team') ->toContain('status="Permanent"') ->toContain('border-red-300') - ->toContain("'Sources' => currentTeam()->sources()") - ->not->toContain("'Sources' => currentTeam()->sources,"); + ->toContain('') + ->toContain('wire:click="refreshResources"') + ->not->toContain('wire:loading.class="animate-spin"') + ->toContain("route('project.show', ['project_uuid' => \$project->uuid])") + ->toContain("route('server.show', ['server_uuid' => \$server->uuid])") + ->toContain('target="_blank" rel="noopener noreferrer"') + ->toContain('Delete every server owned by this team before deleting it.') + ->toContain('currentTeam()->servers->isEmpty()') + ->not->toContain('currentTeam()->isEmpty()'); expect(file_get_contents(resource_path('views/livewire/switch-team.blade.php'))) ->toContain('New team') ->toContain('team-switcher-create-expanded') diff --git a/tests/Unit/Policies/TeamPolicyTest.php b/tests/Unit/Policies/TeamPolicyTest.php index 3b341d4888..9da59d767c 100644 --- a/tests/Unit/Policies/TeamPolicyTest.php +++ b/tests/Unit/Policies/TeamPolicyTest.php @@ -56,7 +56,6 @@ it('allows target team admins to perform privileged team actions', function (str expect((new TeamPolicy)->{$ability}($user, $team))->toBeTrue(); })->with([ 'update', - 'delete', 'manageMembers', 'viewAdmin', 'manageInvitations', @@ -72,7 +71,6 @@ it('denies target team members even when their current session role is admin els expect((new TeamPolicy)->{$ability}($user, $team))->toBeFalse(); })->with([ 'update', - 'delete', 'manageMembers', 'viewAdmin', 'manageInvitations', @@ -90,3 +88,15 @@ it('denies non-members from privileged team actions', function (string $ability) 'viewAdmin', 'manageInvitations', ]); + +it('only allows target team owners to delete the team', function (string $role, bool $allowed) { + $user = teamPolicyUserWithTeams([1]); + $user->shouldReceive('roleInTeam')->with(1)->andReturn($role); + $team = teamPolicyTeam(1); + + expect((new TeamPolicy)->delete($user, $team))->toBe($allowed); +})->with([ + 'owner' => ['owner', true], + 'admin' => ['admin', false], + 'member' => ['member', false], +]);