Merge remote-tracking branch 'upstream/master' into ce-to-ee-2017-07-13

app/assets/javascripts/issuable/related_issues/components/add_issuable_form.vue

@@ -85,12 +85,12 @@ export default {
   mounted() {
     const $input = $(this.$refs.input);
 
-    new GfmAutoComplete(this.autoCompleteSources).setup($input, {
+    this.gfmAutoComplete = new GfmAutoComplete(this.autoCompleteSources);
+    this.gfmAutoComplete.setup($input, {
       issues: true,
     });
     $input.on('shown-issues.atwho', this.onAutoCompleteToggled.bind(this, true));
     $input.on('hidden-issues.atwho', this.onAutoCompleteToggled.bind(this, false));
-    $input.on('inserted-issues.atwho', this.onInput);
 
     this.$refs.input.focus();
   },

app/controllers/projects/approver_groups_controller.rb

 class Projects::ApproverGroupsController < Projects::ApplicationController
+  before_action :authorize_for_subject!
+
   def destroy
-    if params[:merge_request_id]
-      authorize_create_merge_request!
-      merge_request = project.merge_requests.find_by!(iid: params[:merge_request_id])
-      merge_request.approver_groups.find(params[:id]).destroy
-    else
-      authorize_admin_project!
-      project.approver_groups.find(params[:id]).destroy
-    end
+    subject.approver_groups.find(params[:id]).destroy
 
     redirect_back_or_default(default: { action: 'index' })
   end
+
+  private
+
+  def authorize_for_subject!
+    access_denied! unless can?(current_user, :update_approvers, subject)
+  end
+
+  def subject
+    @subject ||=
+      if params[:merge_request_id]
+        project.merge_requests.find_by!(iid: params[:merge_request_id])
+      else
+        project
+      end
+  end
 end

app/controllers/projects/approvers_controller.rb

 class Projects::ApproversController < Projects::ApplicationController
+  before_action :authorize_for_subject!
+
   def destroy
-    if params[:merge_request_id]
-      authorize_create_merge_request!
-      merge_request = project.merge_requests.find_by!(iid: params[:merge_request_id])
-      merge_request.approvers.find(params[:id]).destroy
-    else
-      authorize_admin_project!
-      project.approvers.find(params[:id]).destroy
-    end
+    subject.approvers.find(params[:id]).destroy
 
     redirect_back_or_default(default: { action: 'index' })
   end
+
+  private
+
+  def authorize_for_subject!
+    access_denied! unless can?(current_user, :update_approvers, subject)
+  end
+
+  def subject
+    @subject ||=
+      if params[:merge_request_id]
+        project.merge_requests.find_by!(iid: params[:merge_request_id])
+      else
+        project
+      end
+  end
 end

app/policies/ee/merge_request_policy.rb

+module EE
+  module MergeRequestPolicy
+    extend ActiveSupport::Concern
+
+    prepended do
+      with_scope :subject
+      condition(:can_override_approvers, score: 0) do
+        @subject.target_project&.can_override_approvers?
+      end
+
+      rule { ~can_override_approvers }.prevent :update_approvers
+      rule { can?(:update_merge_request) }.enable :update_approvers
+    end
+  end
+end

app/policies/ee/project_policy.rb

@@ -49,6 +49,7 @@ module EE
       rule { can?(:master_access) }.policy do
         enable :push_code_to_protected_branches
         enable :admin_path_locks
+        enable :update_approvers
       end
 
       rule { auditor }.policy do

app/policies/merge_request_policy.rb

 class MergeRequestPolicy < IssuablePolicy
+  prepend EE::MergeRequestPolicy
+
   # pass
 end

app/services/ee/merge_requests/base_service.rb

+module EE
+  module MergeRequests
+    module BaseService
+      private
+
+      def filter_params(merge_request)
+        unless current_user.can?(:update_approvers, merge_request)
+          params.delete(:approvals_before_merge)
+          params.delete(:approver_ids)
+          params.delete(:approver_group_ids)
+        end
+
+        super
+      end
+    end
+  end
+end

app/services/merge_requests/base_service.rb

 module MergeRequests
   class BaseService < ::IssuableBaseService
+    prepend EE::MergeRequests::BaseService
+
     def create_note(merge_request)
       SystemNoteService.change_status(merge_request, merge_request.target_project, current_user, merge_request.state, nil)
     end

app/services/merge_requests/create_service.rb

@@ -7,15 +7,8 @@ module MergeRequests
       source_project = @project
       @project = Project.find(params[:target_project_id]) if params[:target_project_id]
 
-      params[:target_project_id] ||= source_project.id
-
-      unless @project.can_override_approvers?
-        params.delete(:approvals_before_merge)
-        params.delete(:approver_ids)
-        params.delete(:approver_group_ids)
-      end
-
       merge_request = MergeRequest.new
+      merge_request.target_project = @project
       merge_request.source_project = source_project
       merge_request.source_branch = params[:source_branch]
       merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)

app/services/merge_requests/update_service.rb

@@ -17,12 +17,6 @@ module MergeRequests
         merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)
       end
 
-      unless project.can_override_approvers?
-        params.delete(:approvals_before_merge)
-        params.delete(:approver_ids)
-        params.delete(:approver_group_ids)
-      end
-
       old_approvers = merge_request.overall_approvers.to_a
 
       handle_wip_event(merge_request)

app/views/layouts/nav/_new_admin_sidebar.html.haml

@@ -39,7 +39,7 @@
             %span
               Cohorts
 
-    = nav_link(controller: %w(conversational_development_index system_info background_jobs logs health_check requests_profiles)) do
+    = nav_link(controller: %w(conversational_development_index system_info background_jobs logs health_check requests_profiles audit_logs)) do
       = link_to admin_conversational_development_index_path, title: 'Monitoring' do
         %span
           Monitoring
@@ -69,6 +69,10 @@
           = link_to admin_requests_profiles_path, title: 'Requests Profiles' do
             %span
               Requests Profiles
+        = nav_link path: 'audit_logs#index' do
+          = link_to admin_audit_logs_path, title: 'Audit Log' do
+            %span
+              Audit Log
 
     = nav_link(controller: :broadcast_messages) do
       = link_to admin_broadcast_messages_path, title: 'Messages' do

app/views/shared/issuable/_approvals.html.haml

@@ -4,13 +4,13 @@
 - return unless issuable.is_a?(MergeRequest)
 - return unless issuable.requires_approve?
 
-- can_override_approvers = issuable.target_project.can_override_approvers?
+- can_update_approvers = can?(current_user, :update_approvers, issuable)
 
 .form-group
   = form.label :approver_ids, class: 'control-label' do
     Approvers
   .col-sm-10
-    - if can_override_approvers
+    - if can_update_approvers
       = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', email_user: true, skip_users: issuable.all_approvers_including_groups, project: issuable.target_project)
       .help-block
         This merge request must be approved by these users.
@@ -34,7 +34,7 @@
           - issuable.overall_approvers.each do |approver|
             %li{ id: dom_id(approver.user), class: item_classes + ['approver'] }
               = link_to approver.user.name, approver.user
-              - if can_override_approvers
+              - if can_update_approvers
                 .pull-right
                   - if unsaved_approvers
                     = link_to "#", data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn-xs btn btn-remove", title: 'Remove approver' do
@@ -48,7 +48,7 @@
             %li{ id: dom_id(approver_group.group), class: item_classes + ['approver-group'] }
               Group:
               = link_to approver_group.group.name, approver_group.group
-              - if can_override_approvers
+              - if can_update_approvers
                 .pull-right
                   - if unsaved_approvers
                     = link_to "#", data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}"}, class: "btn-xs btn btn-remove", title: 'Remove group' do
@@ -63,9 +63,9 @@
       .form-group
         = form.label :approvals_before_merge, class: 'label-light' do
           Approvals required
-        = form.number_field :approvals_before_merge, class: 'form-control', value: issuable.approvals_required, readonly: !can_override_approvers
+        = form.number_field :approvals_before_merge, class: 'form-control', value: issuable.approvals_required, readonly: !can_update_approvers
 
-    - if can_override_approvers
+    - if can_update_approvers
       .help-block.suggested-approvers
         - if @suggested_approvers.any?
           Suggested approvers:

doc/integration/elasticsearch.md

@@ -44,7 +44,7 @@ The following Elasticsearch settings are available:
 
 | Parameter                           | Description |
 | ---------                           | ----------- |
-| `Elasticsearch indexing`            | Enables/disables Elasticsearch indexing. You may want to enable indexing but disable search in order to give the index time to be fully completed, for example. Also keep in mind that this option doesn't have any impact on existing data, this only enables/disables background indexer which tracks data changes. So by enabling this you will not get your existing data indexed, use special rake task for that as explained in [Add GitLab's data to the Elasticsearch index](#add-gitlabs-data-to-the-elasticsearch-index). |
+| `Elasticsearch indexing`            | Enables/disables Elasticsearch indexing. You may want to enable indexing but disable search in order to give the index time to be fully completed, for example. Also keep in mind that this option doesn't have any impact on existing data, this only enables/disables background indexer which tracks data changes. So by enabling this you will not get your existing data indexed, use special rake task for that as explained in [Adding GitLab's data to the Elasticsearch index](#adding-gitlabs-data-to-the-elasticsearch-index). |
 | `Use experimental repository indexer` | Perform repository indexing using [GitLab Elasticsearch Indexer](https://gitlab.com/gitlab-org/gitlab-elasticsearch-indexer). |
 | `Search with Elasticsearch enabled` | Enables/disables using Elasticsearch in search. |
 | `URL`                              | The URL to use for connecting to Elasticsearch. Use a comma-separated list to support clustering (e.g., "http://host1, https://host2:9200"). |
@@ -62,7 +62,33 @@ To disable the Elasticsearch integration:
    and 'Elasticsearch indexing'
 1. Click **Save** for the changes to take effect
 
-## Adding GitLab data to the Elasticsearch index
+## Adding GitLab's data to the Elasticsearch index
+
+### Indexing small instances (database size less than 500 MiB, size of repos less than 5 GiB)
+
+Configure Elasticsearch's host and port in **Admin > Settings**. Then create empty indexes using one of the following commands:
+
+```
+# Omnibus installations
+sudo gitlab-rake gitlab:elastic:create_empty_index
+
+# Installations from source
+bundle exec rake gitlab:elastic:create_empty_index RAILS_ENV=production
+```
+
+Then enable Elasticsearch indexing and run repository indexing tasks:
+
+```
+# Omnibus installations
+sudo gitlab-rake gitlab:elastic:index
+
+# Installations from source
+bundle exec rake gitlab:elastic:index
+```
+
+Enable Elasticsearch search.
+
+### Indexing large instances
 
 Configure Elasticsearch's host and port in **Admin > Settings**. Then create empty indexes using one of the following commands:
 
@@ -74,6 +100,18 @@ sudo gitlab-rake gitlab:elastic:create_empty_index
 bundle exec rake gitlab:elastic:create_empty_index RAILS_ENV=production
 ```
 
+Indexing large Git repositories can take a while. To speed up the process, you
+can temporarily disable auto-refreshing and replicating. In our experience you can expect a 20%
+time drop. We'll enable them when indexing is done. This step is optional!
+
+```bash
+curl --request PUT localhost:9200/gitlab-production/_settings --data '{
+    "index" : {
+        "refresh_interval" : "-1",
+        "number_of_replicas" : 0
+    } }'
+```
+
 Then enable Elasticsearch indexing and run repository indexing tasks:
 
 ```
@@ -88,6 +126,7 @@ This enqueues a number of Sidekiq jobs to index your existing repositories.
 You can view the jobs in the admin panel (they are placed in the `elastic_batch_project_indexer`)
 queue), or you can query indexing status using a rake task:
 
+
 ```
 # Omnibus installations
 sudo gitlab-rake gitlab:elastic:index_repositories_status
@@ -114,8 +153,8 @@ sudo gitlab-rake gitlab:elastic:index_repositories
 # Installations from source
 bundle exec rake gitlab:elastic:index_repositories RAILS_ENV=production
 ```
- It might take a while depending on how big your Git repositories are (see
-[Indexing large repositories](#indexing-large-repositories)).
+
+It might take a while depending on how big your Git repositories are.
 
 If you want to run several tasks in parallel (probably in separate terminal
 windows) you can provide the `ID_FROM` and `ID_TO` parameters:
@@ -164,7 +203,7 @@ bundle exec rake gitlab:elastic:index_wikis RAILS_ENV=production
 The wiki indexer also supports the `ID_FROM` and `ID_TO` parameters if you want
 to limit a project set.
 
-To index all database entities:
+Index all database entities (Keep in mind it can take a while so consider using `screen` or `tmux`):
 
 ```
 # Omnibus installations
@@ -174,96 +213,23 @@ sudo gitlab-rake gitlab:elastic:index_database
 bundle exec rake gitlab:elastic:index_database RAILS_ENV=production
 ```
 
-If your instance is small enough you can index everything at once (database records, repositories, wikis):
+Enable replication and refreshing again after indexing (only if you previously disabled it):
 
+```bash
+curl --request PUT localhost:9200/gitlab-production/_settings --data '{
+    "index" : {
+        "number_of_replicas" : 1,
+        "refresh_interval" : "1s"
+    } }'
 ```
-# Omnibus installations
-sudo gitlab-rake gitlab:elastic:index
 
-# Installations from source
-bundle exec rake gitlab:elastic:index RAILS_ENV=production
-```
+A force merge should be called after enabling the refreshing above:
 
-## Special recommendations
-
-Here are some tips to use Elasticsearch with GitLab more efficiently.
-
-### Indexing large repositories
-
-Indexing large Git repositories can take a while. To speed up the process, you
-can temporarily disable auto-refreshing and replicating. In our experience you can expect a 20%
-time drop.
-
-1.  Disable refreshing:
-
-    ```bash
-    curl --request PUT localhost:9200/_settings --data '{
-        "index" : {
-            "refresh_interval" : "-1"
-        } }'
-    ```
-
-1.  Disable replication and enable it after indexing:
-
-    ```bash
-    curl --request PUT localhost:9200/_settings --data '{
-        "index" : {
-            "number_of_replicas" : 0
-        } }'
-    ```
-
-1. [Create the indexes](#add-gitlabs-data-to-the-elasticsearch-index)
-
-1.  Enable replication again after
-    the indexing is done and set it to its default value, which is 1:
-
-    ```bash
-    curl --request PUT localhost:9200/_settings --data '{
-        "index" : {
-            "number_of_replicas" : 1
-        } }'
-    ```
-
-1.  Enable refreshing again (after indexing):
-
-    ```bash
-    curl --request PUT localhost:9200/_settings --data '{
-        "index" : {
-            "refresh_interval" : "1s"
-        } }'
-    ```
-
-1.  A force merge should be called after enabling the refreshing above:
-
-    ```bash
-    curl --request POST 'http://localhost:9200/_forcemerge?max_num_segments=5'
-    ```
-
-To minimize downtime of the search feature we recommend the following:
-
-1. Configure Elasticsearch in **Admin > Settings**, but do not enable it, just set a host and port.
-
-1. Create empty indexes:
-
-    ```
-    # Omnibus installations
-    sudo gitlab-rake gitlab:elastic:create_empty_index
-
-    # Installations from source
-    bundle exec rake gitlab:elastic:create_empty_index RAILS_ENV=production
-    ```
-
-1. Index all repositories using the `gitlab:elastic:index_repositories` Rake
-   task (see above). You'll probably want to do this in parallel.
-
-1. Enable Elasticsearch indexing.
+```bash
+curl --request POST 'http://localhost:9200/_forcemerge?max_num_segments=5'
+```
 
-1. Run indexers for database, wikis, and
-   repositories (with the `UPDATE_INDEX=1` parameter). By running the repository indexer twice you will be sure that
-   everything is indexed because some commits could be pushed while you
-   performed the initial indexing. The repository indexer will skip
-   repositories and commits that are already indexed, so it will be much
-   shorter than the first run.
+Enable Elasticsearch search in **Admin > Settings**. That's it. Enjoy it!
 
 ## Troubleshooting
 

spec/controllers/projects/approver_groups_controller_spec.rb

@@ -2,65 +2,105 @@ require 'rails_helper'
 
 describe Projects::ApproverGroupsController do
   describe '#destroy' do
+    let(:user) { create(:user) }
+    let(:project) { create(:empty_project) }
+    let(:merge_request) { create(:merge_request, source_project: project) }
+
     before do
       # Allow redirect_back_or_default to work
       request.env['HTTP_REFERER'] = '/'
+
+      project.add_guest(user)
+      sign_in(user)
     end
 
     context 'on a merge request' do
-      it 'authorizes create_merge_request' do
-        merge    = create(:merge_request)
-        project  = stub_project(merge.target_project)
-        approver = create(:approver, target: merge)
+      let!(:approver_group) { create(:approver_group, target: merge_request) }
+
+      def destroy_merge_request_approver_group
+        delete :destroy,
+               namespace_id: project.namespace.to_param,
+               project_id: project.to_param,
+               merge_request_id: merge_request.to_param,
+               id: approver_group.id
+      end
+
+      context 'when the user cannot update approvers because they do not have access' do
+        it 'returns a 404' do
+          destroy_merge_request_approver_group
 
-        expect(controller).to receive(:authorize_create_merge_request!)
+          expect(response).to have_http_status(404)
+        end
 
-        go_delete(project, merge_request_id: merge.to_param, id: approver.id)
+        it 'does not destroy any approver groups' do
+          expect { destroy_merge_request_approver_group }
+            .not_to change { merge_request.reload.approver_groups.count }
+        end
       end
 
-      it 'destroys the provided approver group' do
-        merge          = create(:merge_request)
-        project        = stub_project(merge.target_project)
-        approver_group = create(:approver_group, target: merge)
+      context 'when the user cannot update approvers because of the project setting' do
+        before do
+          project.add_developer(user)
+          project.update!(disable_overriding_approvers_per_merge_request: true)
+        end
+
+        it 'returns a 404' do
+          destroy_merge_request_approver_group
 
-        allow(controller).to receive(:authorize_create_merge_request!)
+          expect(response).to have_http_status(404)
+        end
 
-        expect { go_delete(project, merge_request_id: merge.to_param, id: approver_group.id) }
-          .to change { merge.reload.approver_groups.count }.by(-1)
+        it 'does not destroy any approver groups' do
+          expect { destroy_merge_request_approver_group }
+            .not_to change { merge_request.reload.approver_groups.count }
+        end
+      end
+
+      context 'when the user can update approvers' do
+        before do
+          project.add_developer(user)
+        end
+
+        it 'destroys the provided approver group' do
+          expect { destroy_merge_request_approver_group }
+            .to change { merge_request.reload.approver_groups.count }.by(-1)
+        end
       end
     end
 
     context 'on a project' do
-      it 'authorizes admin_project' do
-        project        = stub_project
-        approver_group = create(:approver_group, target: project)
+      let!(:approver_group) { create(:approver_group, target: project) }
 
-        expect(controller).to receive(:authorize_admin_project!)
-
-        go_delete(project, id: approver_group.id)
+      def destroy_project_approver_group
+        delete :destroy,
+               namespace_id: project.namespace.to_param,
+               project_id: project.to_param,
+               id: approver_group.id
       end
 
-      it 'destroys the provided approver' do
-        project        = stub_project
-        approver_group = create(:approver_group, target: project)
+      context 'when the user cannot update approvers because they do not have access' do
+        it 'returns a 404' do
+          destroy_project_approver_group
 
-        allow(controller).to receive(:authorize_admin_project!).and_return(true)
+          expect(response).to have_http_status(404)
+        end
 
-        expect { go_delete(project, id: approver_group.id) }
-          .to change { project.approver_groups.count }.by(-1)
+        it 'does not destroy any approver groups' do
+          expect { destroy_project_approver_group }
+            .not_to change { merge_request.reload.approver_groups.count }
+        end
       end
-    end
 
-    def go_delete(project, params = {})
-      delete :destroy, {
-        namespace_id: project.namespace.to_param,
-        project_id: project.to_param
-      }.merge(params)
-    end
+      context 'when the user can update approvers' do
+        before do
+          project.add_master(user)
+          project.update!(disable_overriding_approvers_per_merge_request: true)
+        end
 
-    def stub_project(project = build_stubbed(:empty_project))
-      project.tap do |p|
-        allow(controller).to receive(:project).and_return(p)
+        it 'destroys the provided approver' do
+          expect { destroy_project_approver_group }
+            .to change { project.reload.approver_groups.count }.by(-1)
+        end
       end
     end
   end

spec/controllers/projects/approvers_controller_spec.rb

@@ -2,65 +2,105 @@ require 'rails_helper'
 
 describe Projects::ApproversController do
   describe '#destroy' do
+    let(:user) { create(:user) }
+    let(:project) { create(:empty_project) }
+    let(:merge_request) { create(:merge_request, source_project: project) }
+
     before do
       # Allow redirect_back_or_default to work
       request.env['HTTP_REFERER'] = '/'
+
+      project.add_guest(user)
+      sign_in(user)
     end
 
     context 'on a merge request' do
-      it 'authorizes create_merge_request' do
-        merge    = create(:merge_request)
-        project  = stub_project(merge.target_project)
-        approver = create(:approver, target: merge)
+      let!(:approver) { create(:approver, target: merge_request) }
+
+      def destroy_merge_request_approver
+        delete :destroy,
+               namespace_id: project.namespace.to_param,
+               project_id: project.to_param,
+               merge_request_id: merge_request.to_param,
+               id: approver.id
+      end
+
+      context 'when the user cannot update approvers because they do not have access' do
+        it 'returns a 404' do
+          destroy_merge_request_approver
 
-        expect(controller).to receive(:authorize_create_merge_request!)
+          expect(response).to have_http_status(404)
+        end
 
-        go_delete(project, merge_request_id: merge.to_param, id: approver.id)
+        it 'does not destroy any approvers' do
+          expect { destroy_merge_request_approver }
+            .not_to change { merge_request.reload.approvers.count }
+        end
       end
 
-      it 'destroys the provided approver' do
-        merge    = create(:merge_request)
-        project  = stub_project(merge.target_project)
-        approver = create(:approver, target: merge)
+      context 'when the user cannot update approvers because of the project setting' do
+        before do
+          project.add_developer(user)
+          project.update!(disable_overriding_approvers_per_merge_request: true)
+        end
+
+        it 'returns a 404' do
+          destroy_merge_request_approver
 
-        allow(controller).to receive(:authorize_create_merge_request!)
+          expect(response).to have_http_status(404)
+        end
 
-        expect { go_delete(project, merge_request_id: merge.to_param, id: approver.id) }
-          .to change { merge.reload.approvers.count }.by(-1)
+        it 'does not destroy any approvers' do
+          expect { destroy_merge_request_approver }
+            .not_to change { merge_request.reload.approvers.count }
+        end
+      end
+
+      context 'when the user can update approvers' do
+        before do
+          project.add_developer(user)
+        end
+
+        it 'destroys the provided approver' do
+          expect { destroy_merge_request_approver }
+            .to change { merge_request.reload.approvers.count }.by(-1)
+        end
       end
     end
 
     context 'on a project' do
-      it 'authorizes admin_project' do
-        project  = stub_project
-        approver = create(:approver, target: project)
+      let!(:approver) { create(:approver, target: project) }
 
-        expect(controller).to receive(:authorize_admin_project!)
-
-        go_delete(project, id: approver.id)
+      def destroy_project_approver
+        delete :destroy,
+               namespace_id: project.namespace.to_param,
+               project_id: project.to_param,
+               id: approver.id
       end
 
-      it 'destroys the provided approver' do
-        project  = stub_project
-        approver = create(:approver, target: project)
+      context 'when the user cannot update approvers because they do not have access' do
+        it 'returns a 404' do
+          destroy_project_approver
 
-        allow(controller).to receive(:authorize_admin_project!).and_return(true)
+          expect(response).to have_http_status(404)
+        end
 
-        expect { go_delete(project, id: approver.id) }
-          .to change { project.approvers.count }.by(-1)
+        it 'does not destroy any approvers' do
+          expect { destroy_project_approver }
+            .not_to change { merge_request.reload.approvers.count }
+        end
       end
-    end
 
-    def go_delete(project, params = {})
-      delete :destroy, {
-        namespace_id: project.namespace.to_param,
-        project_id: project.to_param
-      }.merge(params)
-    end
+      context 'when the user can update approvers' do
+        before do
+          project.add_master(user)
+          project.update!(disable_overriding_approvers_per_merge_request: true)
+        end
 
-    def stub_project(project = build_stubbed(:empty_project))
-      project.tap do |p|
-        allow(controller).to receive(:project).and_return(p)
+        it 'destroys the provided approver' do
+          expect { destroy_project_approver }
+            .to change { project.reload.approvers.count }.by(-1)
+        end
       end
     end
   end

spec/javascripts/issuable/related_issues/components/add_issuable_form_spec.js

@@ -198,6 +198,32 @@ describe('AddIssuableForm', () => {
       });
     });
 
+    it('when using the autocomplete', (done) => {
+      const $input = $(vm.$refs.input);
+
+      vm.gfmAutoComplete.loadData($input, '#', [{
+        id: 1,
+        iid: 111,
+        title: 'foo',
+      }]);
+
+      $input
+        .val('#')
+        .trigger('input')
+        .trigger('click');
+
+      $('.atwho-container li').trigger('click');
+
+      setTimeout(() => {
+        Vue.nextTick(() => {
+          expect(vm.$refs.input.value).toEqual('');
+          expect(addIssuableFormInputSpy.calls.count()).toEqual(1);
+
+          done();
+        });
+      });
+    });
+
     it('when submitting pending issues', () => {
       expect(addIssuableFormSubmitSpy).not.toHaveBeenCalled();
 

spec/policies/ee/merge_request_policy_spec.rb

+require 'spec_helper'
+
+describe MergeRequestPolicy, models: true do
+  let(:guest) { create(:user) }
+  let(:developer) { create(:user) }
+  let(:master) { create(:user) }
+
+  let(:fork_guest) { create(:user) }
+  let(:fork_developer) { create(:user) }
+  let(:fork_master) { create(:user) }
+
+  let(:project) { create(:empty_project, :public) }
+  let(:fork_project) { create(:empty_project, :public, forked_from_project: project) }
+
+  let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+  let(:fork_merge_request) { create(:merge_request, author: fork_developer, source_project: fork_project, target_project: project) }
+
+  before do
+    project.add_guest(guest)
+    project.add_developer(developer)
+    project.add_master(master)
+
+    fork_project.add_guest(fork_guest)
+    fork_project.add_developer(fork_guest)
+    fork_project.add_master(fork_master)
+  end
+
+  context 'for a merge request within the same project' do
+    def policy_for(user)
+      described_class.new(user, merge_request)
+    end
+
+    context 'when overwriting approvers is disabled on the project' do
+      before do
+        project.update!(disable_overriding_approvers_per_merge_request: true)
+      end
+
+      it 'does not allow anyone to update approvers' do
+        expect(policy_for(guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(developer)).to be_disallowed(:update_approvers)
+        expect(policy_for(master)).to be_disallowed(:update_approvers)
+
+        expect(policy_for(fork_guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_developer)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_master)).to be_disallowed(:update_approvers)
+      end
+    end
+
+    context 'when overwriting approvers is enabled on the project' do
+      it 'allows only project developers and above to update the approvers' do
+        expect(policy_for(developer)).to be_allowed(:update_approvers)
+        expect(policy_for(master)).to be_allowed(:update_approvers)
+
+        expect(policy_for(guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_developer)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_master)).to be_disallowed(:update_approvers)
+      end
+    end
+  end
+
+  context 'for a merge request from a fork' do
+    def policy_for(user)
+      described_class.new(user, fork_merge_request)
+    end
+
+    context 'when overwriting approvers is disabled on the target project' do
+      before do
+        project.update!(disable_overriding_approvers_per_merge_request: true)
+      end
+
+      it 'does not allow anyone to update approvers' do
+        expect(policy_for(guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(developer)).to be_disallowed(:update_approvers)
+        expect(policy_for(master)).to be_disallowed(:update_approvers)
+
+        expect(policy_for(fork_guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_developer)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_master)).to be_disallowed(:update_approvers)
+      end
+    end
+
+    context 'when overwriting approvers is disabled on the source project' do
+      before do
+        fork_project.update!(disable_overriding_approvers_per_merge_request: true)
+      end
+
+      it 'has no effect - project developers and above, as well as the author, can update the approvers' do
+        expect(policy_for(developer)).to be_allowed(:update_approvers)
+        expect(policy_for(master)).to be_allowed(:update_approvers)
+        expect(policy_for(fork_developer)).to be_allowed(:update_approvers)
+
+        expect(policy_for(guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_master)).to be_disallowed(:update_approvers)
+      end
+    end
+
+    context 'when overwriting approvers is enabled on the target project' do
+      it 'allows project developers and above, as well as the author, to update the approvers' do
+        expect(policy_for(developer)).to be_allowed(:update_approvers)
+        expect(policy_for(master)).to be_allowed(:update_approvers)
+        expect(policy_for(fork_developer)).to be_allowed(:update_approvers)
+
+        expect(policy_for(guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_guest)).to be_disallowed(:update_approvers)
+        expect(policy_for(fork_master)).to be_disallowed(:update_approvers)
+      end
+    end
+  end
+end

spec/views/shared/issuable/_approvals.html.haml_spec.rb

 require 'spec_helper'
 
-describe 'shared/issuable/_approvals.html.haml' do
+describe 'shared/issuable/_approvals.html.haml', :view do
+  let(:user) { create(:user) }
   let(:project) { build(:empty_project) }
   let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
   let(:form) { double('form') }
 
   before do
+    allow(view).to receive(:can?).and_return(true)
+    allow(view).to receive(:current_user).and_return(user)
     allow(form).to receive(:label)
     allow(form).to receive(:number_field)
     allow(merge_request).to receive(:requires_approve?).and_return(true)
@@ -22,7 +25,6 @@ describe 'shared/issuable/_approvals.html.haml' do
 
     context 'can override approvers' do
       before do
-        allow(project).to receive(:can_override_approvers?).and_return(true)
         render 'shared/issuable/approvals', form: form, issuable: merge_request
       end
 
@@ -41,7 +43,7 @@ describe 'shared/issuable/_approvals.html.haml' do
 
     context 'can not override approvers' do
       before do
-        allow(project).to receive(:can_override_approvers?).and_return(false)
+        allow(view).to receive(:can?).with(user, :update_approvers, merge_request).and_return(false)
         render 'shared/issuable/approvals', form: form, issuable: merge_request
       end
 
@@ -86,7 +88,7 @@ describe 'shared/issuable/_approvals.html.haml' do
 
     context 'can not override approvers' do
       it 'hides remove button' do
-        allow(project).to receive(:can_override_approvers?).and_return(false)
+        allow(view).to receive(:can?).with(user, :update_approvers, merge_request).and_return(false)
 
         render 'shared/issuable/approvals', form: form, issuable: merge_request