Allow reporters to approve MRs if they are explicitly listed in the approval rules

@poffey21 can you also describe why operators need to approve MRs?

@jmeshell I set some labels and weight here, not sure of the category though.

@vshushlin - I will let @poffey21 weigh in too, but I have found that users want to use an approval workflow for deployments to production from a non-coder reviewer.

@jmeshell if you mean something like https://about.gitlab.com/blog/2020/02/20/protecting-manual-jobs/, then it doesn't require MR approval, only the access to protected environment which is covered by #225482 (closed)

How are the edits made in the description (and quoted below)?

Managers or Operators (with reporter access) to a project sometimes need to be required approvers of an MR before a merge to a protected branch commences. These managers/operators ought not be allowed to push/merge any code to any branches but. The only users that are able to "Approve" a Merge Request are users that can also push/merge code. This prevents the MR Approvers from having segregation of duties.

Oftentimes in regulated environments, an engineering manager or business user is required to "approve" of a finalized change before it's allowed to be deployed - and we don't want those managers be the actual deployers.

Love those edits @poffey21

@vshushlin - yes they can use those as well. Sometimes, customers are not using protected branches or environments for their production instances. So, that is one limitation.

@vshushlin Unlike protected environments it looks like we can not achieve this using groups (#225482 (comment 372054582))? Should we explore this option?

The tool tip says "Users or groups set as approvers in the project's or merge request's settings" but I can't find how to do it.

added Category:Compliance Management devopsrelease [DEPRECATED] + 1 deleted label

set weight to 2

marked this issue as related to #201898 (closed)

mentioned in issue #201898 (closed)

changed the description

added Category:Release Orchestration permissions + 1 deleted label

changed milestone to %13.4

added cicdactive workflowready for development labels

marked this issue as related to #225482 (closed)

changed the description

added [deprecated] Accepting merge requests label

marked this issue as related to #5425

mentioned in issue #5425

@weitzman - saw a comment on #5425, does this issue meet your expectations?

@jmeshell If the feature supports naming Groups as Approvers, then this will meet my expectations. The last 2 comments make me a little unsure about whether thats in the plan or not.

allow reporters to approve MRs if they are explicitly listed in the approval rules is the idea @weitzman

Thanks but that omits my key question. Can one name a Group as an explicit approver in Project merge approval rules. If explicit approval means naming individuals or if this doesnt work in project settings, then it would not work for Drupal. Thanks for your attention.

I see what you are asking - this specific issue is enabling the reporter role in Gitlab to approve MRs in the approval rules - which already support adding a group to the approval rule: https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html#eligible-approvers.

Groups and users are supported today, but roles are not. This feature will allow the Reporter role (from a group or user) to be able to approve MRs.

changed the description

added direction label

Link to request:
- https://gitlab.my.salesforce.com/0014M00001gAdAj
Why interested:
- Currently are able to set protected branches and protected environments based on user roles (if only developer and higher, perhaps consider all roles as an option to be selected), but it would be nice to have this same role level selection capability for MR approvals.
Current solution for this problem:
- Developer or higher permissions required which at times can come into conflict due to parent namespace user inheritance
PM to mention: @jmeshell @danielgruesso @mattgonzales

Thank you @jeffersonj

changed the description

@kencjohnston this is a a second part delivery for configuration of separation of duties

mentioned in issue gitlab-com/Product#1413 (closed)

@jreporter - There are two parts of this issue that contain what I'd consider proposals:

Further details

We will need to add a configuration option for groups to select maintainers cannot deploy, and only this deployer role can deploy.

and Proposal

Allow reporter roles to approve MRs if they are explicitly listed in the approval rules.

I think the MVC you're looking for me to evaluate is the proposal, and I'll respond accordingly - let me know if I got that wrong.

I think this is a great small iteration. Is the real iteration that you can now add Reporters to Merge Request approval rules? If so that seems sufficiently small and utilizing existing features but definitely improving the pain from the defined problem.

Yes, further details are in place so the dev has other context adjacent and knows to create a follow up as required

. Is the real iteration that you can now add Reporters to Merge Request approval rules?

Yes that’s right @kencjohnston

Great thanks @jreporter . I think that is a great MVC approach to this problem. Nice work!

Great thank you @kencjohnston

mentioned in epic &4180 (closed)

added to epic &4180 (closed)

added Deliverable label

assigned to @vshushlin

added 1 deleted label and removed 1 deleted label

removed [deprecated] Accepting merge requests label

assigned to @sean_carroll and unassigned @vshushlin

added workflowin dev label and removed workflowready for development label

Complete	Confident	Notes
30%	80%	Researching options but solution looks viable without large changes.

created merge request !40491 (merged) to address this issue

mentioned in merge request !40491 (merged)

mentioned in commit 9a319a7e

Have run into an issue here and would like to clarify the approach.

We are trying to allow Reporters to be granted Allow to Merge on a protected branch (eg master). The MVC would be to create a new group with Reporter access and grant that group to Allow to Merge.

From the backend the approach would be very similiar to the Deployer authorisation for protected environments MR, where we allowed users granted access to the protected environment to deploy.

In this case if the user is granted access to the protected branch under Allow to Merge, they will be able to merge despite not having push access.

It appears that there will need to be some frontend changes, as it is not currently possible to add a group to Allow to Merge unless it is also added to Allowed to push.

Specific steps to view this:

Create a new group and add to the project with Reporter access

Add the group to "Allowed to push"

The protect button is not enabled, unless the group is added to Allowed to push.

Changes needed

Enable protection of a branch when only Allowed to Merge is selected
Ensure that members of groups assigned to Allowed to Merge can actually do so
This could be for any role, but will include Reporters

@poffey21 can you confirm?

CC @jreporter

I think as a first iteration @sean_carroll we can add documentation that specifies:

In order to leverage merge approval rules for a group for Reporters, the group needs to be added to Allowed to push.

Then, we can add a follow up issue to only support merge actions, WDYT @sean_carroll?

@sean_carroll it is important to note that this is not enabling reporters to "merge" any code, only allowing them to "approve" the code can be merged. The Merge Button will never appear for the reporter, but the "approve" button will appear.

I'll confirm w/ @jreporter that this is an acceptable approach. This still prevents developers from merging without approval, whilst allowing a reporter Zero Access to anything in the commit history.

I don't like the approach where the Reporter can merge because then the Git History is being modified by the Reporter.

it is important to note that this is not enabling reporters to "merge" any code, only allowing them to "approve" the code can be merged.

This makes sense to me @poffey21

Thank you @poffey21 and @jreporter. To summarize:

In a future iteration:

we might add a field called Allowed to Approve, which would grant whoever is assigned that specific permission only

For this iteration:

Create a group and assign users to it
Group is attached to the project as Reporter
Group is assigned to a protected branch under both Allowed to Merge and Allowed to push
As the group has an access level of Reporter, it is only allowed to approve, and in fact not allowed to merge or push, despite being listed as such. This we can clearly document.
Groups attached as Developer or Maintainer are not affected.

Is that correct @jreporter @poffey21 ?

This is correct @sean_carroll

Perfect, thanks for the clarification @jreporter !

@sean_carroll this looks near perfect - I'd suggest a few minor tweaks.

Create a group and assign users to it
Group is attached to the project as Reporter
A merge request approval rule is added (Settings > General > Merge request approvals) and the group is attached to that approval rule.
Groups attached as Developer or Maintainer are not affected.
Users in that group will have zero access to Merge/Push and protected branches are not modified, but the users now have the ability to "approve" merge requests.

It sounds like there is a disconnect between how Approval rules are presented and how the mechanism actually works - @sean_carroll @poffey21

Group is assigned to a protected branch under both Allowed to Merge and Allowed to push

As the group has an access level of Reporter, it is only allowed to approve, and in fact not allowed to merge or push, despite being listed as such. This we can clearly document.

vs.

A merge request approval rule is added (Settings > General > Merge request approvals) and the group is attached to that approval rule.

Is it true @sean_carroll that Approval rules are assigned by being allowed to Merge and Allowed to push? Or can they just be added to Merge Approval Rules?

Approval rules are assigned by being allowed to Merge and Allowed to push? Or can they just be added to Merge Approval Rules?

@jreporter I am investigating this now.

Perfect thank you @sean_carroll

@jreporter @poffey21

I did some testing (in a local instance) on the current state for adding groups to Merge Request approvals. In order to approve a MR the user must be added to the group as a Developer and the group added to the project as Developer.

Scenario	User	User in Group	Group in Project	Approve MR ?
1	Valentina Kohler	Reporter	Developer	No
2	Tam Wolff	Developer	Developer	Yes
3	Chelsie Klein	Developer	Reporter	No
4	Brandon Waters	Reporter	Reporter	No

All groups are also added to the Merge Request approvers page

Observations

all usernames show up in the Merge Request approvers page (see above screenshot), even though they cannot approve
only the actual person able to approve shows up against the MR (Tam Wolff)
it is possible to add a group to the Merge Request approvers page without first adding it to the Project. This group cannot approve but I wonder if this should not be possible without adding the group to the project?

Is it true @sean_carroll that Approval rules are assigned by being allowed to Merge and Allowed to push? Or can they just be added to Merge Approval Rules?

Groups can be added to Merge Approval rules only, as noted above, but it doesn't grant any access to actually approve.
Being a Reporter either when added to the group of when the group is added to the project means the user cannot approve
In the current state, the user must be a Developer, so they will get push access, although not to the protected branch.

I think the criteria for Reporters to be able to approve should be:

User is added to the group (at any access level)
Group is added to the project as Reporter
Group is added to Merge Request approvers page

User is added to the group (at any access level)

Group is added to the project as Reporter

Group is added to Merge Request approvers page

Is this a user action to take or are there mechanics that you must implement to allow Reporters to approve MRs without push access @sean_carroll?

@jreporter some code changes will be needed.

got it thank you for clarifying @sean_carroll

@jreporter I'm curious if #227468 (closed) might be of interest to you here. Based on the above discussion, I'm wondering if there's an opportunity to create a unified experience in an abstracted/global list of approvers. They could be approvers for two-person approvals (current intent) or be a list of authorized deployers in the future.

Is this relevant or did I misunderstand some context? Starting this new comment so as not to distract from the thread above

This particular feature can be used in conjunction with #227468 (closed) @mattgonzales. In theory, people would just assign specific users to approve and those people now (with the addition of this feature) can be reporter roles.

@jreporter Gotcha. Thank you for the confirmation

I'll see myself out now

lol, you are the best @mattgonzales! appreciate you engaging.

Complete	Confident	Notes
50%	90%	Have a clearer understanding now, should be straight-forward.

thanks @sean_carroll - let me know if we need to reduce scope or iterate on anything!

@jreporter thanks I think the scope is as small as it can be. Hope to have it into review today.

Thank you for that confirmation and update @sean_carroll

@sean_carroll just checking in on this since I haven't seen a recent update. This is a Deliverable for 13.4 so I want to make sure we prioritize getting this through. Are we still looking good to get this completed?

thanks @nicolewilliams for the ping for an update!

mentioned in commit da156322

mentioned in commit 861c790b

added sectionops label

mentioned in commit 922f2a6b

mentioned in commit e5cbd263

mentioned in commit 21dc415b

Allow reporters to approve MRs if they are explicitly listed in the approval rules

Problem to solve

Intended users

Further details

User experience goal

Proposal

Designs

Child items ...

Activity