diff options
-rw-r--r-- | app/controllers/projects/settings/ci_cd_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/projects/triggers_controller.rb | 7 | ||||
-rw-r--r-- | app/models/ci/trigger.rb | 3 | ||||
-rw-r--r-- | app/presenters/ci/trigger_presenter.rb | 19 | ||||
-rw-r--r-- | app/views/projects/triggers/_trigger.html.haml | 2 | ||||
-rw-r--r-- | changelogs/unreleased/security-pipeline-trigger-tokens-exposure.yml | 5 | ||||
-rw-r--r-- | lib/api/entities.rb | 5 | ||||
-rw-r--r-- | lib/api/helpers/presentable.rb | 29 | ||||
-rw-r--r-- | lib/api/triggers.rb | 10 | ||||
-rw-r--r-- | spec/presenters/ci/trigger_presenter_spec.rb | 51 | ||||
-rw-r--r-- | spec/requests/api/triggers_spec.rb | 14 |
11 files changed, 17 insertions, 130 deletions
diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index 0c0f81a69c6..3a1344651df 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -99,9 +99,7 @@ module Projects def define_triggers_variables @triggers = @project.triggers - .present(current_user: current_user) @trigger = ::Ci::Trigger.new - .present(current_user: current_user) end def define_badges_variables diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb index c7b4ebb2b24..f5fdfb8accc 100644 --- a/app/controllers/projects/triggers_controller.rb +++ b/app/controllers/projects/triggers_controller.rb @@ -66,11 +66,12 @@ class Projects::TriggersController < Projects::ApplicationController end def trigger - @trigger ||= project.triggers.find(params[:id]) - .present(current_user: current_user) + @trigger ||= project.triggers.find(params[:id]) || render_404 end def trigger_params - params.require(:trigger).permit(:description) + params.require(:trigger).permit( + :description + ) end end diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index 637148c4ce4..55db42162ca 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -4,7 +4,6 @@ module Ci class Trigger < ActiveRecord::Base extend Gitlab::Ci::Model include IgnorableColumn - include Presentable ignore_column :deleted_at @@ -30,7 +29,7 @@ module Ci end def short_token - token[0...4] if token.present? + token[0...4] end def legacy? diff --git a/app/presenters/ci/trigger_presenter.rb b/app/presenters/ci/trigger_presenter.rb deleted file mode 100644 index 605c8f328a4..00000000000 --- a/app/presenters/ci/trigger_presenter.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -module Ci - class TriggerPresenter < Gitlab::View::Presenter::Delegated - presents :trigger - - def has_token_exposed? - can?(current_user, :admin_trigger, trigger) - end - - def token - if has_token_exposed? - trigger.token - else - trigger.short_token - end - end - end -end diff --git a/app/views/projects/triggers/_trigger.html.haml b/app/views/projects/triggers/_trigger.html.haml index 6f6f1e5e0c5..7e4618e1a88 100644 --- a/app/views/projects/triggers/_trigger.html.haml +++ b/app/views/projects/triggers/_trigger.html.haml @@ -1,6 +1,6 @@ %tr %td - - if trigger.has_token_exposed? + - if can?(current_user, :admin_trigger, trigger) %span= trigger.token = clipboard_button(text: trigger.token, title: "Copy trigger token to clipboard") - else diff --git a/changelogs/unreleased/security-pipeline-trigger-tokens-exposure.yml b/changelogs/unreleased/security-pipeline-trigger-tokens-exposure.yml deleted file mode 100644 index 97d743eead1..00000000000 --- a/changelogs/unreleased/security-pipeline-trigger-tokens-exposure.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Expose CI/CD trigger token only to the trigger owner -merge_request: -author: -type: security diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 9c2683702e7..fd36381aa51 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1161,11 +1161,8 @@ module API end class Trigger < Grape::Entity - include ::API::Helpers::Presentable - expose :id - expose :token - expose :description + expose :token, :description expose :created_at, :updated_at, :last_used expose :owner, using: Entities::UserBasic end diff --git a/lib/api/helpers/presentable.rb b/lib/api/helpers/presentable.rb deleted file mode 100644 index 973c2132efe..00000000000 --- a/lib/api/helpers/presentable.rb +++ /dev/null @@ -1,29 +0,0 @@ -# frozen_string_literal: true - -module API - module Helpers - ## - # This module makes it possible to use `app/presenters` with - # Grape Entities. It instantiates model presenter and passes - # options defined in the API endpoint to the presenter itself. - # - # present object, with: Entities::Something, - # current_user: current_user, - # another_option: 'my options' - # - # Example above will make `current_user` and `another_option` - # values available in the subclass of `Gitlab::View::Presenter` - # thorough a separate method in the presenter. - # - # The model class needs to have `::Presentable` module mixed in - # if you want to use `API::Helpers::Presentable`. - # - module Presentable - extend ActiveSupport::Concern - - def initialize(object, options = {}) - super(object.present(options), options) - end - end - end -end diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index f3a0d7b7b43..f784c857883 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -51,7 +51,7 @@ module API triggers = user_project.triggers.includes(:trigger_requests) - present paginate(triggers), with: Entities::Trigger, current_user: current_user + present paginate(triggers), with: Entities::Trigger end # rubocop: enable CodeReuse/ActiveRecord @@ -68,7 +68,7 @@ module API trigger = user_project.triggers.find(params.delete(:trigger_id)) break not_found!('Trigger') unless trigger - present trigger, with: Entities::Trigger, current_user: current_user + present trigger, with: Entities::Trigger end desc 'Create a trigger' do @@ -85,7 +85,7 @@ module API declared_params(include_missing: false).merge(owner: current_user)) if trigger.valid? - present trigger, with: Entities::Trigger, current_user: current_user + present trigger, with: Entities::Trigger else render_validation_error!(trigger) end @@ -106,7 +106,7 @@ module API break not_found!('Trigger') unless trigger if trigger.update(declared_params(include_missing: false)) - present trigger, with: Entities::Trigger, current_user: current_user + present trigger, with: Entities::Trigger else render_validation_error!(trigger) end @@ -127,7 +127,7 @@ module API if trigger.update(owner: current_user) status :ok - present trigger, with: Entities::Trigger, current_user: current_user + present trigger, with: Entities::Trigger else render_validation_error!(trigger) end diff --git a/spec/presenters/ci/trigger_presenter_spec.rb b/spec/presenters/ci/trigger_presenter_spec.rb deleted file mode 100644 index 231b539c188..00000000000 --- a/spec/presenters/ci/trigger_presenter_spec.rb +++ /dev/null @@ -1,51 +0,0 @@ -require 'spec_helper' - -describe Ci::TriggerPresenter do - set(:user) { create(:user) } - set(:project) { create(:project) } - - set(:trigger) do - create(:ci_trigger, token: '123456789abcd', project: project) - end - - subject do - described_class.new(trigger, current_user: user) - end - - before do - project.add_maintainer(user) - end - - context 'when user is not a trigger owner' do - describe '#token' do - it 'exposes only short token' do - expect(subject.token).not_to eq trigger.token - expect(subject.token).to eq '1234' - end - end - - describe '#has_token_exposed?' do - it 'does not have token exposed' do - expect(subject).not_to have_token_exposed - end - end - end - - context 'when user is a trigger owner and builds admin' do - before do - trigger.update(owner: user) - end - - describe '#token' do - it 'exposes full token' do - expect(subject.token).to eq trigger.token - end - end - - describe '#has_token_exposed?' do - it 'has token exposed' do - expect(subject).to have_token_exposed - end - end - end -end diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index e7ab1a2a097..0ae6796d1e4 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -1,9 +1,8 @@ require 'spec_helper' describe API::Triggers do - set(:user) { create(:user) } - set(:user2) { create(:user) } - + let(:user) { create(:user) } + let(:user2) { create(:user) } let!(:trigger_token) { 'secure_token' } let!(:trigger_token_2) { 'secure_token_2' } let!(:project) { create(:project, :repository, creator: user) } @@ -133,17 +132,14 @@ describe API::Triggers do end describe 'GET /projects/:id/triggers' do - context 'authenticated user who can access triggers' do - it 'returns a list of triggers with tokens exposed correctly' do + context 'authenticated user with valid permissions' do + it 'returns list of triggers' do get api("/projects/#{project.id}/triggers", user) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers - expect(json_response).to be_a(Array) - expect(json_response.size).to eq 2 - expect(json_response.dig(0, 'token')).to eq trigger_token - expect(json_response.dig(1, 'token')).to eq trigger_token_2[0..3] + expect(json_response[0]).to have_key('token') end end |