Merge branch 'security-sharing_group_to_respect_member_access_level-master' into 'master'

Respect member access level for group shares

Closes #56

See merge request gitlab-org/security/gitlab!192

app/models/group.rb

@@ -512,18 +512,29 @@ class Group < Namespace
 
     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_alias = cte.table.alias(GroupGroupLink.table_name)
 
     link = GroupGroupLink
              .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)])
              .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_type].eq('Namespace'))
              .reorder(Arel::Nodes::Descending.new(group_group_link_table[:group_access]))
              .first
 
     link&.group_access
   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)
     Gitlab::ObjectHierarchy
       .new(Group.where(id: group_ids))

changelogs/unreleased/199415-sharing_group_to_respect_member_access_level.yml

+---
+title: Respect member access level for group shares
+merge_request:
+author:
+type: security

lib/gitlab/project_authorizations.rb

@@ -62,6 +62,7 @@ module Gitlab
       cte = Gitlab::SQL::RecursiveCTE.new(:namespaces_cte)
       members = Member.arel_table
       namespaces = Namespace.arel_table
+      group_group_links = GroupGroupLink.arel_table
 
       # Namespaces the user is a member of.
       cte << user.groups
@@ -69,7 +70,10 @@ module Gitlab
         .except(:order)
 
       # 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_members_on_group_group_links)
 

spec/controllers/groups/group_links_controller_spec.rb

@@ -56,6 +56,10 @@ describe Groups::GroupLinksController do
       context 'when owner access is requested' do
         let(:shared_group_access) { Gitlab::Access::OWNER }
 
+        before do
+          shared_with_group.add_owner(group_member)
+        end
+
         include_examples 'creates group group link'
 
         it 'allows admin access for group member' do

spec/lib/gitlab/project_authorizations_spec.rb

@@ -109,6 +109,20 @@ describe Gitlab::ProjectAuthorizations do
       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
       let(:user) { parent_group_user }
 

spec/models/group_spec.rb

@@ -563,6 +563,18 @@ describe Group do
             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)
           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
 
         context 'with user in the parent 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)
           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
 
       context 'when feature flag share_group_with_group is disabled' do