Merge branch 'security-guests-can-see-list-of-merge-requests' into 'master'

[master] Group Guests are no longer able to see merge requests

See merge request gitlab/gitlabhq!2694

3 files changed, 110 insertions, 9 deletions

diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb
index ff4c6b8dd42..107da08a0a9 100644
--- a/spec/finders/merge_requests_finder_spec.rb
+++ b/spec/finders/merge_requests_finder_spec.rb
@@ -68,20 +68,34 @@ describe MergeRequestsFinder do
       expect(merge_requests.size).to eq(2)
     end
 
-    it 'filters by group' do
-      params = { group_id: group.id }
+    context 'filtering by group' do
+      it 'includes all merge requests when user has access' do
+        params = { group_id: group.id }
 
-      merge_requests = described_class.new(user, params).execute
+        merge_requests = described_class.new(user, params).execute
 
-      expect(merge_requests.size).to eq(3)
-    end
+        expect(merge_requests.size).to eq(3)
+      end
 
-    it 'filters by group including subgroups', :nested_groups do
-      params = { group_id: group.id, include_subgroups: true }
+      it 'excludes merge requests from projects the user does not have access to' do
+        private_project = create_project_without_n_plus_1(:private, group: group)
+        private_mr = create(:merge_request, :simple, author: user, source_project: private_project, target_project: private_project)
+        params = { group_id: group.id }
 
-      merge_requests = described_class.new(user, params).execute
+        private_project.add_guest(user)
+        merge_requests = described_class.new(user, params).execute
 
-      expect(merge_requests.size).to eq(6)
+        expect(merge_requests.size).to eq(3)
+        expect(merge_requests).not_to include(private_mr)
+      end
+
+      it 'filters by group including subgroups', :nested_groups do
+        params = { group_id: group.id, include_subgroups: true }
+
+        merge_requests = described_class.new(user, params).execute
+
+        expect(merge_requests.size).to eq(6)
+      end
     end
 
     it 'filters by non_archived' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 7a8dc59039e..585dfe46189 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3074,6 +3074,66 @@ describe Project do
     end
   end
 
+  describe '.with_feature_available_for_user' do
+    let!(:user) { create(:user) }
+    let!(:feature) { MergeRequest }
+    let!(:project) { create(:project, :public, :merge_requests_enabled) }
+
+    subject { described_class.with_feature_available_for_user(feature, user) }
+
+    context 'when user has access to project' do
+      subject { described_class.with_feature_available_for_user(feature, user) }
+
+      before do
+        project.add_guest(user)
+      end
+
+      context 'when public project' do
+        context 'when feature is public' do
+          it 'returns project' do
+            is_expected.to include(project)
+          end
+        end
+
+        context 'when feature is private' do
+          let!(:project) { create(:project, :public, :merge_requests_private) }
+
+          it 'returns project when user has access to the feature' do
+            project.add_maintainer(user)
+
+            is_expected.to include(project)
+          end
+
+          it 'does not return project when user does not have the minimum access level required' do
+            is_expected.not_to include(project)
+          end
+        end
+      end
+
+      context 'when private project' do
+        let!(:project) { create(:project) }
+
+        it 'returns project when user has access to the feature' do
+          project.add_maintainer(user)
+
+          is_expected.to include(project)
+        end
+
+        it 'does not return project when user does not have the minimum access level required' do
+          is_expected.not_to include(project)
+        end
+      end
+    end
+
+    context 'when user does not have access to project' do
+      let!(:project) { create(:project) }
+
+      it 'does not return project when user cant access project' do
+        is_expected.not_to include(project)
+      end
+    end
+  end
+
   describe '#pages_available?' do
     let(:project) { create(:project, group: group) }
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 33842e74b92..78477ab0a5a 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1997,6 +1997,33 @@ describe User do
       expect(subject).to include(accessible)
       expect(subject).not_to include(other)
     end
+
+    context 'with min_access_level' do
+      let!(:user) { create(:user) }
+      let!(:project) { create(:project, :private, namespace: user.namespace) }
+
+      before do
+        project.add_developer(user)
+      end
+
+      subject { Project.where("EXISTS (?)", user.authorizations_for_projects(min_access_level: min_access_level)) }
+
+      context 'when developer access' do
+        let(:min_access_level) { Gitlab::Access::DEVELOPER }
+
+        it 'includes projects a user has access to' do
+          expect(subject).to include(project)
+        end
+      end
+
+      context 'when owner access' do
+        let(:min_access_level) { Gitlab::Access::OWNER }
+
+        it 'does not include projects with higher access level' do
+          expect(subject).not_to include(project)
+        end
+      end
+    end
   end
 
   describe '#authorized_projects', :delete do