diff --git a/internal/app/compliance/simulation.go b/internal/app/compliance/simulation.go index 4768ffa7..53630391 100644 --- a/internal/app/compliance/simulation.go +++ b/internal/app/compliance/simulation.go @@ -270,7 +270,9 @@ func (s *SimulationService) RecordControlTestResult(ctx context.Context, input R return nil, err } - ct.RecordResult(simulation.ControlTestStatus(input.Status), input.Evidence, input.Notes, testerID) + if err := ct.RecordResult(simulation.ControlTestStatus(input.Status), input.Evidence, input.Notes, testerID); err != nil { + return nil, err + } if err := s.controlRepo.Update(ctx, ct); err != nil { return nil, fmt.Errorf("failed to record control test result: %w", err) diff --git a/pkg/domain/simulation/control.go b/pkg/domain/simulation/control.go index 08f25134..b7df0c31 100644 --- a/pkg/domain/simulation/control.go +++ b/pkg/domain/simulation/control.go @@ -19,6 +19,27 @@ const ( ControlTestStatusNotApplicable ControlTestStatus = "not_applicable" ) +// AllControlTestStatuses is the complete accepted vocabulary. +// +// There is no CHECK constraint on control_tests.status, so this list is the +// only thing standing between a client-supplied string and the column. Adding +// a constant above without adding it here makes it unusable; adding it here +// without the constant will not compile. +func AllControlTestStatuses() []ControlTestStatus { + return []ControlTestStatus{ + ControlTestStatusUntested, + ControlTestStatusPass, + ControlTestStatusFail, + ControlTestStatusPartial, + ControlTestStatusNotApplicable, + } +} + +// IsValid reports whether the status is one the platform understands. +func (s ControlTestStatus) IsValid() bool { + return slices.Contains(AllControlTestStatuses(), s) +} + // ControlTest represents a security control effectiveness test. type ControlTest struct { id shared.ID @@ -84,7 +105,7 @@ func ReconstituteControlTest( framework: framework, controlID: controlID, controlName: controlName, category: category, testProcedure: testProcedure, expectedResult: expectedResult, - status: status, + status: status, lastTestedAt: lastTestedAt, lastTestedBy: lastTestedBy, evidence: evidence, notes: notes, riskLevel: riskLevel, linkedSimulationIDs: linkedSimulationIDs, tags: tags, @@ -94,25 +115,25 @@ func ReconstituteControlTest( // Getters func (c *ControlTest) ID() shared.ID { return c.id } -func (c *ControlTest) TenantID() shared.ID { return c.tenantID } -func (c *ControlTest) Name() string { return c.name } -func (c *ControlTest) Description() string { return c.description } -func (c *ControlTest) Framework() string { return c.framework } -func (c *ControlTest) ControlID() string { return c.controlID } -func (c *ControlTest) ControlName() string { return c.controlName } -func (c *ControlTest) Category() string { return c.category } -func (c *ControlTest) TestProcedure() string { return c.testProcedure } -func (c *ControlTest) ExpectedResult() string { return c.expectedResult } -func (c *ControlTest) Status() ControlTestStatus { return c.status } -func (c *ControlTest) LastTestedAt() *time.Time { return c.lastTestedAt } -func (c *ControlTest) LastTestedBy() *shared.ID { return c.lastTestedBy } -func (c *ControlTest) Evidence() string { return c.evidence } -func (c *ControlTest) Notes() string { return c.notes } -func (c *ControlTest) RiskLevel() string { return c.riskLevel } -func (c *ControlTest) LinkedSimulationIDs() []string { return c.linkedSimulationIDs } -func (c *ControlTest) Tags() []string { return c.tags } -func (c *ControlTest) CreatedAt() time.Time { return c.createdAt } -func (c *ControlTest) UpdatedAt() time.Time { return c.updatedAt } +func (c *ControlTest) TenantID() shared.ID { return c.tenantID } +func (c *ControlTest) Name() string { return c.name } +func (c *ControlTest) Description() string { return c.description } +func (c *ControlTest) Framework() string { return c.framework } +func (c *ControlTest) ControlID() string { return c.controlID } +func (c *ControlTest) ControlName() string { return c.controlName } +func (c *ControlTest) Category() string { return c.category } +func (c *ControlTest) TestProcedure() string { return c.testProcedure } +func (c *ControlTest) ExpectedResult() string { return c.expectedResult } +func (c *ControlTest) Status() ControlTestStatus { return c.status } +func (c *ControlTest) LastTestedAt() *time.Time { return c.lastTestedAt } +func (c *ControlTest) LastTestedBy() *shared.ID { return c.lastTestedBy } +func (c *ControlTest) Evidence() string { return c.evidence } +func (c *ControlTest) Notes() string { return c.notes } +func (c *ControlTest) RiskLevel() string { return c.riskLevel } +func (c *ControlTest) LinkedSimulationIDs() []string { return c.linkedSimulationIDs } +func (c *ControlTest) Tags() []string { return c.tags } +func (c *ControlTest) CreatedAt() time.Time { return c.createdAt } +func (c *ControlTest) UpdatedAt() time.Time { return c.updatedAt } // Update sets mutable fields. func (c *ControlTest) Update(name, description, controlName, category string) { @@ -133,7 +154,16 @@ func (c *ControlTest) SetTestDetails(procedure, expected string) { } // RecordResult records a test result. -func (c *ControlTest) RecordResult(status ControlTestStatus, evidence, notes string, testedBy shared.ID) { +// RecordResult stores a test outcome. An unknown status is rejected rather than +// stored: nothing else validates it — RecordControlTestResult casts the request +// string straight through and control_tests has no CHECK constraint — so an +// arbitrary value would persist and then match none of the status filters, +// leaving a control that reads as neither passed nor failed. +func (c *ControlTest) RecordResult(status ControlTestStatus, evidence, notes string, testedBy shared.ID) error { + if !status.IsValid() { + return fmt.Errorf("%w: unknown control test status %q", shared.ErrValidation, status) + } + now := time.Now() c.status = status c.evidence = evidence @@ -141,6 +171,7 @@ func (c *ControlTest) RecordResult(status ControlTestStatus, evidence, notes str c.lastTestedAt = &now c.lastTestedBy = &testedBy c.updatedAt = now + return nil } // LinkSimulation links a simulation to this control test. diff --git a/pkg/domain/simulation/control_status_test.go b/pkg/domain/simulation/control_status_test.go new file mode 100644 index 00000000..b9fe1fa8 --- /dev/null +++ b/pkg/domain/simulation/control_status_test.go @@ -0,0 +1,99 @@ +package simulation + +import ( + "errors" + "testing" + + "github.com/openctemio/api/pkg/domain/shared" +) + +// RecordResult used to cast whatever string arrived straight into the entity +// and persist it. Nothing else checked: RecordControlTestResult does +// `simulation.ControlTestStatus(input.Status)` on the raw request field, and +// control_tests has no CHECK constraint on status — unlike compensating_controls, +// which does. +// +// The consequence is quiet. A control recorded as "Pass" or "passed" is stored +// happily and then matches neither `status === 'pass'` nor `'fail'` in the +// Control Testing page's own summary, so it reads as neither passed nor failed — +// a tested control that looks untested. + +func TestRecordResult_RejectsUnknownStatus(t *testing.T) { + bad := []string{ + "Pass", // capitalised + "passed", // near-miss + "PARTIAL", // + "not-applicable", // hyphen instead of underscore + "", // empty + "untested ", // trailing space + } + + for _, in := range bad { + t.Run(in, func(t *testing.T) { + ct := &ControlTest{status: ControlTestStatusUntested} + + err := ct.RecordResult(ControlTestStatus(in), "evidence", "notes", shared.NewID()) + if err == nil { + t.Fatalf("status %q was accepted; it will persist and then match none "+ + "of the status filters, so the control reads as neither passed nor "+ + "failed", in) + } + if !errors.Is(err, shared.ErrValidation) { + t.Errorf("error should be a validation error, got %v", err) + } + if ct.status != ControlTestStatusUntested { + t.Errorf("the entity was mutated despite the rejection: status = %q", ct.status) + } + if ct.lastTestedAt != nil { + t.Error("lastTestedAt was set despite the rejection — the control would " + + "show a test date for a test that was refused") + } + }) + } +} + +func TestRecordResult_AcceptsEveryKnownStatus(t *testing.T) { + for _, status := range AllControlTestStatuses() { + t.Run(string(status), func(t *testing.T) { + ct := &ControlTest{status: ControlTestStatusUntested} + tester := shared.NewID() + + if err := ct.RecordResult(status, "evidence", "notes", tester); err != nil { + t.Fatalf("a status from AllControlTestStatuses was rejected: %v", err) + } + if ct.status != status { + t.Errorf("status = %q, want %q", ct.status, status) + } + if ct.lastTestedAt == nil { + t.Error("lastTestedAt was not set on a successful record") + } + if ct.lastTestedBy == nil || *ct.lastTestedBy != tester { + t.Error("lastTestedBy was not recorded") + } + }) + } +} + +// The vocabulary must stay closed. A constant added above without being listed +// in AllControlTestStatuses is silently unusable — IsValid would reject it and +// the failure would look like a client bug. +func TestAllControlTestStatuses_CoversEveryConstant(t *testing.T) { + declared := []ControlTestStatus{ + ControlTestStatusUntested, + ControlTestStatusPass, + ControlTestStatusFail, + ControlTestStatusPartial, + ControlTestStatusNotApplicable, + } + + for _, c := range declared { + if !c.IsValid() { + t.Errorf("%q is a declared constant but AllControlTestStatuses omits it, "+ + "so RecordResult would reject a value the platform defines", c) + } + } + if got, want := len(AllControlTestStatuses()), len(declared); got != want { + t.Errorf("AllControlTestStatuses has %d entries, %d constants are declared "+ + "in this test — add the new one to both", got, want) + } +}