Merge branch 'security-id-restricted-access-to-private-repo-11-7' into '11-7-stable'

Forbid creating discussions for users with restricted access

See merge request gitlab/gitlabhq!2891

5 files changed, 137 insertions, 60 deletions

diff --git a/changelogs/unreleased/security-id-restricted-access-to-private-repo.yml b/changelogs/unreleased/security-id-restricted-access-to-private-repo.yml
new file mode 100644
index 00000000000..7d7478d297b
--- /dev/null
+++ b/changelogs/unreleased/security-id-restricted-access-to-private-repo.yml
@@ -0,0 +1,5 @@
+---
+title: Forbid creating discussions for users with restricted access
+merge_request:
+author:
+type: security
diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb
index 216b2c45741..795dca5cf03 100644
--- a/lib/api/helpers/notes_helpers.rb
+++ b/lib/api/helpers/notes_helpers.rb
@@ -70,14 +70,7 @@ module API
       def find_noteable(parent, noteables_str, noteable_id)
         noteable = public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend
 
-        readable =
-          if noteable.is_a?(Commit)
-            # for commits there is not :read_commit policy, check if user
-            # has :read_note permission on the commit's project
-            can?(current_user, :read_note, user_project)
-          else
-            can?(current_user, noteable_read_ability_name(noteable), noteable)
-          end
+        readable = can?(current_user, noteable_read_ability_name(noteable), noteable)
 
         return not_found!(noteables_str) unless readable
 
@@ -89,12 +82,11 @@ module API
       end
 
       def create_note(noteable, opts)
-        policy_object = noteable.is_a?(Commit) ? user_project : noteable
-        authorize!(:create_note, policy_object)
+        authorize!(:create_note, noteable)
 
         parent = noteable_parent(noteable)
 
-        opts.delete(:created_at) unless current_user.can?(:set_note_created_at, policy_object)
+        opts.delete(:created_at) unless current_user.can?(:set_note_created_at, noteable)
 
         opts[:updated_at] = opts[:created_at] if opts[:created_at]
 
diff --git a/spec/policies/commit_policy_spec.rb b/spec/policies/commit_policy_spec.rb
new file mode 100644
index 00000000000..2259693cf01
--- /dev/null
+++ b/spec/policies/commit_policy_spec.rb
@@ -0,0 +1,53 @@
+require 'spec_helper'
+
+describe CommitPolicy do
+  describe '#rules' do
+    let(:user) { create(:user) }
+    let(:commit) { project.repository.head_commit }
+    let(:policy) { described_class.new(user, commit) }
+
+    context 'when project is public' do
+      let(:project) { create(:project, :public, :repository) }
+
+      it 'can read commit and create a note' do
+        expect(policy).to be_allowed(:read_commit)
+      end
+
+      context 'when repository access level is private' do
+        let(:project) { create(:project, :public, :repository, :repository_private) }
+
+        it 'can not read commit and create a note' do
+          expect(policy).to be_disallowed(:read_commit)
+        end
+
+        context 'when the user is a project member' do
+          before do
+            project.add_developer(user)
+          end
+
+          it 'can read commit and create a note' do
+            expect(policy).to be_allowed(:read_commit)
+          end
+        end
+      end
+    end
+
+    context 'when project is private' do
+      let(:project) { create(:project, :private, :repository) }
+
+      it 'can not read commit and create a note' do
+        expect(policy).to be_disallowed(:read_commit)
+      end
+
+      context 'when the user is a project member' do
+        before do
+          project.add_developer(user)
+        end
+
+        it 'can read commit and create a note' do
+          expect(policy).to be_allowed(:read_commit)
+        end
+      end
+    end
+  end
+end
diff --git a/spec/policies/note_policy_spec.rb b/spec/policies/note_policy_spec.rb
index 0e848c74659..4be7a0266d1 100644
--- a/spec/policies/note_policy_spec.rb
+++ b/spec/policies/note_policy_spec.rb
@@ -1,28 +1,15 @@
 require 'spec_helper'
 
-describe NotePolicy, mdoels: true do
+describe NotePolicy do
   describe '#rules' do
     let(:user) { create(:user) }
     let(:project) { create(:project, :public) }
     let(:issue) { create(:issue, project: project) }
-
-    def policies(noteable = nil)
-      return @policies if @policies
-
-      noteable ||= issue
-      note = if noteable.is_a?(Commit)
-               create(:note_on_commit, commit_id: noteable.id, author: user, project: project)
-             else
-               create(:note, noteable: noteable, author: user, project: project)
-             end
-
-      @policies = described_class.new(user, note)
-    end
+    let(:noteable) { issue }
+    let(:policy) { described_class.new(user, note) }
+    let(:note) { create(:note, noteable: noteable, author: user, project: project) }
 
     shared_examples_for 'a discussion with a private noteable' do
-      let(:noteable) { issue }
-      let(:policy) { policies(noteable) }
-
       context 'when the note author can no longer see the noteable' do
         it 'can not edit nor read the note' do
           expect(policy).to be_disallowed(:admin_note)
@@ -46,12 +33,21 @@ describe NotePolicy, mdoels: true do
       end
     end
 
-    context 'when the project is private' do
-      let(:project) { create(:project, :private, :repository) }
+    context 'when the noteable is a commit' do
+      let(:commit) { project.repository.head_commit }
+      let(:note) { create(:note_on_commit, commit_id: commit.id, author: user, project: project) }
+
+      context 'when the project is private' do
+        let(:project) { create(:project, :private, :repository) }
+
+        it_behaves_like 'a discussion with a private noteable'
+      end
 
-      context 'when the noteable is a commit' do
-        it_behaves_like 'a discussion with a private noteable' do
-          let(:noteable) { project.repository.head_commit }
+      context 'when the project is public' do
+        context 'when repository access level is private' do
+          let(:project) { create(:project, :public, :repository, :repository_private) }
+
+          it_behaves_like 'a discussion with a private noteable'
         end
       end
     end
@@ -59,44 +55,44 @@ describe NotePolicy, mdoels: true do
     context 'when the project is public' do
       context 'when the note author is not a project member' do
         it 'can edit a note' do
-          expect(policies).to be_allowed(:admin_note)
-          expect(policies).to be_allowed(:resolve_note)
-          expect(policies).to be_allowed(:read_note)
+          expect(policy).to be_allowed(:admin_note)
+          expect(policy).to be_allowed(:resolve_note)
+          expect(policy).to be_allowed(:read_note)
         end
       end
 
       context 'when the noteable is a project snippet' do
-        it 'can edit note' do
-          policies = policies(create(:project_snippet, :public, project: project))
+        let(:noteable) { create(:project_snippet, :public, project: project) }
 
-          expect(policies).to be_allowed(:admin_note)
-          expect(policies).to be_allowed(:resolve_note)
-          expect(policies).to be_allowed(:read_note)
+        it 'can edit note' do
+          expect(policy).to be_allowed(:admin_note)
+          expect(policy).to be_allowed(:resolve_note)
+          expect(policy).to be_allowed(:read_note)
         end
 
         context 'when it is private' do
-          it_behaves_like 'a discussion with a private noteable' do
-            let(:noteable) { create(:project_snippet, :private, project: project) }
-          end
+          let(:noteable) { create(:project_snippet, :private, project: project) }
+
+          it_behaves_like 'a discussion with a private noteable'
         end
       end
 
       context 'when the noteable is a personal snippet' do
-        it 'can edit note' do
-          policies = policies(create(:personal_snippet, :public))
+        let(:noteable) { create(:personal_snippet, :public) }
 
-          expect(policies).to be_allowed(:admin_note)
-          expect(policies).to be_allowed(:resolve_note)
-          expect(policies).to be_allowed(:read_note)
+        it 'can edit note' do
+          expect(policy).to be_allowed(:admin_note)
+          expect(policy).to be_allowed(:resolve_note)
+          expect(policy).to be_allowed(:read_note)
         end
 
         context 'when it is private' do
-          it 'can not edit nor read the note' do
-            policies = policies(create(:personal_snippet, :private))
+          let(:noteable) { create(:personal_snippet, :private) }
 
-            expect(policies).to be_disallowed(:admin_note)
-            expect(policies).to be_disallowed(:resolve_note)
-            expect(policies).to be_disallowed(:read_note)
+          it 'can not edit nor read the note' do
+            expect(policy).to be_disallowed(:admin_note)
+            expect(policy).to be_disallowed(:resolve_note)
+            expect(policy).to be_disallowed(:read_note)
           end
         end
       end
@@ -120,20 +116,20 @@ describe NotePolicy, mdoels: true do
           end
 
           it 'can edit a note' do
-            expect(policies).to be_allowed(:admin_note)
-            expect(policies).to be_allowed(:resolve_note)
-            expect(policies).to be_allowed(:read_note)
+            expect(policy).to be_allowed(:admin_note)
+            expect(policy).to be_allowed(:resolve_note)
+            expect(policy).to be_allowed(:read_note)
           end
         end
 
         context 'when the note author is not a project member' do
           it 'can not edit a note' do
-            expect(policies).to be_disallowed(:admin_note)
-            expect(policies).to be_disallowed(:resolve_note)
+            expect(policy).to be_disallowed(:admin_note)
+            expect(policy).to be_disallowed(:resolve_note)
           end
 
           it 'can read a note' do
-            expect(policies).to be_allowed(:read_note)
+            expect(policy).to be_allowed(:read_note)
           end
         end
       end
diff --git a/spec/support/shared_examples/requests/api/discussions.rb b/spec/support/shared_examples/requests/api/discussions.rb
index e44da4faa5a..eff8e401bad 100644
--- a/spec/support/shared_examples/requests/api/discussions.rb
+++ b/spec/support/shared_examples/requests/api/discussions.rb
@@ -86,6 +86,37 @@ shared_examples 'discussions API' do |parent_type, noteable_type, id_name|
         expect(response).to have_gitlab_http_status(404)
       end
     end
+
+    context 'when a project is public with private repo access' do
+      let!(:parent) { create(:project, :public, :repository, :repository_private, :snippets_private) }
+      let!(:user_without_access) { create(:user) }
+
+      context 'when user is not a team member of private repo' do
+        before do
+          project.team.truncate
+        end
+
+        context "creating a new note" do
+          before do
+            post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user_without_access), params: { body: 'hi!' }
+          end
+
+          it 'raises 404 error' do
+            expect(response).to have_gitlab_http_status(404)
+          end
+        end
+
+        context "fetching a discussion" do
+          before do
+            get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions/#{note.discussion_id}", user_without_access)
+          end
+
+          it 'raises 404 error' do
+            expect(response).to have_gitlab_http_status(404)
+          end
+        end
+      end
+    end
   end
 
   describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id/notes" do