Merge branch 'security-remove-take-trigger-ownership-feature' into 'master'

Drop feature to take ownership of a trigger token

Closes #2868

See merge request gitlab/gitlabhq!3198

11 files changed, 9 insertions, 141 deletions

diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb
index 284e119ca06..7159d0243a3 100644
--- a/app/controllers/projects/triggers_controller.rb
+++ b/app/controllers/projects/triggers_controller.rb
@@ -4,7 +4,7 @@ class Projects::TriggersController < Projects::ApplicationController
   before_action :authorize_admin_build!
   before_action :authorize_manage_trigger!, except: [:index, :create]
   before_action :authorize_admin_trigger!, only: [:edit, :update]
-  before_action :trigger, only: [:take_ownership, :edit, :update, :destroy]
+  before_action :trigger, only: [:edit, :update, :destroy]
 
   layout 'project_settings'
 
@@ -24,16 +24,6 @@ class Projects::TriggersController < Projects::ApplicationController
     redirect_to project_settings_ci_cd_path(@project, anchor: 'js-pipeline-triggers')
   end
 
-  def take_ownership
-    if trigger.update(owner: current_user)
-      flash[:notice] = _('Trigger was re-assigned.')
-    else
-      flash[:alert] = _('You could not take ownership of trigger.')
-    end
-
-    redirect_to project_settings_ci_cd_path(@project, anchor: 'js-pipeline-triggers')
-  end
-
   def edit
   end
 
diff --git a/app/views/projects/triggers/_trigger.html.haml b/app/views/projects/triggers/_trigger.html.haml
index 31a598ccd5e..9899cf9c6de 100644
--- a/app/views/projects/triggers/_trigger.html.haml
+++ b/app/views/projects/triggers/_trigger.html.haml
@@ -33,10 +33,7 @@
       Never
 
   %td.text-right.trigger-actions
-    - take_ownership_confirmation = "By taking ownership you will bind this trigger to your user account. With this the trigger will have access to all your projects as if it was you. Are you sure?"
     - revoke_trigger_confirmation = "By revoking a trigger you will break any processes making use of it. Are you sure?"
-    - if trigger.owner != current_user && can?(current_user, :manage_trigger, trigger)
-      = link_to 'Take ownership', take_ownership_project_trigger_path(@project, trigger), data: { confirm: take_ownership_confirmation }, method: :post, class: "btn btn-default btn-sm btn-trigger-take-ownership"
     - if can?(current_user, :admin_trigger, trigger)
       = link_to edit_project_trigger_path(@project, trigger), method: :get, title: "Edit", class: "btn btn-default btn-sm" do
         %i.fa.fa-pencil
diff --git a/changelogs/unreleased/security-remove-take-trigger-ownership-feature.yml b/changelogs/unreleased/security-remove-take-trigger-ownership-feature.yml
new file mode 100644
index 00000000000..201f66e1f18
--- /dev/null
+++ b/changelogs/unreleased/security-remove-take-trigger-ownership-feature.yml
@@ -0,0 +1,5 @@
+---
+title: Drop feature to take ownership of trigger token.
+merge_request:
+author:
+type: security
diff --git a/config/routes/project.rb b/config/routes/project.rb
index c202463dadb..1f632765317 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -339,11 +339,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
 
       resource :variables, only: [:show, :update]
 
-      resources :triggers, only: [:index, :create, :edit, :update, :destroy] do
-        member do
-          post :take_ownership
-        end
-      end
+      resources :triggers, only: [:index, :create, :edit, :update, :destroy]
 
       resource :mirror, only: [:show, :update] do
         member do
diff --git a/doc/api/pipeline_triggers.md b/doc/api/pipeline_triggers.md
index 736312df116..e207ff8e98a 100644
--- a/doc/api/pipeline_triggers.md
+++ b/doc/api/pipeline_triggers.md
@@ -120,35 +120,6 @@ curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" --form descript
 }
 ```
 
-## Take ownership of a project trigger
-
-Update an owner of a project trigger.
-
-```
-POST /projects/:id/triggers/:trigger_id/take_ownership
-```
-
-| Attribute     | Type    | required | Description              |
-|---------------|---------|----------|--------------------------|
-| `id`          | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user      |
-| `trigger_id`  | integer | yes      | The trigger id           |
-
-```
-curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/1/triggers/10/take_ownership"
-```
-
-```json
-{
-    "id": 10,
-    "description": "my trigger",
-    "created_at": "2016-01-07T09:53:58.235Z",
-    "last_used": null,
-    "token": "6d056f63e50fe6f8c5f8f4aa10edb7",
-    "updated_at": "2016-01-07T09:53:58.235Z",
-    "owner": null
-}
-```
-
 ## Remove a project trigger
 
 Remove a project's build trigger.
diff --git a/doc/ci/triggers/README.md b/doc/ci/triggers/README.md
index d1f9aa03b6b..2a382f18038 100644
--- a/doc/ci/triggers/README.md
+++ b/doc/ci/triggers/README.md
@@ -97,17 +97,6 @@ overview of the time the triggers were last used.
 
 ![Triggers page overview](img/triggers_page.png)
 
-## Taking ownership of a trigger
-
-> **Note**:
-GitLab 9.0 introduced a trigger ownership to solve permission problems.
-
-Each created trigger when run will impersonate their associated user including
-their access to projects and their project permissions.
-
-You can take ownership of existing triggers by clicking *Take ownership*.
-From now on the trigger will be run as you.
-
 ## Revoking a trigger
 
 You can revoke a trigger any time by going at your project's
@@ -282,8 +271,7 @@ Old triggers, created before GitLab 9.0 will be marked as legacy.
 
 Triggers with the legacy label do not have an associated user and only have
 access to the current project. They are considered deprecated and will be
-removed with one of the future versions of GitLab. You are advised to
-[take ownership](#taking-ownership-of-a-trigger) of any legacy triggers.
+removed with one of the future versions of GitLab. 
 
 [ee-2017]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2017
 [ee-2346]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2346
diff --git a/doc/user/project/new_ci_build_permissions_model.md b/doc/user/project/new_ci_build_permissions_model.md
index 494dd539da7..03ae24242e3 100644
--- a/doc/user/project/new_ci_build_permissions_model.md
+++ b/doc/user/project/new_ci_build_permissions_model.md
@@ -90,8 +90,7 @@ to steal the tokens of other jobs.
 
 Since 9.0 [pipeline triggers][triggers] do support the new permission model.
 The new triggers do impersonate their associated user including their access
-to projects and their project permissions. To migrate trigger to use new permission
-model use **Take ownership**.
+to projects and their project permissions. 
 
 ## Before GitLab 8.12
 
diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb
index 0e829c5699b..eeecc390256 100644
--- a/lib/api/triggers.rb
+++ b/lib/api/triggers.rb
@@ -112,27 +112,6 @@ module API
         end
       end
 
-      desc 'Take ownership of trigger' do
-        success Entities::Trigger
-      end
-      params do
-        requires :trigger_id, type: Integer, desc: 'The trigger ID'
-      end
-      post ':id/triggers/:trigger_id/take_ownership' do
-        authenticate!
-        authorize! :admin_build, user_project
-
-        trigger = user_project.triggers.find(params.delete(:trigger_id))
-        break not_found!('Trigger') unless trigger
-
-        if trigger.update(owner: current_user)
-          status :ok
-          present trigger, with: Entities::Trigger, current_user: current_user
-        else
-          render_validation_error!(trigger)
-        end
-      end
-
       desc 'Delete a trigger' do
         success Entities::Trigger
       end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 9607be988be..b06e38cc094 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -11507,9 +11507,6 @@ msgstr ""
 msgid "Trigger was created successfully."
 msgstr ""
 
-msgid "Trigger was re-assigned."
-msgstr ""
-
 msgid "Trigger was successfully updated."
 msgstr ""
 
@@ -12454,9 +12451,6 @@ msgstr ""
 msgid "You could not create a new trigger."
 msgstr ""
 
-msgid "You could not take ownership of trigger."
-msgstr ""
-
 msgid "You do not have any subscriptions yet"
 msgstr ""
 
diff --git a/spec/features/triggers_spec.rb b/spec/features/triggers_spec.rb
index 7c44680e9f7..f76cecbbc40 100644
--- a/spec/features/triggers_spec.rb
+++ b/spec/features/triggers_spec.rb
@@ -81,29 +81,6 @@ describe 'Triggers', :js do
     end
   end
 
-  describe 'trigger "Take ownership" workflow' do
-    before do
-      create(:ci_trigger, owner: user2, project: @project, description: trigger_title)
-      visit project_settings_ci_cd_path(@project)
-    end
-
-    it 'button "Take ownership" has correct alert' do
-      expected_alert = 'By taking ownership you will bind this trigger to your user account. With this the trigger will have access to all your projects as if it was you. Are you sure?'
-      expect(page.find('a.btn-trigger-take-ownership')['data-confirm']).to eq expected_alert
-    end
-
-    it 'take trigger ownership' do
-      # See if "Take ownership" on trigger works post trigger creation
-      page.accept_confirm do
-        first(:link, "Take ownership").send_keys(:return)
-      end
-
-      expect(page.find('.flash-notice')).to have_content 'Trigger was re-assigned.'
-      expect(page.find('.triggers-list')).to have_content trigger_title
-      expect(page.find('.triggers-list .trigger-owner')).to have_content user.name
-    end
-  end
-
   describe 'trigger "Revoke" workflow' do
     before do
       create(:ci_trigger, owner: user2, project: @project, description: trigger_title)
diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb
index f0f01e97f1d..8ea3d16a41f 100644
--- a/spec/requests/api/triggers_spec.rb
+++ b/spec/requests/api/triggers_spec.rb
@@ -270,34 +270,6 @@ describe API::Triggers do
     end
   end
 
-  describe 'POST /projects/:id/triggers/:trigger_id/take_ownership' do
-    context 'authenticated user with valid permissions' do
-      it 'updates owner' do
-        post api("/projects/#{project.id}/triggers/#{trigger.id}/take_ownership", user)
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(json_response).to include('owner')
-        expect(trigger.reload.owner).to eq(user)
-      end
-    end
-
-    context 'authenticated user with invalid permissions' do
-      it 'does not update owner' do
-        post api("/projects/#{project.id}/triggers/#{trigger.id}/take_ownership", user2)
-
-        expect(response).to have_gitlab_http_status(403)
-      end
-    end
-
-    context 'unauthenticated user' do
-      it 'does not update owner' do
-        post api("/projects/#{project.id}/triggers/#{trigger.id}/take_ownership")
-
-        expect(response).to have_gitlab_http_status(401)
-      end
-    end
-  end
-
   describe 'DELETE /projects/:id/triggers/:trigger_id' do
     context 'authenticated user with valid permissions' do
       it 'deletes trigger' do