Commit 9e2ec37f authored by Imre Farkas's avatar Imre Farkas Committed by GitLab Release Tools Bot

Respect member access level for group shares

Previously, we only considered the access level set for the
GroupGroupLink when calculated ProjectAuthorization or
Group#max_member_access_for_user for the shared group. We need to
consider access level in the shared with group as well, which might be
lower than the one set for GroupGroupLink.
parent c118dd50
...@@ -509,18 +509,29 @@ class Group < Namespace ...@@ -509,18 +509,29 @@ class Group < Namespace
group_group_links_query = GroupGroupLink.where(shared_group_id: self_and_ancestors_ids) group_group_links_query = GroupGroupLink.where(shared_group_id: self_and_ancestors_ids)
cte = Gitlab::SQL::CTE.new(:group_group_links_cte, group_group_links_query) cte = Gitlab::SQL::CTE.new(:group_group_links_cte, group_group_links_query)
cte_alias = cte.table.alias(GroupGroupLink.table_name)
link = GroupGroupLink link = GroupGroupLink
.with(cte.to_arel) .with(cte.to_arel)
.select(smallest_value_arel([cte_alias[:group_access], group_member_table[:access_level]],
'group_access'))
.from([group_member_table, cte.alias_to(group_group_link_table)]) .from([group_member_table, cte.alias_to(group_group_link_table)])
.where(group_member_table[:user_id].eq(user.id)) .where(group_member_table[:user_id].eq(user.id))
.where(group_member_table[:requested_at].eq(nil))
.where(group_member_table[:source_id].eq(group_group_link_table[:shared_with_group_id])) .where(group_member_table[:source_id].eq(group_group_link_table[:shared_with_group_id]))
.where(group_member_table[:source_type].eq('Namespace'))
.reorder(Arel::Nodes::Descending.new(group_group_link_table[:group_access])) .reorder(Arel::Nodes::Descending.new(group_group_link_table[:group_access]))
.first .first
link&.group_access link&.group_access
end end
def smallest_value_arel(args, column_alias)
Arel::Nodes::As.new(
Arel::Nodes::NamedFunction.new('LEAST', args),
Arel::Nodes::SqlLiteral.new(column_alias))
end
def self.groups_including_descendants_by(group_ids) def self.groups_including_descendants_by(group_ids)
Gitlab::ObjectHierarchy Gitlab::ObjectHierarchy
.new(Group.where(id: group_ids)) .new(Group.where(id: group_ids))
......
---
title: Respect member access level for group shares
merge_request:
author:
type: security
...@@ -62,6 +62,7 @@ module Gitlab ...@@ -62,6 +62,7 @@ module Gitlab
cte = Gitlab::SQL::RecursiveCTE.new(:namespaces_cte) cte = Gitlab::SQL::RecursiveCTE.new(:namespaces_cte)
members = Member.arel_table members = Member.arel_table
namespaces = Namespace.arel_table namespaces = Namespace.arel_table
group_group_links = GroupGroupLink.arel_table
# Namespaces the user is a member of. # Namespaces the user is a member of.
cte << user.groups cte << user.groups
...@@ -69,7 +70,10 @@ module Gitlab ...@@ -69,7 +70,10 @@ module Gitlab
.except(:order) .except(:order)
# Namespaces shared with any of the group # Namespaces shared with any of the group
cte << Group.select([namespaces[:id], 'group_group_links.group_access AS access_level']) cte << Group.select([namespaces[:id],
least(members[:access_level],
group_group_links[:group_access],
'access_level')])
.joins(join_group_group_links) .joins(join_group_group_links)
.joins(join_members_on_group_group_links) .joins(join_members_on_group_group_links)
......
...@@ -56,6 +56,10 @@ describe Groups::GroupLinksController do ...@@ -56,6 +56,10 @@ describe Groups::GroupLinksController do
context 'when owner access is requested' do context 'when owner access is requested' do
let(:shared_group_access) { Gitlab::Access::OWNER } let(:shared_group_access) { Gitlab::Access::OWNER }
before do
shared_with_group.add_owner(group_member)
end
include_examples 'creates group group link' include_examples 'creates group group link'
it 'allows admin access for group member' do it 'allows admin access for group member' do
......
...@@ -109,6 +109,20 @@ describe Gitlab::ProjectAuthorizations do ...@@ -109,6 +109,20 @@ describe Gitlab::ProjectAuthorizations do
end end
end end
context 'with lower group access level than max access level for share' do
let(:user) { create(:user) }
it 'creates proper authorizations' do
group.add_reporter(user)
mapping = map_access_levels(authorizations)
expect(mapping[project_parent.id]).to be_nil
expect(mapping[project.id]).to eq(Gitlab::Access::REPORTER)
expect(mapping[project_child.id]).to eq(Gitlab::Access::REPORTER)
end
end
context 'parent group user' do context 'parent group user' do
let(:user) { parent_group_user } let(:user) { parent_group_user }
......
...@@ -563,6 +563,18 @@ describe Group do ...@@ -563,6 +563,18 @@ describe Group do
expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER) expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER)
expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER) expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER)
end end
context 'with lower group access level than max access level for share' do
let(:user) { create(:user) }
it 'returns correct access level' do
group.add_reporter(user)
expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER)
expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER)
end
end
end end
context 'with user in the parent group' do context 'with user in the parent group' do
...@@ -584,6 +596,33 @@ describe Group do ...@@ -584,6 +596,33 @@ describe Group do
expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
end end
end end
context 'unrelated project owner' do
let(:common_id) { [Project.maximum(:id).to_i, Namespace.maximum(:id).to_i].max + 999 }
let!(:group) { create(:group, id: common_id) }
let!(:unrelated_project) { create(:project, id: common_id) }
let(:user) { unrelated_project.owner }
it 'returns correct access level' do
expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
end
end
context 'user without accepted access request' do
let!(:user) { create(:user) }
before do
create(:group_member, :developer, :access_request, user: user, group: group)
end
it 'returns correct access level' do
expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
end
end
end end
context 'when feature flag share_group_with_group is disabled' do context 'when feature flag share_group_with_group is disabled' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment