diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb index 284e119ca06859ab11a478dcef594ddeca3ca033..7159d0243a31e40f2ce8c78a6cd45b1c59f81e4a 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 6f6f1e5e0c5e01f5cab636437ff8ba745c1648d0..7367a96f8e48ef7694e15f465e99bb069e7f87c7 100644 --- a/app/views/projects/triggers/_trigger.html.haml +++ b/app/views/projects/triggers/_trigger.html.haml @@ -30,10 +30,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 0000000000000000000000000000000000000000..201f66e1f18fb1939e77e1df91775e953b31f7a7 --- /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 0f7621728d9f46d9ca7cb3fce2dcfddc8b6abde6..6278ce72352f829c45d2d6f3b15c4acadb50a19e 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -176,11 +176,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 736312df1161f66049db6f6cffbba1374fcc0dcf..e207ff8e98a8db659686e16652aafbebedde47c8 100644 --- a/doc/api/pipeline_triggers.md +++ b/doc/api/pipeline_triggers.md @@ -120,35 +120,6 @@ curl --request PUT --header "PRIVATE-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: " "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 ad80b5d88185b746e3151895e17c37ef558c7f2c..7fec4c7a867d8c7f6570836d08ad0fd9667d42cf 100644 --- a/doc/ci/triggers/README.md +++ b/doc/ci/triggers/README.md @@ -93,17 +93,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 @@ -278,8 +267,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 6c3fa5eb463ddc44e8c1ae9b5093835296c6439f..9c4871c5e15cdafd500b0e479e8083ccc434f90e 100644 --- a/doc/user/project/new_ci_build_permissions_model.md +++ b/doc/user/project/new_ci_build_permissions_model.md @@ -91,8 +91,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 0e829c5699b37fb63cfc91e8a14e7fc2c5907dfb..eeecc3902561ec7111ab318224565e049e110b40 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 e44f7e07e61d374f6246deb6ba63816ef5e56e6b..67932bda31694af0895585047e0c7e751ed10fec 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -10232,9 +10232,6 @@ msgstr "" msgid "Trigger was created successfully." msgstr "" -msgid "Trigger was re-assigned." -msgstr "" - msgid "Trigger was successfully updated." msgstr "" @@ -11065,9 +11062,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 919859c145ac7e2404fc75c4386a743587bbe7f7..41b640bb53aaa190fed88f32eb6768cacbde88a7 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 f0f01e97f1d69b1414a77d1af91e479ffd231b32..8ea3d16a41f6bb3e0e0b201c054f84638dbfce04 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