batches: add organization setting to enable all members of org to become batch changes admins

[BolajiOlajide]

Closes https://github.com/sourcegraph/sourcegraph/issues/50447

Walkthrough

https://github.com/sourcegraph/accounts/issues/3 identified some areas where Batch Changes didn't solve some of their use cases. One is that Batch Changes created in an org namespace can only be operated by a site admin or the same user who created it.

This PR is a band-aid fix; the hope is that this is appropriately addressed when support for Batch Changes and teams is worked on.

I created a method in the batches service package to check for access to a batch change. The breakdown of the logic of the method is as follows:

• If a Batch Change is in a user namespace, check if the current user is a site admin or the same user
• If a Batch Change is in an org namespace, then we check the organization settings for orgs.allMembersBatchChangesAdmin boolean field:
  • If false, we default to the existing check for if the current user is a site admin or the same user
  • If true, we check if the user belongs to the org or is a site admin or the same user

The following resolver operations are affected by this change:

Operations
MoveBatchChange
CloseBatchChange
DeleteBatchChange
EnqueueChangesetSync
ReenqueueChangeset
ApplyBatchChange
CreateChangesetJobs (this is used for all bulk operations)

The following resolver operations remain unchanged:

Operations
CreateEmptyBatchChange
UpsertEmptyBatchChange
CreateBatchSpec
CreateBatchSpecFromRaw
ExecuteBatchSpec
CancelBatchSpec
ReplaceBatchSpecInput
UpsertBatchSpecInput

The logic for the above operations requires the current user to have access to the namespace regardless of where it was created (user or org namespace).

Test plan

• org.allMembersBatchChangesAdmin enabled

  • You'll need to create an organization and navigate to the settings page for the organization /organizations/sourcegraph/settings.

  • In the settings page, set the value of orgs.allMembersBatchChangesAdmin to true (this will grant all members of the organization the ability to perform some administrative tasks on Batch Changes created in the org namespace)

  • Create a Batch Change in the organization namespace.

  • Access the Batch Change with another user account; this account shouldn't be a site admin but should be a member of the org you created.

  • This new account should be able to perform the following actions on the Batch Change created by the previous user in the org namespace:

    Operations
    MoveBatchChange
    CloseBatchChange
    DeleteBatchChange
    ApplyBatchChange
    Retry a changeset in failed state
    All bulk operations

• org.allMembersBatchChangesAdmin disabled or not set

  • You'll need to create an organization and navigate to the settings page for the organization /organizations/sourcegraph/settings.

  • Create a Batch Change in the organization namespace.

  • Access the Batch Change with another user account; this account shouldn't be a site admin but should be a member of the org you created.

  • The non-site admin account shouldn't be able to perform the following actions on Batch Changes created in the org namespace:

    Operations
    MoveBatchChange
    CloseBatchChange
    DeleteBatchChange
    ApplyBatchChange
    Retry a changeset in failed state
    All bulk operations

[BolajiOlajide]

Not sure if this is the best name for this method, I'm open to suggestions.

[courier-new]

I don't mind this name. Or maybe checkBatchChangeAdmin?

[sourcegraph-bot]

Codenotify: Notifying subscribers in CODENOTIFY files for diff f797d27...b1936da.

Notify	File(s)
@eseliger	enterprise/internal/batches/service/BUILD.bazel enterprise/internal/batches/service/service.go enterprise/internal/batches/service/service_apply_batch_change.go enterprise/internal/batches/service/service_test.go

[courier-new]

Hehehe your PR description made me a bit nervous at first because I think you accidentally omitted the operation we care most about (ApplyBatchChange) from your list, but I see you have added it to that service method as well so hooray! 😄

This looks great so far but left a couple small suggestions inline. Here's just a few more:

1. Could we add some test cases to service_apply_batch_change_test.go, too?

2. As you pointed out, these resolver methods are currently unchanged:

   • CreateBatchSpecFromRaw
   • ExecuteBatchSpec
   • CancelBatchSpec
   • ReplaceBatchSpecInput
   • UpsertBatchSpecInput

   And so I believe all of them at the moment only perform a basic CheckNamespaceAccess. However, I wonder if it actually makes sense to apply the same checkBatchChangeAccess to them, too? Because I believe the way it works today, this is technically possible:

   1. User A and User B belong to org O. O does not have our new admin setting enabled.
   2. User A creates a batch change in O and starts executing it.
   3. User B can cancel User A's execution, because they pass CheckNamespaceAccess.
   4. User B can edit and execute a new batch spec to override User A's as the latest execution, even if they can't apply it, because they pass CheckNamespaceAccess.

   And those interactions feel... like they shouldn't be allowed. 😅

   Once again all of this will get ironed out by teams and RBAC, so we could also just leave it alone for now and just let it be an undocumented quirk of orgs, but maybe since we're adding this more robust bandaid setting right now anyway, it makes sense to use it in a couple more places where we probably should have been more restrictive to begin with? 😅

3. Because I'm lazy and haven't actually pulled your branch to test, do you mind just sharing a quick video/gif of the UI feedback that occurs when a non-admin org member goes to apply a spec in another user's batch change that's in their org in cases with both the org setting enabled and disabled? 🙏

[courier-new]

I don't mind this name. Or maybe checkBatchChangeAdmin?

[BolajiOlajide]

1. Could we add some test cases to service_apply_batch_change_test.go, too?

Hey @courier-new ,

1. I know it's a bit weird, but we do have test cases for permissions at the resolver level in service_test.go, in there lies permissions tests for every resolver method.

As you pointed out, these resolver methods are currently unchanged:

CreateBatchSpecFromRaw
ExecuteBatchSpec
CancelBatchSpec
ReplaceBatchSpecInput
UpsertBatchSpecInput
And so I believe all of them at the moment only perform a basic CheckNamespaceAccess. However, I wonder if it actually makes sense to apply the same checkBatchChangeAccess to them, too?

2. Yes, that makes sense. Considering all this is hidden behind the orgs.allMembersBatchChangesAdmin setting, it's a good breaking change that would only impact customers that turn on the setting. I'll update.

Because I'm lazy and haven't actually pulled your branch to test, do you mind just sharing a quick video/gif of the UI feedback that occurs when a non-admin org member goes to apply a spec in another user's batch change that's in their org in cases with both the org setting enabled and disabled? 🙏

3. I'll record one and share it here.

[courier-new]

I know it's a bit weird, but we do have test cases for permissions at the resolver level in service_test.go, in there lies permissions tests for every resolver method.

Oh thank you, I'll take a look there. Sorry I missed it!

Thanks for your time on this! ❤️

[BolajiOlajide]

Here's a video walkthrough of some of the changes made by this PR.

[courier-new]

Awesome, thanks for the walkthrough recording!! Is this ready for re-review or are you still making any other changes?

[BolajiOlajide]

Awesome, thanks for the walkthrough recording!! Is this ready for re-review or are you still making any other changes?

It's ready for re-review @courier-new

[courier-new]

Thanks for this!! Approving to unblock, but I see a couple test failures and had a few suggestions just to clean things up a bit!

[courier-new]

Oh!! I kinda forgot we had this method lol. It's a little confusing to see the docstring here and understand how this is different from our new private method checkBatchChangeAdmin. I also see from searching the codebase that this public method is exclusively used to compute the viewerCanAdminister field on the GraphQL resolvers for batch changes and batch specs.

To make these methods work together a little better, maybe we could do the following:

• Move this method up in the file to be next to checkBatchChangeAdmin
• Rename this method to CheckViewerCanAdminister so it more closely corresponds to the GraphQL field
• Rename our private method to checkViewerCanAdminister (I know, sorry... 😂 Turns out we already had a great name for this!)
• Update this method's docstring to just the first sentence

[courier-new]

Separately, I don't think I understand why this method needs the escape hatch to "simply [...] check for org namespace access". 🤔 I'd think we'd want the viewerCanAdminister field to reflect the org setting as well, no?

[BolajiOlajide]

Not really; the CanAdministerInNamespace checks for namespace access, which means we always check for the following:

• If the batch change is in a user namespace, we check if the current user is the creator or a site admin
• If the batch change is in an org namespace, we check if the current user is a member or site admin.

This differs from other places where checkBatchChangeAdmin is used because we don't explicitly give org members access to every operation.

[courier-new]

Ah okay, I think I understand. So viewerCanAdminister is still effectively more lenient than checkBatchChangeAdmin normally wants to be, even with the org setting toggle on? Makes sense.

I guess I'm curious if this distinction actually stills makes sense to keep going forward, now that we have both RBAC and the org setting as other ways of determining "administration" permissions, but that's a conversation for another day. 😄

[BolajiOlajide]

It's definitely a good conversation to have , but yeah let's have it another day because it's a change in the current experience.