fix(auth): expose system_managed on API groups and guard UpdateGroup

internal/api/openapi.yaml

@@ -6199,6 +6199,12 @@ components:
             account matches nothing.
           items:
             type: string
+        system_managed:
+          type: boolean
+          description: >
+            True for a built-in group seeded and maintained by migrations
+            (for example Administrators and Purchaser). Updating or deleting
+            it returns 403. Omitted when false.
         created_at:
           type: string
           format: date-time

internal/auth/group_system_managed_919_test.go

@@ -0,0 +1,130 @@
+package auth
+
+// Issue #919: the system_managed guard must live on Service.UpdateGroup
+// itself, not only on its caller UpdateGroupAPI, and the API representation
+// of a group must carry the flag so the dashboard can hide Edit and Delete.
+
+import (
+	"context"
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+func seededPurchaserGroup() *Group {
+	return &Group{
+		ID:            DefaultPurchaserGroupID,
+		Name:          GroupPurchaser,
+		SystemManaged: true,
+		Permissions: []Permission{
+			{Action: ActionExecute, Resource: ResourcePurchases},
+			{Action: ActionApproveAny, Resource: ResourcePurchases},
+		},
+	}
+}
+
+func TestServiceUpdateGroup_SystemManagedGuard(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("refuses a system-managed group even when the caller's struct omits the flag", func(t *testing.T) {
+		mockStore := new(MockStore)
+		t.Cleanup(func() { mockStore.AssertExpectations(t) })
+		svc := newCeilingService(t, mockStore)
+
+		mockStore.On("GetGroup", ctx, DefaultPurchaserGroupID).Return(seededPurchaserGroup(), nil).Once()
+		mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Maybe()
+
+		// A caller-built struct: SystemManaged is false here, so the guard
+		// has to consult the stored row rather than trust its argument.
+		wiped := &Group{
+			ID:          DefaultPurchaserGroupID,
+			Name:        GroupPurchaser,
+			Permissions: []Permission{{Action: ActionView, Resource: ResourceAll}},
+		}
+		err := svc.UpdateGroup(ctx, wiped)
+
+		require.Error(t, err)
+		assert.ErrorIs(t, err, ErrSystemManagedGroup)
+		assert.Contains(t, err.Error(), GroupPurchaser)
+		mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything)
+	})
+
+	t.Run("a missing group is reported as not found without writing", func(t *testing.T) {
+		mockStore := new(MockStore)
+		t.Cleanup(func() { mockStore.AssertExpectations(t) })
+		svc := newCeilingService(t, mockStore)
+
+		mockStore.On("GetGroup", ctx, ceilingTargetID).Return(nil, nil).Once()
+
+		err := svc.UpdateGroup(ctx, &Group{ID: ceilingTargetID, Name: "Gone"})
+
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "group not found")
+		assert.NotErrorIs(t, err, ErrSystemManagedGroup)
+		mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything)
+	})
+
+	t.Run("an ordinary group is still updatable", func(t *testing.T) {
+		mockStore := new(MockStore)
+		t.Cleanup(func() { mockStore.AssertExpectations(t) })
+		svc := newCeilingService(t, mockStore)
+
+		group := &Group{ID: ceilingTargetID, Name: "Team"}
+		mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{ID: ceilingTargetID, Name: "Old"}, nil).Once()
+		mockStore.On("UpdateGroup", ctx, group).Return(nil).Once()
+
+		require.NoError(t, svc.UpdateGroup(ctx, group))
+	})
+}
+
+// TestAPIGroup_CarriesSystemManaged pins the wire field the dashboard reads
+// (frontend/src/api/types.ts APIGroup.system_managed) on both read paths.
+func TestAPIGroup_CarriesSystemManaged(t *testing.T) {
+	ctx := context.Background()
+	custom := &Group{ID: ceilingTargetID, Name: "Team"}
+
+	wire := func(t *testing.T, v any) map[string]any {
+		t.Helper()
+		raw, err := json.Marshal(v)
+		require.NoError(t, err)
+		var m map[string]any
+		require.NoError(t, json.Unmarshal(raw, &m))
+		return m
+	}
+
+	t.Run("GetGroupAPI", func(t *testing.T) {
+		mockStore := new(MockStore)
+		t.Cleanup(func() { mockStore.AssertExpectations(t) })
+		svc := newCeilingService(t, mockStore)
+		mockStore.On("GetGroup", ctx, DefaultPurchaserGroupID).Return(seededPurchaserGroup(), nil).Once()
+		mockStore.On("GetGroup", ctx, ceilingTargetID).Return(custom, nil).Once()
+
+		seeded, err := svc.GetGroupAPI(ctx, DefaultPurchaserGroupID)
+		require.NoError(t, err)
+		assert.Equal(t, true, wire(t, seeded)["system_managed"])
+
+		plain, err := svc.GetGroupAPI(ctx, ceilingTargetID)
+		require.NoError(t, err)
+		assert.NotContains(t, wire(t, plain), "system_managed", "false is omitted, matching the optional frontend field")
+	})
+
+	t.Run("ListGroupsAPI", func(t *testing.T) {
+		mockStore := new(MockStore)
+		t.Cleanup(func() { mockStore.AssertExpectations(t) })
+		svc := newCeilingService(t, mockStore)
+		mockStore.On("ListGroups", ctx).Return([]Group{*seededPurchaserGroup(), *custom}, nil).Once()
+
+		list, err := svc.ListGroupsAPI(ctx)
+		require.NoError(t, err)
+		raw, err := json.Marshal(list)
+		require.NoError(t, err)
+		var groups []map[string]any
+		require.NoError(t, json.Unmarshal(raw, &groups))
+		require.Len(t, groups, 2)
+		assert.Equal(t, true, groups[0]["system_managed"])
+		assert.NotContains(t, groups[1], "system_managed")
+	})
+}

internal/auth/group_system_managed_db_test.go

@@ -0,0 +1,62 @@
+//go:build integration
+
+package auth
+
+// Issue #919 against the real schema: the seeded groups come back from the
+// API layer flagged system_managed, and Service.UpdateGroup refuses to write
+// one even when handed a struct that does not carry the flag, leaving the
+// stored row unchanged.
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestIntegration_SystemManagedGroup_APIFlagAndUpdateGuard(t *testing.T) {
+	store := NewPostgresStore(setupAuthTestDB(t))
+	svc := NewService(ServiceConfig{Store: store})
+	ctx := t.Context()
+
+	listed, err := svc.ListGroupsAPI(ctx)
+	require.NoError(t, err)
+	groups, ok := listed.([]*APIGroup)
+	require.True(t, ok)
+	flags := make(map[string]bool, len(groups))
+	for _, g := range groups {
+		flags[g.ID] = g.SystemManaged
+	}
+	require.Contains(t, flags, DefaultAdminGroupID)
+	require.Contains(t, flags, DefaultPurchaserGroupID)
+	assert.True(t, flags[DefaultAdminGroupID], "Administrators must be flagged system_managed")
+	assert.True(t, flags[DefaultPurchaserGroupID], "Purchaser must be flagged system_managed")
+
+	before, err := store.GetGroup(ctx, DefaultPurchaserGroupID)
+	require.NoError(t, err)
+	require.NotNil(t, before)
+
+	wiped := &Group{
+		ID:          DefaultPurchaserGroupID,
+		Name:        before.Name + " (renamed)",
+		Permissions: []Permission{{Action: ActionView, Resource: ResourceAll}},
+	}
+	err = svc.UpdateGroup(ctx, wiped)
+	require.ErrorIs(t, err, ErrSystemManagedGroup)
+
+	after, err := store.GetGroup(ctx, DefaultPurchaserGroupID)
+	require.NoError(t, err)
+	assert.Equal(t, before.Name, after.Name)
+	assert.Equal(t, before.Permissions, after.Permissions)
+	assert.True(t, after.UpdatedAt.Equal(before.UpdatedAt), "a refused update must not touch the row")
+
+	// Negative control: a custom group still updates through the same path.
+	custom := &Group{Name: "Team 919", Permissions: []Permission{{Action: ActionView, Resource: ResourceAll}}}
+	require.NoError(t, svc.CreateGroup(ctx, custom, ""))
+	custom.Description = "edited"
+	require.NoError(t, svc.UpdateGroup(ctx, custom))
+	stored, err := store.GetGroup(ctx, custom.ID)
+	require.NoError(t, err)
+	assert.Equal(t, "edited", stored.Description)
+	assert.False(t, stored.SystemManaged)
+}

internal/auth/service_api.go

@@ -44,6 +44,10 @@ type APIGroup struct {
 	UpdatedAt       string          `json:"updated_at,omitempty"`
 	Permissions     []APIPermission `json:"permissions"`
 	AllowedAccounts []string        `json:"allowed_accounts"`
+	// SystemManaged marks a group seeded and maintained by migrations. The
+	// API refuses to update or delete it (ErrSystemManagedGroup), and the
+	// dashboard reads this flag to hide Edit and Delete (issue #919).
+	SystemManaged bool `json:"system_managed,omitempty"`
 }
 
 // APIPermission is the permission type for API responses.
@@ -199,6 +203,7 @@ func groupToAPIGroup(g *Group) *APIGroup {
 		Description:     g.Description,
 		Permissions:     apiPerms,
 		AllowedAccounts: allowedAccounts,
+		SystemManaged:   g.SystemManaged,
 		CreatedAt:       g.CreatedAt.Format(time.RFC3339),
 		UpdatedAt:       g.UpdatedAt.Format(time.RFC3339),
 	}

internal/auth/service_api_fields_test.go

@@ -188,7 +188,9 @@ func TestAPIAdapters_UpdateGroup_AllFieldsReachStore(t *testing.T) {
 		store := new(MockStore)
 		svc := createTestService(store, new(MockEmailSender))
 		var got *Group
-		store.On("GetGroup", ctx, "group-123").Return(existing(), nil).Once()
+		// Twice: UpdateGroupAPI loads the group, then Service.UpdateGroup
+		// re-reads it for its own system_managed guard (issue #919).
+		store.On("GetGroup", ctx, "group-123").Return(existing(), nil).Twice()
 		store.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).
 			Run(func(args mock.Arguments) { got = args.Get(1).(*Group) }).Return(nil).Once()
 		_, err := svc.UpdateGroupAPI(ctx, AdminAPIKeyActorID, "group-123", req)

internal/auth/service_api_test.go

@@ -484,7 +484,9 @@ func TestService_UpdateGroupAPI(t *testing.T) {
 			Description: "Old description",
 			Permissions: []Permission{{Action: ActionAdmin, Resource: ResourceAll}},
 		}
-		mockStore.On("GetGroup", ctx, "group-123").Return(existingGroup, nil).Once()
+		// Twice: UpdateGroupAPI loads the group, then Service.UpdateGroup
+		// re-reads it for its own system_managed guard (issue #919).
+		mockStore.On("GetGroup", ctx, "group-123").Return(existingGroup, nil).Twice()
 		mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once()
 
 		req := APIUpdateGroupRequest{
@@ -515,7 +517,9 @@ func TestService_UpdateGroupAPI(t *testing.T) {
 			Permissions: []Permission{},
 		}
 
-		mockStore.On("GetGroup", ctx, "group-123").Return(existingGroup, nil).Once()
+		// Twice: UpdateGroupAPI loads the group, then Service.UpdateGroup
+		// re-reads it for its own system_managed guard (issue #919).
+		mockStore.On("GetGroup", ctx, "group-123").Return(existingGroup, nil).Twice()
 		// The group gains its first permission, so the issue #1409 member
 		// check runs; the group has no members.
 		mockStore.On("ListUsers", ctx).Return([]User{}, nil).Once()

internal/auth/service_group.go

@@ -27,7 +27,18 @@ func (s *Service) CreateGroup(ctx context.Context, group *Group, createdBy strin
 }
 
 // UpdateGroup updates a permission group.
+//
+// Updating a system-managed group is refused here as well as in
+// UpdateGroupAPI, so the guard does not depend on every caller remembering
+// to check first (issue #919). The check reads the STORED row rather than
+// trusting group.SystemManaged: the caller's struct is built by the caller
+// and may not carry the flag. The stored flag cannot change at runtime (no
+// write path sets system_managed; only migrations do), so the read-then-write
+// has no window in which a concurrent request could flip it.
 func (s *Service) UpdateGroup(ctx context.Context, group *Group) error {
+	if err := s.refuseSystemManagedGroup(ctx, group.ID); err != nil {
+		return err
+	}
 	return s.store.UpdateGroup(ctx, group)
 }
 
@@ -40,6 +51,16 @@ func (s *Service) UpdateGroup(ctx context.Context, group *Group) error {
 // so the purchase path would be dead tenant-wide until someone re-ran the
 // migration by hand.
 func (s *Service) DeleteGroup(ctx context.Context, groupID string) error {
+	if err := s.refuseSystemManagedGroup(ctx, groupID); err != nil {
+		return err
+	}
+	return s.store.DeleteGroup(ctx, groupID)
+}
+
+// refuseSystemManagedGroup loads the stored group and returns
+// ErrSystemManagedGroup if it is seeded by migrations, or a not-found error
+// if it does not exist. Shared by the UpdateGroup and DeleteGroup write paths.
+func (s *Service) refuseSystemManagedGroup(ctx context.Context, groupID string) error {
 	group, err := s.store.GetGroup(ctx, groupID)
 	if err != nil && !errors.Is(err, pgx.ErrNoRows) {
 		return fmt.Errorf("failed to load group %s: %w", groupID, err)
@@ -50,7 +71,7 @@ func (s *Service) DeleteGroup(ctx context.Context, groupID string) error {
 	if group.SystemManaged {
 		return fmt.Errorf("%w: %q is seeded and maintained by migrations", ErrSystemManagedGroup, group.Name)
 	}
-	return s.store.DeleteGroup(ctx, groupID)
+	return nil
 }
 
 // GetGroup returns a group by ID.

internal/auth/service_group_test.go

@@ -229,6 +229,10 @@ func TestService_UpdateGroup(t *testing.T) {
 			Name: "Updated Team",
 		}
 
+		// UpdateGroup loads the stored group first so it can refuse to
+		// reshape a system-managed one (issue #919).
+		mockStore.On("GetGroup", ctx, "group-123").
+			Return(&Group{ID: "group-123", Name: "Team"}, nil).Once()
 		mockStore.On("UpdateGroup", ctx, group).Return(nil).Once()
 
 		err := service.UpdateGroup(ctx, group)

internal/server/adapter_test.go

@@ -396,10 +396,12 @@ func TestAuthServiceAdapter_UpdateGroupAPI(t *testing.T) {
 	adapter, mockStore := createMockAuthService(t)
 	ctx := context.Background()
 
+	// Twice: UpdateGroupAPI loads the group, then Service.UpdateGroup
+	// re-reads it for its own system_managed guard (issue #919).
 	mockStore.On("GetGroup", ctx, "group-1").Return(&auth.Group{
 		ID:   "group-1",
 		Name: "old-name",
-	}, nil).Once()
+	}, nil).Twice()
 	mockStore.On("UpdateGroup", ctx, mock.MatchedBy(func(g *auth.Group) bool {
 		return g.ID == "group-1" && g.Name == "new-name"
 	})).Return(nil).Once()