Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

[Backport 5.0] batches: add organization setting to enable all members of org to become batch changes admins#52655

Merged
keegancsmith merged 1 commit into
5.0from
backport-50724-to-5.0
May 31, 2023
Merged

[Backport 5.0] batches: add organization setting to enable all members of org to become batch changes admins#52655
keegancsmith merged 1 commit into
5.0from
backport-50724-to-5.0

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

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


Backport 8f73e37 from #50724

@courier-new

Copy link
Copy Markdown
Contributor

Quick context for historical purposes:

Slack thread

This is a high-impact change that was flagged as a blocker for https://github.com/sourcegraph/accounts/issues/3, https://github.com/sourcegraph/accounts/issues/580 after 5.0 and planned by the team to go out in a patch release before 5.1.

@courier-new

Copy link
Copy Markdown
Contributor

Arg I forgot https://github.com/sourcegraph/sourcegraph/pull/50604 was separate. Will backport that first, then rebase this to make sure builds are passing.

…ome batch changes admins (#50724)

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

[Walkthrough](https://www.loom.com/share/895f3b1fe45d4d0a93cc4618ce51bfbf)

sourcegraph/accounts#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`](https://sourcegraph.test:3443/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`](https://sourcegraph.test:3443/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 |

(cherry picked from commit 8f73e37)
@keegancsmith keegancsmith force-pushed the backport-50724-to-5.0 branch from 8b05ce3 to 1ae6ba5 Compare May 31, 2023 11:52
@keegancsmith

Copy link
Copy Markdown
Member

@courier-new I went ahead and landed the dependent PR then rebased this one onto 5.0. Will enable auto-merge.

@keegancsmith keegancsmith enabled auto-merge (squash) May 31, 2023 11:54
@sourcegraph-bot

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 8ae3a98...1ae6ba5.

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

@BolajiOlajide

Copy link
Copy Markdown
Contributor

@courier-new I went ahead and landed the dependent PR then rebased this one onto 5.0. Will enable auto-merge.

Thank you, Keegan. I'm slotting in for @courier-new here since she's away in dreamland - I'll be monitoring and ready to swoop in if the need arises for CI fixes.

@keegancsmith keegancsmith merged commit 7da051f into 5.0 May 31, 2023
@keegancsmith keegancsmith deleted the backport-50724-to-5.0 branch May 31, 2023 12:12
@courier-new

Copy link
Copy Markdown
Contributor

Thank you both!! 🙇‍♀️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backports batch-changes Issues related to Batch Changes cla-signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants