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:
The link of GroupB group members: https://gitlab.com/groups/groupb8614205/-/group_members?tab=groups:
The link of GroupB runner in account ws665544: https://gitlab.com/groups/groupb8614205/-/runners:
The link of GroupB runner in account wd665544: https://gitlab.com/groups/groupb8614205/-/runners:
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.
-
I have evaluated the MR acceptance checklist for this MR.