Skip to content

Fix N+1 query for EpicsFinder when guest is searching in a private group

What does this MR do?

Prior to this change a guest in a private group would end up in the groups_user_can_read_epics branch of logic in #permissioned_related_groups. This loops through each subgroup one at a time checking permissions. Instead of unwinding that logic for now we are just going to add a logical short circuit for a common specific case.

The #permissioned_related_groups only needs to check if the user has the ability to view all epics in all subgroups in a group. Previously we were using the read_confidential_epic permission to confirm that the user could access all epics in the subgroups as well. But this is not necessary because this class will already filter confidential epics later in with_confidentiality_access_check so really we just need to answer the question "Can a user view all epics in this group and all of it's subgroups". This seems deceptively like we could just check the read_epic permission but there is a weird edge case for public groups. A user can view epics in a public group even if they aren't a member but that public group can contain private subgroups and they won't be able to view those epics. So we can conclude that if a group is private? as well as the user having the ability to read_epic in that group then they will also be able to read_epic in all child groups as well.

This MR also introduces a new explicit test for guest permissions with confidential issues. The reason being that we don't appear to have any such tests about guest users already and it may give us more confidence that confidentiality filtering is working correctly prior to implementing this optimization.

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Related to #327106

Edited by Dylan Griffith

Merge request reports

Loading