Skip to content

Fix runner exhibition during shared groups

What does this MR do and why?

Current MR is a replacement of this MR because the original engineer @wd665544 has taken a long vacation. so it's up to me to solve this issue from now. Thanks everyone.)

The full discussion is at the original issue, and my MR mainly solve the N+1 query problem based on @wd665544 works and @manojmj's advice

Also closes #364094 (closed)

Queries

Produced by User#ci_namespace_mirrors_for_group_members at L2445:

Previously

SELECT * FROM "namespaces" INNER JOIN "members" ON "members"."source_type" = 'Namespace' AND "members"."requested_at" IS NULL AND "members"."source_id" = "namespaces"."id" AND "members"."type" = 'GroupMember' WHERE "namespaces"."type" = 'Group' AND "members"."source_type" = 'Namespace' AND "members"."type" = 'GroupMember' AND "members"."user_id" = 2450 AND "members"."requested_at" IS NULL AND (access_level >= 10) AND (access_level >= 50)

Now

SELECT * FROM (( SELECT "namespaces".* FROM "namespaces" INNER JOIN "members" ON "members"."source_type" = 'Namespace' AND "members"."requested_at" IS NULL AND "members"."source_id" = "namespaces"."id" AND "members"."type" = 'GroupMember' WHERE "namespaces"."type" = 'Group' AND "members"."source_type" = 'Namespace' AND "members"."type" = 'GroupMember' AND "members"."user_id" = 2447 AND "members"."requested_at" IS NULL AND (access_level >= 10) AND (access_level >= 50)) UNION ( SELECT "namespaces".* FROM "namespaces" WHERE "namespaces"."type" = 'Group' AND (id IN ( SELECT "members"."source_id" FROM (( SELECT "members"."id", LEAST ("group_group_links"."group_access", "members"."access_level") AS access_level, "group_group_links"."shared_group_id" AS source_id, "members"."source_type", "members"."user_id", "members"."notification_level", "members"."type", "members"."created_at", "members"."updated_at", "members"."created_by_id", "members"."invite_email", "members"."invite_token", "members"."invite_accepted_at", "members"."requested_at", "members"."expires_at", "members"."ldap", "members"."override", "members"."state", "members"."invite_email_success", "members"."member_namespace_id", "members"."member_role_id" FROM "members" INNER JOIN group_group_links ON members.source_id = group_group_links.shared_with_group_id WHERE "members"."source_type" = 'Namespace' AND "members"."type" = 'GroupMember' AND "members"."user_id" = 2447 AND "members"."requested_at" IS NULL AND (access_level >= 10))) members WHERE "members"."type" = 'GroupMember' AND "members"."source_type" = 'Namespace' AND (access_level >= 50))))) namespaces WHERE "namespaces"."type" = 'Group'

================= Original Description ===================

Hi, Gitlab

We found one issue related with runner exhibition.

Here is the background:
Firstly, UserA create GroupA and GroupB, and invite UserB join GroupA to be owner and GroupA join GroupB respectively. After that, UserA register a runner in GroupB. However, when switch account to UserB, UserB can not see and modify runner in GroupB.

after reading the code, we found the key method ci_namespace_mirrors_for_group_members(level) of authority check, which will filter member who has group owners and return these groups id. following is the ci_namespace_mirrors_for_group_members(level) method code

 # gitlab/app/models/user.rb
  def ci_namespace_mirrors_for_group_members(level)
    search_members = group_members.where('access_level >= ?', level)

    # This reduces searched prefixes to only shortest ones
    # to avoid querying descendants since they are already covered
    # by ancestor namespaces. If the FF is not available fallback to
    # inefficient search: https://gitlab.com/gitlab-org/gitlab/-/issues/336436
    unless Feature.enabled?(:use_traversal_ids)
      return Ci::NamespaceMirror.contains_any_of_namespaces(search_members.pluck(:source_id))
    end

    traversal_ids = Group.joins(:all_group_members)
      .merge(search_members)
      .shortest_traversal_ids_prefixes

    Ci::NamespaceMirror.contains_traversal_ids(traversal_ids)
  end

The variable: group_members in this method only return GroupA, so we need to add a method to check if GroupA is owner in other groups. So we need to introduce group_group_links table to search. If GroupA is also other group's ownner, we need to record other group's id. So when runner policy checks the authority, UserB has GroupA and GroupB id, and GroupB has the runner, it will authorize UserB to see and modify runner in GroupB.

Screenshots or screen recordings

The link of GroupA user members: https://gitlab.com/groups/groupa8414205/-/group_members:

img1

The link of GroupB group members: https://gitlab.com/groups/groupb8614205/-/group_members?tab=groups:

img2

The link of GroupB runner in account ws665544: https://gitlab.com/groups/groupb8614205/-/runners:

img3

The link of GroupB runner in account wd665544: https://gitlab.com/groups/groupb8614205/-/runners:

img4

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

@chaomao

Edited by xfyuan

Merge request reports

Loading