Security policy group approvers not properly added to eligible approvers
Summary
A user created a policy that required approvals from three groups by path. However, no eligible approvers were appearing in MRs for the enforced projects. We investigated and learned that one of the groups was properly added, but 2 were not.
It's possible that because user who added the groups to the approval rule is not part of the 2 groups that were not added, that it was not accepted.
We use the user who last updated the policy to create approval rules and the lack of access to the groups may mean they don't have permission to make this change.
Steps to reproduce
- Create 4 groups.
- Create policies and a project in group 1.
- Create a user in each of the other 3 groups.
- Add those groups by path to a policy as group approvers.
- Ensure the user who creates the policy is only in group 1 and 2, not 3 and 4.
- Create an MR in a project in group 3 or 4.
- Observe if eligible approvers appear in the MR as expected.
Example Project
What is the current bug behavior?
Eligible approvers are not available on the target project/MR.
What is the expected correct behavior?
We should see that a policy requires approval and who is eligible to provide the approval.
Relevant logs and/or screenshots
See internal comment.
Output of checks
Results of GitLab environment info
Expand for output related to GitLab environment info
(For installations with omnibus-gitlab package run and paste the output of: `sudo gitlab-rake gitlab:env:info`) (For installations from source run and paste the output of: `sudo -u git -H bundle exec rake gitlab:env:info RAILS_ENV=production`)
Results of GitLab application Check
Expand for output related to the GitLab application check
(For installations with omnibus-gitlab package run and paste the output of:
sudo gitlab-rake gitlab:check SANITIZE=true
)(For installations from source run and paste the output of:
sudo -u git -H bundle exec rake gitlab:check RAILS_ENV=production SANITIZE=true
)(we will only investigate if the tests are passing)
Possible fixes
diff --git a/ee/app/finders/security/approval_groups_finder.rb b/ee/app/finders/security/approval_groups_finder.rb
index 07a8e7801278..af22058931c3 100644
--- a/ee/app/finders/security/approval_groups_finder.rb
+++ b/ee/app/finders/security/approval_groups_finder.rb
@@ -32,7 +32,7 @@ def global_groups
# Using GroupFinder here would make groups more restrictive than current features related to others approval project rules as in:
# https://gitlab.com/gitlab-org/gitlab/-/blob/0aa924eaa1a4ca5ed6b226d826f7298ec847ea5f/ee/app/services/concerns/approval_rules/updater.rb#L44
# Therefore data migrated from Vulnerability-Check into Scan result policies would be inconsistent.
- Group.public_or_visible_to_user(user).by_ids_or_paths(group_ids, group_paths) # rubocop: disable Cop/GroupPublicOrVisibleToUser
+ Group.by_ids_or_paths(group_ids, group_paths)
end
# rubocop: enable Layout/LineLength
@@ -41,7 +41,6 @@ def groups_within_container_hierarchy
.root_ancestor
.self_and_descendants
.by_ids_or_paths(group_ids, group_paths)
- .public_or_visible_to_user(user)
end
def user_namespace?