From 0870e1dd15d7de6b55fbaf2553d614d3f70c581c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 20 Jul 2026 15:41:29 +0000 Subject: [PATCH] Use sentinel errors for campaign validation failures Follow the cookiebanner pattern: grouped var Err* sentinels in the service package, wrapped with fmt.Errorf where context is needed, and explicit errors.Is checks in GraphQL resolvers. Signed-off-by: Cursor Agent Co-authored-by: Bryan FRIMIN --- pkg/accessreview/campaign_service.go | 71 +++++++++------- pkg/accessreview/errors.go | 74 +--------------- pkg/accessreview/errors_test.go | 85 ------------------- .../v1/access_review_campaign_resolvers.go | 8 +- 4 files changed, 46 insertions(+), 192 deletions(-) delete mode 100644 pkg/accessreview/errors_test.go diff --git a/pkg/accessreview/campaign_service.go b/pkg/accessreview/campaign_service.go index 88c6cc88d..3360150e1 100644 --- a/pkg/accessreview/campaign_service.go +++ b/pkg/accessreview/campaign_service.go @@ -65,10 +65,11 @@ func (s *Service) CreateCampaign( } if source.OrganizationID != campaign.OrganizationID { - return &CampaignSourceOrganizationMismatchError{ - Operation: "create", - SourceID: sourceID, - } + return fmt.Errorf( + "cannot create campaign: access source %s does not belong to the same organization: %w", + sourceID, + ErrCampaignSourceOrganizationMismatch, + ) } if err := s.upsertCampaignSource(ctx, conn, scope, campaign.ID, source); err != nil { @@ -157,11 +158,12 @@ func (s *Service) UpdateCampaign( } if campaign.Status != coredata.AccessReviewCampaignStatusDraft { - return &CampaignInvalidStatusError{ - Operation: "update", - Status: campaign.Status, - Expected: coredata.AccessReviewCampaignStatusDraft, - } + return fmt.Errorf( + "cannot update campaign: status is %s, expected %s: %w", + campaign.Status, + coredata.AccessReviewCampaignStatusDraft, + ErrCampaignNotDraft, + ) } if req.Name != nil && *req.Name != nil { @@ -244,11 +246,12 @@ func (s *Service) AddCampaignSource( } if campaign.Status != coredata.AccessReviewCampaignStatusDraft { - return &CampaignInvalidStatusError{ - Operation: "add scope source", - Status: campaign.Status, - Expected: coredata.AccessReviewCampaignStatusDraft, - } + return fmt.Errorf( + "cannot add scope source: campaign status is %s, expected %s: %w", + campaign.Status, + coredata.AccessReviewCampaignStatusDraft, + ErrCampaignNotDraft, + ) } source := &coredata.AccessReviewSource{} @@ -257,10 +260,11 @@ func (s *Service) AddCampaignSource( } if source.OrganizationID != campaign.OrganizationID { - return &CampaignSourceOrganizationMismatchError{ - Operation: "add scope source", - SourceID: req.AccessReviewSourceID, - } + return fmt.Errorf( + "cannot add scope source: access source %q does not belong to the same organization: %w", + req.AccessReviewSourceID, + ErrCampaignSourceOrganizationMismatch, + ) } if err := s.upsertCampaignSource(ctx, conn, scope, campaign.ID, source); err != nil { @@ -296,11 +300,12 @@ func (s *Service) RemoveCampaignSource( } if campaign.Status != coredata.AccessReviewCampaignStatusDraft { - return &CampaignInvalidStatusError{ - Operation: "remove scope source", - Status: campaign.Status, - Expected: coredata.AccessReviewCampaignStatusDraft, - } + return fmt.Errorf( + "cannot remove scope source: campaign status is %s, expected %s: %w", + campaign.Status, + coredata.AccessReviewCampaignStatusDraft, + ErrCampaignNotDraft, + ) } campaignSource := &coredata.AccessReviewCampaignSource{} @@ -348,10 +353,11 @@ func (s *Service) syncCampaignSources( } if source.OrganizationID != campaign.OrganizationID { - return &CampaignSourceOrganizationMismatchError{ - Operation: "update", - SourceID: sourceID, - } + return fmt.Errorf( + "cannot update campaign: access source %s does not belong to the same organization: %w", + sourceID, + ErrCampaignSourceOrganizationMismatch, + ) } if err := s.upsertCampaignSource(ctx, conn, scope, campaign.ID, source); err != nil { @@ -408,11 +414,12 @@ func (s *Service) StartCampaign( } if campaign.Status != coredata.AccessReviewCampaignStatusDraft { - return &CampaignInvalidStatusError{ - Operation: "start", - Status: campaign.Status, - Expected: coredata.AccessReviewCampaignStatusDraft, - } + return fmt.Errorf( + "cannot start campaign: status is %s, expected %s: %w", + campaign.Status, + coredata.AccessReviewCampaignStatusDraft, + ErrCampaignNotDraft, + ) } var campaignSources coredata.AccessReviewCampaignSources diff --git a/pkg/accessreview/errors.go b/pkg/accessreview/errors.go index 4e559518a..62db3b9a3 100644 --- a/pkg/accessreview/errors.go +++ b/pkg/accessreview/errors.go @@ -20,80 +20,10 @@ package accessreview -import ( - "errors" - "fmt" - - "go.probo.inc/probo/pkg/coredata" - "go.probo.inc/probo/pkg/gid" -) +import "errors" var ( ErrCampaignNoScopeSources = errors.New("cannot start campaign: no scope sources configured") - ErrCampaignInvalidStatus = errors.New("campaign status does not allow this operation") + ErrCampaignNotDraft = errors.New("campaign must be in draft status") ErrCampaignSourceOrganizationMismatch = errors.New("access source does not belong to the same organization") ) - -type CampaignInvalidStatusError struct { - Operation string - Status coredata.AccessReviewCampaignStatus - Expected coredata.AccessReviewCampaignStatus -} - -func (e *CampaignInvalidStatusError) Error() string { - switch e.Operation { - case "add scope source", "remove scope source": - return fmt.Sprintf( - "cannot %s: campaign status is %s, expected %s", - e.Operation, - e.Status, - e.Expected, - ) - default: - return fmt.Sprintf( - "cannot %s campaign: status is %s, expected %s", - e.Operation, - e.Status, - e.Expected, - ) - } -} - -func (e *CampaignInvalidStatusError) Is(target error) bool { - return target == ErrCampaignInvalidStatus -} - -type CampaignSourceOrganizationMismatchError struct { - Operation string - SourceID gid.GID -} - -func (e *CampaignSourceOrganizationMismatchError) Error() string { - switch e.Operation { - case "create": - return fmt.Sprintf( - "cannot create campaign: access source %s does not belong to the same organization", - e.SourceID, - ) - case "update": - return fmt.Sprintf( - "cannot update campaign: access source %s does not belong to the same organization", - e.SourceID, - ) - default: - return fmt.Sprintf( - "cannot add scope source: access source %q does not belong to the same organization", - e.SourceID, - ) - } -} - -func (e *CampaignSourceOrganizationMismatchError) Is(target error) bool { - return target == ErrCampaignSourceOrganizationMismatch -} - -func IsCampaignClientError(err error) bool { - return errors.Is(err, ErrCampaignNoScopeSources) || - errors.Is(err, ErrCampaignInvalidStatus) || - errors.Is(err, ErrCampaignSourceOrganizationMismatch) -} diff --git a/pkg/accessreview/errors_test.go b/pkg/accessreview/errors_test.go deleted file mode 100644 index 683da4ddc..000000000 --- a/pkg/accessreview/errors_test.go +++ /dev/null @@ -1,85 +0,0 @@ -// Copyright (c) 2026 Probo Inc . -// -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files (the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: -// -// The above copyright notice and this permission notice shall be included in -// all copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -// SOFTWARE. - -package accessreview - -import ( - "errors" - "fmt" - "testing" - - "github.com/stretchr/testify/assert" - "go.probo.inc/probo/pkg/coredata" - "go.probo.inc/probo/pkg/gid" -) - -func TestCampaignClientErrors(t *testing.T) { - t.Parallel() - - statusErr := &CampaignInvalidStatusError{ - Operation: "start", - Status: coredata.AccessReviewCampaignStatusInProgress, - Expected: coredata.AccessReviewCampaignStatusDraft, - } - sourceErr := &CampaignSourceOrganizationMismatchError{ - Operation: "update", - SourceID: gid.GID("source-id"), - } - - tests := []struct { - name string - err error - want bool - }{ - { - name: "no scope sources", - err: ErrCampaignNoScopeSources, - want: true, - }, - { - name: "invalid status", - err: statusErr, - want: true, - }, - { - name: "wrapped invalid status", - err: fmt.Errorf("cannot start access review campaign: %w", statusErr), - want: true, - }, - { - name: "source organization mismatch", - err: sourceErr, - want: true, - }, - { - name: "internal error", - err: errors.New("cannot lock campaign: timeout"), - want: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - assert.Equal(t, tt.want, IsCampaignClientError(tt.err)) - }) - } -} diff --git a/pkg/server/api/console/v1/access_review_campaign_resolvers.go b/pkg/server/api/console/v1/access_review_campaign_resolvers.go index 435ffc78a..e507e7e17 100644 --- a/pkg/server/api/console/v1/access_review_campaign_resolvers.go +++ b/pkg/server/api/console/v1/access_review_campaign_resolvers.go @@ -724,7 +724,7 @@ func (r *mutationResolver) CreateAccessReviewCampaign(ctx context.Context, input }, ) if err != nil { - if accessreview.IsCampaignClientError(err) { + if errors.Is(err, accessreview.ErrCampaignSourceOrganizationMismatch) { return nil, gqlutils.Invalid(ctx, err) } @@ -760,7 +760,8 @@ func (r *mutationResolver) UpdateAccessReviewCampaign(ctx context.Context, input return nil, gqlutils.NotFound(ctx, err) } - if accessreview.IsCampaignClientError(err) { + if errors.Is(err, accessreview.ErrCampaignNotDraft) || + errors.Is(err, accessreview.ErrCampaignSourceOrganizationMismatch) { return nil, gqlutils.Invalid(ctx, err) } @@ -805,7 +806,8 @@ func (r *mutationResolver) StartAccessReviewCampaign(ctx context.Context, input campaign, err := r.accessReview.StartCampaign(ctx, scope, input.AccessReviewCampaignID) if err != nil { - if accessreview.IsCampaignClientError(err) { + if errors.Is(err, accessreview.ErrCampaignNoScopeSources) || + errors.Is(err, accessreview.ErrCampaignNotDraft) { return nil, gqlutils.Invalid(ctx, err) }