Commit 6c83592e authored by Yorick Peterse's avatar Yorick Peterse

Merge branch 'security-pipeline-trigger-tokens-exposure' into 'master'

[master] Do not expose trigger token when user should not see it

See merge request gitlab/gitlabhq!2735
parents ab04f05c 63e5a314
...@@ -99,7 +99,9 @@ module Projects ...@@ -99,7 +99,9 @@ module Projects
def define_triggers_variables def define_triggers_variables
@triggers = @project.triggers @triggers = @project.triggers
.present(current_user: current_user)
@trigger = ::Ci::Trigger.new @trigger = ::Ci::Trigger.new
.present(current_user: current_user)
end end
def define_badges_variables def define_badges_variables
......
...@@ -66,12 +66,11 @@ class Projects::TriggersController < Projects::ApplicationController ...@@ -66,12 +66,11 @@ class Projects::TriggersController < Projects::ApplicationController
end end
def trigger def trigger
@trigger ||= project.triggers.find(params[:id]) || render_404 @trigger ||= project.triggers.find(params[:id])
.present(current_user: current_user)
end end
def trigger_params def trigger_params
params.require(:trigger).permit( params.require(:trigger).permit(:description)
:description
)
end end
end end
...@@ -4,6 +4,7 @@ module Ci ...@@ -4,6 +4,7 @@ module Ci
class Trigger < ActiveRecord::Base class Trigger < ActiveRecord::Base
extend Gitlab::Ci::Model extend Gitlab::Ci::Model
include IgnorableColumn include IgnorableColumn
include Presentable
ignore_column :deleted_at ignore_column :deleted_at
...@@ -29,7 +30,7 @@ module Ci ...@@ -29,7 +30,7 @@ module Ci
end end
def short_token def short_token
token[0...4] token[0...4] if token.present?
end end
def legacy? def legacy?
......
# 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
%tr %tr
%td %td
- if can?(current_user, :admin_trigger, trigger) - if trigger.has_token_exposed?
%span= trigger.token %span= trigger.token
= clipboard_button(text: trigger.token, title: "Copy trigger token to clipboard") = clipboard_button(text: trigger.token, title: "Copy trigger token to clipboard")
- else - else
......
---
title: Expose CI/CD trigger token only to the trigger owner
merge_request:
author:
type: security
...@@ -1223,8 +1223,11 @@ module API ...@@ -1223,8 +1223,11 @@ module API
end end
class Trigger < Grape::Entity class Trigger < Grape::Entity
include ::API::Helpers::Presentable
expose :id expose :id
expose :token, :description expose :token
expose :description
expose :created_at, :updated_at, :last_used expose :created_at, :updated_at, :last_used
expose :owner, using: Entities::UserBasic expose :owner, using: Entities::UserBasic
end end
......
# 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
...@@ -51,7 +51,7 @@ module API ...@@ -51,7 +51,7 @@ module API
triggers = user_project.triggers.includes(:trigger_requests) triggers = user_project.triggers.includes(:trigger_requests)
present paginate(triggers), with: Entities::Trigger present paginate(triggers), with: Entities::Trigger, current_user: current_user
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
...@@ -68,7 +68,7 @@ module API ...@@ -68,7 +68,7 @@ module API
trigger = user_project.triggers.find(params.delete(:trigger_id)) trigger = user_project.triggers.find(params.delete(:trigger_id))
break not_found!('Trigger') unless trigger break not_found!('Trigger') unless trigger
present trigger, with: Entities::Trigger present trigger, with: Entities::Trigger, current_user: current_user
end end
desc 'Create a trigger' do desc 'Create a trigger' do
...@@ -85,7 +85,7 @@ module API ...@@ -85,7 +85,7 @@ module API
declared_params(include_missing: false).merge(owner: current_user)) declared_params(include_missing: false).merge(owner: current_user))
if trigger.valid? if trigger.valid?
present trigger, with: Entities::Trigger present trigger, with: Entities::Trigger, current_user: current_user
else else
render_validation_error!(trigger) render_validation_error!(trigger)
end end
...@@ -106,7 +106,7 @@ module API ...@@ -106,7 +106,7 @@ module API
break not_found!('Trigger') unless trigger break not_found!('Trigger') unless trigger
if trigger.update(declared_params(include_missing: false)) if trigger.update(declared_params(include_missing: false))
present trigger, with: Entities::Trigger present trigger, with: Entities::Trigger, current_user: current_user
else else
render_validation_error!(trigger) render_validation_error!(trigger)
end end
...@@ -127,7 +127,7 @@ module API ...@@ -127,7 +127,7 @@ module API
if trigger.update(owner: current_user) if trigger.update(owner: current_user)
status :ok status :ok
present trigger, with: Entities::Trigger present trigger, with: Entities::Trigger, current_user: current_user
else else
render_validation_error!(trigger) render_validation_error!(trigger)
end end
......
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
require 'spec_helper' require 'spec_helper'
describe API::Triggers do describe API::Triggers do
let(:user) { create(:user) } set(:user) { create(:user) }
let(:user2) { create(:user) } set(:user2) { create(:user) }
let!(:trigger_token) { 'secure_token' } let!(:trigger_token) { 'secure_token' }
let!(:trigger_token_2) { 'secure_token_2' } let!(:trigger_token_2) { 'secure_token_2' }
let!(:project) { create(:project, :repository, creator: user) } let!(:project) { create(:project, :repository, creator: user) }
...@@ -132,14 +133,17 @@ describe API::Triggers do ...@@ -132,14 +133,17 @@ describe API::Triggers do
end end
describe 'GET /projects/:id/triggers' do describe 'GET /projects/:id/triggers' do
context 'authenticated user with valid permissions' do context 'authenticated user who can access triggers' do
it 'returns list of triggers' do it 'returns a list of triggers with tokens exposed correctly' do
get api("/projects/#{project.id}/triggers", user) get api("/projects/#{project.id}/triggers", user)
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers expect(response).to include_pagination_headers
expect(json_response).to be_a(Array) expect(json_response).to be_a(Array)
expect(json_response[0]).to have_key('token') 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]
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment