Group approval invalidated if user is later removed from the group

added customer devopscreate groupsource code typebug + 1 deleted label

added sectiondev + 1 deleted label

Large GitLab Premium customer encountering this on 345772 (internal)

Link to request: https://gitlab.my.salesforce.com/0016100000sPtxlAAC
Priority: customer priority:: 3
Why interested: Makes auditing very cumbersome
Problem they are trying to solve: Group approval invalidated if user is later removed from the group
Current solution for this problem:No solution
Impact to the customer of not having this: Makes auditing very cumbersome
Questions: what milestone would be be looking at for this issue
PM to mention:

added to epic &8499

changed milestone to %Backlog

set weight to 2

added bugfunctional + 1 deleted label

added backend + 1 deleted label

I've added this issue to epic that tracks other approval rules configuration issues.

This issue requires a minor research to confirm the problem and detect the root cause. I suspect that we might delete given approvals when user is removed from the group.

When you say 'research', what kind of research are you thinking, @vyaklushin ?

@tlinz probably "verification and root cause analysis" is a better term here. We need to reproduce it locally and determine exactly what is causing this behavior. The logic of approval rules is complex and it's difficult to reason about it without looking into the code.

Thank you @vyaklushin, I will then leave it up to @a_mcdonald to prioritize it. cc: @sean_carroll

I would say let's try to add it as a spike to %15.8, I will update the labels

Moving to %Backlog

added priority2 severity2 + 1 deleted label

added spike + 1 deleted label

changed milestone to %15.8

mentioned in epic &6592 (closed)

changed milestone to %15.9

mentioned in issue create-stage#13095 (closed)

Setting label(s) Category:Source Code Management based on groupsource code.

added Category:Source Code Management + 1 deleted label

@sean_carroll, @tlinz and @a_mcdonald,

This groupsource code bug has at most 25% of the SLO duration remaining and is ~"approaching-SLO" breach. Please consider taking action before this becomes a ~"missed-SLO" in 14 days (2023-01-27).

added SLONear Miss + 1 deleted label

Moving to %15.10 planning

changed milestone to %15.10

added SLOMissed + 1 deleted label

@phikai @mnohr this issue is related to the usage of approval rules for merge requests and requires input from Product/UX perspective to figure out the correct behavior here. I believe the MR should stay approved even if a user/group has been removed from an approval rule.

Let me assign it to groupcode review but feel free to assign it back if you disagree

@igor.drozdov I'd guess this stems from the fact that we return these items in real-time from whatever the configuration is? Although maybe it's also related to #373727 (closed) (cc: @marc_shaw).

I'd think in either case, I'm not sure how we'd leave a user there... particularly if they didn't exist on the platform anymore... we wouldn't have an object. Then I'd sort of expect that if they were left there, you'd have a different problem that it looks like someone who shouldn't be in the organization approved something?

@stkerr @derekferguson any thoughts on this from an audit/compliance perspective? What's the correct way to handle this?

I'd think in either case, I'm not sure how we'd leave a user there... particularly if they didn't exist on the platform anymore... we wouldn't have an object.

@phikai yes, if a user has been completely removed it becomes complicated. But if it was removed from a group/rule, then probably we can do something about it. For example, it's still shown in Approved By section and I see that we have some explanation in the code about this particular case

@igor.drozdov That's weird... cause it seems that the code indicates it shouldn't change... but it is?

cc: @kerrizor cause maybe the patch-id work would make sense to keep this in mind

@phikai yes, I didn't try to reproduce it, but if anything went wrong it's probably related to that part of the codebase

When a user is deleted, it makes sense to me; the approval record is tied to the MR and the user, so if the user is deleted, the approval should be deleted as well.

For the case where the user is removed from the group, my gut says to look at the logic around determining what approvals are still required. I suspect it is stemming from our not implementing a means to tell which rules a user has fulfilled.. the approval record still exists, after all, but the language shows it as "invalidated" as it is (IIRC) matching the user_id of the existing approvals against the Set of user IDs of people who COULD approve it. Since the user is out of the gorup, they're not longer a user we test for, so their approval is "invalidated"

@phikai @igor.drozdov @derekferguson I think this is a decision that could be made either way, so GitLab will need to take a position on the "correct" way to do it, document it, and then possibly offer the option to do the other way if users prefer in an opt-in manner.

My personal opinion is that if the person approved something while they were a group member, that approval should stay valid. Imagine a scenario where a person approves something, leaves the company, and is removed from all groups - it would appear as if certain items were not approved, even though they were approved by a valid person at the time. If the approval is removed, an audit an audit in the future could identify those "merged but not approved" items as a non-coformance that would need to be remediated. Again, that is my opinion so I'll defer to you all for making a final decision.

@phikai Potentially related Let me check

I tried to replicate this, and couldn't

Maybe I am missing something, but from my understanding, it shouldn't matter if the group changes or not, at the time of merge, we keep track of who was able to approve, and who did approve it.

@patrickbajao Any idea here ?

@phikai I agree with @stkerr's assessment here. From a compliance perspective, you'd want to know who approved things, regardless of their current status at the company. If it was something that they legitimately had the authority to do when they worked at the company or in a specific group, then their approval shouldn't change. If it is just removed, that is going to cause a problem for our customers during audits. I'm not completely sure how we would keep the approval status with the name if the user is completely removed, but it seems to me that it is something that could significantly affect our users who rely on us for audit reports. At least the audit event for the approval should stay the same, but I'm wondering if the MR would suddenly show up in the Compliance Violation report. @nrosandich That's probably something we should test.

@marc_shaw I can only replicate this bug when "Prevent editing approval rules in merge requests" setting is checked under Settings > Merge Requests > Approval Settings.

When approval rules are overridden at the MR level, in the ApprovalRules::FinalizeService, we are merging the group_users to users of the approval rule after merging. We also copy the project level rules as MR level rules after merge (see #copy_project_approval_rules) but since we don't return the MR level rules when overriding is disabled, they don't show up and we still return the project level approval rules. When the member gets removed from a group, it reflects on the project level approval rule so the approval rule shows up as unapproved.

I think the fix here is to show the MR level rules when MR is merged since we are already copying the project level rules as MR rules. But we also need to ensure that protected branch scope is kept or we might re-introduce the bug in #373727 (closed) (which we are already fixing this milestone).

WDYT?