Merge branch 'guests_cant_see_mrs' into 'master'

Make guests unable to view MRs

## What does this MR do?

Make guests unable to view MRs. This also fixes a bug when a non-member user could get notification if mentioned (in private project, it's OK for public project).

Now if you are a guest and you will be mentioned in one of the MRs you won't get a notification. 

## What are the relevant issue numbers?

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/1410

See merge request !6673

CHANGELOG

@@ -97,6 +97,7 @@ v 8.13.0 (unreleased)
   - Cleanup Ci::ApplicationController. !6757 (Takuya Noguchi)
   - Fix a typo in doc/api/labels.md
   - API: all unknown routing will be handled with 404 Not Found
+  - Make guests unable to view MRs on private projects
 
 v 8.12.5 (unreleased)
   - Update the mail_room gem to 0.8.1 to fix a race condition with the mailbox watching thread

app/models/event.rb

@@ -68,8 +68,10 @@ class Event < ActiveRecord::Base
       true
     elsif issue? || issue_note?
       Ability.allowed?(user, :read_issue, note? ? note_target : target)
+    elsif merge_request? || merge_request_note?
+      Ability.allowed?(user, :read_merge_request, note? ? note_target : target)
     else
-      ((merge_request? || note?) && target.present?) || milestone?
+      milestone?
     end
   end
 
@@ -280,6 +282,10 @@ class Event < ActiveRecord::Base
     note? && target && target.for_issue?
   end
 
+  def merge_request_note?
+    note? && target && target.for_merge_request?
+  end
+
   def project_snippet_note?
     target.for_snippet?
   end

app/policies/project_policy.rb

@@ -40,7 +40,6 @@ class ProjectPolicy < BasePolicy
     can! :read_milestone
     can! :read_project_snippet
     can! :read_project_member
-    can! :read_merge_request
     can! :read_note
     can! :create_project
     can! :create_issue
@@ -63,6 +62,7 @@ class ProjectPolicy < BasePolicy
     can! :read_pipeline
     can! :read_environment
     can! :read_deployment
+    can! :read_merge_request
   end
 
   # Permissions given when an user is team member of a project
@@ -117,6 +117,7 @@ class ProjectPolicy < BasePolicy
     can! :read_container_image
     can! :build_download_code
     can! :build_read_container_image
+    can! :read_merge_request
   end
 
   def owner_access!

app/services/notification_service.rb

@@ -475,10 +475,12 @@ class NotificationService
   end
 
   def reject_users_without_access(recipients, target)
-    return recipients unless target.is_a?(Issue)
+    return recipients unless target.is_a?(Issuable)
+
+    ability = :"read_#{target.to_ability_name}"
 
     recipients.select do |user|
-      user.can?(:read_issue, target)
+      user.can?(ability, target)
     end
   end
 

app/services/todo_service.rb

@@ -273,12 +273,12 @@ class TodoService
   end
 
   def reject_users_without_access(users, project, target)
-    if target.is_a?(Note) && target.for_issue?
+    if target.is_a?(Note) && (target.for_issue? || target.for_merge_request?)
       target = target.noteable
     end
 
-    if target.is_a?(Issue)
-      select_users(users, :read_issue, target)
+    if target.is_a?(Issuable)
+      select_users(users, :"read_#{target.to_ability_name}", target)
     else
       select_users(users, :read_project, project)
     end

doc/user/permissions.md

@@ -32,6 +32,7 @@ The following table depicts the various user permission levels in a project.
 | See a commit status                   |         | ✓          | ✓           | ✓        | ✓      |
 | See a container registry              |         | ✓          | ✓           | ✓        | ✓      |
 | See environments                      |         | ✓          | ✓           | ✓        | ✓      |
+| See a list of merge requests          |         | ✓          | ✓           | ✓        | ✓      |
 | Manage/Accept merge requests          |         |            | ✓           | ✓        | ✓      |
 | Create new merge request              |         |            | ✓           | ✓        | ✓      |
 | Create new branches                   |         |            | ✓           | ✓        | ✓      |

spec/features/projects/guest_navigation_menu_spec.rb

+require 'spec_helper'
+
+describe "Guest navigation menu" do
+  let(:project) { create :empty_project, :private }
+  let(:guest) { create :user }
+
+  before do
+    project.team << [guest, :guest]
+
+    login_as(guest)
+  end
+
+  it "shows allowed tabs only" do
+    visit namespace_project_path(project.namespace, project)
+
+    within(".nav-links") do
+      expect(page).to have_content 'Project'
+      expect(page).to have_content 'Activity'
+      expect(page).to have_content 'Issues'
+      expect(page).to have_content 'Wiki'
+
+      expect(page).not_to have_content 'Repository'
+      expect(page).not_to have_content 'Pipelines'
+      expect(page).not_to have_content 'Graphs'
+      expect(page).not_to have_content 'Merge Requests'
+    end
+  end
+end

spec/features/security/project/private_access_spec.rb

@@ -203,7 +203,7 @@ describe "Private Project Access", feature: true  do
     it { is_expected.to be_allowed_for master }
     it { is_expected.to be_allowed_for developer }
     it { is_expected.to be_allowed_for reporter }
-    it { is_expected.to be_allowed_for guest }
+    it { is_expected.to be_denied_for  guest }
     it { is_expected.to be_denied_for :user }
     it { is_expected.to be_denied_for :external }
     it { is_expected.to be_denied_for :visitor }

spec/models/event_spec.rb

@@ -135,6 +135,17 @@ describe Event, models: true do
       it { expect(event.visible_to_user?(member)).to eq true }
       it { expect(event.visible_to_user?(guest)).to eq true }
       it { expect(event.visible_to_user?(admin)).to eq true }
+
+      context 'private project' do
+        let(:project) { create(:project, :private) }
+
+        it { expect(event.visible_to_user?(non_member)).to eq false }
+        it { expect(event.visible_to_user?(author)).to eq true }
+        it { expect(event.visible_to_user?(assignee)).to eq true }
+        it { expect(event.visible_to_user?(member)).to eq true }
+        it { expect(event.visible_to_user?(guest)).to eq false }
+        it { expect(event.visible_to_user?(admin)).to eq true }
+      end
     end
   end
 

spec/policies/project_policy_spec.rb

@@ -12,7 +12,7 @@ describe ProjectPolicy, models: true do
     [
       :read_project, :read_board, :read_list, :read_wiki, :read_issue, :read_label,
       :read_milestone, :read_project_snippet, :read_project_member,
-      :read_merge_request, :read_note, :create_project, :create_issue, :create_note,
+      :read_note, :create_project, :create_issue, :create_note,
       :upload_file
     ]
   end
@@ -21,7 +21,8 @@ describe ProjectPolicy, models: true do
     [
       :download_code, :fork_project, :create_project_snippet, :update_issue,
       :admin_issue, :admin_label, :admin_list, :read_commit_status, :read_build,
-      :read_container_image, :read_pipeline, :read_environment, :read_deployment
+      :read_container_image, :read_pipeline, :read_environment, :read_deployment,
+      :read_merge_request
     ]
   end
 

spec/services/merge_requests/update_service_spec.rb

@@ -17,6 +17,7 @@ describe MergeRequests::UpdateService, services: true do
   before do
     project.team << [user, :master]
     project.team << [user2, :developer]
+    project.team << [user3, :developer]
   end
 
   describe 'execute' do
@@ -188,6 +189,11 @@ describe MergeRequests::UpdateService, services: true do
       let!(:non_subscriber) { create(:user) }
       let!(:subscriber) { create(:user).tap { |u| label.toggle_subscription(u) } }
 
+      before do
+        project.team << [non_subscriber, :developer]
+        project.team << [subscriber, :developer]
+      end
+
       it 'sends notifications for subscribers of newly added labels' do
         opts = { label_ids: [label.id] }
 

spec/services/notification_service_spec.rb

@@ -331,7 +331,7 @@ describe NotificationService, services: true do
       describe '#new_note' do
         it "records sent notifications" do
           # Ensure create SentNotification by noteable = merge_request 6 times, not noteable = note
-          expect(SentNotification).to receive(:record_note).with(note, any_args).exactly(4).times.and_call_original
+          expect(SentNotification).to receive(:record_note).with(note, any_args).exactly(3).times.and_call_original
 
           notification.new_note(note)
 
@@ -1169,6 +1169,61 @@ describe NotificationService, services: true do
     end
   end
 
+  context 'guest user in private project' do
+    let(:private_project) { create(:empty_project, :private) }
+    let(:guest) { create(:user) }
+    let(:developer) { create(:user) }
+    let(:assignee) { create(:user) }
+    let(:merge_request) { create(:merge_request, source_project: private_project, assignee: assignee) }
+    let(:merge_request1) { create(:merge_request, source_project: private_project, assignee: assignee, description: "cc @#{guest.username}") }
+    let(:note) { create(:note, noteable: merge_request, project: private_project) }
+
+    before do
+      private_project.team << [assignee, :developer]
+      private_project.team << [developer, :developer]
+      private_project.team << [guest, :guest]
+
+      ActionMailer::Base.deliveries.clear
+    end
+
+    it 'filters out guests when new note is created' do
+      expect(SentNotification).to receive(:record).with(merge_request, any_args).exactly(1).times
+
+      notification.new_note(note)
+
+      should_not_email(guest)
+      should_email(assignee)
+    end
+
+    it 'filters out guests when new merge request is created' do
+      notification.new_merge_request(merge_request1, @u_disabled)
+
+      should_not_email(guest)
+      should_email(assignee)
+    end
+
+    it 'filters out guests when merge request is closed' do
+      notification.close_mr(merge_request, developer)
+
+      should_not_email(guest)
+      should_email(assignee)
+    end
+
+    it 'filters out guests when merge request is reopened' do
+      notification.reopen_mr(merge_request, developer)
+
+      should_not_email(guest)
+      should_email(assignee)
+    end
+
+    it 'filters out guests when merge request is merged' do
+      notification.merge_mr(merge_request, developer)
+
+      should_not_email(guest)
+      should_email(assignee)
+    end
+  end
+
   def build_team(project)
     @u_watcher               = create_global_setting_for(create(:user), :watch)
     @u_participating         = create_global_setting_for(create(:user), :participating)

spec/services/todo_service_spec.rb

@@ -345,7 +345,7 @@ describe TodoService, services: true do
         service.new_merge_request(mr_assigned, author)
 
         should_create_todo(user: member, target: mr_assigned, action: Todo::MENTIONED)
-        should_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
+        should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
         should_create_todo(user: author, target: mr_assigned, action: Todo::MENTIONED)
         should_not_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED)
         should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED)
@@ -357,7 +357,7 @@ describe TodoService, services: true do
         service.update_merge_request(mr_assigned, author)
 
         should_create_todo(user: member, target: mr_assigned, action: Todo::MENTIONED)
-        should_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
+        should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
         should_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED)
         should_create_todo(user: author, target: mr_assigned, action: Todo::MENTIONED)
         should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED)
@@ -381,6 +381,7 @@ describe TodoService, services: true do
           should_not_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED)
           should_not_create_todo(user: member, target: mr_assigned, action: Todo::MENTIONED)
           should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED)
+          should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
         end
 
         it 'does not raise an error when description not change' do
@@ -430,6 +431,11 @@ describe TodoService, services: true do
 
         should_create_todo(user: john_doe, target: mr_assigned, author: john_doe, action: Todo::ASSIGNED)
       end
+
+      it 'does not create a todo for guests' do
+        service.reassigned_merge_request(mr_assigned, author)
+        should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
+      end
     end
 
     describe '#merge_merge_request' do
@@ -441,6 +447,11 @@ describe TodoService, services: true do
         expect(first_todo.reload).to be_done
         expect(second_todo.reload).to be_done
       end
+
+      it 'does not create todo for guests' do
+        service.merge_merge_request(mr_assigned, john_doe)
+        should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
+      end
     end
 
     describe '#new_award_emoji' do
@@ -495,6 +506,13 @@ describe TodoService, services: true do
 
         should_create_todo(user: john_doe, target: mr_unassigned, author: author, action: Todo::MENTIONED, note: legacy_diff_note_on_merge_request)
       end
+
+      it 'does not create todo for guests' do
+        note_on_merge_request = create :note_on_merge_request, project: project, noteable: mr_assigned, note: mentions
+        service.new_note(note_on_merge_request, author)
+
+        should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
+      end
     end
   end