From 3f564f9b2efe2c55655671ab80cd13eedf8c6be0 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Sun, 5 Apr 2026 18:08:06 +0200 Subject: [PATCH 1/2] fix(user-deletion): handle GitHub app sources across team cleanup Limit team cleanup to apps owned by the deleted team and nullify cross-team application source references before deleting team-owned sources. Adds feature tests covering user deletion with GitHub app-backed applications, preserving system-wide apps, and nullifying external source links. --- app/Models/Team.php | 6 +- app/Models/User.php | 17 ++ .../Feature/UserDeletionWithGithubAppTest.php | 166 ++++++++++++++++++ 3 files changed, 187 insertions(+), 2 deletions(-) create mode 100644 tests/Feature/UserDeletionWithGithubAppTest.php diff --git a/app/Models/Team.php b/app/Models/Team.php index 8a54a9dee..479bf4e00 100644 --- a/app/Models/Team.php +++ b/app/Models/Team.php @@ -76,8 +76,10 @@ protected static function booted() foreach ($keys as $key) { $key->delete(); } - $sources = $team->sources(); - foreach ($sources as $source) { + // Only delete sources owned by this team, not system-wide ones from other teams + $teamSources = GithubApp::where('team_id', $team->id)->get() + ->merge(GitlabApp::where('team_id', $team->id)->get()); + foreach ($teamSources as $source) { $source->delete(); } $tags = Tag::whereTeamId($team->id)->get(); diff --git a/app/Models/User.php b/app/Models/User.php index 3199d2024..7a9600ec4 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -176,6 +176,23 @@ private static function finalizeTeamDeletion(User $user, Team $team) $project->forceDelete(); } + // Detach applications from other teams that reference this team's sources, + // so the GithubApp/GitlabApp deleting guard doesn't block team deletion + $githubAppIds = GithubApp::where('team_id', $team->id)->pluck('id'); + $gitlabAppIds = GitlabApp::where('team_id', $team->id)->pluck('id'); + + if ($githubAppIds->isNotEmpty()) { + Application::where('source_type', GithubApp::class) + ->whereIn('source_id', $githubAppIds) + ->update(['source_id' => null, 'source_type' => null]); + } + + if ($gitlabAppIds->isNotEmpty()) { + Application::where('source_type', GitlabApp::class) + ->whereIn('source_id', $gitlabAppIds) + ->update(['source_id' => null, 'source_type' => null]); + } + $team->members()->detach($user->id); $team->delete(); } diff --git a/tests/Feature/UserDeletionWithGithubAppTest.php b/tests/Feature/UserDeletionWithGithubAppTest.php new file mode 100644 index 000000000..d8986e378 --- /dev/null +++ b/tests/Feature/UserDeletionWithGithubAppTest.php @@ -0,0 +1,166 @@ +rootTeam = Team::factory()->create(['id' => 0, 'name' => 'Root Team']); + $this->adminUser = User::factory()->create(); + $this->rootTeam->members()->attach($this->adminUser->id, ['role' => 'owner']); + $this->actingAs($this->adminUser); + session(['currentTeam' => $this->rootTeam]); +}); + +it('deletes a user whose team has a github app with applications', function () { + // Create the user to be deleted with their own team + $targetUser = User::factory()->create(); + $targetTeam = $targetUser->teams()->first(); // created by User::created event + + // Create a private key for the team + $privateKey = PrivateKey::factory()->create(['team_id' => $targetTeam->id]); + + // Create a server and destination for the team + $server = Server::factory()->create([ + 'team_id' => $targetTeam->id, + 'private_key_id' => $privateKey->id, + ]); + $destination = StandaloneDocker::factory()->create(['server_id' => $server->id]); + + // Create a project and environment + $project = Project::factory()->create(['team_id' => $targetTeam->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + + // Create a GitHub App owned by the target team + $githubApp = GithubApp::create([ + 'name' => 'Test GitHub App', + 'team_id' => $targetTeam->id, + 'private_key_id' => $privateKey->id, + 'api_url' => 'https://api.github.com', + 'html_url' => 'https://github.com', + 'is_public' => false, + ]); + + // Create an application that uses the GitHub App as its source + $application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => StandaloneDocker::class, + 'source_id' => $githubApp->id, + 'source_type' => GithubApp::class, + ]); + + // Delete the user — this should NOT throw a GithubApp exception + $targetUser->delete(); + + // Assert user is deleted + expect(User::find($targetUser->id))->toBeNull(); + + // Assert the GitHub App is deleted + expect(GithubApp::find($githubApp->id))->toBeNull(); + + // Assert the application is deleted + expect(Application::find($application->id))->toBeNull(); +}); + +it('does not delete system-wide github apps when deleting a different team', function () { + // Create a system-wide GitHub App owned by the root team + $rootPrivateKey = PrivateKey::factory()->create(['team_id' => $this->rootTeam->id]); + $systemGithubApp = GithubApp::create([ + 'name' => 'System GitHub App', + 'team_id' => $this->rootTeam->id, + 'private_key_id' => $rootPrivateKey->id, + 'api_url' => 'https://api.github.com', + 'html_url' => 'https://github.com', + 'is_public' => false, + 'is_system_wide' => true, + ]); + + // Create a target user with their own team + $targetUser = User::factory()->create(); + $targetTeam = $targetUser->teams()->first(); + + // Create an application on the target team that uses the system-wide GitHub App + $privateKey = PrivateKey::factory()->create(['team_id' => $targetTeam->id]); + $server = Server::factory()->create([ + 'team_id' => $targetTeam->id, + 'private_key_id' => $privateKey->id, + ]); + $destination = StandaloneDocker::factory()->create(['server_id' => $server->id]); + $project = Project::factory()->create(['team_id' => $targetTeam->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + + $application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => StandaloneDocker::class, + 'source_id' => $systemGithubApp->id, + 'source_type' => GithubApp::class, + ]); + + // Delete the target user — should NOT throw or delete the system-wide GitHub App + $targetUser->delete(); + + // Assert user is deleted + expect(User::find($targetUser->id))->toBeNull(); + + // Assert the system-wide GitHub App still exists + expect(GithubApp::find($systemGithubApp->id))->not->toBeNull(); +}); + +it('nullifies source references on other teams apps when deleting a user', function () { + // Create the user to be deleted with their own team + $targetUser = User::factory()->create(); + $targetTeam = $targetUser->teams()->first(); + + // Create a GitHub App owned by the target team + $targetPrivateKey = PrivateKey::factory()->create(['team_id' => $targetTeam->id]); + $githubApp = GithubApp::create([ + 'name' => 'Target GitHub App', + 'team_id' => $targetTeam->id, + 'private_key_id' => $targetPrivateKey->id, + 'api_url' => 'https://api.github.com', + 'html_url' => 'https://github.com', + 'is_public' => false, + ]); + + // Create an application on the ADMIN's team that uses the target team's GitHub App + $adminPrivateKey = PrivateKey::factory()->create(['team_id' => $this->rootTeam->id]); + $adminServer = Server::factory()->create([ + 'team_id' => $this->rootTeam->id, + 'private_key_id' => $adminPrivateKey->id, + ]); + $adminDestination = StandaloneDocker::factory()->create(['server_id' => $adminServer->id]); + $adminProject = Project::factory()->create(['team_id' => $this->rootTeam->id]); + $adminEnvironment = Environment::factory()->create(['project_id' => $adminProject->id]); + + $otherTeamApp = Application::factory()->create([ + 'environment_id' => $adminEnvironment->id, + 'destination_id' => $adminDestination->id, + 'destination_type' => StandaloneDocker::class, + 'source_id' => $githubApp->id, + 'source_type' => GithubApp::class, + ]); + + // Delete the target user — should succeed, nullifying the source reference + $targetUser->delete(); + + // Assert user is deleted + expect(User::find($targetUser->id))->toBeNull(); + + // Assert the other team's application still exists but source is nullified + $otherTeamApp->refresh(); + expect($otherTeamApp)->not->toBeNull(); + expect($otherTeamApp->source_id)->toBeNull(); + expect($otherTeamApp->source_type)->toBeNull(); +}); From 7d2c776ae7e2f0b442a6d46416345bb638742f60 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Thu, 9 Apr 2026 14:51:52 +0200 Subject: [PATCH 2/2] fix(team): transfer instance-wide sources to root team on deletion Instead of nullifying source references on applications when a team is deleted, transfer instance-wide GitHub/GitLab apps to the root team (team_id=0) so they remain available to other teams that depend on them. Non-instance-wide sources are still deleted along with the team. --- app/Models/Team.php | 27 +++--- app/Models/User.php | 17 ---- .../Feature/UserDeletionWithGithubAppTest.php | 92 +++++++++++++++---- 3 files changed, 88 insertions(+), 48 deletions(-) diff --git a/app/Models/Team.php b/app/Models/Team.php index 479bf4e00..d5d564444 100644 --- a/app/Models/Team.php +++ b/app/Models/Team.php @@ -71,27 +71,31 @@ protected static function booted() } }); - static::deleting(function ($team) { - $keys = $team->privateKeys; - foreach ($keys as $key) { + static::deleting(function (Team $team) { + foreach ($team->privateKeys as $key) { $key->delete(); } - // Only delete sources owned by this team, not system-wide ones from other teams + + // Transfer instance-wide sources to root team so they remain available + GithubApp::where('team_id', $team->id)->where('is_system_wide', true)->update(['team_id' => 0]); + GitlabApp::where('team_id', $team->id)->where('is_system_wide', true)->update(['team_id' => 0]); + + // Delete non-instance-wide sources owned by this team $teamSources = GithubApp::where('team_id', $team->id)->get() ->merge(GitlabApp::where('team_id', $team->id)->get()); foreach ($teamSources as $source) { $source->delete(); } - $tags = Tag::whereTeamId($team->id)->get(); - foreach ($tags as $tag) { + + foreach (Tag::whereTeamId($team->id)->get() as $tag) { $tag->delete(); } - $shared_variables = $team->environment_variables(); - foreach ($shared_variables as $shared_variable) { - $shared_variable->delete(); + + foreach ($team->environment_variables()->get() as $sharedVariable) { + $sharedVariable->delete(); } - $s3s = $team->s3s; - foreach ($s3s as $s3) { + + foreach ($team->s3s as $s3) { $s3->delete(); } }); @@ -340,4 +344,5 @@ public function webhookNotificationSettings() { return $this->hasOne(WebhookNotificationSettings::class); } + } diff --git a/app/Models/User.php b/app/Models/User.php index 7a9600ec4..3199d2024 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -176,23 +176,6 @@ private static function finalizeTeamDeletion(User $user, Team $team) $project->forceDelete(); } - // Detach applications from other teams that reference this team's sources, - // so the GithubApp/GitlabApp deleting guard doesn't block team deletion - $githubAppIds = GithubApp::where('team_id', $team->id)->pluck('id'); - $gitlabAppIds = GitlabApp::where('team_id', $team->id)->pluck('id'); - - if ($githubAppIds->isNotEmpty()) { - Application::where('source_type', GithubApp::class) - ->whereIn('source_id', $githubAppIds) - ->update(['source_id' => null, 'source_type' => null]); - } - - if ($gitlabAppIds->isNotEmpty()) { - Application::where('source_type', GitlabApp::class) - ->whereIn('source_id', $gitlabAppIds) - ->update(['source_id' => null, 'source_type' => null]); - } - $team->members()->detach($user->id); $team->delete(); } diff --git a/tests/Feature/UserDeletionWithGithubAppTest.php b/tests/Feature/UserDeletionWithGithubAppTest.php index d8986e378..b2ccbccb0 100644 --- a/tests/Feature/UserDeletionWithGithubAppTest.php +++ b/tests/Feature/UserDeletionWithGithubAppTest.php @@ -118,49 +118,101 @@ expect(GithubApp::find($systemGithubApp->id))->not->toBeNull(); }); -it('nullifies source references on other teams apps when deleting a user', function () { - // Create the user to be deleted with their own team +it('transfers instance-wide github app to root team when owning user is deleted', function () { + // Create a user whose team owns an instance-wide GitHub App $targetUser = User::factory()->create(); $targetTeam = $targetUser->teams()->first(); - // Create a GitHub App owned by the target team $targetPrivateKey = PrivateKey::factory()->create(['team_id' => $targetTeam->id]); - $githubApp = GithubApp::create([ - 'name' => 'Target GitHub App', + $instanceWideApp = GithubApp::create([ + 'name' => 'Instance-Wide GitHub App', 'team_id' => $targetTeam->id, 'private_key_id' => $targetPrivateKey->id, 'api_url' => 'https://api.github.com', 'html_url' => 'https://github.com', 'is_public' => false, + 'is_system_wide' => true, ]); - // Create an application on the ADMIN's team that uses the target team's GitHub App - $adminPrivateKey = PrivateKey::factory()->create(['team_id' => $this->rootTeam->id]); - $adminServer = Server::factory()->create([ + // Create an application on the ROOT team that uses this instance-wide GitHub App + $rootPrivateKey = PrivateKey::factory()->create(['team_id' => $this->rootTeam->id]); + $rootServer = Server::factory()->create([ 'team_id' => $this->rootTeam->id, - 'private_key_id' => $adminPrivateKey->id, + 'private_key_id' => $rootPrivateKey->id, ]); - $adminDestination = StandaloneDocker::factory()->create(['server_id' => $adminServer->id]); - $adminProject = Project::factory()->create(['team_id' => $this->rootTeam->id]); - $adminEnvironment = Environment::factory()->create(['project_id' => $adminProject->id]); + $rootDestination = StandaloneDocker::factory()->create(['server_id' => $rootServer->id]); + $rootProject = Project::factory()->create(['team_id' => $this->rootTeam->id]); + $rootEnvironment = Environment::factory()->create(['project_id' => $rootProject->id]); $otherTeamApp = Application::factory()->create([ - 'environment_id' => $adminEnvironment->id, - 'destination_id' => $adminDestination->id, + 'environment_id' => $rootEnvironment->id, + 'destination_id' => $rootDestination->id, 'destination_type' => StandaloneDocker::class, - 'source_id' => $githubApp->id, + 'source_id' => $instanceWideApp->id, 'source_type' => GithubApp::class, ]); - // Delete the target user — should succeed, nullifying the source reference + // Delete the user — should succeed and transfer the instance-wide app to root team $targetUser->delete(); // Assert user is deleted expect(User::find($targetUser->id))->toBeNull(); - // Assert the other team's application still exists but source is nullified + // Assert the instance-wide GitHub App is preserved and transferred to root team + $instanceWideApp->refresh(); + expect($instanceWideApp)->not->toBeNull(); + expect($instanceWideApp->team_id)->toBe($this->rootTeam->id); + + // Assert the other team's application still has its source intact $otherTeamApp->refresh(); - expect($otherTeamApp)->not->toBeNull(); - expect($otherTeamApp->source_id)->toBeNull(); - expect($otherTeamApp->source_type)->toBeNull(); + expect($otherTeamApp->source_id)->toBe($instanceWideApp->id); + expect($otherTeamApp->source_type)->toBe(GithubApp::class); +}); + +it('transfers instance-wide github app to root team when team is deleted directly', function () { + // Create a team that owns an instance-wide GitHub App + $targetUser = User::factory()->create(); + $targetTeam = $targetUser->teams()->first(); + + $targetPrivateKey = PrivateKey::factory()->create(['team_id' => $targetTeam->id]); + $instanceWideApp = GithubApp::create([ + 'name' => 'Instance-Wide GitHub App', + 'team_id' => $targetTeam->id, + 'private_key_id' => $targetPrivateKey->id, + 'api_url' => 'https://api.github.com', + 'html_url' => 'https://github.com', + 'is_public' => false, + 'is_system_wide' => true, + ]); + + // Create an application on the ROOT team that uses this instance-wide GitHub App + $rootPrivateKey = PrivateKey::factory()->create(['team_id' => $this->rootTeam->id]); + $rootServer = Server::factory()->create([ + 'team_id' => $this->rootTeam->id, + 'private_key_id' => $rootPrivateKey->id, + ]); + $rootDestination = StandaloneDocker::factory()->create(['server_id' => $rootServer->id]); + $rootProject = Project::factory()->create(['team_id' => $this->rootTeam->id]); + $rootEnvironment = Environment::factory()->create(['project_id' => $rootProject->id]); + + $otherTeamApp = Application::factory()->create([ + 'environment_id' => $rootEnvironment->id, + 'destination_id' => $rootDestination->id, + 'destination_type' => StandaloneDocker::class, + 'source_id' => $instanceWideApp->id, + 'source_type' => GithubApp::class, + ]); + + // Delete the team directly — should transfer instance-wide app to root team + $targetTeam->delete(); + + // Assert the instance-wide GitHub App is preserved and transferred to root team + $instanceWideApp->refresh(); + expect($instanceWideApp)->not->toBeNull(); + expect($instanceWideApp->team_id)->toBe($this->rootTeam->id); + + // Assert the other team's application still has its source intact + $otherTeamApp->refresh(); + expect($otherTeamApp->source_id)->toBe($instanceWideApp->id); + expect($otherTeamApp->source_type)->toBe(GithubApp::class); });