From 87d12829c4a40b259a1a32e8677a564f1db6a387 Mon Sep 17 00:00:00 2001 From: Thomas Krampl Date: Wed, 30 Sep 2026 15:31:23 +0200 Subject: [PATCH] Disallow service accounts from team deletion flow Block service accounts from requesting or confirming team deletion, even when they have the Team owner role. Add authz coverage and integration tests to verify rejected attempts do not create keys, confirm deletions, or write activity log entries. Update deletion docs to clarify the human-user requirement. --- docs/deleting_teams.md | 3 ++ integration_tests/confirm_team_deletion.lua | 47 +++++++++++++++++++++ integration_tests/request_team_deletion.lua | 38 +++++++++++++++++ internal/auth/authz/authz_test.go | 11 +++++ internal/auth/authz/queries.go | 4 ++ 5 files changed, 103 insertions(+) diff --git a/docs/deleting_teams.md b/docs/deleting_teams.md index 368b31829..beaf800cb 100644 --- a/docs/deleting_teams.md +++ b/docs/deleting_teams.md @@ -23,6 +23,9 @@ have to perform the following operations: with owner B. 2. Owner B visits the URL generated by owner A, and confirms the delete request. +Both participants must be human users. Service accounts cannot request or confirm +team deletion, even when assigned the Team owner role. + Once a delete request is confirmed it **cannot** be aborted. Since a team has many external resources (GitHub team, GCP project, CDN bucket, ...) the deletion process is diff --git a/integration_tests/confirm_team_deletion.lua b/integration_tests/confirm_team_deletion.lua index b3b9486c6..0a7859deb 100644 --- a/integration_tests/confirm_team_deletion.lua +++ b/integration_tests/confirm_team_deletion.lua @@ -4,6 +4,9 @@ local user2 = User.new() team:addOwner(user1, user2) +local serviceAccount = ServiceAccount.new("team-owner", team:slug()) +serviceAccount:assignRole("Team owner") + Test.gql("Create delete key", function(t) t.addHeader("x-user-email", user1:email()) @@ -56,6 +59,50 @@ Test.gql("Confirm team deletion with the same user", function(t) } end) +Test.gql("Service account owner cannot confirm team deletion", function(t) + t.addHeader("authorization", "Bearer " .. serviceAccount:token()) + + t.query(string.format([[ + mutation { + confirmTeamDeletion(input: { + slug: "%s" + key: "%s" + }) { + deletionStarted + } + } + ]], team:slug(), State.key)) + + t.check { + data = Null, + errors = { + { + locations = NotNull(), + message = "You are authenticated, but your account is not authorized to perform this action.", + path = { "confirmTeamDeletion" }, + }, + }, + } +end) + +Test.sql("Rejected service account confirmation leaves deletion pending", function(t) + t.queryRow([[ + SELECT + team_delete_keys.confirmed_at, + teams.delete_key_confirmed_at, + (SELECT COUNT(*) FROM activity_log_entries WHERE team_slug = $2 AND action = 'CONFIRM_DELETE_KEY') AS activity_count + FROM team_delete_keys + JOIN teams ON teams.slug = team_delete_keys.team_slug + WHERE team_delete_keys.key = $1 + ]], State.key, team:slug()) + + t.check { + confirmed_at = Null, + delete_key_confirmed_at = Null, + activity_count = 0, + } +end) + Test.gql("Confirm team deletion", function(t) t.addHeader("x-user-email", user2:email()) diff --git a/integration_tests/request_team_deletion.lua b/integration_tests/request_team_deletion.lua index 8152dd340..55768a456 100644 --- a/integration_tests/request_team_deletion.lua +++ b/integration_tests/request_team_deletion.lua @@ -3,6 +3,8 @@ local user = User.new("Authenticated User", "auth@example.com", "auth-external-i team:addOwner(user) local otherUser = User.new("Other User", "other@example.com", "other") +local serviceAccount = ServiceAccount.new("team-owner", team:slug()) +serviceAccount:assignRole("Team owner") local deleteKey = Helper.SQLQueryRow([[ INSERT INTO team_delete_keys ( @@ -14,6 +16,42 @@ local deleteKey = Helper.SQLQueryRow([[ ) RETURNING key::TEXT; ]], team:slug(), otherUser:email()) +Test.gql("Service account owner cannot request team deletion", function(t) + t.addHeader("authorization", "Bearer " .. serviceAccount:token()) + + t.query(string.format([[ + mutation { + requestTeamDeletion(input: { slug: "%s" }) { + key { key } + } + } + ]], team:slug())) + + t.check { + data = Null, + errors = { + { + locations = NotNull(), + message = "You are authenticated, but your account is not authorized to perform this action.", + path = { "requestTeamDeletion" }, + }, + }, + } +end) + +Test.sql("Rejected service account request creates no key or activity log", function(t) + t.queryRow([[ + SELECT + (SELECT COUNT(*) FROM team_delete_keys WHERE team_slug = $1) AS key_count, + (SELECT COUNT(*) FROM activity_log_entries WHERE team_slug = $1 AND action = 'CREATE_DELETE_KEY') AS activity_count + ]], team:slug()) + + t.check { + key_count = 1, + activity_count = 0, + } +end) + Test.gql("Request team deletion", function(t) t.addHeader("x-user-email", user:email()) diff --git a/internal/auth/authz/authz_test.go b/internal/auth/authz/authz_test.go index 5a8a9591c..d62e5dbe8 100644 --- a/internal/auth/authz/authz_test.go +++ b/internal/auth/authz/authz_test.go @@ -2,6 +2,7 @@ package authz_test import ( "context" + "errors" "testing" "github.com/google/go-cmp/cmp" @@ -34,3 +35,13 @@ func TestContextWithUser(t *testing.T) { t.Errorf("diff: -want +got\n%s", diff) } } + +func TestCanDeleteTeamRejectsServiceAccounts(t *testing.T) { + actor := authz.NewMockAuthenticatedUser(t) + actor.EXPECT().IsServiceAccount().Return(true).Once() + + ctx := authz.ContextWithActor(context.Background(), actor, nil) + if err := authz.CanDeleteTeam(ctx, "some-team"); !errors.Is(err, authz.ErrUnauthorized) { + t.Fatalf("expected unauthorized error, got %v", err) + } +} diff --git a/internal/auth/authz/queries.go b/internal/auth/authz/queries.go index e9df53b29..b62feb0b2 100644 --- a/internal/auth/authz/queries.go +++ b/internal/auth/authz/queries.go @@ -265,6 +265,10 @@ func CanUpdateTeamMetadata(ctx context.Context, teamSlug slug.Slug) error { } func CanDeleteTeam(ctx context.Context, teamSlug slug.Slug) error { + if ActorFromContext(ctx).User.IsServiceAccount() { + return ErrUnauthorized + } + return requireTeamAuthorization(ctx, teamSlug, "teams:delete") }