Improve pipeline triggers UI

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;
+  }
+}

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

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

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

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'

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"

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
 

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.last_trigger_request
-      #{time_ago_in_words(trigger.last_trigger_request.created_at)} ago
+    - 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_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

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"

changelogs/unreleased/add-pipeline-triggers.yml

@@ -0,0 +1,4 @@
+---
+title: Add pipeline trigger API with user permissions
+merge_request: 9277
+author:

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

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
 
-    it 'contains trigger token' do
-      expect(page).to have_content(@project.triggers.first.token)
+    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
 
-    it 'revokes the trigger' do
-      click_on 'Revoke'
-      expect(@project.triggers.count).to eq(0)
+    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
+
+    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')
+
+      # 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

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

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