Filter out campaign.status.errors based on repo permissions

Author: mrnugget

enterprise/internal/campaigns/resolvers/campaigns.go

@@ -8,9 +8,11 @@ import (
 	"time"
 
 	"github.com/graph-gophers/graphql-go"
+	"github.com/sourcegraph/sourcegraph/cmd/frontend/db"
 	"github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend"
 	"github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend/graphqlutil"
 	ee "github.com/sourcegraph/sourcegraph/enterprise/internal/campaigns"
+	"github.com/sourcegraph/sourcegraph/internal/api"
 	"github.com/sourcegraph/sourcegraph/internal/campaigns"
 )
 
@@ -322,9 +324,78 @@ func (r *campaignResolver) Status(ctx context.Context) (graphqlbackend.Backgroun
 		return nil, err
 	}
 
+	if !canAdmin {
+		// If the user doesn't have admin permissions for this campaign, we
+		// don't need to filter out specific errors, but can simply exclude
+		// _all_ errors.
+		return r.store.GetCampaignStatus(ctx, ee.GetCampaignStatusOpts{
+			ID:            r.Campaign.ID,
+			ExcludeErrors: true,
+		})
+	}
+
+	// TODO: Wow, this is horrible. We're loading way too many patches.
+	// What we actually want is this:
+
+	//   SELECT repo.id
+	//   FROM   patches
+	//   JOIN   repo           ON repo.id = patches.repo_id
+	//   JOIN   changeset_jobs ON changeset_jobs.patch_id = patches.id
+	//   WHERE patches.patch_set_id = patch.patch_set_id;
+
+	// Or we do:
+	//
+	//   SELECT repo.id
+	//   FROM   changeset_jobs
+	//   JOIN   patches ON patches.id = changeset_jobs.patch_id
+	//   JOIN   repo    ON patches.repo_id = repo.id
+	//   WHERE  changeset_jobs.campaign_id = <campaign_id>
+
+	// And then we put those repo IDs through `db.Repo.GetByIDs`, which
+	// uses the authz filter what we then get back are the repos we have access
+	// to.
+
+	// And then we need to filter out the error messages of changeset_jobs that
+	// are attached to patches that are attached to filtered out repositories in the
+	// `GetCampaignStatus` query below.
+
+	patches, _, err := r.store.ListPatches(ctx, ee.ListPatchesOpts{
+		PatchSetID: r.Campaign.PatchSetID,
+		Limit:      -1,
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	repoIDs := make([]api.RepoID, 0, len(patches))
+	for _, p := range patches {
+		repoIDs = append(repoIDs, p.RepoID)
+	}
+
+	// 🚨 SECURITY: We use db.Repos.GetByIDs to filter out repositories the
+	// user doesn't have access to.
+	accessibleRepos, err := db.Repos.GetByIDs(ctx, repoIDs...)
+	if err != nil {
+		return nil, err
+	}
+
+	accessibleRepoIDs := make(map[api.RepoID]struct{}, len(accessibleRepos))
+	for _, r := range accessibleRepos {
+		accessibleRepoIDs[r.ID] = struct{}{}
+	}
+
+	// We now check which repositories in `repoIDs` are not in `accessibleRepoIDs`.
+	// We have to filter the error messages associated with those out.
+	excludedRepos := make([]api.RepoID, 0, len(accessibleRepoIDs))
+	for _, id := range repoIDs {
+		if _, ok := accessibleRepoIDs[id]; !ok {
+			excludedRepos = append(excludedRepos, id)
+		}
+	}
+
 	return r.store.GetCampaignStatus(ctx, ee.GetCampaignStatusOpts{
-		ID:            r.Campaign.ID,
-		ExcludeErrors: !canAdmin,
+		ID:                   r.Campaign.ID,
+		ExcludeErrorsInRepos: excludedRepos,
 	})
 }
 

enterprise/internal/campaigns/resolvers/changesets.go

@@ -85,6 +85,7 @@ func (r *changesetsConnectionResolver) PageInfo(ctx context.Context) (*graphqlut
 	if err != nil {
 		return nil, err
 	}
+
 	return graphqlutil.HasNextPage(next != 0), nil
 }
 

enterprise/internal/campaigns/resolvers/permissions_test.go

@@ -5,6 +5,7 @@ import (
 	"database/sql"
 	"fmt"
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/sourcegraph/sourcegraph/cmd/frontend/authz"
@@ -30,6 +31,8 @@ func TestRepositoryPermissions(t *testing.T) {
 		t.Skip()
 	}
 
+	now := time.Now().UTC().Truncate(time.Microsecond)
+
 	// We need to enable read access so that non-site-admin users can access
 	// the API and we can check for their admin rights.
 	// This can be removed once we enable campaigns for all users and only
@@ -129,6 +132,24 @@ func TestRepositoryPermissions(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// Create 2 failed ChangesetJobs for the patchess to produce error messages
+	// on the campaign.
+	changesetJobs := make([]*campaigns.ChangesetJob, 0, 2)
+	for _, p := range patches {
+		job := &campaigns.ChangesetJob{
+			CampaignID: campaign.ID,
+			PatchID:    p.ID,
+			Error:      fmt.Sprintf("error patch %d", p.ID),
+			StartedAt:  now,
+			FinishedAt: now,
+		}
+		if err := store.CreateChangesetJob(ctx, job); err != nil {
+			t.Fatal(err)
+		}
+
+		changesetJobs = append(changesetJobs, job)
+	}
+
 	// TODO: Do we also need failed ChangesetJobs to check for hidden errors
 
 	var response struct{ Node apitest.Campaign }
@@ -143,6 +164,14 @@ func TestRepositoryPermissions(t *testing.T) {
 		t.Fatalf("campaign id is wrong. have %q, want %q", have, want)
 	}
 
+	wantErrors := []string{
+		fmt.Sprintf("error patch %d", patches[0].ID),
+		fmt.Sprintf("error patch %d", patches[1].ID),
+	}
+	if diff := cmp.Diff(response.Node.Status.Errors, wantErrors); diff != "" {
+		t.Fatalf("unexpected status errors (-want +got):\n%s", diff)
+	}
+
 	changesetTypes := map[string]int{}
 	for _, c := range response.Node.Changesets.Nodes {
 		changesetTypes[c.Typename]++
@@ -162,10 +191,12 @@ func TestRepositoryPermissions(t *testing.T) {
 	}
 
 	// Now we add the authzFilter and filter out 2 repositories
+
 	filteredRepoIDs := map[api.RepoID]bool{
 		patches[0].RepoID:    true,
 		changesets[0].RepoID: true,
 	}
+
 	db.MockAuthzFilter = func(ctx context.Context, repos []*types.Repo, p authz.Perms) ([]*types.Repo, error) {
 		var filtered []*types.Repo
 		for _, r := range repos {
@@ -185,6 +216,14 @@ func TestRepositoryPermissions(t *testing.T) {
 		t.Fatalf("campaign id is wrong. have %q, want %q", have, want)
 	}
 
+	wantErrors = []string{
+		// patches[0] is filtered out
+		fmt.Sprintf("error patch %d", patches[1].ID),
+	}
+	if diff := cmp.Diff(response.Node.Status.Errors, wantErrors); diff != "" {
+		t.Fatalf("unexpected status errors (-want +got):\n%s", diff)
+	}
+
 	changesetTypes = map[string]int{}
 	for _, c := range response.Node.Changesets.Nodes {
 		changesetTypes[c.Typename]++
@@ -203,13 +242,23 @@ func TestRepositoryPermissions(t *testing.T) {
 	if diff := cmp.Diff(wantPatchTypes, patchTypes); diff != "" {
 		t.Fatalf("unexpected patch types (-want +got):\n%s", diff)
 	}
+
+	// TODO: Test that the diffStat on `patchset` doesn't include the filtered patches diff stats
+	// TODO: Test that the diffStat on `campaign` doesn't include the filtered changesets diff stats
+	// TODO: Test that ChangesetByID and PatchByID don't return the filtered out changesets/patches
 }
 
 const queryCampaignPermLevels = `
 query {
   node(id: %q) {
     ... on Campaign {
       id
+
+	  status {
+	    state
+		errors
+	  }
+
       changesets(first: 100) {
         nodes {
           __typename

enterprise/internal/campaigns/store.go

@@ -1744,6 +1744,13 @@ type GetCampaignStatusOpts struct {
 	// BackgroundProcessStatus returned by GetCampaignStatus won't be
 	// populated.
 	ExcludeErrors bool
+
+	// ExcludeErrorsInRepos filters out error messages from ChangesetJobs that
+	// are associated with Patches that have the given repository IDs set in
+	// `patches.repo_id`.
+	// This is used to filter out error messages from repositories the user
+	// doesn't have access to.
+	ExcludeErrorsInRepos []api.RepoID
 }
 
 // GetCampaignStatus gets the campaigns.BackgroundProcessStatus for a Campaign
@@ -1767,6 +1774,19 @@ func getCampaignStatusQuery(opts *GetCampaignStatusOpts) *sqlf.Query {
 		errorsPreds = append(errorsPreds, sqlf.Sprintf("FALSE"))
 	}
 
+	if len(opts.ExcludeErrorsInRepos) > 0 {
+		ids := make([]*sqlf.Query, 0, len(opts.ExcludeErrorsInRepos))
+
+		for _, repoID := range opts.ExcludeErrorsInRepos {
+			ids = append(ids, sqlf.Sprintf("%s", repoID))
+		}
+
+		joined := sqlf.Join(ids, ",")
+
+		errorsPreds = append(errorsPreds, sqlf.Sprintf("patches.repo_id NOT IN (%s)", joined))
+		errorsPreds = append(errorsPreds, sqlf.Sprintf("error != ''"))
+	}
+
 	if len(errorsPreds) == 0 {
 		errorsPreds = append(errorsPreds, sqlf.Sprintf("error != ''"))
 	}
@@ -1813,6 +1833,7 @@ SELECT
   COUNT(*) FILTER (WHERE error != '') AS failed,
   array_agg(error) FILTER (WHERE %s) AS errors
 FROM changeset_jobs
+JOIN patches ON patches.id = changeset_jobs.patch_id
 WHERE %s
 LIMIT 1
 `

enterprise/internal/campaigns/store_test.go

@@ -2679,8 +2679,19 @@ func testStoreChangesetJobs(t *testing.T, ctx context.Context, s *Store, _ repos
 
 		for campaignID, tc := range tests {
 			for i, j := range tc.jobs {
+				p := &cmpgn.Patch{
+					RepoID:     api.RepoID(i),
+					PatchSetID: int64(campaignID),
+					BaseRef:    "deadbeef",
+					Diff:       "foobar",
+				}
+
+				if err := s.CreatePatch(ctx, p); err != nil {
+					t.Fatal(err)
+				}
+
 				j.CampaignID = int64(campaignID)
-				j.PatchID = int64(i)
+				j.PatchID = p.ID
 
 				err := s.CreateChangesetJob(ctx, j)
 				if err != nil {
@@ -2701,6 +2712,93 @@ func testStoreChangesetJobs(t *testing.T, ctx context.Context, s *Store, _ repos
 			}
 		}
 	})
+	t.Run("BackgroundProcessStatus_ErrorsOnlyInRepos", func(t *testing.T) {
+		var campaignID int64 = 123456
+
+		patches := []*cmpgn.Patch{
+			{RepoID: 444, PatchSetID: 888, BaseRef: "deadbeef", Diff: "foobar"},
+			{RepoID: 555, PatchSetID: 888, BaseRef: "deadbeef", Diff: "foobar"},
+			{RepoID: 666, PatchSetID: 888, BaseRef: "deadbeef", Diff: "foobar"},
+		}
+		for _, p := range patches {
+			if err := s.CreatePatch(ctx, p); err != nil {
+				t.Fatal(err)
+			}
+		}
+
+		jobs := []*cmpgn.ChangesetJob{
+			// completed, no errors
+			{PatchID: patches[0].ID, StartedAt: clock.now(), FinishedAt: clock.now(), ChangesetID: 23},
+			// completed, error
+			{PatchID: patches[1].ID, StartedAt: clock.now(), FinishedAt: clock.now(), Error: "error1"},
+			// completed, another error
+			{PatchID: patches[2].ID, StartedAt: clock.now(), FinishedAt: clock.now(), Error: "error2"},
+		}
+
+		for _, j := range jobs {
+			j.CampaignID = campaignID
+
+			err := s.CreateChangesetJob(ctx, j)
+			if err != nil {
+				t.Fatal(err)
+			}
+		}
+
+		opts := GetCampaignStatusOpts{
+			ID:                   campaignID,
+			ExcludeErrorsInRepos: []api.RepoID{patches[2].RepoID},
+		}
+
+		want := &cmpgn.BackgroundProcessStatus{
+			ProcessState:  cmpgn.BackgroundProcessStateErrored,
+			Total:         3,
+			Completed:     3,
+			Failed:        2,
+			Pending:       0,
+			ProcessErrors: []string{"error1"},
+			// error2 should be excluded
+		}
+
+		status, err := s.GetCampaignStatus(ctx, opts)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if diff := cmp.Diff(status, want); diff != "" {
+			t.Fatalf("wrong diff: %s", diff)
+		}
+
+		// Now we filter out all errors, but still want the ProcessState to be
+		// correct
+		opts = GetCampaignStatusOpts{
+			ID: campaignID,
+			ExcludeErrorsInRepos: []api.RepoID{
+				patches[0].RepoID,
+				patches[1].RepoID,
+				patches[2].RepoID,
+			},
+		}
+
+		want = &cmpgn.BackgroundProcessStatus{
+			// This should stay "Errored", even though no errors are returned.
+			ProcessState:  cmpgn.BackgroundProcessStateErrored,
+			Total:         3,
+			Completed:     3,
+			Failed:        2,
+			Pending:       0,
+			ProcessErrors: nil,
+			// error1 and error2 should be excluded
+		}
+
+		status, err = s.GetCampaignStatus(ctx, opts)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if diff := cmp.Diff(status, want); diff != "" {
+			t.Fatalf("wrong diff: %s", diff)
+		}
+	})
 
 	t.Run("ResetFailedChangesetJobs", func(t *testing.T) {
 		campaignID := 9999