Skip to content

Commit

Permalink
Implement permission levels for campaigns (#10961)
Browse files Browse the repository at this point in the history 
* Set campaign.ViewerCanAdminister to true for authors of campaign

* Hide errors for non-campaign-owners in campaign status

* Move permission checks to campaigns.Service and add tests

* Move tests

* Finish tests and remove permission checks for resolver level

* Remove duplicated test file that came back from the dead through a rebase

* Fix broken reference after rebase

* Clean up TestPermissionLevels

* Clean up TestServicePermissionLevels

* Add tracing to campaigns.Service methods
  • Loading branch information
@mrnugget
mrnugget committed May 27, 2020
1 parent 83deb2a commit 70a4174
Show file tree
Hide file tree
Showing 6 changed files with 742 additions and 117 deletions.
24 changes: 13 additions & 11 deletions enterprise/internal/campaigns/resolvers/apitest/types.go
View file
Expand Up @@ -93,17 +93,19 @@ type UserOrg struct {
}

type Campaign struct {
ID string
Name string
Description string
Branch string
Author User
Namespace UserOrg
CreatedAt string
UpdatedAt string
PublishedAt string
Status struct {
State string
ID string
Name string
Description string
Branch string
Author User
ViewerCanAdminister bool
Namespace UserOrg
CreatedAt string
UpdatedAt string
PublishedAt string
Status struct {
State string
Errors []string
}
Patches PatchConnection
Changesets ChangesetConnection
Expand Down
17 changes: 10 additions & 7 deletions enterprise/internal/campaigns/resolvers/campaigns.go
View file
Expand Up @@ -7,7 +7,6 @@ import (
"time"

"github.com/graph-gophers/graphql-go"
"github.com/sourcegraph/sourcegraph/cmd/frontend/backend"
"github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend"
"github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend/graphqlutil"
ee "github.com/sourcegraph/sourcegraph/enterprise/internal/campaigns"
Expand Down Expand Up @@ -94,11 +93,7 @@ func (r *campaignResolver) Author(ctx context.Context) (*graphqlbackend.UserReso
}

func (r *campaignResolver) ViewerCanAdminister(ctx context.Context) (bool, error) {
currentUser, err := backend.CurrentUser(ctx)
if err != nil {
return false, err
}
return currentUser.SiteAdmin, nil
return currentUserCanAdministerCampaign(ctx, r.Campaign)
}

func (r *campaignResolver) URL(ctx context.Context) (string, error) {
Expand Down Expand Up @@ -321,7 +316,15 @@ func (r *campaignResolver) DiffStat(ctx context.Context) (*graphqlbackend.DiffSt
}

func (r *campaignResolver) Status(ctx context.Context) (graphqlbackend.BackgroundProcessStatus, error) {
return r.store.GetCampaignStatus(ctx, ee.GetCampaignStatusOpts{ID: r.Campaign.ID})
canAdmin, err := currentUserCanAdministerCampaign(ctx, r.Campaign)
if err != nil {
return nil, err
}

return r.store.GetCampaignStatus(ctx, ee.GetCampaignStatusOpts{
ID: r.Campaign.ID,
ExcludeErrors: !canAdmin,
})
}

type changesetDiffsConnectionResolver struct {
Expand Down
123 changes: 30 additions & 93 deletions enterprise/internal/campaigns/resolvers/resolver.go
View file
Expand Up @@ -18,7 +18,6 @@ import (
"github.com/sourcegraph/sourcegraph/internal/campaigns"
"github.com/sourcegraph/sourcegraph/internal/conf"
"github.com/sourcegraph/sourcegraph/internal/httpcli"
"github.com/sourcegraph/sourcegraph/internal/repoupdater"
"github.com/sourcegraph/sourcegraph/internal/trace"
)

Expand Down Expand Up @@ -153,11 +152,6 @@ func (r *Resolver) PatchSetByID(ctx context.Context, id graphql.ID) (graphqlback
}

func (r *Resolver) AddChangesetsToCampaign(ctx context.Context, args *graphqlbackend.AddChangesetsToCampaignArgs) (_ graphqlbackend.CampaignResolver, err error) {
// 🚨 SECURITY: Only site admins may modify changesets and campaigns for now.
if err := backend.CheckCurrentUserIsSiteAdmin(ctx); err != nil {
return nil, err
}

campaignID, err := campaigns.UnmarshalCampaignID(args.Campaign)
if err != nil {
return nil, err
Expand All @@ -184,45 +178,13 @@ func (r *Resolver) AddChangesetsToCampaign(ctx context.Context, args *graphqlbac
}
}

tx, err := r.store.Transact(ctx)
if err != nil {
return nil, err
}
defer tx.Done(&err)

campaign, err := tx.GetCampaign(ctx, ee.GetCampaignOpts{ID: campaignID})
if err != nil {
return nil, err
}

if campaign.PatchSetID != 0 {
return nil, errors.New("Changesets can only be added to campaigns that don't create their own changesets")
}

changesets, _, err := tx.ListChangesets(ctx, ee.ListChangesetsOpts{IDs: changesetIDs})
svc := ee.NewService(r.store, r.httpFactory)
// 🚨 SECURITY: AddChangesetsToCampaign checks whether current user is authorized.
campaign, err := svc.AddChangesetsToCampaign(ctx, campaignID, changesetIDs)
if err != nil {
return nil, err
}

for _, c := range changesets {
delete(set, c.ID)
c.CampaignIDs = append(c.CampaignIDs, campaign.ID)
c.AddedToCampaign = true
}

if len(set) > 0 {
return nil, errors.Errorf("changesets %v not found", set)
}

if err = tx.UpdateChangesets(ctx, changesets...); err != nil {
return nil, err
}

campaign.ChangesetIDs = append(campaign.ChangesetIDs, changesetIDs...)
if err = tx.UpdateCampaign(ctx, campaign); err != nil {
return nil, err
}

return &campaignResolver{store: r.store, Campaign: campaign}, nil
}

Expand Down Expand Up @@ -297,11 +259,6 @@ func (r *Resolver) UpdateCampaign(ctx context.Context, args *graphqlbackend.Upda
tr.Finish()
}()

// 🚨 SECURITY: Only site admins may update campaigns for now
if err := backend.CheckCurrentUserIsSiteAdmin(ctx); err != nil {
return nil, err
}

campaignID, err := campaigns.UnmarshalCampaignID(args.Input.ID)
if err != nil {
return nil, err
Expand All @@ -325,12 +282,14 @@ func (r *Resolver) UpdateCampaign(ctx context.Context, args *graphqlbackend.Upda
}

svc := ee.NewService(r.store, r.httpFactory)

// 🚨 SECURITY: UpdateCampaign checks whether current user is authorized.
campaign, detachedChangesets, err := svc.UpdateCampaign(ctx, updateArgs)
if err != nil {
return nil, err
}

if detachedChangesets != nil {
if len(detachedChangesets) != 0 {
go func() {
ctx := trace.ContextWithTrace(context.Background(), tr)
err := svc.CloseOpenChangesets(ctx, detachedChangesets)
Expand All @@ -350,11 +309,6 @@ func (r *Resolver) DeleteCampaign(ctx context.Context, args *graphqlbackend.Dele
tr.Finish()
}()

// 🚨 SECURITY: Only site admins may update campaigns for now
if err := backend.CheckCurrentUserIsSiteAdmin(ctx); err != nil {
return nil, err
}

campaignID, err := campaigns.UnmarshalCampaignID(args.Campaign)
if err != nil {
return nil, err
Expand All @@ -365,6 +319,7 @@ func (r *Resolver) DeleteCampaign(ctx context.Context, args *graphqlbackend.Dele
}

svc := ee.NewService(r.store, r.httpFactory)
// 🚨 SECURITY: DeleteCampaign checks whether current user is authorized.
err = svc.DeleteCampaign(ctx, campaignID, args.CloseChangesets)
return &graphqlbackend.EmptyResponse{}, err
}
Expand All @@ -377,11 +332,6 @@ func (r *Resolver) RetryCampaign(ctx context.Context, args *graphqlbackend.Retry
tr.Finish()
}()

// 🚨 SECURITY: Only site admins may update campaigns for now
if err := backend.CheckCurrentUserIsSiteAdmin(ctx); err != nil {
return nil, errors.Wrap(err, "checking if user is admin")
}

campaignID, err := campaigns.UnmarshalCampaignID(args.Campaign)
if err != nil {
return nil, errors.Wrap(err, "unmarshaling campaign id")
Expand All @@ -391,14 +341,11 @@ func (r *Resolver) RetryCampaign(ctx context.Context, args *graphqlbackend.Retry
return nil, ErrIDIsZero
}

campaign, err := r.store.GetCampaign(ctx, ee.GetCampaignOpts{ID: campaignID})
if err != nil {
return nil, errors.Wrap(err, "getting campaign")
}

err = r.store.ResetFailedChangesetJobs(ctx, campaign.ID)
svc := ee.NewService(r.store, r.httpFactory)
// 🚨 SECURITY: RetryPublishCampaign checks whether current user is authorized.
campaign, err := svc.RetryPublishCampaign(ctx, campaignID)
if err != nil {
return nil, errors.Wrap(err, "resetting failed changeset jobs")
return nil, errors.Wrap(err, "publishing campaign")
}

return &campaignResolver{store: r.store, Campaign: campaign}, nil
Expand Down Expand Up @@ -608,11 +555,6 @@ func (r *Resolver) CloseCampaign(ctx context.Context, args *graphqlbackend.Close
tr.Finish()
}()

// 🚨 SECURITY: Only site admins may update campaigns for now
if err := backend.CheckCurrentUserIsSiteAdmin(ctx); err != nil {
return nil, errors.Wrap(err, "checking if user is admin")
}

campaignID, err := campaigns.UnmarshalCampaignID(args.Campaign)
if err != nil {
return nil, errors.Wrap(err, "unmarshaling campaign id")
Expand All @@ -623,7 +565,7 @@ func (r *Resolver) CloseCampaign(ctx context.Context, args *graphqlbackend.Close
}

svc := ee.NewService(r.store, r.httpFactory)

// 🚨 SECURITY: CloseCampaign checks whether current user is authorized.
campaign, err := svc.CloseCampaign(ctx, campaignID, args.CloseChangesets)
if err != nil {
return nil, errors.Wrap(err, "closing campaign")
Expand All @@ -639,11 +581,6 @@ func (r *Resolver) PublishCampaign(ctx context.Context, args *graphqlbackend.Pub
tr.Finish()
}()

// 🚨 SECURITY: Only site admins may update campaigns for now
if err := backend.CheckCurrentUserIsSiteAdmin(ctx); err != nil {
return nil, errors.Wrap(err, "checking if user is admin")
}

campaignID, err := campaigns.UnmarshalCampaignID(args.Campaign)
if err != nil {
return nil, errors.Wrap(err, "unmarshaling campaign id")
Expand All @@ -654,6 +591,7 @@ func (r *Resolver) PublishCampaign(ctx context.Context, args *graphqlbackend.Pub
}

svc := ee.NewService(r.store, r.httpFactory)
// 🚨 SECURITY: PublishCampaign checks whether current user is authorized.
campaign, err := svc.PublishCampaign(ctx, campaignID)
if err != nil {
return nil, errors.Wrap(err, "publishing campaign")
Expand All @@ -669,11 +607,6 @@ func (r *Resolver) PublishChangeset(ctx context.Context, args *graphqlbackend.Pu
tr.Finish()
}()

// 🚨 SECURITY: Only site admins may update campaigns for now
if err := backend.CheckCurrentUserIsSiteAdmin(ctx); err != nil {
return nil, errors.Wrap(err, "checking if user is admin")
}

patchID, err := unmarshalPatchID(args.Patch)
if err != nil {
return nil, err
Expand All @@ -683,9 +616,9 @@ func (r *Resolver) PublishChangeset(ctx context.Context, args *graphqlbackend.Pu
return nil, ErrIDIsZero
}

// 🚨 SECURITY: CreateChangesetJobForPatch checks whether current user is authorized.
svc := ee.NewService(r.store, r.httpFactory)
err = svc.CreateChangesetJobForPatch(ctx, patchID)
if err != nil {
if err = svc.CreateChangesetJobForPatch(ctx, patchID); err != nil {
return nil, err
}

Expand All @@ -699,11 +632,6 @@ func (r *Resolver) SyncChangeset(ctx context.Context, args *graphqlbackend.SyncC
tr.Finish()
}()

// 🚨 SECURITY: Only site admins may update campaigns for now
if err := backend.CheckCurrentUserIsSiteAdmin(ctx); err != nil {
return nil, errors.Wrap(err, "checking if user is admin")
}

changesetID, err := unmarshalChangesetID(args.Changeset)
if err != nil {
return nil, err
Expand All @@ -713,12 +641,9 @@ func (r *Resolver) SyncChangeset(ctx context.Context, args *graphqlbackend.SyncC
return nil, ErrIDIsZero
}

// Check for existence of changeset so we don't swallow that error.
if _, err = r.store.GetChangeset(ctx, ee.GetChangesetOpts{ID: changesetID}); err != nil {
return nil, err
}

if err := repoupdater.DefaultClient.EnqueueChangesetSync(ctx, []int64{changesetID}); err != nil {
// 🚨 SECURITY: EnqueueChangesetSync checks whether current user is authorized.
svc := ee.NewService(r.store, r.httpFactory)
if err = svc.EnqueueChangesetSync(ctx, changesetID); err != nil {
return nil, err
}

Expand All @@ -738,3 +663,15 @@ func parseCampaignState(s *string) (campaigns.CampaignState, error) {
return campaigns.CampaignStateAny, fmt.Errorf("unknown state %q", *s)
}
}

func currentUserCanAdministerCampaign(ctx context.Context, c *campaigns.Campaign) (bool, error) {
// 🚨 SECURITY: Only site admins or the authors of a campaign have campaign admin rights.
if err := backend.CheckSiteAdminOrSameUser(ctx, c.AuthorID); err != nil {
if _, ok := err.(*backend.InsufficientAuthorizationError); ok {
return false, nil
}

return false, err
}
return true, nil
}

0 comments on commit 70a4174

Please sign in to comment.