Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rbac: protect batch changes bulk actions resolvers #49594

Merged
merged 4 commits into from Mar 17, 2023
Merged

rbac: protect batch changes bulk actions resolvers #49594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
12e088b
fix tests
BolajiOlajide Mar 17, 2023
0ab35b8
fix remaining tests
BolajiOlajide Mar 17, 2023
e4ad10f
use rbac constants
BolajiOlajide Mar 17, 2023
faeb648
fix invalid types
BolajiOlajide Mar 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 36 additions & 0 deletions enterprise/cmd/frontend/internal/batches/resolvers/resolver.go
View file
Open in desktop
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
Open in desktop
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