Skip to content

Commit

Permalink
rbac: protect batch changes bulk actions resolvers (sourcegraph#49594)
Browse files Browse the repository at this point in the history 
This PR adds an extra layer of RBAC protection to Bulk Operations. While
testing, we noticed that users without the required permission could
perform bulk operations.

Examples of bulk operation errors from an authenticated context.

* Close Changesets
<img width="584" alt="CleanShot 2023-03-17 at 17 48 21@2x"
src="https://user-images.githubusercontent.com/25608335/225967900-b5c2f365-63f0-417c-9df9-cf633b00c326.png">

* Publish Changesets
<img width="631" alt="CleanShot 2023-03-17 at 17 48 31@2x"
src="https://user-images.githubusercontent.com/25608335/225967869-ccff6850-7545-4f39-a741-cbbc3d43d33c.png">

* Create Comment
<img width="549" alt="CleanShot 2023-03-17 at 17 48 12@2x"
src="https://user-images.githubusercontent.com/25608335/225967913-daa6e0f7-9a8e-4342-bdac-51e6bcbabb61.png">


## Test plan

<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->
* Manual test
* Add unit tests
  • Loading branch information
@BolajiOlajide
BolajiOlajide committed Mar 17, 2023
1 parent bf603e4 commit bb59fcf
Show file tree
Hide file tree
Showing 2 changed files with 143 additions and 11 deletions.
36 changes: 36 additions & 0 deletions enterprise/cmd/frontend/internal/batches/resolvers/resolver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1090,6 +1090,10 @@ func (r *Resolver) ReenqueueChangeset(ctx context.Context, args *graphqlbackend.
return nil, err
}

if err := rbac.CheckCurrentUserHasPermission(ctx, r.store.DatabaseDB(), rbac.BatchChangesWritePermission); err != nil {
return nil, err
}

changesetID, err := unmarshalChangesetID(args.Changeset)
if err != nil {
return nil, err
Expand Down Expand Up @@ -1338,6 +1342,10 @@ func (r *Resolver) DetachChangesets(ctx context.Context, args *graphqlbackend.De
return nil, err
}

if err := rbac.CheckCurrentUserHasPermission(ctx, r.store.DatabaseDB(), rbac.BatchChangesWritePermission); err != nil {
return nil, err
}

batchChangeID, changesetIDs, err := unmarshalBulkOperationBaseArgs(args.BulkOperationBaseArgs)
if err != nil {
return nil, err
Expand Down Expand Up @@ -1373,6 +1381,10 @@ func (r *Resolver) CreateChangesetComments(ctx context.Context, args *graphqlbac
return nil, err
}

if err := rbac.CheckCurrentUserHasPermission(ctx, r.store.DatabaseDB(), rbac.BatchChangesWritePermission); err != nil {
return nil, err
}

if args.Body == "" {
return nil, errors.New("empty comment body is not allowed")
}
Expand Down Expand Up @@ -1417,6 +1429,10 @@ func (r *Resolver) ReenqueueChangesets(ctx context.Context, args *graphqlbackend
return nil, err
}

if err := rbac.CheckCurrentUserHasPermission(ctx, r.store.DatabaseDB(), rbac.BatchChangesWritePermission); err != nil {
return nil, err
}

batchChangeID, changesetIDs, err := unmarshalBulkOperationBaseArgs(args.BulkOperationBaseArgs)
if err != nil {
return nil, err
Expand Down Expand Up @@ -1452,6 +1468,10 @@ func (r *Resolver) MergeChangesets(ctx context.Context, args *graphqlbackend.Mer
return nil, err
}

if err := rbac.CheckCurrentUserHasPermission(ctx, r.store.DatabaseDB(), rbac.BatchChangesWritePermission); err != nil {
return nil, err
}

batchChangeID, changesetIDs, err := unmarshalBulkOperationBaseArgs(args.BulkOperationBaseArgs)
if err != nil {
return nil, err
Expand Down Expand Up @@ -1490,6 +1510,10 @@ func (r *Resolver) CloseChangesets(ctx context.Context, args *graphqlbackend.Clo
return nil, err
}

if err := rbac.CheckCurrentUserHasPermission(ctx, r.store.DatabaseDB(), rbac.BatchChangesWritePermission); err != nil {
return nil, err
}

batchChangeID, changesetIDs, err := unmarshalBulkOperationBaseArgs(args.BulkOperationBaseArgs)
if err != nil {
return nil, err
Expand Down Expand Up @@ -1527,6 +1551,10 @@ func (r *Resolver) PublishChangesets(ctx context.Context, args *graphqlbackend.P
return nil, err
}

if err := rbac.CheckCurrentUserHasPermission(ctx, r.store.DatabaseDB(), rbac.BatchChangesWritePermission); err != nil {
return nil, err
}

batchChangeID, changesetIDs, err := unmarshalBulkOperationBaseArgs(args.BulkOperationBaseArgs)
if err != nil {
return nil, err
Expand Down Expand Up @@ -1795,6 +1823,10 @@ func (r *Resolver) RetryBatchSpecWorkspaceExecution(ctx context.Context, args *g
return nil, err
}

if err := rbac.CheckCurrentUserHasPermission(ctx, r.store.DatabaseDB(), rbac.BatchChangesWritePermission); err != nil {
return nil, err
}

var workspaceIDs []int64
for _, raw := range args.BatchSpecWorkspaces {
id, err := unmarshalBatchSpecWorkspaceID(raw)
Expand Down Expand Up @@ -1930,6 +1962,10 @@ func (r *Resolver) RetryBatchSpecExecution(ctx context.Context, args *graphqlbac
return nil, err
}

if err := rbac.CheckCurrentUserHasPermission(ctx, r.store.DatabaseDB(), rbac.BatchChangesWritePermission); err != nil {
return nil, err
}

batchSpecRandID, err := unmarshalBatchSpecID(args.BatchSpec)
if err != nil {
return nil, err
Expand Down
118 changes: 107 additions & 11 deletions enterprise/cmd/frontend/internal/batches/resolvers/resolver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ func TestCreateBatchSpec(t *testing.T) {
userID int32
unauthorized bool
}{
"unauthorized user": {
"unauthorized access": {
changesetSpecs: []*btypes.ChangesetSpec{},
licenseInfo: licensingInfo("starter"),
wantErr: true,
Expand Down Expand Up @@ -398,7 +398,7 @@ func TestCreateBatchSpecFromRaw(t *testing.T) {
"batchChange": batchChangeID,
}

t.Run("unauthorized user", func(t *testing.T) {
t.Run("unauthorized access", func(t *testing.T) {
var response struct{ CreateBatchSpecFromRaw apitest.BatchSpec }
actorCtx := actor.WithActor(ctx, actor.FromUser(unauthorizedUser.ID))

Expand Down Expand Up @@ -506,7 +506,7 @@ func TestCreateChangesetSpec(t *testing.T) {
"changesetSpec": bt.NewRawChangesetSpecGitBranch(graphqlbackend.MarshalRepositoryID(repo.ID), "d34db33f"),
}

t.Run("unauthorized user", func(t *testing.T) {
t.Run("unauthorized access", func(t *testing.T) {
var response struct{ CreateChangesetSpec apitest.ChangesetSpec }
actorCtx := actor.WithActor(ctx, actor.FromUser(unauthorizedUser.ID))
errs := apitest.Exec(actorCtx, t, s, input, &response, mutationCreateChangesetSpec)
Expand Down Expand Up @@ -604,7 +604,7 @@ func TestCreateChangesetSpecs(t *testing.T) {
},
}

t.Run("unauthorized user", func(t *testing.T) {
t.Run("unauthorized access", func(t *testing.T) {
var response struct{ CreateChangesetSpecs []apitest.ChangesetSpec }
actorCtx := actor.WithActor(ctx, actor.FromUser(unauthorizedUser.ID))
errs := apitest.Exec(actorCtx, t, s, input, &response, mutationCreateChangesetSpecs)
Expand Down Expand Up @@ -747,7 +747,7 @@ func TestApplyBatchChange(t *testing.T) {
"batchSpec": string(marshalBatchSpecRandID(batchSpec.RandID)),
}

t.Run("unauthorized user", func(t *testing.T) {
t.Run("unauthorized access", func(t *testing.T) {
var response struct{ ApplyBatchChange apitest.BatchChange }
actorCtx := actor.WithActor(ctx, actor.FromUser(unauthorizedUser.ID))
errs := apitest.Exec(actorCtx, t, s, input, &response, mutationApplyBatchChange)
Expand Down Expand Up @@ -886,7 +886,7 @@ func TestCreateEmptyBatchChange(t *testing.T) {
"name": "my-batch-change",
}

t.Run("unauthorized user", func(t *testing.T) {
t.Run("unauthorized access", func(t *testing.T) {
var response struct{ CreateEmptyBatchChange apitest.BatchChange }
actorCtx := actor.WithActor(ctx, actor.FromUser(unauthorizedUser.ID))
errs := apitest.Exec(actorCtx, t, s, input, &response, mutationCreateEmptyBatchChange)
Expand Down Expand Up @@ -994,7 +994,7 @@ func TestUpsertEmptyBatchChange(t *testing.T) {
"name": "my-batch-change",
}

t.Run("unauthorized user", func(t *testing.T) {
t.Run("unauthorized access", func(t *testing.T) {
var response struct{ UpsertEmptyBatchChange apitest.BatchChange }
actorCtx := actor.WithActor(ctx, actor.FromUser(unauthorizedUser.ID))
errs := apitest.Exec(actorCtx, t, s, input, &response, mutationUpsertEmptyBatchChange)
Expand Down Expand Up @@ -1094,7 +1094,7 @@ func TestCreateBatchChange(t *testing.T) {
"batchSpec": string(marshalBatchSpecRandID(batchSpec.RandID)),
}

t.Run("unauthorized user", func(t *testing.T) {
t.Run("unauthorized access", func(t *testing.T) {
var response struct{ CreateBatchChange apitest.BatchChange }
actorCtx := actor.WithActor(ctx, actor.FromUser(unauthorizedUser.ID))
errs := apitest.Exec(actorCtx, t, s, input, &response, mutationCreateBatchChange)
Expand Down Expand Up @@ -1255,7 +1255,7 @@ func TestApplyOrCreateBatchSpecWithPublicationStates(t *testing.T) {
Published: true,
})

t.Run("unauthorized user", func(t *testing.T) {
t.Run("unauthorized access", func(t *testing.T) {
input := map[string]any{
"batchSpec": string(marshalBatchSpecRandID(batchSpec.RandID)),
"publicationStates": map[string]any{
Expand Down Expand Up @@ -1452,7 +1452,7 @@ func TestApplyBatchChangeWithLicenseFail(t *testing.T) {
numChangesets: 11,
},
{
name: "Unauthorized user",
name: "unauthorized access",
numChangesets: 1,
isunauthorized: true,
},
Expand Down Expand Up @@ -1567,7 +1567,7 @@ func TestMoveBatchChange(t *testing.T) {
"newName": newBatchChagneName,
}

t.Run("unauthorized user", func(t *testing.T) {
t.Run("unauthorized access", func(t *testing.T) {
var response struct{ MoveBatchChange apitest.BatchChange }
actorCtx := actor.WithActor(ctx, actor.FromUser(unauthorizedUser.ID))
errs := apitest.Exec(actorCtx, t, s, input, &response, mutationMoveBatchChange)
Expand Down Expand Up @@ -2052,6 +2052,12 @@ func TestCreateChangesetComments(t *testing.T) {
bstore := store.New(db, &observation.TestContext, nil)

userID := bt.CreateTestUser(t, db, true).ID
// We give this user the `BATCH_CHANGES#WRITE` permission so they're authorized
// to create Batch Changes.
assignBatchChangesWritePermissionToUser(ctx, t, db, userID)

unauthorizedUser := bt.CreateTestUser(t, db, false)

batchSpec := bt.CreateBatchSpec(t, ctx, bstore, "test-comments", userID, 0)
otherBatchSpec := bt.CreateBatchSpec(t, ctx, bstore, "test-comments-other", userID, 0)
batchChange := bt.CreateBatchChange(t, ctx, bstore, "test-comments", userID, batchSpec.ID)
Expand Down Expand Up @@ -2087,6 +2093,19 @@ func TestCreateChangesetComments(t *testing.T) {
}
actorCtx := actor.WithActor(ctx, actor.FromUser(userID))

t.Run("unauthorized access", func(t *testing.T) {
input := generateInput()
unauthorizedCtx := actor.WithActor(ctx, actor.FromUser(unauthorizedUser.ID))
errs := apitest.Exec(unauthorizedCtx, t, s, input, &response, mutationCreateChangesetComments)
if errs == nil {
t.Fatal("expected error")
}
firstErr := errs[0]
if !strings.Contains(firstErr.Error(), fmt.Sprintf("user is missing permission %s", rbac.BatchChangesWritePermission)) {
t.Fatalf("expected unauthorized error, got %+v", err)
}
})

t.Run("empty body fails", func(t *testing.T) {
input := generateInput()
input["body"] = ""
Expand Down Expand Up @@ -2153,6 +2172,12 @@ func TestReenqueueChangesets(t *testing.T) {
bstore := store.New(db, &observation.TestContext, nil)

userID := bt.CreateTestUser(t, db, true).ID
// We give this user the `BATCH_CHANGES#WRITE` permission so they're authorized
// to create Batch Changes.
assignBatchChangesWritePermissionToUser(ctx, t, db, userID)

unauthorizedUser := bt.CreateTestUser(t, db, false)

batchSpec := bt.CreateBatchSpec(t, ctx, bstore, "test-reenqueue", userID, 0)
otherBatchSpec := bt.CreateBatchSpec(t, ctx, bstore, "test-reenqueue-other", userID, 0)
batchChange := bt.CreateBatchChange(t, ctx, bstore, "test-reenqueue", userID, batchSpec.ID)
Expand Down Expand Up @@ -2196,6 +2221,20 @@ func TestReenqueueChangesets(t *testing.T) {
}
actorCtx := actor.WithActor(ctx, actor.FromUser(userID))

t.Run("unauthorized access", func(t *testing.T) {
unauthorizedCtx := actor.WithActor(ctx, actor.FromUser(unauthorizedUser.ID))
input := generateInput()
input["changesets"] = []string{string(bgql.MarshalChangesetID(successfulChangeset.ID))}
errs := apitest.Exec(unauthorizedCtx, t, s, input, &response, mutationReenqueueChangesets)
if errs == nil {
t.Fatal("expected error")
}
firstErr := errs[0]
if !strings.Contains(firstErr.Error(), fmt.Sprintf("user is missing permission %s", rbac.BatchChangesWritePermission)) {
t.Fatalf("expected unauthorized error, got %+v", err)
}
})

t.Run("0 changesets fails", func(t *testing.T) {
input := generateInput()
input["changesets"] = []string{}
Expand Down Expand Up @@ -2262,6 +2301,12 @@ func TestMergeChangesets(t *testing.T) {
bstore := store.New(db, &observation.TestContext, nil)

userID := bt.CreateTestUser(t, db, true).ID
// We give this user the `BATCH_CHANGES#WRITE` permission so they're authorized
// to create Batch Changes.
assignBatchChangesWritePermissionToUser(ctx, t, db, userID)

unauthorizedUser := bt.CreateTestUser(t, db, false)

batchSpec := bt.CreateBatchSpec(t, ctx, bstore, "test-merge", userID, 0)
otherBatchSpec := bt.CreateBatchSpec(t, ctx, bstore, "test-merge-other", userID, 0)
batchChange := bt.CreateBatchChange(t, ctx, bstore, "test-merge", userID, batchSpec.ID)
Expand Down Expand Up @@ -2307,6 +2352,19 @@ func TestMergeChangesets(t *testing.T) {
}
actorCtx := actor.WithActor(ctx, actor.FromUser(userID))

t.Run("unauthorized access", func(t *testing.T) {
unauthorizedCtx := actor.WithActor(ctx, actor.FromUser(unauthorizedUser.ID))
input := generateInput()
errs := apitest.Exec(unauthorizedCtx, t, s, input, &response, mutationMergeChangesets)
if errs == nil {
t.Fatal("expected error")
}
firstErr := errs[0]
if !strings.Contains(firstErr.Error(), fmt.Sprintf("user is missing permission %s", rbac.BatchChangesWritePermission)) {
t.Fatalf("expected unauthorized error, got %+v", err)
}
})

t.Run("0 changesets fails", func(t *testing.T) {
input := generateInput()
input["changesets"] = []string{}
Expand Down Expand Up @@ -2373,6 +2431,12 @@ func TestCloseChangesets(t *testing.T) {
bstore := store.New(db, &observation.TestContext, nil)

userID := bt.CreateTestUser(t, db, true).ID
// We give this user the `BATCH_CHANGES#WRITE` permission so they're authorized
// to create Batch Changes.
assignBatchChangesWritePermissionToUser(ctx, t, db, userID)

unauthorizedUser := bt.CreateTestUser(t, db, false)

batchSpec := bt.CreateBatchSpec(t, ctx, bstore, "test-close", userID, 0)
otherBatchSpec := bt.CreateBatchSpec(t, ctx, bstore, "test-close-other", userID, 0)
batchChange := bt.CreateBatchChange(t, ctx, bstore, "test-close", userID, batchSpec.ID)
Expand Down Expand Up @@ -2418,6 +2482,19 @@ func TestCloseChangesets(t *testing.T) {
}
actorCtx := actor.WithActor(ctx, actor.FromUser(userID))

t.Run("unauthorized access", func(t *testing.T) {
unauthorizedCtx := actor.WithActor(ctx, actor.FromUser(unauthorizedUser.ID))
input := generateInput()
errs := apitest.Exec(unauthorizedCtx, t, s, input, &response, mutationCloseChangesets)
if errs == nil {
t.Fatal("expected error")
}
firstErr := errs[0]
if !strings.Contains(firstErr.Error(), fmt.Sprintf("user is missing permission %s", rbac.BatchChangesWritePermission)) {
t.Fatalf("expected unauthorized error, got %+v", err)
}
})

t.Run("0 changesets fails", func(t *testing.T) {
input := generateInput()
input["changesets"] = []string{}
Expand Down Expand Up @@ -2484,6 +2561,12 @@ func TestPublishChangesets(t *testing.T) {
bstore := store.New(db, &observation.TestContext, nil)

userID := bt.CreateTestUser(t, db, true).ID
// We give this user the `BATCH_CHANGES#WRITE` permission so they're authorized
// to create Batch Changes.
assignBatchChangesWritePermissionToUser(ctx, t, db, userID)

unauthorizedUser := bt.CreateTestUser(t, db, false)

batchSpec := bt.CreateBatchSpec(t, ctx, bstore, "test-close", userID, 0)
otherBatchSpec := bt.CreateBatchSpec(t, ctx, bstore, "test-close-other", userID, 0)
batchChange := bt.CreateBatchChange(t, ctx, bstore, "test-close", userID, batchSpec.ID)
Expand Down Expand Up @@ -2555,6 +2638,19 @@ func TestPublishChangesets(t *testing.T) {
}
actorCtx := actor.WithActor(ctx, actor.FromUser(userID))

t.Run("unauthorized access", func(t *testing.T) {
unauthorizedCtx := actor.WithActor(ctx, actor.FromUser(unauthorizedUser.ID))
input := generateInput()
errs := apitest.Exec(unauthorizedCtx, t, s, input, &response, mutationPublishChangesets)
if errs == nil {
t.Fatal("expected error")
}
firstErr := errs[0]
if !strings.Contains(firstErr.Error(), fmt.Sprintf("user is missing permission %s", rbac.BatchChangesWritePermission)) {
t.Fatalf("expected unauthorized error, got %+v", err)
}
})

t.Run("0 changesets fails", func(t *testing.T) {
input := generateInput()
input["changesets"] = []string{}
Expand Down

0 comments on commit bb59fcf

Please sign in to comment.