diff options
author | Kamil TrzciĆski <ayufan@ayufan.eu> | 2017-03-07 13:02:56 +0000 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2017-03-07 13:02:56 +0000 |
commit | 32dee03b2fec293a8581034301bab9d4a3f86d1a (patch) | |
tree | ad477e7c5f843e7d14dad02784fab4c540e8818e | |
parent | b729b171981c6156164a1070c2b366f7d1986a1d (diff) | |
download | gitlab-ce-32dee03b2fec293a8581034301bab9d4a3f86d1a.tar.gz |
Improve pipeline triggers UI
-rw-r--r-- | app/assets/stylesheets/pages/settings_ci_cd.scss | 12 | ||||
-rw-r--r-- | app/controllers/projects/triggers_controller.rb | 59 | ||||
-rw-r--r-- | app/models/ci/trigger.rb | 8 | ||||
-rw-r--r-- | app/policies/ci/trigger_policy.rb | 13 | ||||
-rw-r--r-- | app/views/projects/triggers/_content.html.haml | 14 | ||||
-rw-r--r-- | app/views/projects/triggers/_form.html.haml | 11 | ||||
-rw-r--r-- | app/views/projects/triggers/_index.html.haml | 24 | ||||
-rw-r--r-- | app/views/projects/triggers/_trigger.html.haml | 40 | ||||
-rw-r--r-- | app/views/projects/triggers/edit.html.haml | 9 | ||||
-rw-r--r-- | changelogs/unreleased/add-pipeline-triggers.yml | 4 | ||||
-rw-r--r-- | config/routes/project.rb | 6 | ||||
-rw-r--r-- | spec/features/triggers_spec.rb | 169 | ||||
-rw-r--r-- | spec/models/ci/trigger_spec.rb | 58 | ||||
-rw-r--r-- | spec/policies/ci/trigger_policy_spec.rb | 103 |
14 files changed, 489 insertions, 41 deletions
diff --git a/app/assets/stylesheets/pages/settings_ci_cd.scss b/app/assets/stylesheets/pages/settings_ci_cd.scss new file mode 100644 index 00000000000..b97a29cd1a0 --- /dev/null +++ b/app/assets/stylesheets/pages/settings_ci_cd.scss @@ -0,0 +1,12 @@ +.triggers-container { + .label-container { + display: inline-block; + margin-left: 10px; + } +} + +.trigger-actions { + .btn { + margin-left: 10px; + } +} diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb index b2c11ea4156..c47198c5eb6 100644 --- a/app/controllers/projects/triggers_controller.rb +++ b/app/controllers/projects/triggers_controller.rb @@ -1,5 +1,8 @@ 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] layout 'project_settings' @@ -8,27 +11,67 @@ class Projects::TriggersController < Projects::ApplicationController end def create - @trigger = project.triggers.new - @trigger.save + @trigger = project.triggers.create(create_params.merge(owner: current_user)) if @trigger.valid? - redirect_to namespace_project_variables_path(project.namespace, project), notice: 'Trigger was created successfully.' + flash[:notice] = 'Trigger was created successfully.' else - @triggers = project.triggers.select(&:persisted?) - render action: "show" + flash[:alert] = 'You could not create a new trigger.' + end + + redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project) + 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 namespace_project_settings_ci_cd_path(@project.namespace, @project) + end + + def edit + end + + def update + if trigger.update(update_params) + redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project), notice: 'Trigger was successfully updated.' + else + render action: "edit" end end def destroy - trigger.destroy - flash[:alert] = "Trigger removed" + if trigger.destroy + flash[:notice] = "Trigger removed." + else + flash[:alert] = "Could not remove the trigger." + end redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project) end private + def authorize_manage_trigger! + access_denied! unless can?(current_user, :manage_trigger, trigger) + end + + def authorize_admin_trigger! + access_denied! unless can?(current_user, :admin_trigger, trigger) + end + def trigger - @trigger ||= project.triggers.find(params[:id]) + @trigger ||= project.triggers.find(params[:id]) || render_404 + end + + def create_params + params.require(:trigger).permit(:description) + end + + def update_params + params.require(:trigger).permit(:description) end end diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index 8aa45b2f02e..90473d41c04 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -29,8 +29,12 @@ module Ci token[0...4] end - def can_show_token?(user) - owner.blank? || owner == user + def legacy? + self.owner_id.blank? + end + + def can_access_project? + self.owner_id.blank? || Ability.allowed?(self.owner, :create_build, project) end end end diff --git a/app/policies/ci/trigger_policy.rb b/app/policies/ci/trigger_policy.rb new file mode 100644 index 00000000000..c90c9ac0583 --- /dev/null +++ b/app/policies/ci/trigger_policy.rb @@ -0,0 +1,13 @@ +module Ci + class TriggerPolicy < BasePolicy + def rules + delegate! @subject.project + + if can?(:admin_build) + can! :admin_trigger if @subject.owner.blank? || + @subject.owner == @user + can! :manage_trigger + end + end + end +end diff --git a/app/views/projects/triggers/_content.html.haml b/app/views/projects/triggers/_content.html.haml new file mode 100644 index 00000000000..ea32eac2ae2 --- /dev/null +++ b/app/views/projects/triggers/_content.html.haml @@ -0,0 +1,14 @@ +%h4.prepend-top-0 + Triggers +%p.prepend-top-20 + Triggers can force a specific branch or tag to get rebuilt with an API call. These tokens will + impersonate their associated user including their access to projects and their project + permissions. +%p.prepend-top-20 + Triggers with the + %span.label.label-primary legacy + label do not have an associated user and only have access to the current project. +%p.append-bottom-0 + = succeed '.' do + Learn more in the + = link_to 'triggers documentation', help_page_path('ci/triggers/README'), target: '_blank' diff --git a/app/views/projects/triggers/_form.html.haml b/app/views/projects/triggers/_form.html.haml new file mode 100644 index 00000000000..5f708b3a2ed --- /dev/null +++ b/app/views/projects/triggers/_form.html.haml @@ -0,0 +1,11 @@ += form_for [@project.namespace.becomes(Namespace), @project, @trigger], html: { class: 'gl-show-field-errors' } do |f| + = form_errors(@trigger) + + - if @trigger.token + .form-group + %label.label-light Token + %p.form-control-static= @trigger.token + .form-group + = f.label :key, "Description", class: "label-light" + = f.text_field :description, class: "form-control", required: true, title: 'Trigger description is required.', placeholder: "Trigger description" + = f.submit btn_text, class: "btn btn-save" diff --git a/app/views/projects/triggers/_index.html.haml b/app/views/projects/triggers/_index.html.haml index 33883facf9b..cc74e50a5e3 100644 --- a/app/views/projects/triggers/_index.html.haml +++ b/app/views/projects/triggers/_index.html.haml @@ -1,35 +1,31 @@ -.row.prepend-top-default.append-bottom-default +.row.prepend-top-default.append-bottom-default.triggers-container .col-lg-3 - %h4.prepend-top-0 - Triggers - %p.prepend-top-20 - Triggers can force a specific branch or tag to get rebuilt with an API call. - %p.append-bottom-0 - = succeed '.' do - Learn more in the - = link_to 'triggers documentation', help_page_path('ci/triggers/README'), target: '_blank' + = render "projects/triggers/content" .col-lg-9 .panel.panel-default .panel-heading %h4.panel-title Manage your project's triggers .panel-body + = render "projects/triggers/form", btn_text: "Add trigger" + %hr - if @triggers.any? - .table-responsive + .table-responsive.triggers-list %table.table %thead %th %strong Token %th + %strong Description + %th + %strong Owner + %th %strong Last used %th = render partial: 'projects/triggers/trigger', collection: @triggers, as: :trigger - else %p.settings-message.text-center.append-bottom-default - No triggers have been created yet. Add one using the button below. - - = form_for @trigger, url: url_for(controller: '/projects/triggers', action: 'create') do |f| - = f.submit "Add trigger", class: 'btn btn-success' + No triggers have been created yet. Add one using the form above. .panel-footer diff --git a/app/views/projects/triggers/_trigger.html.haml b/app/views/projects/triggers/_trigger.html.haml index 112b51712ef..ed68e0ed56d 100644 --- a/app/views/projects/triggers/_trigger.html.haml +++ b/app/views/projects/triggers/_trigger.html.haml @@ -1,12 +1,42 @@ %tr %td - %span.monospace= trigger.token + - if can?(current_user, :admin_trigger, trigger) + %span= trigger.token + = clipboard_button(clipboard_text: trigger.token, title: "Copy trigger token to clipboard") + - else + %span= trigger.short_token + + .label-container + - if trigger.legacy? + %span.label.label-primary.has-tooltip{ title: "Trigger makes use of deprecated functionality" } legacy + - if !trigger.can_access_project? + %span.label.label-danger.has-tooltip{ title: "Trigger user has insufficient permissions to project" } invalid + + %td + - if trigger.description? && trigger.description.length > 15 + %span.has-tooltip{ title: trigger.description }= truncate(trigger.description, length: 15) + - else + = trigger.description + + %td + - if trigger.owner + .trigger-owner.sr-only= trigger.owner.name + = user_avatar(user: trigger.owner, size: 20) %td - - if trigger.last_trigger_request - #{time_ago_in_words(trigger.last_trigger_request.created_at)} ago + - if trigger.last_used + #{time_ago_in_words(trigger.last_used)} ago - else Never - %td.text-right - = link_to 'Revoke', namespace_project_trigger_path(@project.namespace, @project, trigger), data: { confirm: 'Are you sure?'}, method: :delete, class: "btn btn-warning btn-sm" + %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_namespace_project_trigger_path(@project.namespace, @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_namespace_project_trigger_path(@project.namespace, @project, trigger), method: :get, title: "Edit", class: "btn btn-default btn-sm" do + %i.fa.fa-pencil + - if can?(current_user, :manage_trigger, trigger) + = link_to namespace_project_trigger_path(@project.namespace, @project, trigger), data: { confirm: revoke_trigger_confirmation }, method: :delete, title: "Revoke", class: "btn btn-default btn-warning btn-sm btn-trigger-revoke" do + %i.fa.fa-trash diff --git a/app/views/projects/triggers/edit.html.haml b/app/views/projects/triggers/edit.html.haml new file mode 100644 index 00000000000..c35df322b9d --- /dev/null +++ b/app/views/projects/triggers/edit.html.haml @@ -0,0 +1,9 @@ +- page_title "Trigger" + +.row.prepend-top-default.append-bottom-default + .col-lg-3 + = render "content" + .col-lg-9 + %h4.prepend-top-0 + Update trigger + = render "form", btn_text: "Save trigger" diff --git a/changelogs/unreleased/add-pipeline-triggers.yml b/changelogs/unreleased/add-pipeline-triggers.yml new file mode 100644 index 00000000000..81b11da0bb2 --- /dev/null +++ b/changelogs/unreleased/add-pipeline-triggers.yml @@ -0,0 +1,4 @@ +--- +title: Add pipeline trigger API with user permissions +merge_request: 9277 +author: diff --git a/config/routes/project.rb b/config/routes/project.rb index f5cc99b6867..df39c3e200c 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -135,7 +135,11 @@ constraints(ProjectUrlConstrainer.new) do resources :protected_branches, only: [:index, :show, :create, :update, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex } resources :variables, only: [:index, :show, :update, :create, :destroy] - resources :triggers, only: [:index, :create, :destroy] + resources :triggers, only: [:index, :create, :edit, :update, :destroy] do + member do + post :take_ownership + end + end resources :pipelines, only: [:index, :new, :create, :show] do collection do diff --git a/spec/features/triggers_spec.rb b/spec/features/triggers_spec.rb index 4a7511589d6..c1ae6db00c6 100644 --- a/spec/features/triggers_spec.rb +++ b/spec/features/triggers_spec.rb @@ -1,28 +1,175 @@ require 'spec_helper' -describe 'Triggers' do +feature 'Triggers', feature: true, js: true do + let(:trigger_title) { 'trigger desc' } let(:user) { create(:user) } + let(:user2) { create(:user) } + let(:guest_user) { create(:user) } before { login_as(user) } before do - @project = FactoryGirl.create :empty_project + @project = create(:empty_project) @project.team << [user, :master] + @project.team << [user2, :master] + @project.team << [guest_user, :guest] visit namespace_project_settings_ci_cd_path(@project.namespace, @project) end - context 'create a trigger' do - before do - click_on 'Add trigger' - expect(@project.triggers.count).to eq(1) + describe 'create trigger workflow' do + scenario 'prevents adding new trigger with no description' do + fill_in 'trigger_description', with: '' + click_button 'Add trigger' + + # See if input has error due to empty value + expect(page.find('form.gl-show-field-errors .gl-field-error')['style']).to eq 'display: block;' + end + + scenario 'adds new trigger with description' do + fill_in 'trigger_description', with: 'trigger desc' + click_button 'Add trigger' + + # See if "trigger creation successful" message displayed and description and owner are correct + expect(page.find('.flash-notice')).to have_content 'Trigger was created successfully.' + expect(page.find('.triggers-list')).to have_content 'trigger desc' + expect(page.find('.triggers-list .trigger-owner')).to have_content @user.name + end + end + + describe 'edit trigger workflow' do + let(:new_trigger_title) { 'new trigger' } + + scenario 'click on edit trigger opens edit trigger page' do + create(:ci_trigger, owner: user, project: @project, description: trigger_title) + visit namespace_project_settings_ci_cd_path(@project.namespace, @project) + + # See if edit page has correct descrption + find('a[title="Edit"]').click + expect(page.find('#trigger_description').value).to have_content 'trigger desc' + end + + scenario 'edit trigger and save' do + create(:ci_trigger, owner: user, project: @project, description: trigger_title) + visit namespace_project_settings_ci_cd_path(@project.namespace, @project) + + # See if edit page opens, then fill in new description and save + find('a[title="Edit"]').click + fill_in 'trigger_description', with: new_trigger_title + click_button 'Save trigger' + + # See if "trigger updated successfully" message displayed and description and owner are correct + expect(page.find('.flash-notice')).to have_content 'Trigger was successfully updated.' + expect(page.find('.triggers-list')).to have_content new_trigger_title + expect(page.find('.triggers-list .trigger-owner')).to have_content @user.name + end + + scenario 'edit "legacy" trigger and save' do + # Create new trigger without owner association, i.e. Legacy trigger + create(:ci_trigger, owner: nil, project: @project) + visit namespace_project_settings_ci_cd_path(@project.namespace, @project) + + # See if the trigger can be edited and description is blank + find('a[title="Edit"]').click + expect(page.find('#trigger_description').value).to have_content '' + + # See if trigger can be updated with description and saved successfully + fill_in 'trigger_description', with: new_trigger_title + click_button 'Save trigger' + expect(page.find('.flash-notice')).to have_content 'Trigger was successfully updated.' + expect(page.find('.triggers-list')).to have_content new_trigger_title + end + end + + describe 'trigger "Take ownership" workflow' do + before(:each) do + create(:ci_trigger, owner: user2, project: @project, description: trigger_title) + visit namespace_project_settings_ci_cd_path(@project.namespace, @project) + end + + scenario '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 'contains trigger token' do - expect(page).to have_content(@project.triggers.first.token) + scenario 'take trigger ownership' do + # See if "Take ownership" on trigger works post trigger creation + find('a.btn-trigger-take-ownership').click + page.accept_confirm do + 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 + end + + describe 'trigger "Revoke" workflow' do + before(:each) do + create(:ci_trigger, owner: user2, project: @project, description: trigger_title) + visit namespace_project_settings_ci_cd_path(@project.namespace, @project) + end + + scenario 'button "Revoke" has correct alert' do + expected_alert = 'By revoking a trigger you will break any processes making use of it. Are you sure?' + expect(page.find('a.btn-trigger-revoke')['data-confirm']).to eq expected_alert + end + + scenario 'revoke trigger' do + # See if "Revoke" on trigger works post trigger creation + find('a.btn-trigger-revoke').click + page.accept_confirm do + expect(page.find('.flash-notice')).to have_content 'Trigger removed' + expect(page).to have_selector('p.settings-message.text-center.append-bottom-default') + end + end + end + + describe 'show triggers workflow' do + scenario 'contains trigger description placeholder' do + expect(page.find('#trigger_description')['placeholder']).to eq 'Trigger description' + end + + scenario 'show "legacy" badge for legacy trigger' do + create(:ci_trigger, owner: nil, project: @project) + visit namespace_project_settings_ci_cd_path(@project.namespace, @project) + + # See if trigger without owner (i.e. legacy) shows "legacy" badge and is editable + expect(page.find('.triggers-list')).to have_content 'legacy' + expect(page.find('.triggers-list')).to have_selector('a[title="Edit"]') + end + + scenario 'show "invalid" badge for trigger with owner having insufficient permissions' do + create(:ci_trigger, owner: guest_user, project: @project, description: trigger_title) + visit namespace_project_settings_ci_cd_path(@project.namespace, @project) + + # See if trigger without owner (i.e. legacy) shows "legacy" badge and is non-editable + expect(page.find('.triggers-list')).to have_content 'invalid' + expect(page.find('.triggers-list')).not_to have_selector('a[title="Edit"]') + end + + scenario 'do not show "Edit" or full token for not owned trigger' do + # Create trigger with user different from current_user + create(:ci_trigger, owner: user2, project: @project, description: trigger_title) + visit namespace_project_settings_ci_cd_path(@project.namespace, @project) + + # See if trigger not owned by current_user shows only first few token chars and doesn't have copy-to-clipboard button + expect(page.find('.triggers-list')).to have_content(@project.triggers.first.token[0..3]) + expect(page.find('.triggers-list')).not_to have_selector('button.btn-clipboard') + + # See if trigger owner name doesn't match with current_user and trigger is non-editable + expect(page.find('.triggers-list .trigger-owner')).not_to have_content @user.name + expect(page.find('.triggers-list')).not_to have_selector('a[title="Edit"]') + end + + scenario 'show "Edit" and full token for owned trigger' do + create(:ci_trigger, owner: user, project: @project, description: trigger_title) + visit namespace_project_settings_ci_cd_path(@project.namespace, @project) + + # See if trigger shows full token and has copy-to-clipboard button + expect(page.find('.triggers-list')).to have_content @project.triggers.first.token + expect(page.find('.triggers-list')).to have_selector('button.btn-clipboard') - it 'revokes the trigger' do - click_on 'Revoke' - expect(@project.triggers.count).to eq(0) + # See if trigger owner name matches with current_user and is editable + expect(page.find('.triggers-list .trigger-owner')).to have_content @user.name + expect(page.find('.triggers-list')).to have_selector('a[title="Edit"]') end end end diff --git a/spec/models/ci/trigger_spec.rb b/spec/models/ci/trigger_spec.rb index 074cf1a0bd7..1bcb673cb16 100644 --- a/spec/models/ci/trigger_spec.rb +++ b/spec/models/ci/trigger_spec.rb @@ -22,4 +22,62 @@ describe Ci::Trigger, models: true do expect(trigger.token).to eq('token') end end + + describe '#short_token' do + let(:trigger) { create(:ci_trigger, token: '12345678') } + + subject { trigger.short_token } + + it 'returns shortened token' do + is_expected.to eq('1234') + end + end + + describe '#legacy?' do + let(:trigger) { create(:ci_trigger, owner: owner, project: project) } + + subject { trigger } + + context 'when owner is blank' do + let(:owner) { nil } + + it { is_expected.to be_legacy } + end + + context 'when owner is set' do + let(:owner) { create(:user) } + + it { is_expected.not_to be_legacy } + end + end + + describe '#can_access_project?' do + let(:trigger) { create(:ci_trigger, owner: owner, project: project) } + + context 'when owner is blank' do + let(:owner) { nil } + + subject { trigger.can_access_project? } + + it { is_expected.to eq(true) } + end + + context 'when owner is set' do + let(:owner) { create(:user) } + + subject { trigger.can_access_project? } + + context 'and is member of the project' do + before do + project.team << [owner, :developer] + end + + it { is_expected.to eq(true) } + end + + context 'and is not member of the project' do + it { is_expected.to eq(false) } + end + end + end end diff --git a/spec/policies/ci/trigger_policy_spec.rb b/spec/policies/ci/trigger_policy_spec.rb new file mode 100644 index 00000000000..63ad5eb7322 --- /dev/null +++ b/spec/policies/ci/trigger_policy_spec.rb @@ -0,0 +1,103 @@ +require 'spec_helper' + +describe Ci::TriggerPolicy, :models do + let(:user) { create(:user) } + let(:project) { create(:empty_project) } + let(:trigger) { create(:ci_trigger, project: project, owner: owner) } + + let(:policies) do + described_class.abilities(user, trigger).to_set + end + + shared_examples 'allows to admin and manage trigger' do + it 'does include ability to admin trigger' do + expect(policies).to include :admin_trigger + end + + it 'does include ability to manage trigger' do + expect(policies).to include :manage_trigger + end + end + + shared_examples 'allows to manage trigger' do + it 'does not include ability to admin trigger' do + expect(policies).not_to include :admin_trigger + end + + it 'does include ability to manage trigger' do + expect(policies).to include :manage_trigger + end + end + + shared_examples 'disallows to admin and manage trigger' do + it 'does not include ability to admin trigger' do + expect(policies).not_to include :admin_trigger + end + + it 'does not include ability to manage trigger' do + expect(policies).not_to include :manage_trigger + end + end + + describe '#rules' do + context 'when owner is undefined' do + let(:owner) { nil } + + context 'when user is master of the project' do + before do + project.team << [user, :master] + end + + it_behaves_like 'allows to admin and manage trigger' + end + + context 'when user is developer of the project' do + before do + project.team << [user, :developer] + end + + it_behaves_like 'disallows to admin and manage trigger' + end + + context 'when user is not member of the project' do + it_behaves_like 'disallows to admin and manage trigger' + end + end + + context 'when owner is an user' do + let(:owner) { user } + + context 'when user is master of the project' do + before do + project.team << [user, :master] + end + + it_behaves_like 'allows to admin and manage trigger' + end + end + + context 'when owner is another user' do + let(:owner) { create(:user) } + + context 'when user is master of the project' do + before do + project.team << [user, :master] + end + + it_behaves_like 'allows to manage trigger' + end + + context 'when user is developer of the project' do + before do + project.team << [user, :developer] + end + + it_behaves_like 'disallows to admin and manage trigger' + end + + context 'when user is not member of the project' do + it_behaves_like 'disallows to admin and manage trigger' + end + end + end +end |