diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index d3611e134..bc6268cea 100644 --- a/internal/api/openapi.yaml +++ b/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 diff --git a/internal/auth/group_system_managed_919_test.go b/internal/auth/group_system_managed_919_test.go new file mode 100644 index 000000000..d7bcea4d5 --- /dev/null +++ b/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") + }) +} diff --git a/internal/auth/group_system_managed_db_test.go b/internal/auth/group_system_managed_db_test.go new file mode 100644 index 000000000..48ae1c21d --- /dev/null +++ b/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) +} diff --git a/internal/auth/service_api.go b/internal/auth/service_api.go index 967f9483d..f8a5e3ff0 100644 --- a/internal/auth/service_api.go +++ b/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), } diff --git a/internal/auth/service_api_fields_test.go b/internal/auth/service_api_fields_test.go index 90e46478d..41aebcbe5 100644 --- a/internal/auth/service_api_fields_test.go +++ b/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) diff --git a/internal/auth/service_api_test.go b/internal/auth/service_api_test.go index cebf35942..dd40e53ed 100644 --- a/internal/auth/service_api_test.go +++ b/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() diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index e1ad905d8..1c977401c 100644 --- a/internal/auth/service_group.go +++ b/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. diff --git a/internal/auth/service_group_test.go b/internal/auth/service_group_test.go index ffe3c7ef9..9971180cb 100644 --- a/internal/auth/service_group_test.go +++ b/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) diff --git a/internal/server/adapter_test.go b/internal/server/adapter_test.go index 60fccb340..55f79f9f0 100644 --- a/internal/server/adapter_test.go +++ b/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()