Allow admins to modify MR approval settings at the project-level after enabling them in admin area

added Category:Compliance Management Enterprise Edition GitLab Premium devopsmanage groupcompliance typefeature labels

changed milestone to %12.9

added workflowready for development label

added backend label

added [deprecated] Accepting merge requests label

Hi @mattgonzales :) I have questions (of course I do;)). I understand general concept, but I have questions about details.

the instance-level settings are all disabled, an admin enables them. They suspect all projects should follow the instance-level settings?
but then administrator disables them for one project. If there were disabled before for this project, how should we know that it changes?

In the nutshell: we can achieve it in couple of ways:

update ALL the projects while changing settings on instance layer (I would not recommend).
follow push-rules behaviour: instance settings are treated as template, so only projects created after changes in instance settings are inheriting those settings.

Wdyt? We can schedule a call to brainstorm ideas about it.

@mksionek no worries! Glad you have them

the instance-level settings are all disabled, an admin enables them. They suspect all projects should follow the instance-level settings?

Yes; however, recent customer conversations have proven that this approach, while useful, is too broad. This was something we knew going into it and one proposal was that we could permit group selection.

My current thinking is this: I'll be pivoting on #118671 (closed) to make the MVC an ability to add project topics to projects. If customers add SOX to all of the relevant projects that are SOX compliant, we could then provide a mechanism to selectively apply these MR approval settings to "only projects that are SOX topics". Does that make sense?

but then administrator disables them for one project. If there were disabled before for this project, how should we know that it changes?

I think if we take the approach above this may be less relevant since there'd be more deliberate actions taken to apply these settings only to specific projects.

follow push-rules behaviour: instance settings are treated as template, so only projects created after changes in instance settings are inheriting those settings.

I think this should be our approach in the interim until we can get project topics (or similar mechanism) working to help customers label projects for specific compliance frameworks as detailed above.

Summary:

My vote is to follow push-rules behavior on inheritance
I'll be refining #118671 (closed) to reflect the changes I'm proposing about project topics
A future iteration on this could select projects to enforce these settings for using new project topics concept

Does this sound good to you? Happy to setup a call as well if you'd like

This is shifting to %13.0 due to a re-prioritization of some issues in %12.10

We will attempt to move this into %12.10 if time allows.

@mksionek Out of curiosity, is someone else running with this particular issue or is this still on your plate?

Happy to discuss the shuffle if I can shed some light here

changed milestone to %13.0

I was meaning to do it, but I got buried in group push rules :(

No problem at all! I think this works out perfectly since I'm not disrupting any progress on this issue

Does it make sense to have @manojmj or @tancnle pick this up in %13.0? My understanding is they're supporting groupcompliance full-time, but I'm not sure about the dynamics here. (cc @lmcandrew)

I don't want you to worry about something unnecessarily, so please let me know your thoughts

added missed-deliverable missed:12.9 labels

mentioned in issue gl-retrospectives/manage#26 (closed)

added Next Up priority1 labels

@mksionek you seem to be very familiar with this feature, would you happen to have mentally estimated the weight for it already? If not that's okay, I just wanted to check

ok, that is not a super big thing, so something between 3 and 5, I think updating specs may be time-consuming. It's not a complicated change overall.

and - I'm happy to help :) I just didn't have the capacity to do it on my own :(((((

assigned to @tancnle

added workflowplanning breakdown label and removed workflowready for development label

@tancnle please consider this your "absolute" top priority for 13.0 if that's okay

removed [deprecated] Accepting merge requests label

set weight to 5

added workflowscheduling label and removed workflowplanning breakdown label

added workflowready for development label and removed workflowscheduling label

added Deliverable direction labels and removed Next Up label

mentioned in epic &3083

@djensen @tancnle A thought I had as I'm writing the feature block (early? what? madness!):

My original premise here was that an admin should be able to modify the settings at the project-level if they had been enabled at the instance-level. That's still the intention here.

The thought I had was: what about if the settings aren't enabled at the instance-level and the admins want to apply them, and have them be unchangeable by non-admins, at the project-level? Would this implementation make it so that:

Admin enables MR settings at the project-level
Those settings cannot be modified by non-admins (until an admin disables it?)

This would provide some additional flexibility in the interim until we can ship #213601 (closed). WDYT?

@mattgonzales at the risk of being repetitive, I'd like to re-state the requirements to make sure I understand the dynamics:

MR approval settings exist at the instance and project levels.
Only admins can modify MR approval settings.
If a setting exists at the project level, apply it to the project. If it exists at the instance level, that is irrelevant and ignored.
If a setting does not exist at the project level, then check for it at the instance level. If it exists at the instance level, apply it to the project.

Is that all correct? Anything missing?

@mattgonzales I have attempted to sum up the rules here. Please let me know if I miss anything.

Use case	Instance (Admin)	Project (Admin)	Project (Non-admin)
1	Enable	Enable	Non-editable
2	Enable	Disable	Editable
3	Disable	Enable	Non-editable
4	Disable	Disable	Editable

The challenge for me at the moment is Use case 2 & 4. There is no current mechanism to distinguish that a settings change was done by an admin or a non-admin user. A non-admin user can enable that setting and effectively lock all non-admin users out for future changes. This is a bit counter-intuitive for non-admin users IMO.

Another approach is to only allow admins to modify project approval settings until we formalise our approach for #213601 (closed).

Tan, have you seen my comment?

allow push-rules behaviour: instance settings are treated as template, so only projects created after changes in instance settings are inheriting those settings.

@djensen I beleive @tancnle has summarized it exactly as I'm thinking about it I hope that's helpful!

@tancnle What we know about this pain point is that GitLab administrators want to be able to lock down MR approval settings. Right now, the current implementation is very broad and heavy-handed. Implementing #213601 (closed) will help to reduce the scope, but that's a somewhat different issue than what we're discussing here, I think...

That being said, maybe the dynamics of use cases 2 and 4 will be rendered moot by implementing #213601 (closed) since it would allow admins to scope those settings only to regulated projects and we know they don't want non-admins to edit those settings in regulated projects.

A non-admin user can enable that setting and effectively lock all non-admin users out for future changes.

I think that might be an existing bug? The proposal for #39060 says: "At the Project level, these settings should only be editable by Administrators, but still be visible to non-admins for information purposes." But in the code it looks like owners and maintainers can modify them. Maybe I'm not understanding - I think @mksionek or @tancnle could both be more insightful on this.

only allow admins to modify project approval settings

That seems like the solution here to me, whether it's a bugfix or a new feature.

@djensen You're absolutely correct that the original issue should have implemented it, but it was a miscommunication/misunderstanding during the breakdown that resulted in the intended-not-intended behavior

I opted not to call it a bug because, while a misunderstanding, it was the intentional implementation at the time. I think here we should consider it an iteration on the existing feature

The settings should only be modifiable by administrators once they've been set at the instance level. Maybe there's a more elegant solution here though? To recap from our call, and for clarity here in the issue, we're trying to solve:

Administrators, often the Compliance Manager persona, want to enforce specific MR approval rules for regulated projects
- Default number of approvers
- Authors cannot merge their MRs
- Committers cannot merge their MRs
- Approvers cannot be overridden at the individual MR level

We chose the settings in #39060 (closed) as an iterative first step, but if there's an opportunity to pivot, I think now would be the time

mentioned in merge request gitlab-com/www-gitlab-com!47500 (merged)

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

mentioned in merge request !30358 (merged)

added workflowin dev label and removed workflowready for development label

What we know about this pain point is that GitLab administrators want to be able to lock down MR approval settings.

@mattgonzales This sounds like an additional setting to me. So the admin can set those 3 rules and something like a do-not-allow-override flag perhaps?

That being said, maybe the dynamics of use cases 2 and 4 will be rendered moot by implementing #213601 (closed) since it would allow admins to scope those settings only to regulated projects and we know they don't want non-admins to edit those settings in regulated projects.

Totally. I don't want to go overboard too much, but in the backend, I'd imagine we still need to have our rule engine to be smart around scopes, however that also depending when we derive the meaning of those rules. For example, at the time a compliance rule is enabled to force prevention of MR approval from author at group-level, do we:

set this rules on all subgroups/projects; or
defer until the users view those lower-scoped settings or the enactment of the MR approval kicks in.

allow push-rules behaviour: instance settings are treated as template, so only projects created after changes in instance settings are inheriting those settings.

@mksionek I am aware that we already had some established behaviours around push rules and can understand that there is a need to have common patterns in code and for end-user to avoid future surprises. I was unsure and love to discover more if MR approval settings template for new project is part of this issue or according to #207250 (comment 330305544), it is post-project creation behaviour that we are interested here .I am keen to set up a chat with you over Zoom to discuss more this week.

A non-admin user can enable that setting and effectively lock all non-admin users out for future changes. I think that might be an existing bug?

@djensen It is not at the moment given we treat instance level settings as SSOT. Changes at project level will be reverted back IF they are already enabled at instance level settings.

Based on my recent tests, the below table summarises the current behaviour:

Use case	Instance (Admin)	Project (Admin)	Project (Non-admin)
1	Enable	Editable, but reverted on save	Non-editable
2	Disable	Enable	Editable
3	Disable	Disable	Editable

This sounds like an additional setting to me. So the admin can set those 3 rules and something like a do-not-allow-override flag perhaps?

@tancnle I'm not opposed to this I provide some concise context above that might be helpful here.

however that also depending when we derive the meaning of those rules.

I'm not sure I understand completely but based on my understanding, I think the ideal situation is option 1, "set this rules on all [regulated] ~~subgroups~~/projects" where "regulated" is any project that has the compliance framework label.

As I write this out, I imagine there's probably a conflict here since projects are designated as regulated, but not groups. Is that problematic?

@mattgonzales update for you: In the spirit of MVC, I asked Tan to start with implementing the project-level rules for admins only. This feature set is already a bit complex, so I'm concerned about 1) breaking things and 2) opening security gaps. Totally happy to come back and implement for non-admins under a fresh issue if you want. Is this incremental approach alright with you?

@mattgonzales @djensen After having a conversation with @mksionek and digging around more the code, it feels like the template concept is quite appealing when we start introducing project-scoped rules for compliance framework. Conceptually, I was thinking about something like this for the domain model:

With that view in mind, I have traveled through a possible path for !30358 (merged)

Divorce the instance-level rules from project-rules. In other words, changes in instance-level rules does not affect project-rules.
Project-level rules are editable by admin only. Owner and maintainer can still see those rules but can't edit.

For #213601 (closed), we can start possibly forming the rule template concept and apply those templates to regulated projects.

What do you think?

@djensen @tancnle My apologies, but I'm not understanding

I asked Tan to start with implementing the project-level rules for admins only

What would this look like from a UX/UI perspective?

Totally happy to come back and implement for non-admins under a fresh issue if you want.

Same question as above. What does this look like for "non-admins"?

it feels like the template concept is quite appealing when we start introducing project-scoped rules for compliance framework

@tancnle I may be misunderstanding this completely, but my takeaway is this would a template that applies to new projects? Or does template refer to a concept on the backend independent of the project template feature? I don't know if we want to tackle pre-defined values, per framework, just yet until we've had time to collect feedback about what values are most commonly used for specific frameworks.

The feedback I received from a customer recently, when asked if they wanted the settings to apply retroactively or only to new projects, was "Retroactively, yes. That is very important to me." So there's two considerations here:

Enable admins to define MR approval rules at the instance (eventually group) level
Apply those settings retroactively to the appropriate projects (those labeled with compliance frameworks)

It sounds like the second consideration is the challenge we're trying to address with these discussions, right?

Thank you both for your patience and understanding. I'm sure you both have a clear idea of the path forward; I just need some nudging

@mattgonzales Thank you for -ing with me while I am trying to get more understanding of the requirements and navigating through the technical implementations.

What would this look like from a UX/UI perspective? What does this look like for "non-admins"?

Instance (Admin)	Project (Admin)	Project (Maintainer)

Or does template refer to a concept on the backend independent of the project template feature?

Sorry for the misunderstanding. The template can be used for new project as well as be retroactively applied to existing project when they are labelled with compliance frameworks. This is an expansion on the existing template concept. To be clear, this is more for #213601 (closed) and beyond.

I don't know if we want to tackle pre-defined values, per framework, just yet until we've had time to collect feedback about what values are most commonly used for specific frameworks.

Good point! I was jumping ahead of myself here For now, we can consider the existing instance-level rules as the template.

It sounds like the second consideration is the challenge we're trying to address with these discussions, right?

Yes, it does. I think we are trying to iteratively build our implementations to fulfill the second consideration which is (please correct me if I misunderstood) the intention of #213601 (closed).

To present the project-level rules to the end-user, we currently combine instance-level and project-level rules (as per the table in #207250 (comment 330877195)). In other words, we derive the final rules on view. This combinatorial logic together with roles makes the code undesirably more complex and less performant.
To mitigate this, we can set the project-level rules in bulk when instance-level rules are set. Without the ability to selectively scope these rules to a set of projects, I'd not recommend this as it could result in a lot of writes to the projects table.

Apply those settings retroactively to the appropriate projects

I am thinking of two potential trigger points.

When a project is labeled => Retrieve the instance settings => Set them to project-level rules
When the settings are changed => Retrieve the list of appropriate projects => Set them to project-level rules

I am leaning towards having those two tackled in #213601 (closed). What do you think?

My proposed flows:

This MR

In #213601 (closed)

Thank you @tancnle for the detail! That's helpful!

So my understanding is that your proposal would accomplish:

Admins enable instance-level MR settings
The admin area settings can be scoped to project compliance labels (e.g. SOX). This means both new projects and existing projects (retroactively), with the SOX label, will inherit these admin rules.
Project MR settings are editable by Admins only (in compliance-labeled projects)
Projects with no compliance label are unaffected and retain their existing behavior (maintainers can modify the MR rules)

Is that right so far?

If my understanding is correct, then this sounds good to me and I believe meets the intent of what we're trying to achieve

If I'm not correct in my understanding, then I'll just be over here in the corner crying gently

Outcome from my conversation with @mattgonzales (please correct me if I miss anything crucial or incorrectly rephrased )

For this issue:

Admins enable instance-level MR settings. This setting does not change any project settings.
Project MR settings are editable by Admins only on GitLab Premium
Project MR settings are editable by Maintainers and above on GitLab Starter
Have this change under a feature flag

For #213601 (closed):

The admin area settings can be scoped to project compliance labels (e.g. SOX). This means both new projects and existing projects (retroactively), with the SOX label, will inherit these admin rules.
Project MR settings are editable by Admins only in compliance-labeled projects on GitLab Premium
Project MR settings are editable by Maintainers and above on GitLab Starter

We can also discuss further work-breakdowns of #213601 (closed) in the issue itself.

@tancnle This sounds correct to me

For further clarification: we agreed that it's ok for this issue to essentially break the intent of the instance-level MR approval rules since we'll be improving the underlying mechanisms and provide a holistic, working-as-intended solution with the completion of #213601 (closed)

With all of this said, do you agree this issue is still a weight of 5?

Let me adjust the weight given the agreed scope.

set weight to 3

@mattgonzales As I have dug deeper into the implementation of adding a feature flag, it looks like a dearer endeavour to me and I am concerning about the value it adds, given we are going to revert that in #213601 (closed)

To recap, so originally we have agreed:

Feature flag: OFF

Existing behaviour as per #39060 (closed)

Feature flag: ON

Break the intent of the instance-level MR approval rules.
Only allow admin to modify project-level MR approval rules

Can I entertain you with other alternatives?

Option A: With feature flag

What do you think about an alternative to use feature flag to hide the instance-level MR approval rules as it is considered an unfinished feature?

Feature flag: OFF

Does not show instance-level MR approval rules
Only allow admin to modify project-level MR approval rules

Feature flag: ON

Show instance-level MR approval rules. This setting does not change any project settings.
Only allow admin to modify project-level MR approval rules

Option B: Without feature flag

Communicate this breaking change to our users.

Option C: Schedule this to be rolled out as a complete solution in %13.1

My sincere apology on having a bit back-and-forth on this issue, I'd like to be absolute clear on the expectation for this issue.

@tancnle Please don't apologize! I really appreciate the due diligence

I didn't even know it was an option to feature flag an existing feature That seems to make sense and we can communicate that we've put it behind a feature flag. That seems like the best option.

I don't believe customers are currently using this feature given the broad scope of enabling the settings, but how would we account for customers who might be using it? Would they just automatically set feature flag to on if those settings are currently in use?

@jeremy is there any special process for this type of scenario?

@mattgonzales

how would we account for customers who might be using it? Would they just automatically set feature flag to on if those settings are currently in use?

I am not sure how to handle this case. In both ON and OFF state, the instance-level settings do not affect project-level ones. It is just that in OFF state, we hide these controls to avoid confusion for our customers (i.e. they enable the settings and expect all project-level settings are updated accordingly). The admin can still set those settings per project on project settings page.

It's also worth noting that since this is an instance settings, the feature flag is applied at instance level, aka. we can't selectively disable/enable for project or group. And by default, feature flag state is OFF.

@tancnle Ahhh, gotcha. Oversight on my part

Then, in theory, this should be fine

mentioned in issue #213601 (closed)

added workflowverification label and removed workflowin dev label

@djensen how does this impact .com customers? Are we (as GitLab) able to prevent our customers from modifying their approval rules with this setting?

@m_gill I don't think this feature impacts .com, only because there are no customer admin users on .com. On the second question the answer is yes, although we wouldn't want to, would we?

I'm worried about it accidentally happening. Initially, that's what we had thought was the source of #218454 (closed) - that perhaps we had unknowingly turned this off for customers.

@m_gill It's worth explaining the context here and what we're thinking moving forward. I think it also makes sense to sync up on this with you and @danielgruesso (I think?) to make sure we aren't causing any additional headaches for you

The groupcompliance implication here is tied into segregation of duties. Organizations don't want maintainers to be able to modify MR approval rules. We tackled this with an instance-level setting first to support several enterprise customers who were about to undergo, or just previously underwent, an audit and this was identified as a gap.

The impact for .com customers: we'd like to next focus on adding default approvers to the group level because this would allow two things ->

Control over a default set of approvers that maintainers can't change; and
Use the permissions precedent from the instance-level setting to prevent changes to specific, critical settings (e.g. prevent authors from merging their MRs)

I think an element of implementing this for .com customers is providing an instance-level setting to "Allow group owners to manage MR approval rules" or something similar, which GitLab.com would enable by default and self-managed customers could optionally disable for the more strict organizations.

Should we hop on a call sometime soon to talk about this?

I'm happy to hop on a call, I have been OOO recently and so has @danielgruesso. Would you set something up and we can record it for async?

For context: The MR to allow only admins to modify MR approval settings at the project-level was reverted in !32610 (merged) because of reports that Owner and Maintainer users were unable to change merge request approval settings.

We will re-implement the feature to include a fix for that bug. I'm bumping this issue to %13.1 to reflect.

@mattgonzales @djensen I have contemplating further on the implication of allowing Owner and Maintainer users for this issue. It turns out to be more of a complete revert of #39060 (closed). And I am not sure about the value of doing that.

Is the intention to allow admin to change MR settings at individual project-level settings still required for &3432 (closed)? For example, after the MR settings are scoped on instance-level, admin still want to manually exclude to some projects.

@tancnle That's a good question. I'm second-guessing that decision

The original motivation to allow MRs to modify the local project settings, after setting them at the instance level, was because we didn't yet have the compliance framework labels.

I wonder if, now that we have those labels, it makes sense to just nix that capability (allow admins to modify the local project settings after being set) altogether. At least for now?

I also think the implementation of two-person approvals could solve for the one-off changes that might need to be made and may even be a better solution since it would empower developers, maintainers, or owners and require a second set of eyes to approve the change

A follow-on to this note :

If a MR approval setting is overridden, it would need to be temporary and/or permit someone (probably an administrator or group owner) to re-enable those settings once the activity (that required the override) is completed.

@mattgonzales

I wonder if, now that we have those labels, it makes sense to just nix that capability (allow admins to modify the local project settings after being set) altogether. At least for now?

Yep I totally agree We can focus on implementation for &3432 (closed) for now and revisit this later.

I also think the implementation of two-person approvals could solve for the one-off changes that might need to be made and may even be a better solution

Good point. I also think that flow fits better with GitLab model of everyone can contribute

@tancnle Awesome! So sorry to have sent you down a rabbit hole... but I'm really happy with this outcome! For clarity and alignment:

We'll focus efforts on &3432 (closed) in %13.1
We'll rely on compliance framework labels (only) for scoping and not project-level admin toggles
We'll ensure two-person approvers can handle changes to these settings for one-off exceptions

Do you feel &3432 (closed) is still achievable in %13.1?

@mattgonzales No drama. It was a wonderful collaboration and excellent learning opportunity, cobber

I agree with all above three points

Do you feel &3432 (closed) is still achievable in %13.1?

Yes, I feel like we have a clear MVC path on #219359 (closed). Let's push it for %13.1

No drama. It was a wonderful collaboration and excellent learning opportunity, cobber

@tancnle I know exactly what this means

Yes, I feel like we have a clear MVC path on #219359 (closed). Let's push it for %13.1

Awesome! Thanks so much for the update and collaboration. The moment you feel this may slip, please do let me know so I can communicate that to the enterprise customers expecting this to ship in %13.1. Otherwise, I'll be happy to send coffee, snacks, or any other consumables to support your efforts

changed milestone to %13.1

added to epic &2423 (closed)

added workflowsolution validation label and removed workflowverification label

@mattgonzales we're still trying to figure out the best path for the re-implementation on this. There is some risk of slip, but we're doing our best

Thanks for the update @djensen! I know this has grown into a bit of a booger, but hopefully we can figure out that path forward together

@djensen Given @tancnle's comment above I think the best option for this issue is to close it out since it's likely no longer relevant.

You and I have also discussed the thinking here, which I outlined above as well.

If you agree, I think we can close this out

@mattgonzales this means miracles really do happen!

mentioned in epic &3432 (closed)

closed

mentioned in issue #239349 (closed)

mentioned in issue #273765 (closed)

mentioned in issue gitlab-org/manage/general-discussion#17290

mentioned in merge request !46637 (merged)

mentioned in merge request !47144 (merged)

Allow admins to modify MR approval settings at the project-level after enabling them in admin area

Problem to solve

Intended users

Proposal

Permissions and Security

Designs

Child items ...

Activity

Option A: With feature flag

Option B: Without feature flag

Option C: Schedule this to be rolled out as a complete solution in %13.1

Allow admins to modify MR approval settings at the project-level after enabling them in admin area

Problem to solve

Intended users

Proposal

Permissions and Security

Relates to

Activity

Option A: With feature flag

Option B: Without feature flag

Option C: Schedule this to be rolled out as a complete solution in %13.1