Skip to content

fix(user-deletion): safely clean up team-owned Git app sources#9435

Merged
andrasbacsai merged 6 commits intonextfrom
8172-investigate-user-delete-bug
Apr 9, 2026
Merged

fix(user-deletion): safely clean up team-owned Git app sources#9435
andrasbacsai merged 6 commits intonextfrom
8172-investigate-user-delete-bug

Conversation

@andrasbacsai
Copy link
Copy Markdown
Member

Summary

  • Fixes user/team deletion flow so team cleanup only deletes GitHub/GitLab app sources owned by the team being deleted.
  • Prevents accidental deletion of system-wide or other-team app sources during Team deletion.
  • Before deleting a team, nullifies applications.source_id/source_type references (for GitHub/GitLab app sources) that point to the soon-to-be-deleted team-owned sources, avoiding guard exceptions.
  • Adds feature tests covering:
  • deleting a user whose team owns a GitHub App used by applications,
  • preserving system-wide GitHub Apps when deleting another team,
  • nullifying cross-team application source references while keeping those applications.

Breaking Changes

  • None.

Fixes #8172

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.
@Iisyourdad
Copy link
Copy Markdown
Contributor

Hey @andrasbacsai this looks good for the User::delete() path, but I think the fix may still be incomplete for direct team deletion.

The new source-nullification logic lives in User::finalizeTeamDeletion(), while other flows can still call $team->delete() directly. In those cases, Team::deleting() still deletes the team-owned GitHub/GitLab app sources, but cross-team applications.source_* references may not have been cleared first, so the existing deleting guard could still block the delete.

Would it make sense to move the source-detach/nullify step into Team::deleting() (or a shared helper used by all team-delete paths), and add a regression test that deletes the team directly rather than only via User::delete()?

I could 100% be wrong, you do know the code much, much better than I ever will.

Here is my suggested fix.

protected static function booted()
{
    static::created(function ($team) {
        $team->emailNotificationSettings()->create([
            'use_instance_email_settings' => isDev(),
        ]);
        $team->discordNotificationSettings()->create();
        $team->slackNotificationSettings()->create();
        $team->telegramNotificationSettings()->create();
        $team->pushoverNotificationSettings()->create();
        $team->webhookNotificationSettings()->create();
    });

    static::saving(function ($team) {
        if (auth()->user()?->isMember()) {
            throw new \Exception('You are not allowed to update this team.');
        }
    });

    static::deleting(function (Team $team) {
        $team->detachSourceReferences();

        foreach ($team->privateKeys as $key) {
            $key->delete();
        }

        // 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();
        }

        foreach (Tag::whereTeamId($team->id)->get() as $tag) {
            $tag->delete();
        }

        foreach ($team->environment_variables()->get() as $sharedVariable) {
            $sharedVariable->delete();
        }

        foreach ($team->s3s as $s3) {
            $s3->delete();
        }
    });
}

private function detachSourceReferences(): void
{
    $githubAppIds = GithubApp::where('team_id', $this->id)->pluck('id');

    if ($githubAppIds->isNotEmpty()) {
        Application::where('source_type', GithubApp::class)
            ->whereIn('source_id', $githubAppIds)
            ->update([
                'source_id' => null,
                'source_type' => null,
            ]);
    }

    $gitlabAppIds = GitlabApp::where('team_id', $this->id)->pluck('id');

    if ($gitlabAppIds->isNotEmpty()) {
        Application::where('source_type', GitlabApp::class)
            ->whereIn('source_id', $gitlabAppIds)
            ->update([
                'source_id' => null,
                'source_type' => null,
            ]);
    }
}

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.
@andrasbacsai andrasbacsai merged commit 8adc213 into next Apr 9, 2026
4 checks passed
@andrasbacsai andrasbacsai deleted the 8172-investigate-user-delete-bug branch April 9, 2026 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants