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") }