Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/frontend/graphqlbackend/campaigns.go
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,7 @@ type PatchResolver interface {
Diff() PatchResolver
FileDiffs(ctx context.Context, args *FileDiffsConnectionArgs) (FileDiffConnection, error)
PublicationEnqueued(ctx context.Context) (bool, error)
Publishable(ctx context.Context) (bool, error)
}

type ChangesetEventsConnectionResolver interface {
Expand Down
3 changes: 3 additions & 0 deletions cmd/frontend/graphqlbackend/schema.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions cmd/frontend/graphqlbackend/schema.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,9 @@ type Patch implements PatchInterface & Node {
# - A campaign has been created with the patchset to which this patch belongs.
# - The patch has been individually published through the publishChangeset mutation.
publicationEnqueued: Boolean!

# True, when the code host of the associated repository is supported by campaigns.
publishable: Boolean!
}

# A hidden patch is a patch in a repository that the user does NOT have
Expand Down
1 change: 1 addition & 0 deletions enterprise/internal/campaigns/resolvers/apitest/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ type Patch struct {
Typename string `json:"__typename"`
ID string
PublicationEnqueued bool
Publishable bool
Repository struct{ Name, URL string }
Diff struct {
FileDiffs FileDiffs
Expand Down
8 changes: 8 additions & 0 deletions enterprise/internal/campaigns/resolvers/patch_sets.go
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,14 @@ func (r *patchResolver) PublicationEnqueued(ctx context.Context) (bool, error) {
return cj.FinishedAt.IsZero(), nil
}

func (r *patchResolver) Publishable(ctx context.Context) (bool, error) {
repo, _, err := r.computeRepoCommit(ctx)
if err != nil {
return false, err
}
return campaigns.IsRepoSupported(repo.ExternalRepo()), nil
}

func (r *patchResolver) Diff() graphqlbackend.PatchResolver {
return r
}
Expand Down
12 changes: 11 additions & 1 deletion enterprise/internal/campaigns/resolvers/patch_sets_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,9 @@ func TestPatchSetResolver(t *testing.T) {
... on Patch {
repository {
name
}
}
publicationEnqueued
publishable
diff {
fileDiffs(first: %d, after: %s) {
rawDiff
Expand Down Expand Up @@ -179,6 +181,14 @@ func TestPatchSetResolver(t *testing.T) {
t.Fatalf("wrong Repository Name %q. want=%q", have, want)
}

if have, want := patch.PublicationEnqueued, false; have != want {
t.Fatalf("wrong publication enqueued status %t. want=%t", have, want)
}

if have, want := patch.Publishable, true; have != want {
t.Fatalf("wrong publishable status %t. want=%t", have, want)
}

if have, want := patch.Diff.FileDiffs.RawDiff, testDiff; have != want {
t.Fatalf("wrong RawDiff. diff=%s", cmp.Diff(have, want))
}
Expand Down
57 changes: 36 additions & 21 deletions enterprise/internal/campaigns/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func (s *Service) CreatePatchSetFromPatches(ctx context.Context, patches []*camp

repoIDs := make([]api.RepoID, len(patches))
for i, patch := range patches {
repoIDs[i] = api.RepoID(patch.RepoID)
repoIDs[i] = patch.RepoID
}
// 🚨 SECURITY: We use db.Repos.GetByIDs to check for which the user has access.
repos, err := db.Repos.GetByIDs(ctx, repoIDs...)
Expand All @@ -79,13 +79,9 @@ func (s *Service) CreatePatchSetFromPatches(ctx context.Context, patches []*camp
}

for _, patch := range patches {
repo, ok := reposByID[patch.RepoID]
if !ok {
if _, ok := reposByID[patch.RepoID]; !ok {
return nil, &db.RepoNotFoundErr{ID: patch.RepoID}
}
if !campaigns.IsRepoSupported(&repo.ExternalRepo) {
continue
}

patch.PatchSetID = patchSet.ID
if err := tx.CreatePatch(ctx, patch); err != nil {
Expand Down Expand Up @@ -166,6 +162,9 @@ var ErrNoPatches = errors.New("cannot create or update a Campaign without any ch
// finished execution.
var ErrCloseProcessingCampaign = errors.New("cannot close a Campaign while changesets are being created on codehosts")

// ErrUnsupportedCodehost is returned by EnqueueChangesetJobForPatch if the target repo of a patch is an unsupported repo.
var ErrUnsupportedCodehost = errors.New("cannot publish patch for unsupported codehost")

// CloseCampaign closes the Campaign with the given ID if it has not been closed yet.
func (s *Service) CloseCampaign(ctx context.Context, id int64, closeChangesets bool) (campaign *campaigns.Campaign, err error) {
traceTitle := fmt.Sprintf("campaign: %d, closeChangesets: %t", id, closeChangesets)
Expand Down Expand Up @@ -405,13 +404,13 @@ func (s *Service) AddChangesetsToCampaign(ctx context.Context, campaignID int64,
return nil, err
}

accessibleRepoIDs, err := accessibleRepos(ctx, changesets.RepoIDs())
accessibleRepos, err := accessibleRepos(ctx, changesets.RepoIDs())
if err != nil {
return nil, err
}

for _, c := range changesets {
if _, ok := accessibleRepoIDs[c.RepoID]; !ok {
if _, ok := accessibleRepos[c.RepoID]; !ok {
return nil, &db.RepoNotFoundErr{ID: c.RepoID}
}

Expand Down Expand Up @@ -524,14 +523,14 @@ func (s *Service) RetryPublishCampaign(ctx context.Context, id int64) (campaign
return nil, err
}

accessibleRepoIDs, err := accessibleRepos(ctx, repoIDs)
accessibleRepos, err := accessibleRepos(ctx, repoIDs)
if err != nil {
return nil, err
}

var resetPatchIDs []int64
for _, p := range patches {
if _, ok := accessibleRepoIDs[p.RepoID]; !ok {
if _, ok := accessibleRepos[p.RepoID]; !ok {
continue
}

Expand Down Expand Up @@ -597,7 +596,7 @@ func (s *Service) EnqueueChangesetJobs(ctx context.Context, campaignID int64) (_
repoIDs = append(repoIDs, p.RepoID)
}

accessibleRepoIDs, err := accessibleRepos(ctx, repoIDs)
accessibleRepos, err := accessibleRepos(ctx, repoIDs)
if err != nil {
return nil, err
}
Expand All @@ -620,7 +619,13 @@ func (s *Service) EnqueueChangesetJobs(ctx context.Context, campaignID int64) (_
continue
}

if _, ok := accessibleRepoIDs[p.RepoID]; !ok {
r, ok := accessibleRepos[p.RepoID]
if !ok {
continue
}

// Check if the repo is on a supported codehost.
if !campaigns.IsRepoSupported(&r.ExternalRepo) {
continue
}

Expand Down Expand Up @@ -648,6 +653,13 @@ func (s *Service) EnqueueChangesetJobForPatch(ctx context.Context, patchID int64
if err != nil {
return err
}
repo, err := db.Repos.Get(ctx, job.RepoID)
if err != nil {
return err
}
if !campaigns.IsRepoSupported(&repo.ExternalRepo) {
return ErrUnsupportedCodehost
}

campaign, err := s.store.GetCampaign(ctx, GetCampaignOpts{PatchSetID: job.PatchSetID})
if err != nil {
Expand Down Expand Up @@ -1020,14 +1032,14 @@ func resetAccessibleChangesetJobs(ctx context.Context, tx *Store, campaign *camp
repoIDs = append(repoIDs, p.RepoID)
}

accessibleRepoIDs, err := accessibleRepos(ctx, repoIDs)
accessibleRepos, err := accessibleRepos(ctx, repoIDs)
if err != nil {
return err
}

var resetPatchIDs []int64
for _, p := range patches {
if _, ok := accessibleRepoIDs[p.RepoID]; !ok {
if _, ok := accessibleRepos[p.RepoID]; !ok {
continue
}

Expand Down Expand Up @@ -1111,7 +1123,7 @@ func computeCampaignUpdateDiff(
}
// 🚨 SECURITY: Check which repositories the user has access to. If the
// user doesn't have access, don't create/delete/update anything.
accessibleRepoIDs, err := accessibleRepos(ctx, repoIDs)
accessibleRepos, err := accessibleRepos(ctx, repoIDs)
if err != nil {
return nil, err
}
Expand All @@ -1121,14 +1133,17 @@ func computeCampaignUpdateDiff(
// return an error instead of skipping patches, so we don't end up with
// an unfixable state (i.e. unpublished patch + changeset for same
// repo).
if _, ok := accessibleRepoIDs[j.RepoID]; !ok {
repo, ok := accessibleRepos[j.RepoID]
if !ok {
return nil, &db.RepoNotFoundErr{ID: j.RepoID}
}
if !campaigns.IsRepoSupported(&repo.ExternalRepo) {
continue
}

if group, ok := byRepoID[j.RepoID]; ok {
group.newPatch = j
} else {

// If we have new Patches that don't match an existing
// ChangesetJob we need to create new ChangesetJobs.
diff.Create = append(diff.Create, &campaigns.ChangesetJob{
Expand All @@ -1141,7 +1156,7 @@ func computeCampaignUpdateDiff(
for repoID, group := range byRepoID {
// If the user is lacking permissions for this repository we don't
// delete/update the changeset.
if _, ok := accessibleRepoIDs[repoID]; !ok {
if _, ok := accessibleRepos[repoID]; !ok {
continue
}

Expand Down Expand Up @@ -1282,17 +1297,17 @@ func hasCampaignAdminPermissions(ctx context.Context, c *campaigns.Campaign) (bo
// accessibleRepos collects the RepoIDs of the changesets and returns a set of
// the api.RepoID for which the subset of repositories for which the actor in
// ctx has read permissions.
func accessibleRepos(ctx context.Context, ids []api.RepoID) (map[api.RepoID]struct{}, error) {
func accessibleRepos(ctx context.Context, ids []api.RepoID) (map[api.RepoID]*types.Repo, error) {
// 🚨 SECURITY: We use db.Repos.GetByIDs to filter out repositories the
// user doesn't have access to.
accessibleRepos, err := db.Repos.GetByIDs(ctx, ids...)
if err != nil {
return nil, err
}

accessibleRepoIDs := make(map[api.RepoID]struct{}, len(accessibleRepos))
accessibleRepoIDs := make(map[api.RepoID]*types.Repo, len(accessibleRepos))
for _, r := range accessibleRepos {
accessibleRepoIDs[r.ID] = struct{}{}
accessibleRepoIDs[r.ID] = r
}

return accessibleRepoIDs, nil
Expand Down
32 changes: 30 additions & 2 deletions enterprise/internal/campaigns/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,13 @@ func TestService(t *testing.T) {
rs = append(rs, r)
}

awsCodeCommitRepoID := 4
{
r := testRepo(awsCodeCommitRepoID, extsvc.TypeAWSCodeCommit)
r.Sources = map[string]*repos.SourceInfo{ext.URN(): {ID: ext.URN()}}
rs = append(rs, r)
}

err := reposStore.UpsertRepos(ctx, rs...)
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -615,6 +622,15 @@ func TestService(t *testing.T) {
if !haveJob3.FinishedAt.IsZero() {
t.Errorf("unexpected changesetJob FinishedAt value: %v. want=%v", haveJob3.FinishedAt, time.Time{})
}

// Update repo to unsupported codehost type.
awsPatch := testPatch(patchSet.ID, rs[awsCodeCommitRepoID].ID, now)
if err := store.CreatePatch(ctx, awsPatch); err != nil {
t.Fatal(err)
}
if wantErr, haveErr := ErrUnsupportedCodehost, svc.EnqueueChangesetJobForPatch(ctx, awsPatch.ID); wantErr != haveErr {
t.Fatalf("got invalid error, want=%v have=%v", wantErr, haveErr)
}
})

t.Run("EnqueueChangesetJobs", func(t *testing.T) {
Expand All @@ -640,7 +656,7 @@ func TestService(t *testing.T) {
}

// Filter out one repository to make sure it's skipped
filteredOutPatch := patches[len(patches)-1]
filteredOutPatch := patches[len(patches)-2]
db.MockAuthzFilter = func(ctx context.Context, repos []*types.Repo, p authz.Perms) ([]*types.Repo, error) {
var filtered []*types.Repo
for _, r := range repos {
Expand All @@ -665,7 +681,7 @@ func TestService(t *testing.T) {
t.Fatal(err)
}

wantJobsCount := len(patches) - 1 // We filtered out one repository
wantJobsCount := len(patches) - 2 // We filtered out one repository, and one is unsupported.
if have, want := len(haveJobs), wantJobsCount; have != want {
t.Fatal("wrong number of changeset jobs created")
}
Expand Down Expand Up @@ -1285,6 +1301,7 @@ func TestService_UpdateCampaignWithNewPatchSetID(t *testing.T) {
for i := 0; i < 4; i++ {
rs = append(rs, testRepo(i, extsvc.TypeGitHub))
}
rs = append(rs, testRepo(len(rs), extsvc.TypeAWSCodeCommit))

reposStore := repos.NewDBStore(dbconn.Global, sql.TxOptions{})
err := reposStore.UpsertRepos(ctx, rs...)
Expand Down Expand Up @@ -1521,6 +1538,17 @@ func TestService_UpdateCampaignWithNewPatchSetID(t *testing.T) {
wantUnmodified: repoNames{"repo-0"},
wantCreated: repoNames{"repo-1"},
},
{
name: "1 added on unsupported codehost",
updatePatchSet: true,
oldPatches: repoNames{"repo-0"},
newPatches: []newPatchSpec{
{repo: "repo-0"},
{repo: "repo-4"},
},
wantUnmodified: repoNames{"repo-0"},
wantCreated: []string{},
},
}

for _, tt := range tests {
Expand Down
2 changes: 0 additions & 2 deletions web/src/enterprise/campaigns/detail/CampaignDetails.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -583,8 +583,6 @@ export const CampaignDetails: React.FunctionComponent<Props> = ({
patchSet={patchSet!}
campaignUpdates={campaignUpdates}
changesetUpdates={changesetUpdates}
// No publishing allowed in create view.
enablePublishing={false}
history={history}
location={location}
isLightTheme={isLightTheme}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,6 @@ exports[`CampaignDetails creation form given existing patch set 1`] = `
<PatchSetPatches
campaignUpdates="[Subject]"
changesetUpdates="[Subject]"
enablePublishing={false}
history="[History]"
isLightTheme={true}
location="[Location path=/?patchSet=p]"
Expand Down
2 changes: 2 additions & 0 deletions web/src/enterprise/campaigns/detail/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ export const queryPatchesFromCampaign = (
name
url
}
publishable
publicationEnqueued
diff {
fileDiffs {
Expand Down Expand Up @@ -383,6 +384,7 @@ export const queryPatchesFromPatchSet = (
name
url
}
publishable
publicationEnqueued
diff {
fileDiffs {
Expand Down
Loading