[meta] Branch permissions

Issues

This issue serves as an umbrella issue for the following features:

• gitlab-org/gitlab-ce#18627 Allow specifying protected branches using wildcards
• gitlab-org/gitlab-ce#18193 Add "developers can merge" as an option for protected branches
• gitlab-org/gitlab-ce#18193 Add "no one can push" as an option for protected branches
• #674 (closed) Restrict pushes / merges (separately) to specific people
• #675 (closed) Restrict pushes / merges (separately) to specific groups
• #720 Add support for branch level read permissions

---

Description including problem, use cases, benefits, and/or goals

Some projects have complex requirements in terms of deployments and pushing code. To still be able to use git easily as a team, but not run the risk of accidentally making changes on a branch you shouldn't, you can use protected branches.

However, manually changing every branch to protected can be a lot of work and prevents automation and quick shipping, while maintaining this security. Therefore we want to be able to

1. Automatically protect branches based on certain wildcards
2. Restrict push access to these branches for specific people (groups or single users)

old proposal

The ability to create rules for protecting branches (1) and to restrict push access to certain people (2).

A rule should consist of one or two things:

1. A rule based on when a branch should be protected: production/* or production or production/the-real-thing. This should allow for * wildcards or just a branch name. Other examples: *-stable, *-stable. Restrict wildcard use to edges if necessary.
2. A user or group. If none is used, normal rules for protected branches are applied. If a user or group is added, pushing should ONLY be allowed by these people. This can be anyone*.

When setting a rule for a group

• any group can be set
• the group should automatically have this project shared with it (using the 'share with group' feature)
• should be communicated in the UI

When setting a rule for a person

• the person should be added to the project with master level access
• this should be communicated in the UI

old issue

Something like:

• allow wildcard for protected branches
• allow per-user permission to protected branches.
• some other feature So people who used protected branches feature in CE can continue use it in EE but with more flexibility.

Zendesk issue: https://gitlab.zendesk.com/agent/tickets/13653 and https://gitlab.zendesk.com/agent/tickets/19387

Gerrit allows per branch permissions. The customer uses this feature but would like to switch entirely to GitLab. They use a similar release branching strategy to GitLab. Each release has a release manager, or release team, that is responsible for that release branch going forward. Patches should only be approved by the release team so they want the ability to set these permissions on a branch directly.

cc/ @JobV @dzaporozhets @sytses

mentioned in issue #180 (closed)

@JobV Thoughts on this feature?

I'm resistant to setting permission per branch, although I can see a future where we add more granular permissions to protected branches. Their use case is definitely interesting and understandable.

We will not be doing this in the next couple of releases (simply because of capacity), but after that I think we can spend some time exploring this.

thanks for commenting. I'm glad we're willing to explore it

Added ~139512 label

BitBucket can use wildcards and pattern matches on branches so some groups can’t create a branch or submit to a branch. This came up with https://na34.salesforce.com/00261000002aFis and https://na34.salesforce.com/00T6100000EQJ2o

I assume we're talking about https://blog.bitbucket.org/2013/09/16/take-control-with-branch-restrictions/

@JobV we last significant deals because of this. I hear people being enthusiastic about this feature in Bitbucket. I think we should have it even though it is a lot of work. /cc @dzaporozhets

Milestone changed to 8.8

mentioned in issue gitlab-com/blog-posts#183

@sytses it's a very cool feature. We'll have to move some stuff from 8.8, but it'll be worth it.

I have a large Chinese prospect for which this feature is Key.

@sytses @JobV I want to avoid having two similar features (protected branches and per-branch permissions) so I propose we do this as advance to existing protected branches feature.

Something like:

• allow wildcard for protected branches
• allow per-user permission to protected branches.
• some other feature

So people who used protected branches feature in CE can continue use it in EE but with more flexibility.

@dzaporozhets I agree

mentioned in issue #460

Also requested by: https://na34.salesforce.com/00661000009SaUz

Added Product work label

Milestone changed to 8.9

Requested by prospect: https://na34.salesforce.com/0066100000AFBkL

Added ~126459 and removed Product work labels

@stanhu @DouweM this one has strong priority and will be significant work.

Should consider to include the merging of protected branches and branches page cc @dzaporozhets

somewhat relevant MR to consider when implementing this: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4220

@jschatz1 Who can work on a mockup/prototype for the merged protected branch and branches pages, and the new branch permission UI? Then backend can work on it, and that frontend dev can implement the frontend part.

https://blog.bitbucket.org/2013/09/16/take-control-with-branch-restrictions/ can be used for inspiration.

@DouweM I will work on a prototype for this. Once I do you can start backend work. Should have something to you by tomorrow.

@jschatz1 You tha real MVP

@jschatz1 foh real.

Reassigned to @jschatz1

---

**Hover Rule:**

---

**Click Rule to Edit:**

Clicking "edit users" should open up the same dropdown as assigning users.

@jschatz1 Overall, I like it! Thinking as a new user, I'm a little confused about the difference between rules and protected branches. There's also a feeling of disconnect between the top form and protected branches on the bottom (with rules in between). I'm not sure of a better suggestion, though.

@dblessing Thanks! I'll see if I can make some improvements.

@DouweM you should be good to go at least to start. I can improve as we go.

Added frontend and removed ~126459 labels

Reassigned to @timothyandrew

1. It might be better if we blur the distinction between "rules" and protected branches, and have these three broad goals (with each possibly being a separate MR):

• Allow protecting branches both by explicit branch name (current behavior) and wildcard branch name

• Allow restricting access to a protected branch to a specific list of users

• Allow restricting access to a protected branch to a specific list of groups

  This way, there's no distinction between "Rules" and "Protected Branches", and this should serve to keep the backend simpler, as well. I'm not sure if goes against what everyone here has in mind for this feature...please let me know if this is a bad idea, for whatever reason.

2. Regarding an estimate for completion:

• I haven't dived into the parts of the codebase that are directly git-related yet, so I'll need some time to get acquainted with this portion of the codebase.
• The actual implementation seems straightforward to me; I'll have more after I've looked at the existing code.
• I'm starting work on this today.

@timothyandrew I agree that we do not need the distinction between Rules and Protected Branches, as this would only be confusing to users. Bitbucket doesn't have this distinction either.

As Protected Branches are in CE, I would suggest adding wildcard protected branches to CE, and the user/group restriction feature to EE, so that the implementations between CE and EE won't need to differ too much. Also note that an issue and MR already exist in CE to add that wildcard/regex feature: https://gitlab.com/gitlab-org/gitlab-ce/issues/5977 https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3936. The MR was closed because of this issue, but that doesn't make sense if this would be EE only, while that would be community contributed and go into CE.

@JobV What do you think?

I agree this can be simpler and I like the proposal by @timothyandrew.

I propose we do as @timothyandrew suggests, with some specificity:

• CE: You can protect a branch by name
• CE: You can protect a branch by wildcard (as suggested above)
• CE: You can set whether developers / masters / no one can merge or push. I've detailed this here, but it makes sense to do this now: https://gitlab.com/gitlab-org/gitlab-ce/issues/18193#note_12280834
• EE: You can restrict pushing and merging to specific people
• EE: You can restrict pushing and merging to specific groups

The proposed UI would need to be updated. cc @jschatz1

@timothyandrew @DouweM if you agree, let me know and I'll update the issue body.

@JobV I agree, and I think @timothyandrew can start on the first 3 CE steps right now.

As for the UI, we can forget about the "Rules" table.

We need to change the third column in the "Already Protected" table. It is currently "Developers can push" with checkboxes in the different rows, but should become "Who can push" with "Developers", "Masters", "Nobody" as options, and later in EE, specific users or groups. Adding a separate restriction for "Who can merge" is a different problem.

Second, there would be a way to change the protection of a branch between these options. Potentially inline in the table row, although that may not scale. The backend can be implemented before that is finalized, though.

@DouweM disagree on the UI, I'll make a mockup. Checkboxes don't work for OR relationships. Agree with the rest.

Mockup:

@JobV Right, the "with checkboxes" I had was referring to the current behavior. Your mockup is exactly what I had in mind. :)

@JobV: I like the five-fold feature split - I'll get them all linked to relevant issues (edit: this is done; the new issues are in this issue's description), and we can treat this one as an umbrella issue.

mentioned in issue #674 (closed)

mentioned in issue #675 (closed)

@JobV where does the add rule fit in? Same as before?

@jschatz1 either on top or below :)

Changed title: Branch permissions → [meta] Branch permissions

Added ~360295 label

Milestone changed to %8.10

mentioned in issue #720

mentioned in issue #388 (closed)

Marked the task gitlab-org/gitlab-ce#18627 Allow specifying protected branches using wildcards as completed

mentioned in issue gitlab-com/marketing#352

Marked the task gitlab-org/gitlab-ce#18193 Add "developers can merge" as an option for protected branches as completed

Milestone changed to %8.11

mentioned in merge request !581 (merged)

Frontend Status Update

@tauriedavis @alfredo1: Here's a quick look at where things stand with the frontend for protected branches. Let me know if you need any more context on any of these points.

---

1. Two features (wildcards for protected branches, allow developers to merge [but not push]) have been merged in.

2. Here's what the Protected Branches page looks like on master currently:

   

3. The final CE feature (allow "no one can push" as a setting for a protected branch) is in the final stages of review, and should be merged soon (gitlab-org/gitlab-ce!5081). This feature changes the UI to be more consistent with @JobV's mockup:

   

4. I'm working on the first EE-only feature (restrict branch protection to specific people). This changes the UI further due to the need for having more than one access level selected at a time. Here are screenshots of my current (rudimentary) implementation:

   

/cc @DouweM @JobV @jschatz1

Thanks for the recap @timothyandrew! It was super helpful for making these mockups.

The current design looks good and matches a lot of the other settings pages. I think over time, we can work on the other settings pages so there is more distinction between sections on any given page. I've made some changes in hopes of improving the hierarchy for the protected branches now though.

CE Protected Branches

EE Protected Branches

Create:

Edit:

These updated mockups are designed more similarly to how you would add users to a project. For EE, the edit functionality remains on the same page. I also reversed the order of "# role and # users" so it mimics the select dropdown.

cc @timothyandrew @dzaporozhets @hazelyang

Marked the task gitlab-org/gitlab-ce#18193 Add "no one can push" as an option for protected branches as completed

Thanks for the mockups @tauriedavis. They look very polished!

I'd love to hear your thoughts about @zyv's suggestion:

Basically, disable certain options in "Allowed to Merge" based on the current selection in "Allowed to Push". For example, if "Allowed to Push" is set to "Developers", developers will be able to merge even if "Allowed to Merge" is set to "Masters" (since pushes are a superset of merges).

I see, if I understand correctly, I would say the order of the permissions should be switched in the UI. Allowed to push should come first in the UI and when a role/user is added to allowed to push, they would automatically be added to allowed to merge. Does this make sense? @timothyandrew

Very nice, @tauriedavis!

@tauriedavis how exactly this dropdown will behave?

I understand is a dropdown but the help text says the user will be able to write a custom entry. Can you please provide a mockup showing how the dropdown/input works?

@alfredo1 The current protected branch dropdown allows creating a custom wildcard entry. This should work as an example. Although, it can probably be improved a bit - it wasn't 100% clear to me the first time I saw it.

Thanks @dblessing, I didn't see it at first but the dropdown allows to create a custom entry. it just wasn't too obvious for me.

I think some copy changes and small tweaks will help make it more obvious how to create a wildcard:

@tauriedavis thanks for those mockups.

A couple of questions for you (sorry for this long list, I didn't expect it to be that big):

1. When editing a protected branch (CE/EE), which expands a section below the row containing the branch name, can we highlight this row somehow?
2. Again when editing a protected branch (CE/EE), we do have a Save button, but the Cancel action can only be done via the Collapse/Expand symbol. What do you think of having a Cancel button?
3. Again when editing a protected branch (EE only), we display an input field and list inside this field all the groups/users that have the rights to merge or push. What happens when we have, say, 24 people in this input field? Does the input field expand? Do we have to use the keys on the keyboard to navigate to and remove someone specifically? We need to take this case in consideration also.
4. When listing groups, do you think it would bring value to indicate the number of people in each group, so we can measure the impact this decision will have (we might think twice before applying a protection to a group that has 234 developers)?
5. Performance wise, will displaying avatars in the dropdowns affect the rendering of the dropdown? Consider the use case of having 500 developers in a given account. We surely have this use case with some of our customers. Perhaps even much more. A front-end engineer can surely help us answer this question.
6. I feel Push privileges should be listed before Merge in the tables.
7. Should we use the "lock metaphor" on this page as well - a visual metaphor we use already to indicate that a branch/file is protected?
8. Could you provide a blank state for this page as well?

As per the discussion on slack regarding making the UI of the EE version match the CE version, we have decided to move away from the expand/collapse interface. Instead, the EE version will match the design of the CE version, by having a drop down for allowed to push and allowed to merge. The user will be able to select multiple roles and users from this drop down and selected users/roles should be seen at the top of the dropdown.

/cc @DouweM @alfredo1 @jschatz1

---

@regisF All great questions! Some of them aren't relevant anymore with the design change above.

When editing a protected branch (CE/EE), which expands a section below the row containing the branch name, can we highlight this row somehow?

This is a good idea, but we won’t be doing the expanding section any more.

Again when editing a protected branch (CE/EE), we do have a Save button, but the Cancel action can only be done via the Collapse/Expand symbol. What do you think of having a Cancel button?

Also a good idea, but we won’t be doing the expanding section any more.

Again when editing a protected branch (EE only), we display an input field and list inside this field all the groups/users that have the rights to merge or push. What happens when we have, say, 24 people in this input field? Does the input field expand? Do we have to use the keys on the keyboard to navigate to and remove someone specifically? We need to take this case in consideration also.

The idea was that the field would expand. You could use your cursor or keyboard to navigate and remove someone. We wont be doing this anymore, however.

When listing groups, do you think it would bring value to indicate the number of people in each group, so we can measure the impact this decision will have (we might think twice before applying a protection to a group that has 234 developers)?

Yes, I think this is a great idea! Added to mockup above.

Performance wise, will displaying avatars in the dropdowns affect the rendering of the dropdown? Consider the use case of having 500 developers in a given account. We surely have this use case with some of our customers. Perhaps even much more. A front-end engineer can surely help us answer this question.

I dont think it should have an affect. We currently render the avatars in many dropdowns throughout the site.

I feel Push privileges should be listed before Merge in the tables.

I agree. I’ve updated in mockup above..

Should we use the "lock metaphor" on this page as well - a visual metaphor we use already to indicate that a branch/file is protected?

Sure! Added to mockup above

Could you provide a blank state for this page as well?

Yes

@timothyandrew I refactored the code on this view. With my changes this will be relatively easy to implement. I only need to get the right data for each dropdown. I'll create a WIP MR with my progress. So maybe you can help me with the data.

@tauriedavis Is it ok to show No matching results when filtering on the branch dropdown? Since this is actually done by the dropdown. I'm asking because in the proposed design there's no message.

@alfredo1 I removed it from the mockup because you didn't see the create wildcard button originally. But I think it is okay to show the No matching results message. The updated design/copy makes it look more obvious to me and your screenshot matches how we handle this same interaction for adding new labels.

Thanks @tauriedavis for the amazingly detailled answer. The design is great.

@alfredo1 Absolutely. Let me know when I can help.

@timothyandrew https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5652 is ready, and since the EE side of this feature requires additional data I'm wondering if you can help me with that. Currently the dropdown on both CE/EE have the same kind of data. But on EE should also have users as options.

It would be great if you can provide me with the right data for each dropdown type.

@alfredo1 !581 (merged) is still in review, so we can't have users in there until that's done. Would it be easier for you if I remove all frontend code from !581 (merged), so you don't have any rework?

(edit: as an alternative, maybe you can "take over" that MR once it has been reviewed)

mentioned in merge request !645 (merged)

mentioned in commit 3851c497

mentioned in commit f6bb1db8

Marked the task #674 (closed) Restrict pushes / merges (separately) to specific people as completed

Taurie Davis @tauriedavis commented 24 days ago

I see, if I understand correctly, I would say the order of the permissions should be switched in the UI. Allowed to push should come first in the UI and when a role/user is added to allowed to push, they would automatically be added to allowed to merge. Does this make sense? ( https://gitlab.com/gitlab-org/gitlab-ee/issues/179#note_13439287 )

@alfredo1: This behavior needs to be implemented as well, right? Let me know if there's any changes you need in the backend to support this.

/cc @tauriedavis

Milestone changed to %8.12

Milestone changed to %8.13

Mentioned in merge request !645 (merged)

Mentioned in commit 2b252c9a

Marked the task #675 (closed) Restrict pushes / merges (separately) to specific groups as completed

Mentioned in commit 33e3653a

@JobV: Assigning this back to you - I don't have any context about the one remaining issue here (#720).

Reassigned to @JobV

Milestone changed to %8.14

Thanks @timothyandrew

Status changed to closed

Closing this issue, as the remaining request is both hard and very far from the original idea behind branch permissions.

Mentioned in commit f6bb1db8