From b278d886ba65e2d3d438352b6243cd33b1ba4636 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 9 Nov 2018 22:17:43 +0100 Subject: Support both ref and ref-name in protected_for? --- app/models/project.rb | 27 +++++++++++++++--- spec/models/project_spec.rb | 68 ++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 87 insertions(+), 8 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index f5dc58cd67f..61840c972ee 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -36,6 +36,7 @@ class Project < ActiveRecord::Base extend Gitlab::ConfigHelper BoardLimitExceeded = Class.new(StandardError) + AmbiguousRef = Class.new(StandardError) STATISTICS_ATTRIBUTE = 'repositories_count'.freeze NUMBER_OF_PERMITTED_BOARDS = 1 @@ -1160,6 +1161,21 @@ class Project < ActiveRecord::Base end end + def resolve_ref(ref) + tag_exists = repository.tag_exists?(ref) + branch_exists = repository.branch_exists?(ref) + + if tag_exists && branch_exists + raise AmbiguousRef + elsif tag_exists + Gitlab::Git::TAG_REF_PREFIX + ref + elsif branch_exists + Gitlab::Git::BRANCH_REF_PREFIX + ref + else + ref + end + end + def root_ref?(branch) repository.root_ref == branch end @@ -1737,10 +1753,13 @@ class Project < ActiveRecord::Base end def protected_for?(ref) - if repository.branch_exists?(ref) - ProtectedBranch.protected?(self, ref) - elsif repository.tag_exists?(ref) - ProtectedTag.protected?(self, ref) + full_ref = resolve_ref(ref) + ref_name = Gitlab::Git.ref_name(full_ref) + + if Gitlab::Git.branch_ref?(full_ref) + ProtectedBranch.protected?(self, ref_name) + elsif Gitlab::Git.tag_ref?(full_ref) + ProtectedTag.protected?(self, ref_name) end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 1c85411dc3b..a519435322c 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2520,6 +2520,10 @@ describe Project do end context 'when the ref is not protected' do + before do + allow(project).to receive(:protected_for?).with('ref').and_return(false) + end + it 'contains only the CI variables' do is_expected.to contain_exactly(ci_variable) end @@ -2563,7 +2567,13 @@ describe Project do subject { project.protected_for?('ref') } + before do + allow(project).to receive(:resolve_ref).and_return(ref) + end + context 'when the ref is not protected' do + let(:ref) { 'refs/heads/ref' } + before do stub_application_setting( default_branch_protection: Gitlab::Access::PROTECTION_NONE) @@ -2575,9 +2585,9 @@ describe Project do end context 'when the ref is a protected branch' do + let(:ref) { 'refs/heads/ref' } + before do - allow(project).to receive(:repository).and_call_original - allow(project).to receive_message_chain(:repository, :branch_exists?).and_return(true) create(:protected_branch, name: 'ref', project: project) end @@ -2587,9 +2597,9 @@ describe Project do end context 'when the ref is a protected tag' do + let(:ref) { 'refs/tags/ref' } + before do - allow(project).to receive_message_chain(:repository, :branch_exists?).and_return(false) - allow(project).to receive_message_chain(:repository, :tag_exists?).and_return(true) create(:protected_tag, name: 'ref', project: project) end @@ -2778,6 +2788,56 @@ describe Project do end end + describe '#resolve_ref' do + let(:project) { create(:project) } + + subject { project.resolve_ref(ref) } + + context 'when ref is full ref' do + let(:ref) { 'refs/heads/master' } + + it 'returns the unchanged ref' do + is_expected.to eq(ref) + end + end + + context 'when ref is a tag or branch name' do + let(:ref) { 'ref' } + + before do + allow(project.repository).to receive(:tag_exists?).and_return(tag_exists) + allow(project.repository).to receive(:branch_exists?).and_return(branch_exists) + end + + context 'when ref is ambiguous' do + let(:tag_exists) { true } + let(:branch_exists) { true } + + it 'raises an error' do + expect { subject }.to raise_error(described_class::AmbiguousRef) + end + end + + context 'when ref is tag name' do + let(:tag_exists) { true } + let(:branch_exists) { false } + + it 'returns full tag ref path' do + is_expected.to eq('refs/tags/ref') + end + end + + context 'when ref is branch name' do + let(:tag_exists) { false } + let(:branch_exists) { true } + + it 'returns full branch ref path' do + is_expected.to eq('refs/heads/ref') + end + end + end + end + describe '#http_url_to_repo' do let(:project) { create(:project) } -- cgit v1.2.1 From 1bf580688890a3b13e7ac0468f29108dafe08f9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 10 Nov 2018 14:34:53 +0100 Subject: Use full ref when possible to avoid ambiguity --- app/models/ci/build.rb | 5 +++-- app/models/ci/pipeline.rb | 3 ++- app/models/concerns/has_ref.rb | 21 +++++++++++++++++++++ lib/gitlab/ci/pipeline/chain/command.rb | 2 +- spec/models/ci/build_spec.rb | 12 ++++++++---- spec/models/ci/pipeline_spec.rb | 4 ++++ spec/models/concerns/has_ref_spec.rb | 31 +++++++++++++++++++++++++++++++ 7 files changed, 70 insertions(+), 8 deletions(-) create mode 100644 app/models/concerns/has_ref.rb create mode 100644 spec/models/concerns/has_ref_spec.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index d86a6eceb59..f5c21481f63 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -10,6 +10,7 @@ module Ci include Importable include Gitlab::Utils::StrongMemoize include Deployable + include HasRef belongs_to :project, inverse_of: :builds belongs_to :runner @@ -640,11 +641,11 @@ module Ci def secret_group_variables return [] unless project.group - project.group.ci_variables_for(ref, project) + project.group.ci_variables_for(git_ref, project) end def secret_project_variables(environment: persisted_environment) - project.ci_variables_for(ref: ref, environment: environment) + project.ci_variables_for(ref: git_ref, environment: environment) end def steps diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index d06022a0fb7..a1a11d2f7ab 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -11,6 +11,7 @@ module Ci include Gitlab::Utils::StrongMemoize include AtomicInternalId include EnumWithNil + include HasRef belongs_to :project, inverse_of: :all_pipelines belongs_to :user @@ -588,7 +589,7 @@ module Ci end def protected_ref? - strong_memoize(:protected_ref) { project.protected_for?(ref) } + strong_memoize(:protected_ref) { project.protected_for?(git_ref) } end def legacy_trigger diff --git a/app/models/concerns/has_ref.rb b/app/models/concerns/has_ref.rb new file mode 100644 index 00000000000..79816841f7f --- /dev/null +++ b/app/models/concerns/has_ref.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module HasRef + extend ActiveSupport::Concern + + def branch? + !tag? + end + + private + + def git_ref + if branch? + Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s + elsif tag? + Gitlab::Git::TAG_REF_PREFIX + ref.to_s + else + raise ArgumentError, 'Invalid pipeline type!' + end + end +end diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index 100b9521412..316c283d90b 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -54,7 +54,7 @@ module Gitlab def protected_ref? strong_memoize(:protected_ref) do - project.protected_for?(ref) + project.protected_for?(origin_ref) end end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 89f78f629d4..d4056b4f7f1 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2385,6 +2385,8 @@ describe Ci::Build do end context 'when protected variable is defined' do + let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref } + let(:protected_variable) do { key: 'PROTECTED_KEY', value: 'protected_value', public: false } end @@ -2397,7 +2399,7 @@ describe Ci::Build do context 'when the branch is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -2405,7 +2407,7 @@ describe Ci::Build do context 'when the tag is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -2430,6 +2432,8 @@ describe Ci::Build do end context 'when group protected variable is defined' do + let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref } + let(:protected_variable) do { key: 'PROTECTED_KEY', value: 'protected_value', public: false } end @@ -2442,7 +2446,7 @@ describe Ci::Build do context 'when the branch is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } @@ -2450,7 +2454,7 @@ describe Ci::Build do context 'when the tag is protected' do before do - allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true) + allow(build.project).to receive(:protected_for?).with(ref).and_return(true) end it { is_expected.to include(protected_variable) } diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index b67c6a4cffa..17f33785fda 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -397,6 +397,10 @@ describe Ci::Pipeline, :mailer do end describe '#protected_ref?' do + before do + pipeline.project = create(:project, :repository) + end + it 'delegates method to project' do expect(pipeline).not_to be_protected_ref end diff --git a/spec/models/concerns/has_ref_spec.rb b/spec/models/concerns/has_ref_spec.rb new file mode 100644 index 00000000000..ec3cf9a8580 --- /dev/null +++ b/spec/models/concerns/has_ref_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe HasRef do + describe '#branch?' do + let(:pipeline) { create(:ci_pipeline) } + + subject { pipeline.branch? } + + context 'is not a tag' do + before do + pipeline.tag = false + end + + it 'return true when tag is set to false' do + is_expected.to be_truthy + end + end + + context 'is not a tag' do + before do + pipeline.tag = true + end + + it 'return false when tag is set to true' do + is_expected.to be_falsey + end + end + end +end -- cgit v1.2.1 From 2edc02143b2d361275fb97ce2904a58e7dc8ff38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 10 Nov 2018 15:48:22 +0100 Subject: Prevent creating pipelines with ambiguous refs --- lib/gitlab/ci/pipeline/chain/build.rb | 1 - lib/gitlab/ci/pipeline/chain/command.rb | 6 ------ lib/gitlab/ci/pipeline/chain/populate.rb | 5 +++++ lib/gitlab/ci/pipeline/chain/validate/abilities.rb | 2 +- .../ci/pipeline/chain/validate/repository.rb | 6 ++++++ spec/lib/gitlab/ci/pipeline/chain/command_spec.rb | 22 ---------------------- spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb | 2 ++ .../ci/pipeline/chain/validate/repository_spec.rb | 21 +++++++++++++++++++++ 8 files changed, 35 insertions(+), 30 deletions(-) diff --git a/lib/gitlab/ci/pipeline/chain/build.rb b/lib/gitlab/ci/pipeline/chain/build.rb index d33d1edfe35..41632211374 100644 --- a/lib/gitlab/ci/pipeline/chain/build.rb +++ b/lib/gitlab/ci/pipeline/chain/build.rb @@ -17,7 +17,6 @@ module Gitlab user: @command.current_user, pipeline_schedule: @command.schedule, merge_request: @command.merge_request, - protected: @command.protected_ref?, variables_attributes: Array(@command.variables_attributes) ) diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index 316c283d90b..ee5022e47c4 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -51,12 +51,6 @@ module Gitlab def before_sha self[:before_sha] || checkout_sha || Gitlab::Git::BLANK_SHA end - - def protected_ref? - strong_memoize(:protected_ref) do - project.protected_for?(origin_ref) - end - end end end end diff --git a/lib/gitlab/ci/pipeline/chain/populate.rb b/lib/gitlab/ci/pipeline/chain/populate.rb index 633d3cd4f6b..45b4393ecf3 100644 --- a/lib/gitlab/ci/pipeline/chain/populate.rb +++ b/lib/gitlab/ci/pipeline/chain/populate.rb @@ -18,6 +18,11 @@ module Gitlab # @command.seeds_block&.call(pipeline) + ## + # Populate pipeline protected status + # + pipeline.protected = @command.project.protected_for?(@command.origin_ref) + ## # Populate pipeline with all stages, and stages with builds. # diff --git a/lib/gitlab/ci/pipeline/chain/validate/abilities.rb b/lib/gitlab/ci/pipeline/chain/validate/abilities.rb index ebd7e6e8289..e4979102fd9 100644 --- a/lib/gitlab/ci/pipeline/chain/validate/abilities.rb +++ b/lib/gitlab/ci/pipeline/chain/validate/abilities.rb @@ -31,7 +31,7 @@ module Gitlab if current_user allowed_to_create? else # legacy triggers don't have a corresponding user - !@command.protected_ref? + !@command.project.protected_for?(@command.origin_ref) end end diff --git a/lib/gitlab/ci/pipeline/chain/validate/repository.rb b/lib/gitlab/ci/pipeline/chain/validate/repository.rb index d88851d8245..3cec55cdb71 100644 --- a/lib/gitlab/ci/pipeline/chain/validate/repository.rb +++ b/lib/gitlab/ci/pipeline/chain/validate/repository.rb @@ -16,6 +16,12 @@ module Gitlab unless @command.sha return error('Commit not found') end + + begin + @command.project.resolve_ref(@command.origin_ref) + rescue Project::AmbiguousRef + return error('Ref is ambiguous') + end end def break? diff --git a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb index 75a177d2d1f..14b4fbd1f5a 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb @@ -160,26 +160,4 @@ describe Gitlab::Ci::Pipeline::Chain::Command do end end end - - describe '#protected_ref?' do - let(:command) { described_class.new(project: project, origin_ref: 'my-branch') } - - subject { command.protected_ref? } - - context 'when a ref is protected' do - before do - expect_any_instance_of(Project).to receive(:protected_for?).with('my-branch').and_return(true) - end - - it { is_expected.to eq(true) } - end - - context 'when a ref is unprotected' do - before do - expect_any_instance_of(Project).to receive(:protected_for?).with('my-branch').and_return(false) - end - - it { is_expected.to eq(false) } - end - end end diff --git a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb index 284aed91e29..1b014ecfaa4 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb @@ -14,6 +14,7 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do Gitlab::Ci::Pipeline::Chain::Command.new( project: project, current_user: user, + origin_ref: 'master', seeds_block: nil) end @@ -106,6 +107,7 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do Gitlab::Ci::Pipeline::Chain::Command.new( project: project, current_user: user, + origin_ref: 'master', seeds_block: seeds_block) end diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb index fb1b53fc55c..7b30a8381e9 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb @@ -42,6 +42,27 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Repository do end end + context 'when ref is ambiguous' do + let(:project) do + p = create(:project, :repository) + p.repository.add_tag(user, 'master', 'master') + p + end + let(:command) do + Gitlab::Ci::Pipeline::Chain::Command.new( + project: project, current_user: user, origin_ref: 'master') + end + + it 'breaks the chain' do + expect(step.break?).to be true + end + + it 'adds an error about missing ref' do + expect(pipeline.errors.to_a) + .to include 'Ref is ambiguous' + end + end + context 'when does not have existing SHA set' do let(:command) do Gitlab::Ci::Pipeline::Chain::Command.new( -- cgit v1.2.1 From 837ab8c5d83243db8efe1cec1c6e001d1cbeb43f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Thu, 15 Nov 2018 21:28:17 +0100 Subject: Make HasRef#git_ref public --- app/models/concerns/has_ref.rb | 4 ---- spec/models/concerns/has_ref_spec.rb | 28 ++++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/app/models/concerns/has_ref.rb b/app/models/concerns/has_ref.rb index 79816841f7f..d7089294efc 100644 --- a/app/models/concerns/has_ref.rb +++ b/app/models/concerns/has_ref.rb @@ -7,15 +7,11 @@ module HasRef !tag? end - private - def git_ref if branch? Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s elsif tag? Gitlab::Git::TAG_REF_PREFIX + ref.to_s - else - raise ArgumentError, 'Invalid pipeline type!' end end end diff --git a/spec/models/concerns/has_ref_spec.rb b/spec/models/concerns/has_ref_spec.rb index ec3cf9a8580..14db197dfe0 100644 --- a/spec/models/concerns/has_ref_spec.rb +++ b/spec/models/concerns/has_ref_spec.rb @@ -28,4 +28,32 @@ describe HasRef do end end end + + describe '#git_ref' do + subject { pipeline.git_ref } + + context 'when tag is true' do + let(:pipeline) { create(:ci_pipeline, tag: true) } + + it 'returns a tag ref' do + is_expected.to start_with(Gitlab::Git::TAG_REF_PREFIX) + end + end + + context 'when tag is false' do + let(:pipeline) { create(:ci_pipeline, tag: false) } + + it 'returns a branch ref' do + is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX) + end + end + + context 'when tag is nil' do + let(:pipeline) { create(:ci_pipeline, tag: nil) } + + it 'returns a branch ref' do + is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX) + end + end + end end -- cgit v1.2.1 From ce14c20a82af697b927666123443a25f19dab2ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Thu, 15 Nov 2018 21:56:13 +0100 Subject: Avoid using magic variable in spec --- spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb index 7b30a8381e9..a7cad423d09 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb @@ -44,9 +44,9 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Repository do context 'when ref is ambiguous' do let(:project) do - p = create(:project, :repository) - p.repository.add_tag(user, 'master', 'master') - p + create(:project, :repository).tap do |proj| + proj.repository.add_tag(user, 'master', 'master') + end end let(:command) do Gitlab::Ci::Pipeline::Chain::Command.new( -- cgit v1.2.1 From 855e7c32b9f3541fec085726d338802c8ca9b9f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Thu, 15 Nov 2018 22:54:21 +0100 Subject: Use Gitlab::Git::Ref in Project#resolve_ref Reworks Project#resolve_ref to return Gitlab::Git::Branch, Gitlab::Git::Tag or raise an AmbiguousRef error. --- app/models/project.rb | 11 ++++---- app/models/repository.rb | 10 +++++++ lib/gitlab/git/branch.rb | 4 +++ lib/gitlab/git/ref.rb | 4 +++ lib/gitlab/git/tag.rb | 4 +++ spec/lib/gitlab/git/branch_spec.rb | 12 +++++++++ spec/lib/gitlab/git/tag_spec.rb | 13 +++++++++ spec/models/project_spec.rb | 55 ++++++++++++++++++++++---------------- spec/models/repository_spec.rb | 28 +++++++++++++++++++ 9 files changed, 113 insertions(+), 28 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 61840c972ee..6893f76dda9 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1168,11 +1168,11 @@ class Project < ActiveRecord::Base if tag_exists && branch_exists raise AmbiguousRef elsif tag_exists - Gitlab::Git::TAG_REF_PREFIX + ref + repository.find_tag(ref) elsif branch_exists - Gitlab::Git::BRANCH_REF_PREFIX + ref + repository.find_branch(ref) else - ref + repository.find_ref(ref) end end @@ -1753,8 +1753,9 @@ class Project < ActiveRecord::Base end def protected_for?(ref) - full_ref = resolve_ref(ref) - ref_name = Gitlab::Git.ref_name(full_ref) + resolved_ref = resolve_ref(ref) + full_ref = resolved_ref.full_ref + ref_name = resolved_ref.name if Gitlab::Git.branch_ref?(full_ref) ProtectedBranch.protected?(self, ref_name) diff --git a/app/models/repository.rb b/app/models/repository.rb index 35dd120856d..10635eb0cf4 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -182,6 +182,16 @@ class Repository tags.find { |tag| tag.name == name } end + def find_ref(ref) + if Gitlab::Git.tag_ref?(ref) + find_tag(Gitlab::Git.ref_name(ref)) + elsif Gitlab::Git.branch_ref?(ref) + find_branch(Gitlab::Git.ref_name(ref)) + else + nil + end + end + def add_branch(user, branch_name, ref) branch = raw_repository.add_branch(branch_name, user: user, target: ref) diff --git a/lib/gitlab/git/branch.rb b/lib/gitlab/git/branch.rb index 9447cfa0fb6..d25eebd5f47 100644 --- a/lib/gitlab/git/branch.rb +++ b/lib/gitlab/git/branch.rb @@ -28,6 +28,10 @@ module Gitlab def state active? ? :active : :stale end + + def full_ref + Gitlab::Git::BRANCH_REF_PREFIX + name + end end end end diff --git a/lib/gitlab/git/ref.rb b/lib/gitlab/git/ref.rb index eec91194949..b9c8b7c08e9 100644 --- a/lib/gitlab/git/ref.rb +++ b/lib/gitlab/git/ref.rb @@ -39,6 +39,10 @@ module Gitlab nil end end + + def full_ref + raise NotImplementedError + end end end end diff --git a/lib/gitlab/git/tag.rb b/lib/gitlab/git/tag.rb index 23d989ff258..ec89bc4f7e6 100644 --- a/lib/gitlab/git/tag.rb +++ b/lib/gitlab/git/tag.rb @@ -62,6 +62,10 @@ module Gitlab encode! @message end + def full_ref + Gitlab::Git::TAG_REF_PREFIX + name + end + private def message_from_gitaly_tag diff --git a/spec/lib/gitlab/git/branch_spec.rb b/spec/lib/gitlab/git/branch_spec.rb index 0df282d0ae3..1b26b5ef7fd 100644 --- a/spec/lib/gitlab/git/branch_spec.rb +++ b/spec/lib/gitlab/git/branch_spec.rb @@ -124,6 +124,18 @@ describe Gitlab::Git::Branch, :seed_helper do it { expect(repository.branches.size).to eq(SeedRepo::Repo::BRANCHES.size) } + describe '#full_ref' do + subject do + described_class.new(repository, 'master', + repository.commit.sha, + repository.commit).full_ref + end + + it 'returns the full ref' do + is_expected.to eq('refs/heads/master') + end + end + def create_commit params[:message].delete!("\r") Rugged::Commit.create(rugged, params.merge(committer: committer.merge(time: Time.now))) diff --git a/spec/lib/gitlab/git/tag_spec.rb b/spec/lib/gitlab/git/tag_spec.rb index b51e3879f49..f5d0b6af6f0 100644 --- a/spec/lib/gitlab/git/tag_spec.rb +++ b/spec/lib/gitlab/git/tag_spec.rb @@ -69,4 +69,17 @@ describe Gitlab::Git::Tag, :seed_helper do end end end + + describe '#full_ref' do + subject do + described_class.new(repository, { name: 'master', + target: repository.commit.sha, + target_commit: repository.commit }) + .full_ref + end + + it 'returns the full ref' do + is_expected.to eq('refs/tags/master') + end + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index a519435322c..e2e8a76ab72 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2563,7 +2563,7 @@ describe Project do end describe '#protected_for?' do - let(:project) { create(:project) } + let(:project) { create(:project, :repository) } subject { project.protected_for?('ref') } @@ -2572,7 +2572,7 @@ describe Project do end context 'when the ref is not protected' do - let(:ref) { 'refs/heads/ref' } + let(:ref) { project.repository.find_branch('master') } before do stub_application_setting( @@ -2585,10 +2585,10 @@ describe Project do end context 'when the ref is a protected branch' do - let(:ref) { 'refs/heads/ref' } + let(:ref) { project.repository.find_branch('master') } before do - create(:protected_branch, name: 'ref', project: project) + create(:protected_branch, name: 'master', project: project) end it 'returns true' do @@ -2597,10 +2597,10 @@ describe Project do end context 'when the ref is a protected tag' do - let(:ref) { 'refs/tags/ref' } + let(:ref) { project.repository.find_tag('v1.0.0') } before do - create(:protected_tag, name: 'ref', project: project) + create(:protected_tag, name: 'v1.0.0', project: project) end it 'returns true' do @@ -2789,29 +2789,36 @@ describe Project do end describe '#resolve_ref' do - let(:project) { create(:project) } + let(:project) { create(:project, :repository) } subject { project.resolve_ref(ref) } context 'when ref is full ref' do - let(:ref) { 'refs/heads/master' } + context 'when ref exists' do + let(:ref) { 'refs/heads/master' } + + it 'returns the ref' do + is_expected.to be_a(Gitlab::Git::Ref) + end + end + + context 'when ref does not exist' do + let(:ref) { 'refs/tags/doesnotexist' } - it 'returns the unchanged ref' do - is_expected.to eq(ref) + it 'returns nil' do + is_expected.to eq(nil) + end end end context 'when ref is a tag or branch name' do let(:ref) { 'ref' } - before do - allow(project.repository).to receive(:tag_exists?).and_return(tag_exists) - allow(project.repository).to receive(:branch_exists?).and_return(branch_exists) - end - context 'when ref is ambiguous' do - let(:tag_exists) { true } - let(:branch_exists) { true } + before do + project.repository.add_tag(project.creator, ref, 'master') + project.repository.add_branch(project.creator, ref, 'master') + end it 'raises an error' do expect { subject }.to raise_error(described_class::AmbiguousRef) @@ -2819,20 +2826,22 @@ describe Project do end context 'when ref is tag name' do - let(:tag_exists) { true } - let(:branch_exists) { false } + before do + project.repository.add_tag(project.creator, ref, 'master') + end it 'returns full tag ref path' do - is_expected.to eq('refs/tags/ref') + is_expected.to be_a(Gitlab::Git::Tag) end end context 'when ref is branch name' do - let(:tag_exists) { false } - let(:branch_exists) { true } + before do + project.repository.add_branch(project.creator, ref, 'master') + end it 'returns full branch ref path' do - is_expected.to eq('refs/heads/ref') + is_expected.to be_a(Gitlab::Git::Branch) end end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index f09b4b67061..df0ab7bc0f4 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1699,6 +1699,34 @@ describe Repository do end end + describe '#find_ref' do + subject { repository.find_ref(ref) } + + context 'when ref is a tag' do + let(:ref) { 'refs/tags/v1.0.0' } + + it 'returns the tag' do + is_expected.to be_a(Gitlab::Git::Tag) + end + end + + context 'when ref is a branch' do + let(:ref) { 'refs/heads/master' } + + it 'returns the branch' do + is_expected.to be_a(Gitlab::Git::Branch) + end + end + + context 'when ref is invalid' do + let(:ref) { 'refs/notvalid/ref' } + + it 'returns nil' do + is_expected.to eq(nil) + end + end + end + describe '#avatar' do it 'returns nil if repo does not exist' do allow(repository).to receive(:root_ref).and_raise(Gitlab::Git::Repository::NoRepository) -- cgit v1.2.1 From b0b5924eb418851ddfab848ab16b6acac27d42e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Thu, 15 Nov 2018 23:31:02 +0100 Subject: Use nil instead of raising AmbiguousRef --- app/models/project.rb | 5 +++-- lib/gitlab/ci/pipeline/chain/validate/repository.rb | 4 +--- spec/models/project_spec.rb | 12 ++++++++++-- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 6893f76dda9..e1acfbe7770 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -36,7 +36,6 @@ class Project < ActiveRecord::Base extend Gitlab::ConfigHelper BoardLimitExceeded = Class.new(StandardError) - AmbiguousRef = Class.new(StandardError) STATISTICS_ATTRIBUTE = 'repositories_count'.freeze NUMBER_OF_PERMITTED_BOARDS = 1 @@ -1166,7 +1165,7 @@ class Project < ActiveRecord::Base branch_exists = repository.branch_exists?(ref) if tag_exists && branch_exists - raise AmbiguousRef + nil elsif tag_exists repository.find_tag(ref) elsif branch_exists @@ -1754,6 +1753,8 @@ class Project < ActiveRecord::Base def protected_for?(ref) resolved_ref = resolve_ref(ref) + return false unless resolved_ref + full_ref = resolved_ref.full_ref ref_name = resolved_ref.name diff --git a/lib/gitlab/ci/pipeline/chain/validate/repository.rb b/lib/gitlab/ci/pipeline/chain/validate/repository.rb index 3cec55cdb71..0b411422264 100644 --- a/lib/gitlab/ci/pipeline/chain/validate/repository.rb +++ b/lib/gitlab/ci/pipeline/chain/validate/repository.rb @@ -17,9 +17,7 @@ module Gitlab return error('Commit not found') end - begin - @command.project.resolve_ref(@command.origin_ref) - rescue Project::AmbiguousRef + unless @command.project.resolve_ref(@command.origin_ref) return error('Ref is ambiguous') end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index e2e8a76ab72..48cf693b678 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2571,6 +2571,14 @@ describe Project do allow(project).to receive(:resolve_ref).and_return(ref) end + context 'when ref is ambiguous' do + let(:ref) { nil } + + it 'returns false' do + is_expected.to be_falsey + end + end + context 'when the ref is not protected' do let(:ref) { project.repository.find_branch('master') } @@ -2820,8 +2828,8 @@ describe Project do project.repository.add_branch(project.creator, ref, 'master') end - it 'raises an error' do - expect { subject }.to raise_error(described_class::AmbiguousRef) + it 'returns nil' do + is_expected.to eq(nil) end end -- cgit v1.2.1 From b6c8d3ac9f3031f6174dde2f11b3876ab4ac8a20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Thu, 15 Nov 2018 23:38:16 +0100 Subject: Reintroduce Command#protected_ref? --- lib/gitlab/ci/pipeline/chain/build.rb | 1 + lib/gitlab/ci/pipeline/chain/command.rb | 6 ++++++ lib/gitlab/ci/pipeline/chain/populate.rb | 5 ----- lib/gitlab/ci/pipeline/chain/validate/abilities.rb | 2 +- lib/gitlab/git.rb | 4 ++-- spec/lib/gitlab/ci/pipeline/chain/command_spec.rb | 22 ++++++++++++++++++++++ spec/lib/gitlab/ci/pipeline/seed/build_spec.rb | 3 ++- spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb | 3 ++- 8 files changed, 36 insertions(+), 10 deletions(-) diff --git a/lib/gitlab/ci/pipeline/chain/build.rb b/lib/gitlab/ci/pipeline/chain/build.rb index 41632211374..d33d1edfe35 100644 --- a/lib/gitlab/ci/pipeline/chain/build.rb +++ b/lib/gitlab/ci/pipeline/chain/build.rb @@ -17,6 +17,7 @@ module Gitlab user: @command.current_user, pipeline_schedule: @command.schedule, merge_request: @command.merge_request, + protected: @command.protected_ref?, variables_attributes: Array(@command.variables_attributes) ) diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index ee5022e47c4..316c283d90b 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -51,6 +51,12 @@ module Gitlab def before_sha self[:before_sha] || checkout_sha || Gitlab::Git::BLANK_SHA end + + def protected_ref? + strong_memoize(:protected_ref) do + project.protected_for?(origin_ref) + end + end end end end diff --git a/lib/gitlab/ci/pipeline/chain/populate.rb b/lib/gitlab/ci/pipeline/chain/populate.rb index 45b4393ecf3..633d3cd4f6b 100644 --- a/lib/gitlab/ci/pipeline/chain/populate.rb +++ b/lib/gitlab/ci/pipeline/chain/populate.rb @@ -18,11 +18,6 @@ module Gitlab # @command.seeds_block&.call(pipeline) - ## - # Populate pipeline protected status - # - pipeline.protected = @command.project.protected_for?(@command.origin_ref) - ## # Populate pipeline with all stages, and stages with builds. # diff --git a/lib/gitlab/ci/pipeline/chain/validate/abilities.rb b/lib/gitlab/ci/pipeline/chain/validate/abilities.rb index e4979102fd9..ebd7e6e8289 100644 --- a/lib/gitlab/ci/pipeline/chain/validate/abilities.rb +++ b/lib/gitlab/ci/pipeline/chain/validate/abilities.rb @@ -31,7 +31,7 @@ module Gitlab if current_user allowed_to_create? else # legacy triggers don't have a corresponding user - !@command.project.protected_for?(@command.origin_ref) + !@command.protected_ref? end end diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb index c4aac228b2f..2d950bb151c 100644 --- a/lib/gitlab/git.rb +++ b/lib/gitlab/git.rb @@ -54,11 +54,11 @@ module Gitlab end def tag_ref?(ref) - ref.start_with?(TAG_REF_PREFIX) + ref =~ %r{#{TAG_REF_PREFIX}\w+} end def branch_ref?(ref) - ref.start_with?(BRANCH_REF_PREFIX) + ref =~ %r{#{BRANCH_REF_PREFIX}\w+} end def blank_ref?(ref) diff --git a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb index 14b4fbd1f5a..75a177d2d1f 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb @@ -160,4 +160,26 @@ describe Gitlab::Ci::Pipeline::Chain::Command do end end end + + describe '#protected_ref?' do + let(:command) { described_class.new(project: project, origin_ref: 'my-branch') } + + subject { command.protected_ref? } + + context 'when a ref is protected' do + before do + expect_any_instance_of(Project).to receive(:protected_for?).with('my-branch').and_return(true) + end + + it { is_expected.to eq(true) } + end + + context 'when a ref is unprotected' do + before do + expect_any_instance_of(Project).to receive(:protected_for?).with('my-branch').and_return(false) + end + + it { is_expected.to eq(false) } + end + end end diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb index fffa727c2ed..2cf812b26dc 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe Gitlab::Ci::Pipeline::Seed::Build do - let(:pipeline) { create(:ci_empty_pipeline) } + let(:project) { create(:project, :repository) } + let(:pipeline) { create(:ci_empty_pipeline, project: project) } let(:attributes) do { name: 'rspec', diff --git a/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb index 05ce3412fd8..82f741845db 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe Gitlab::Ci::Pipeline::Seed::Stage do - let(:pipeline) { create(:ci_empty_pipeline) } + let(:project) { create(:project, :repository) } + let(:pipeline) { create(:ci_empty_pipeline, project: project) } let(:attributes) do { name: 'test', -- cgit v1.2.1 From 2b4883e05e59eff08088e378bf3061d9d8da13dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 16 Nov 2018 00:27:11 +0100 Subject: Check for Tag/Branch corectness --- spec/models/project_spec.rb | 14 +++++++++++--- spec/models/repository_spec.rb | 12 ++++++++++-- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 48cf693b678..204d611796b 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2805,7 +2805,7 @@ describe Project do context 'when ref exists' do let(:ref) { 'refs/heads/master' } - it 'returns the ref' do + it 'returns a ref' do is_expected.to be_a(Gitlab::Git::Ref) end end @@ -2838,9 +2838,13 @@ describe Project do project.repository.add_tag(project.creator, ref, 'master') end - it 'returns full tag ref path' do + it 'returns a tag' do is_expected.to be_a(Gitlab::Git::Tag) end + + it 'returns the correct tag' do + expect(subject.name).to eq(ref) + end end context 'when ref is branch name' do @@ -2848,9 +2852,13 @@ describe Project do project.repository.add_branch(project.creator, ref, 'master') end - it 'returns full branch ref path' do + it 'returns a branch' do is_expected.to be_a(Gitlab::Git::Branch) end + + it 'returns the correct branch' do + expect(subject.name).to eq(ref) + end end end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index df0ab7bc0f4..6f9fdcf4482 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1705,17 +1705,25 @@ describe Repository do context 'when ref is a tag' do let(:ref) { 'refs/tags/v1.0.0' } - it 'returns the tag' do + it 'returns a tag' do is_expected.to be_a(Gitlab::Git::Tag) end + + it 'returns the correct tag' do + expect(subject.name).to eq('v1.0.0') + end end context 'when ref is a branch' do let(:ref) { 'refs/heads/master' } - it 'returns the branch' do + it 'returns a branch' do is_expected.to be_a(Gitlab::Git::Branch) end + + it 'returns the correct branch' do + expect(subject.name).to eq('master') + end end context 'when ref is invalid' do -- cgit v1.2.1 From 38348aa1215daa2393b7d488c1ca8926d67dc09e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 16 Nov 2018 13:20:52 +0100 Subject: Remove Gitlab::Git::Ref#full_ref --- app/models/project.rb | 12 +++++------- lib/gitlab/git.rb | 4 ++-- lib/gitlab/git/branch.rb | 4 ---- lib/gitlab/git/ref.rb | 4 ---- lib/gitlab/git/tag.rb | 4 ---- spec/lib/gitlab/git/branch_spec.rb | 12 ------------ spec/lib/gitlab/git/tag_spec.rb | 13 ------------- 7 files changed, 7 insertions(+), 46 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index e1acfbe7770..22ce916a36c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1755,13 +1755,11 @@ class Project < ActiveRecord::Base resolved_ref = resolve_ref(ref) return false unless resolved_ref - full_ref = resolved_ref.full_ref - ref_name = resolved_ref.name - - if Gitlab::Git.branch_ref?(full_ref) - ProtectedBranch.protected?(self, ref_name) - elsif Gitlab::Git.tag_ref?(full_ref) - ProtectedTag.protected?(self, ref_name) + case resolved_ref + when Gitlab::Git::Branch + ProtectedBranch.protected?(self, resolved_ref.name) + when Gitlab::Git::Tag + ProtectedTag.protected?(self, resolved_ref.name) end end diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb index 2d950bb151c..f22ac800479 100644 --- a/lib/gitlab/git.rb +++ b/lib/gitlab/git.rb @@ -54,11 +54,11 @@ module Gitlab end def tag_ref?(ref) - ref =~ %r{#{TAG_REF_PREFIX}\w+} + ref =~ /#{TAG_REF_PREFIX}\w+/ end def branch_ref?(ref) - ref =~ %r{#{BRANCH_REF_PREFIX}\w+} + ref =~ /#{BRANCH_REF_PREFIX}\w+/ end def blank_ref?(ref) diff --git a/lib/gitlab/git/branch.rb b/lib/gitlab/git/branch.rb index d25eebd5f47..9447cfa0fb6 100644 --- a/lib/gitlab/git/branch.rb +++ b/lib/gitlab/git/branch.rb @@ -28,10 +28,6 @@ module Gitlab def state active? ? :active : :stale end - - def full_ref - Gitlab::Git::BRANCH_REF_PREFIX + name - end end end end diff --git a/lib/gitlab/git/ref.rb b/lib/gitlab/git/ref.rb index b9c8b7c08e9..eec91194949 100644 --- a/lib/gitlab/git/ref.rb +++ b/lib/gitlab/git/ref.rb @@ -39,10 +39,6 @@ module Gitlab nil end end - - def full_ref - raise NotImplementedError - end end end end diff --git a/lib/gitlab/git/tag.rb b/lib/gitlab/git/tag.rb index ec89bc4f7e6..23d989ff258 100644 --- a/lib/gitlab/git/tag.rb +++ b/lib/gitlab/git/tag.rb @@ -62,10 +62,6 @@ module Gitlab encode! @message end - def full_ref - Gitlab::Git::TAG_REF_PREFIX + name - end - private def message_from_gitaly_tag diff --git a/spec/lib/gitlab/git/branch_spec.rb b/spec/lib/gitlab/git/branch_spec.rb index 1b26b5ef7fd..0df282d0ae3 100644 --- a/spec/lib/gitlab/git/branch_spec.rb +++ b/spec/lib/gitlab/git/branch_spec.rb @@ -124,18 +124,6 @@ describe Gitlab::Git::Branch, :seed_helper do it { expect(repository.branches.size).to eq(SeedRepo::Repo::BRANCHES.size) } - describe '#full_ref' do - subject do - described_class.new(repository, 'master', - repository.commit.sha, - repository.commit).full_ref - end - - it 'returns the full ref' do - is_expected.to eq('refs/heads/master') - end - end - def create_commit params[:message].delete!("\r") Rugged::Commit.create(rugged, params.merge(committer: committer.merge(time: Time.now))) diff --git a/spec/lib/gitlab/git/tag_spec.rb b/spec/lib/gitlab/git/tag_spec.rb index f5d0b6af6f0..b51e3879f49 100644 --- a/spec/lib/gitlab/git/tag_spec.rb +++ b/spec/lib/gitlab/git/tag_spec.rb @@ -69,17 +69,4 @@ describe Gitlab::Git::Tag, :seed_helper do end end end - - describe '#full_ref' do - subject do - described_class.new(repository, { name: 'master', - target: repository.commit.sha, - target_commit: repository.commit }) - .full_ref - end - - it 'returns the full ref' do - is_expected.to eq('refs/tags/master') - end - end end -- cgit v1.2.1 From 35b3392ef7511521653a984a9d9c6dbaf4e6dccb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 16 Nov 2018 13:51:26 +0100 Subject: Allow any character in tag / branch ref regex --- lib/gitlab/git.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb index f22ac800479..81ec0e9f2db 100644 --- a/lib/gitlab/git.rb +++ b/lib/gitlab/git.rb @@ -54,11 +54,11 @@ module Gitlab end def tag_ref?(ref) - ref =~ /#{TAG_REF_PREFIX}\w+/ + ref =~ /#{TAG_REF_PREFIX}.+/ end def branch_ref?(ref) - ref =~ /#{BRANCH_REF_PREFIX}\w+/ + ref =~ /#{BRANCH_REF_PREFIX}.+/ end def blank_ref?(ref) -- cgit v1.2.1 From 96a0a8cfb62d9ea66ee2a22e09a116d64f333703 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 16 Nov 2018 21:03:21 +0100 Subject: Use strings instead of Gitlab::Git::Ref --- app/models/project.rb | 17 +++++++++-------- spec/models/project_spec.rb | 38 ++++++++++---------------------------- 2 files changed, 19 insertions(+), 36 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 22ce916a36c..e9fa9cfabc9 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1167,11 +1167,11 @@ class Project < ActiveRecord::Base if tag_exists && branch_exists nil elsif tag_exists - repository.find_tag(ref) + Gitlab::Git::TAG_REF_PREFIX + ref elsif branch_exists - repository.find_branch(ref) + Gitlab::Git::BRANCH_REF_PREFIX + ref else - repository.find_ref(ref) + ref end end @@ -1755,11 +1755,12 @@ class Project < ActiveRecord::Base resolved_ref = resolve_ref(ref) return false unless resolved_ref - case resolved_ref - when Gitlab::Git::Branch - ProtectedBranch.protected?(self, resolved_ref.name) - when Gitlab::Git::Tag - ProtectedTag.protected?(self, resolved_ref.name) + ref_name = Gitlab::Git.ref_name(resolved_ref) + + if Gitlab::Git.branch_ref?(resolved_ref) + ProtectedBranch.protected?(self, ref_name) + elsif Gitlab::Git.tag_ref?(resolved_ref) + ProtectedTag.protected?(self, ref_name) end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 204d611796b..cbc242308e8 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2580,7 +2580,7 @@ describe Project do end context 'when the ref is not protected' do - let(:ref) { project.repository.find_branch('master') } + let(:ref) { 'refs/heads/master' } before do stub_application_setting( @@ -2593,7 +2593,7 @@ describe Project do end context 'when the ref is a protected branch' do - let(:ref) { project.repository.find_branch('master') } + let(:ref) { 'refs/heads/master' } before do create(:protected_branch, name: 'master', project: project) @@ -2605,7 +2605,7 @@ describe Project do end context 'when the ref is a protected tag' do - let(:ref) { project.repository.find_tag('v1.0.0') } + let(:ref) { 'refs/tags/v1.0.0' } before do create(:protected_tag, name: 'v1.0.0', project: project) @@ -2802,20 +2802,10 @@ describe Project do subject { project.resolve_ref(ref) } context 'when ref is full ref' do - context 'when ref exists' do - let(:ref) { 'refs/heads/master' } + let(:ref) { 'refs/heads/master' } - it 'returns a ref' do - is_expected.to be_a(Gitlab::Git::Ref) - end - end - - context 'when ref does not exist' do - let(:ref) { 'refs/tags/doesnotexist' } - - it 'returns nil' do - is_expected.to eq(nil) - end + it 'returns the ref' do + is_expected.to eq(ref) end end @@ -2838,12 +2828,8 @@ describe Project do project.repository.add_tag(project.creator, ref, 'master') end - it 'returns a tag' do - is_expected.to be_a(Gitlab::Git::Tag) - end - - it 'returns the correct tag' do - expect(subject.name).to eq(ref) + it 'returns the tag ref' do + is_expected.to eq("refs/tags/#{ref}") end end @@ -2852,12 +2838,8 @@ describe Project do project.repository.add_branch(project.creator, ref, 'master') end - it 'returns a branch' do - is_expected.to be_a(Gitlab::Git::Branch) - end - - it 'returns the correct branch' do - expect(subject.name).to eq(ref) + it 'returns the branch ref' do + is_expected.to eq("refs/heads/#{ref}") end end end -- cgit v1.2.1 From e0b5306cb79c7a535f96c0d2d864278b2193d641 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 16 Nov 2018 21:06:58 +0100 Subject: Remove Repository#find_ref --- app/models/repository.rb | 10 ---------- spec/models/repository_spec.rb | 36 ------------------------------------ 2 files changed, 46 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 10635eb0cf4..35dd120856d 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -182,16 +182,6 @@ class Repository tags.find { |tag| tag.name == name } end - def find_ref(ref) - if Gitlab::Git.tag_ref?(ref) - find_tag(Gitlab::Git.ref_name(ref)) - elsif Gitlab::Git.branch_ref?(ref) - find_branch(Gitlab::Git.ref_name(ref)) - else - nil - end - end - def add_branch(user, branch_name, ref) branch = raw_repository.add_branch(branch_name, user: user, target: ref) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 6f9fdcf4482..f09b4b67061 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1699,42 +1699,6 @@ describe Repository do end end - describe '#find_ref' do - subject { repository.find_ref(ref) } - - context 'when ref is a tag' do - let(:ref) { 'refs/tags/v1.0.0' } - - it 'returns a tag' do - is_expected.to be_a(Gitlab::Git::Tag) - end - - it 'returns the correct tag' do - expect(subject.name).to eq('v1.0.0') - end - end - - context 'when ref is a branch' do - let(:ref) { 'refs/heads/master' } - - it 'returns a branch' do - is_expected.to be_a(Gitlab::Git::Branch) - end - - it 'returns the correct branch' do - expect(subject.name).to eq('master') - end - end - - context 'when ref is invalid' do - let(:ref) { 'refs/notvalid/ref' } - - it 'returns nil' do - is_expected.to eq(nil) - end - end - end - describe '#avatar' do it 'returns nil if repo does not exist' do allow(repository).to receive(:root_ref).and_raise(Gitlab::Git::Repository::NoRepository) -- cgit v1.2.1 From d12bc3bfc47f9a4252e3f9eb60b1326eade7d3e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 20 Nov 2018 14:56:43 +0100 Subject: Search for tag / branch ref from beginning --- lib/gitlab/git.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb index 81ec0e9f2db..44a62586a23 100644 --- a/lib/gitlab/git.rb +++ b/lib/gitlab/git.rb @@ -54,11 +54,11 @@ module Gitlab end def tag_ref?(ref) - ref =~ /#{TAG_REF_PREFIX}.+/ + ref =~ /^#{TAG_REF_PREFIX}.+/ end def branch_ref?(ref) - ref =~ /#{BRANCH_REF_PREFIX}.+/ + ref =~ /^#{BRANCH_REF_PREFIX}.+/ end def blank_ref?(ref) -- cgit v1.2.1 From 24d682254a0ae68dfc605fc077c6a7610b97994a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 20 Nov 2018 15:07:25 +0100 Subject: Move Project#resolve_ref to Repository --- app/models/project.rb | 17 +------- app/models/repository.rb | 15 +++++++ .../ci/pipeline/chain/validate/repository.rb | 2 +- spec/models/project_spec.rb | 51 +--------------------- spec/models/repository_spec.rb | 47 ++++++++++++++++++++ 5 files changed, 65 insertions(+), 67 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index e9fa9cfabc9..2ba273c6f98 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1160,21 +1160,6 @@ class Project < ActiveRecord::Base end end - def resolve_ref(ref) - tag_exists = repository.tag_exists?(ref) - branch_exists = repository.branch_exists?(ref) - - if tag_exists && branch_exists - nil - elsif tag_exists - Gitlab::Git::TAG_REF_PREFIX + ref - elsif branch_exists - Gitlab::Git::BRANCH_REF_PREFIX + ref - else - ref - end - end - def root_ref?(branch) repository.root_ref == branch end @@ -1752,7 +1737,7 @@ class Project < ActiveRecord::Base end def protected_for?(ref) - resolved_ref = resolve_ref(ref) + resolved_ref = repository.resolve_ref(ref) return false unless resolved_ref ref_name = Gitlab::Git.ref_name(resolved_ref) diff --git a/app/models/repository.rb b/app/models/repository.rb index 35dd120856d..221d01c5b44 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -182,6 +182,21 @@ class Repository tags.find { |tag| tag.name == name } end + def resolve_ref(ref) + tag_exists = tag_exists?(ref) + branch_exists = branch_exists?(ref) + + if tag_exists && branch_exists + nil + elsif tag_exists + Gitlab::Git::TAG_REF_PREFIX + ref + elsif branch_exists + Gitlab::Git::BRANCH_REF_PREFIX + ref + else + ref + end + end + def add_branch(user, branch_name, ref) branch = raw_repository.add_branch(branch_name, user: user, target: ref) diff --git a/lib/gitlab/ci/pipeline/chain/validate/repository.rb b/lib/gitlab/ci/pipeline/chain/validate/repository.rb index 0b411422264..6e95c0717c6 100644 --- a/lib/gitlab/ci/pipeline/chain/validate/repository.rb +++ b/lib/gitlab/ci/pipeline/chain/validate/repository.rb @@ -17,7 +17,7 @@ module Gitlab return error('Commit not found') end - unless @command.project.resolve_ref(@command.origin_ref) + unless @command.project.repository.resolve_ref(@command.origin_ref) return error('Ref is ambiguous') end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index cbc242308e8..25560ddea8d 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2568,7 +2568,7 @@ describe Project do subject { project.protected_for?('ref') } before do - allow(project).to receive(:resolve_ref).and_return(ref) + allow(project.repository).to receive(:resolve_ref).and_return(ref) end context 'when ref is ambiguous' do @@ -2796,55 +2796,6 @@ describe Project do end end - describe '#resolve_ref' do - let(:project) { create(:project, :repository) } - - subject { project.resolve_ref(ref) } - - context 'when ref is full ref' do - let(:ref) { 'refs/heads/master' } - - it 'returns the ref' do - is_expected.to eq(ref) - end - end - - context 'when ref is a tag or branch name' do - let(:ref) { 'ref' } - - context 'when ref is ambiguous' do - before do - project.repository.add_tag(project.creator, ref, 'master') - project.repository.add_branch(project.creator, ref, 'master') - end - - it 'returns nil' do - is_expected.to eq(nil) - end - end - - context 'when ref is tag name' do - before do - project.repository.add_tag(project.creator, ref, 'master') - end - - it 'returns the tag ref' do - is_expected.to eq("refs/tags/#{ref}") - end - end - - context 'when ref is branch name' do - before do - project.repository.add_branch(project.creator, ref, 'master') - end - - it 'returns the branch ref' do - is_expected.to eq("refs/heads/#{ref}") - end - end - end - end - describe '#http_url_to_repo' do let(:project) { create(:project) } diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index f09b4b67061..3d6cf2cbc19 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1005,6 +1005,53 @@ describe Repository do end end + describe '#resolve_ref' do + subject { repository.resolve_ref(ref) } + + context 'when ref is full ref' do + let(:ref) { 'refs/heads/master' } + + it 'returns the ref' do + is_expected.to eq(ref) + end + end + + context 'when ref is a tag or branch name' do + let(:ref) { 'ref' } + + context 'when ref is ambiguous' do + before do + repository.add_tag(project.creator, ref, 'master') + repository.add_branch(project.creator, ref, 'master') + end + + it 'returns nil' do + is_expected.to eq(nil) + end + end + + context 'when ref is tag name' do + before do + repository.add_tag(project.creator, ref, 'master') + end + + it 'returns the tag ref' do + is_expected.to eq("refs/tags/#{ref}") + end + end + + context 'when ref is branch name' do + before do + repository.add_branch(project.creator, ref, 'master') + end + + it 'returns the branch ref' do + is_expected.to eq("refs/heads/#{ref}") + end + end + end + end + describe '#add_branch' do let(:branch_name) { 'new_feature' } let(:target) { 'master' } -- cgit v1.2.1 From d307dccbc60f088496fb727f40530ba3003b61fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Thu, 22 Nov 2018 14:35:06 +0100 Subject: Use to_s.start_with? in tag/branch ref method --- lib/gitlab/git.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb index 44a62586a23..7c590846f74 100644 --- a/lib/gitlab/git.rb +++ b/lib/gitlab/git.rb @@ -54,11 +54,11 @@ module Gitlab end def tag_ref?(ref) - ref =~ /^#{TAG_REF_PREFIX}.+/ + ref.to_s.start_with?(TAG_REF_PREFIX) end def branch_ref?(ref) - ref =~ /^#{BRANCH_REF_PREFIX}.+/ + ref.to_s.start_with?(BRANCH_REF_PREFIX) end def blank_ref?(ref) -- cgit v1.2.1 From 350ea9c55ab6b980ac5ec74d2a34b9cbac915e20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 23 Nov 2018 00:11:42 +0100 Subject: Implement Repository#ambiguous_ref? This implements Repository#ambiguous_ref? and checks if a ref is ambiguous before trying to resolve the ref in Project#protected_for? --- app/models/project.rb | 6 +- app/models/repository.rb | 13 ++--- .../ci/pipeline/chain/validate/repository.rb | 2 +- spec/models/project_spec.rb | 23 +++++--- spec/models/repository_spec.rb | 68 +++++++++++++--------- 5 files changed, 67 insertions(+), 45 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 2ba273c6f98..800c8afb49f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1737,10 +1737,10 @@ class Project < ActiveRecord::Base end def protected_for?(ref) - resolved_ref = repository.resolve_ref(ref) - return false unless resolved_ref + return false unless ref && !repository.ambiguous_ref?(ref) - ref_name = Gitlab::Git.ref_name(resolved_ref) + resolved_ref = repository.resolve_ref(ref) + ref_name = resolved_ref == ref ? Gitlab::Git.ref_name(resolved_ref) : ref if Gitlab::Git.branch_ref?(resolved_ref) ProtectedBranch.protected?(self, ref_name) diff --git a/app/models/repository.rb b/app/models/repository.rb index 221d01c5b44..9e01f7f0df5 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -182,15 +182,14 @@ class Repository tags.find { |tag| tag.name == name } end - def resolve_ref(ref) - tag_exists = tag_exists?(ref) - branch_exists = branch_exists?(ref) + def ambiguous_ref?(ref) + tag_exists?(ref) && branch_exists?(ref) + end - if tag_exists && branch_exists - nil - elsif tag_exists + def resolve_ref(ref) + if tag_exists?(ref) Gitlab::Git::TAG_REF_PREFIX + ref - elsif branch_exists + elsif branch_exists?(ref) Gitlab::Git::BRANCH_REF_PREFIX + ref else ref diff --git a/lib/gitlab/ci/pipeline/chain/validate/repository.rb b/lib/gitlab/ci/pipeline/chain/validate/repository.rb index 6e95c0717c6..09b3187569c 100644 --- a/lib/gitlab/ci/pipeline/chain/validate/repository.rb +++ b/lib/gitlab/ci/pipeline/chain/validate/repository.rb @@ -17,7 +17,7 @@ module Gitlab return error('Commit not found') end - unless @command.project.repository.resolve_ref(@command.origin_ref) + if @command.project.repository.ambiguous_ref?(@command.origin_ref) return error('Ref is ambiguous') end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 25560ddea8d..18ad69a1bda 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2565,14 +2565,23 @@ describe Project do describe '#protected_for?' do let(:project) { create(:project, :repository) } - subject { project.protected_for?('ref') } + subject { project.protected_for?(ref) } - before do - allow(project.repository).to receive(:resolve_ref).and_return(ref) + context 'when ref is nil' do + let(:ref) { nil } + + it 'returns false' do + is_expected.to be_falsey + end end context 'when ref is ambiguous' do - let(:ref) { nil } + let(:ref) { 'ref' } + + before do + project.repository.add_branch(project.creator, ref, 'master') + project.repository.add_tag(project.creator, ref, 'master') + end it 'returns false' do is_expected.to be_falsey @@ -2580,7 +2589,7 @@ describe Project do end context 'when the ref is not protected' do - let(:ref) { 'refs/heads/master' } + let(:ref) { 'master' } before do stub_application_setting( @@ -2593,7 +2602,7 @@ describe Project do end context 'when the ref is a protected branch' do - let(:ref) { 'refs/heads/master' } + let(:ref) { 'master' } before do create(:protected_branch, name: 'master', project: project) @@ -2605,7 +2614,7 @@ describe Project do end context 'when the ref is a protected tag' do - let(:ref) { 'refs/tags/v1.0.0' } + let(:ref) { 'v1.0.0' } before do create(:protected_tag, name: 'v1.0.0', project: project) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 3d6cf2cbc19..cdececd49e0 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1005,7 +1005,36 @@ describe Repository do end end + describe '#ambiguous_ref?' do + let(:ref) { 'ref' } + + subject { repository.ambiguous_ref?(ref) } + + context 'when ref is ambiguous' do + before do + repository.add_tag(project.creator, ref, 'master') + repository.add_branch(project.creator, ref, 'master') + end + + it 'should be true' do + is_expected.to eq(true) + end + end + + context 'when ref is not ambiguous' do + before do + repository.add_tag(project.creator, ref, 'master') + end + + it 'should be false' do + is_expected.to eq(false) + end + end + end + describe '#resolve_ref' do + let(:ref) { 'ref' } + subject { repository.resolve_ref(ref) } context 'when ref is full ref' do @@ -1016,38 +1045,23 @@ describe Repository do end end - context 'when ref is a tag or branch name' do - let(:ref) { 'ref' } - - context 'when ref is ambiguous' do - before do - repository.add_tag(project.creator, ref, 'master') - repository.add_branch(project.creator, ref, 'master') - end - - it 'returns nil' do - is_expected.to eq(nil) - end + context 'when ref is tag name' do + before do + repository.add_tag(project.creator, ref, 'master') end - context 'when ref is tag name' do - before do - repository.add_tag(project.creator, ref, 'master') - end - - it 'returns the tag ref' do - is_expected.to eq("refs/tags/#{ref}") - end + it 'returns the tag ref' do + is_expected.to eq("refs/tags/#{ref}") end + end - context 'when ref is branch name' do - before do - repository.add_branch(project.creator, ref, 'master') - end + context 'when ref is branch name' do + before do + repository.add_branch(project.creator, ref, 'master') + end - it 'returns the branch ref' do - is_expected.to eq("refs/heads/#{ref}") - end + it 'returns the branch ref' do + is_expected.to eq("refs/heads/#{ref}") end end end -- cgit v1.2.1 From d27de096c2d889be69b8e5ac82284bee8034d158 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 23 Nov 2018 00:21:29 +0100 Subject: Revert "Use to_s.start_with? in tag/branch ref method" This reverts commit ec4730478b798270781257913ee4cede673d4d4e. --- lib/gitlab/git.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb index 7c590846f74..44a62586a23 100644 --- a/lib/gitlab/git.rb +++ b/lib/gitlab/git.rb @@ -54,11 +54,11 @@ module Gitlab end def tag_ref?(ref) - ref.to_s.start_with?(TAG_REF_PREFIX) + ref =~ /^#{TAG_REF_PREFIX}.+/ end def branch_ref?(ref) - ref.to_s.start_with?(BRANCH_REF_PREFIX) + ref =~ /^#{BRANCH_REF_PREFIX}.+/ end def blank_ref?(ref) -- cgit v1.2.1 From 9f6d228d2368911befc11038d64ccb9ccae131ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 26 Nov 2018 15:04:51 +0100 Subject: Simplify conditionals in Project#protected_ref? --- app/models/project.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 800c8afb49f..fa1820b2bdb 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1737,10 +1737,14 @@ class Project < ActiveRecord::Base end def protected_for?(ref) - return false unless ref && !repository.ambiguous_ref?(ref) + return false if ref.nil? || repository.ambiguous_ref?(ref) resolved_ref = repository.resolve_ref(ref) - ref_name = resolved_ref == ref ? Gitlab::Git.ref_name(resolved_ref) : ref + ref_name = if resolved_ref == ref + Gitlab::Git.ref_name(resolved_ref) + else + ref + end if Gitlab::Git.branch_ref?(resolved_ref) ProtectedBranch.protected?(self, ref_name) -- cgit v1.2.1 From a1be58097943f7f568de6a90aa12cf7452ac2f0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 26 Nov 2018 15:43:46 +0100 Subject: Implement Command#ambiguous_ref? --- lib/gitlab/ci/pipeline/chain/command.rb | 6 ++++++ lib/gitlab/ci/pipeline/chain/validate/repository.rb | 2 +- spec/lib/gitlab/ci/pipeline/chain/command_spec.rb | 20 ++++++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index 316c283d90b..90208352c55 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -57,6 +57,12 @@ module Gitlab project.protected_for?(origin_ref) end end + + def ambiguous_ref? + strong_memoize(:ambiguous_ref) do + project.repository.ambiguous_ref?(origin_ref) + end + end end end end diff --git a/lib/gitlab/ci/pipeline/chain/validate/repository.rb b/lib/gitlab/ci/pipeline/chain/validate/repository.rb index 09b3187569c..9c6c2bc8e25 100644 --- a/lib/gitlab/ci/pipeline/chain/validate/repository.rb +++ b/lib/gitlab/ci/pipeline/chain/validate/repository.rb @@ -17,7 +17,7 @@ module Gitlab return error('Commit not found') end - if @command.project.repository.ambiguous_ref?(@command.origin_ref) + if @command.ambiguous_ref? return error('Ref is ambiguous') end end diff --git a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb index 75a177d2d1f..6aa802ce6fd 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb @@ -182,4 +182,24 @@ describe Gitlab::Ci::Pipeline::Chain::Command do it { is_expected.to eq(false) } end end + + describe '#ambiguous_ref' do + let(:project) { create(:project, :repository) } + let(:command) { described_class.new(project: project, origin_ref: 'ref') } + + subject { command.ambiguous_ref? } + + context 'when ref is not ambiguous' do + it { is_expected. to eq(false) } + end + + context 'when ref is ambiguous' do + before do + project.repository.add_tag(project.creator, 'ref', 'master') + project.repository.add_branch(project.creator, 'ref', 'master') + end + + it { is_expected. to eq(true) } + end + end end -- cgit v1.2.1 From 63da51900c205981fc12cda25778265d78b607e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 27 Nov 2018 14:45:25 +0100 Subject: Make full ref in Repository#resolve_ref explicit --- app/models/repository.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 9e01f7f0df5..3db49a5fce0 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -191,7 +191,7 @@ class Repository Gitlab::Git::TAG_REF_PREFIX + ref elsif branch_exists?(ref) Gitlab::Git::BRANCH_REF_PREFIX + ref - else + elsif Gitlab::Git.tag_ref?(ref) || Gitlab::Git.branch_ref?(ref) ref end end -- cgit v1.2.1 From dfe7f57eef0c5c2319c5c9898ba0721b6a1c9913 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Wed, 28 Nov 2018 15:18:14 +0100 Subject: Rename Repository#resolve_ref to expand_ref --- app/models/project.rb | 13 +++++++------ app/models/repository.rb | 4 +--- spec/models/repository_spec.rb | 10 +++++----- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index fa1820b2bdb..a8bef70f505 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1739,12 +1739,13 @@ class Project < ActiveRecord::Base def protected_for?(ref) return false if ref.nil? || repository.ambiguous_ref?(ref) - resolved_ref = repository.resolve_ref(ref) - ref_name = if resolved_ref == ref - Gitlab::Git.ref_name(resolved_ref) - else - ref - end + if Gitlab::Git.branch_ref?(ref) || Gitlab::Git.tag_ref?(ref) + resolved_ref = ref + ref_name = Gitlab::Git.ref_name(ref) + else + resolved_ref = repository.expand_ref(ref) + ref_name = ref + end if Gitlab::Git.branch_ref?(resolved_ref) ProtectedBranch.protected?(self, ref_name) diff --git a/app/models/repository.rb b/app/models/repository.rb index 3db49a5fce0..7352386d9d5 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -186,13 +186,11 @@ class Repository tag_exists?(ref) && branch_exists?(ref) end - def resolve_ref(ref) + def expand_ref(ref) if tag_exists?(ref) Gitlab::Git::TAG_REF_PREFIX + ref elsif branch_exists?(ref) Gitlab::Git::BRANCH_REF_PREFIX + ref - elsif Gitlab::Git.tag_ref?(ref) || Gitlab::Git.branch_ref?(ref) - ref end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index cdececd49e0..2063b4bbe75 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1032,16 +1032,16 @@ describe Repository do end end - describe '#resolve_ref' do + describe '#expand_ref' do let(:ref) { 'ref' } - subject { repository.resolve_ref(ref) } + subject { repository.expand_ref(ref) } - context 'when ref is full ref' do + context 'when ref is not tag or branch name' do let(:ref) { 'refs/heads/master' } - it 'returns the ref' do - is_expected.to eq(ref) + it 'returns nil' do + is_expected.to eq(nil) end end -- cgit v1.2.1 From 08942de9b6a3ad361cbae8a83a8e8b7c7e4768ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Wed, 28 Nov 2018 15:43:58 +0100 Subject: Raise an error on ambiguous refs --- app/models/project.rb | 3 ++- app/models/repository.rb | 1 + spec/models/project_spec.rb | 4 ++-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index a8bef70f505..9a9ef5c2fa9 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1737,7 +1737,8 @@ class Project < ActiveRecord::Base end def protected_for?(ref) - return false if ref.nil? || repository.ambiguous_ref?(ref) + return false if ref.nil? + raise Repository::AmbiguousRefError if repository.ambiguous_ref?(ref) if Gitlab::Git.branch_ref?(ref) || Gitlab::Git.tag_ref?(ref) resolved_ref = ref diff --git a/app/models/repository.rb b/app/models/repository.rb index 7352386d9d5..c685752c294 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -26,6 +26,7 @@ class Repository delegate :bundle_to_disk, to: :raw_repository CreateTreeError = Class.new(StandardError) + AmbiguousRefError = Class.new(StandardError) # Methods that cache data from the Git repository. # diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 18ad69a1bda..a106642c3e5 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2583,8 +2583,8 @@ describe Project do project.repository.add_tag(project.creator, ref, 'master') end - it 'returns false' do - is_expected.to be_falsey + it 'raises an error' do + expect { subject }.to raise_error(Repository::AmbiguousRefError) end end -- cgit v1.2.1 From 0f00c7818bf09e800c4ac6652077fffe7976ed6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 30 Nov 2018 11:34:59 +0100 Subject: Remove resolving conditional from protected_for --- app/models/project.rb | 9 ++------- spec/models/project_spec.rb | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 9a9ef5c2fa9..f71bf65f417 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1740,13 +1740,8 @@ class Project < ActiveRecord::Base return false if ref.nil? raise Repository::AmbiguousRefError if repository.ambiguous_ref?(ref) - if Gitlab::Git.branch_ref?(ref) || Gitlab::Git.tag_ref?(ref) - resolved_ref = ref - ref_name = Gitlab::Git.ref_name(ref) - else - resolved_ref = repository.expand_ref(ref) - ref_name = ref - end + resolved_ref = repository.expand_ref(ref) + ref_name = Gitlab::Git.ref_name(resolved_ref) if Gitlab::Git.branch_ref?(resolved_ref) ProtectedBranch.protected?(self, ref_name) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index a106642c3e5..75f1f779bb0 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2624,6 +2624,28 @@ describe Project do is_expected.to be_truthy end end + + context 'when ref name is a full branch ref' do + let(:ref) { 'refs/tags/something' } + + before do + project.repository.add_branch(project.creator, ref, 'master') + end + + it 'returns false' do + is_expected.to be_falsey + end + + context 'when ref is a protected branch' do + before do + create(:protected_branch, name: 'refs/tags/something', project: project) + end + + it 'returns true' do + is_expected.to be_truthy + end + end + end end describe '#update_project_statistics' do -- cgit v1.2.1 From 93d3c61eca258369bda0c28c5519cb14e4ff1457 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 30 Nov 2018 12:18:53 +0100 Subject: Add specs when full ref is passed to protected_for --- app/models/project.rb | 2 +- spec/models/project_spec.rb | 22 +++++++++++++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index f71bf65f417..0948e4625a8 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1740,7 +1740,7 @@ class Project < ActiveRecord::Base return false if ref.nil? raise Repository::AmbiguousRefError if repository.ambiguous_ref?(ref) - resolved_ref = repository.expand_ref(ref) + resolved_ref = repository.expand_ref(ref) || ref ref_name = Gitlab::Git.ref_name(resolved_ref) if Gitlab::Git.branch_ref?(resolved_ref) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 75f1f779bb0..33bce0c0823 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2625,7 +2625,27 @@ describe Project do end end - context 'when ref name is a full branch ref' do + context 'when ref is a full ref' do + let(:ref) { 'refs/heads/master' } + + context 'when ref is not protected' do + it 'returns false' do + is_expected.to be_falsey + end + end + + context 'when ref is protected' do + before do + create(:protected_branch, name: 'master', project: project) + end + + it 'returns true' do + is_expected.to be_truthy + end + end + end + + context 'when ref name is a full tag ref' do let(:ref) { 'refs/tags/something' } before do -- cgit v1.2.1 From 86c0558c1c473e26df0a1cd5ea4b647175e8b857 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 30 Nov 2018 12:36:46 +0100 Subject: Refactor Project#protected_for? specs This refactors the Project#protected_for? specs to separate them into two contexts: when the ref is a full ref and when the ref is a ref name. --- spec/models/project_spec.rb | 118 +++++++++++++++++++++++++------------------- 1 file changed, 67 insertions(+), 51 deletions(-) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 33bce0c0823..b4b9d921ba4 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2567,30 +2567,7 @@ describe Project do subject { project.protected_for?(ref) } - context 'when ref is nil' do - let(:ref) { nil } - - it 'returns false' do - is_expected.to be_falsey - end - end - - context 'when ref is ambiguous' do - let(:ref) { 'ref' } - - before do - project.repository.add_branch(project.creator, ref, 'master') - project.repository.add_tag(project.creator, ref, 'master') - end - - it 'raises an error' do - expect { subject }.to raise_error(Repository::AmbiguousRefError) - end - end - - context 'when the ref is not protected' do - let(:ref) { 'master' } - + shared_examples 'ref is not protected' do before do stub_application_setting( default_branch_protection: Gitlab::Access::PROTECTION_NONE) @@ -2601,9 +2578,7 @@ describe Project do end end - context 'when the ref is a protected branch' do - let(:ref) { 'master' } - + shared_examples 'ref is protected branch' do before do create(:protected_branch, name: 'master', project: project) end @@ -2613,9 +2588,7 @@ describe Project do end end - context 'when the ref is a protected tag' do - let(:ref) { 'v1.0.0' } - + shared_examples 'ref is protected tag' do before do create(:protected_tag, name: 'v1.0.0', project: project) end @@ -2625,44 +2598,87 @@ describe Project do end end - context 'when ref is a full ref' do - let(:ref) { 'refs/heads/master' } + context 'when ref is nil' do + let(:ref) { nil } - context 'when ref is not protected' do - it 'returns false' do - is_expected.to be_falsey - end + it 'returns false' do + is_expected.to be_falsey end + end + + context 'when ref is ref name' do + context 'when ref is ambiguous' do + let(:ref) { 'ref' } - context 'when ref is protected' do before do - create(:protected_branch, name: 'master', project: project) + project.repository.add_branch(project.creator, 'ref', 'master') + project.repository.add_tag(project.creator, 'ref', 'master') end - it 'returns true' do - is_expected.to be_truthy + it 'raises an error' do + expect { subject }.to raise_error(Repository::AmbiguousRefError) end end + + context 'when the ref is not protected' do + let(:ref) { 'master' } + + it_behaves_like 'ref is not protected' + end + + context 'when the ref is a protected branch' do + let(:ref) { 'master' } + + it_behaves_like 'ref is protected branch' + end + + context 'when the ref is a protected tag' do + let(:ref) { 'v1.0.0' } + + it_behaves_like 'ref is protected tag' + end end - context 'when ref name is a full tag ref' do - let(:ref) { 'refs/tags/something' } + context 'when ref is full ref' do + context 'when the ref is not protected' do + let(:ref) { 'refs/heads/master' } - before do - project.repository.add_branch(project.creator, ref, 'master') + it_behaves_like 'ref is not protected' end - it 'returns false' do - is_expected.to be_falsey + context 'when the ref is a protected branch' do + let(:ref) { 'refs/heads/master' } + + it_behaves_like 'ref is protected branch' end - context 'when ref is a protected branch' do + context 'when the ref is a protected tag' do + let(:ref) { 'refs/tags/v1.0.0' } + + it_behaves_like 'ref is protected tag' + end + + context 'when branch ref name is a full tag ref' do + let(:ref) { 'refs/tags/something' } + before do - create(:protected_branch, name: 'refs/tags/something', project: project) + project.repository.add_branch(project.creator, ref, 'master') end - it 'returns true' do - is_expected.to be_truthy + context 'when ref is not protected' do + it 'returns false' do + is_expected.to be_falsey + end + end + + context 'when ref is a protected branch' do + before do + create(:protected_branch, name: 'refs/tags/something', project: project) + end + + it 'returns true' do + is_expected.to be_truthy + end end end end @@ -2883,7 +2899,7 @@ describe Project do it 'shows full error updating an invalid MR' do error_message = 'Failed to replace merge_requests because one or more of the new records could not be saved.'\ - ' Validate fork Source project is not a fork of the target project' + ' Validate fork Source project is not a fork of the target project' expect { project.append_or_update_attribute(:merge_requests, [create(:merge_request)]) } .to raise_error(ActiveRecord::RecordNotSaved, error_message) -- cgit v1.2.1 From 44374434b5a0b4297686d5388f1124eda3adcbff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 30 Nov 2018 13:07:55 +0100 Subject: Conditionally assign ref_name for more efficiency --- app/models/project.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/project.rb b/app/models/project.rb index 0948e4625a8..79414665001 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1741,7 +1741,11 @@ class Project < ActiveRecord::Base raise Repository::AmbiguousRefError if repository.ambiguous_ref?(ref) resolved_ref = repository.expand_ref(ref) || ref - ref_name = Gitlab::Git.ref_name(resolved_ref) + ref_name = if resolved_ref == ref + Gitlab::Git.ref_name(resolved_ref) + else + ref + end if Gitlab::Git.branch_ref?(resolved_ref) ProtectedBranch.protected?(self, ref_name) -- cgit v1.2.1 From 9b4e45c35253a397f3ff79207736280293848bf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 3 Dec 2018 14:06:07 +0100 Subject: Check for explicit true or false in specs --- spec/models/project_spec.rb | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index b4b9d921ba4..606636e9d58 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2574,7 +2574,7 @@ describe Project do end it 'returns false' do - is_expected.to be_falsey + is_expected.to be false end end @@ -2584,7 +2584,7 @@ describe Project do end it 'returns true' do - is_expected.to be_truthy + is_expected.to be true end end @@ -2594,7 +2594,7 @@ describe Project do end it 'returns true' do - is_expected.to be_truthy + is_expected.to be true end end @@ -2602,7 +2602,7 @@ describe Project do let(:ref) { nil } it 'returns false' do - is_expected.to be_falsey + is_expected.to be false end end @@ -2637,6 +2637,14 @@ describe Project do it_behaves_like 'ref is protected tag' end + + context 'when ref does not exist' do + let(:ref) { 'something' } + + it 'returns false' do + is_expected.to be false + end + end end context 'when ref is full ref' do @@ -2667,7 +2675,7 @@ describe Project do context 'when ref is not protected' do it 'returns false' do - is_expected.to be_falsey + is_expected.to be false end end @@ -2677,10 +2685,18 @@ describe Project do end it 'returns true' do - is_expected.to be_truthy + is_expected.to be true end end end + + context 'when ref does not exist' do + let(:ref) { 'refs/heads/something' } + + it 'returns false' do + is_expected.to be false + end + end end end -- cgit v1.2.1 From 1b28a2c1a9c9651316997f7e12b161b68001126b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 3 Dec 2018 14:10:42 +0100 Subject: Check resolved_ref before checking if protected --- app/models/project.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/project.rb b/app/models/project.rb index 79414665001..d75fecb5ff2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1737,10 +1737,11 @@ class Project < ActiveRecord::Base end def protected_for?(ref) - return false if ref.nil? raise Repository::AmbiguousRefError if repository.ambiguous_ref?(ref) resolved_ref = repository.expand_ref(ref) || ref + return false unless Gitlab::Git.tag_ref?(resolved_ref) || Gitlab::Git.branch_ref?(resolved_ref) + ref_name = if resolved_ref == ref Gitlab::Git.ref_name(resolved_ref) else -- cgit v1.2.1 From 6cd0965233822fe31366d0763633de6bb11d8d72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 8 Dec 2018 19:39:47 +0100 Subject: Support merge_request pipeline ref types --- app/models/ci/pipeline.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index a1a11d2f7ab..4f64fff88ac 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -389,7 +389,7 @@ module Ci end def branch? - !tag? && !merge_request? + super && !merge_request? end def stuck? @@ -721,14 +721,10 @@ module Ci end def git_ref - if branch? + if merge_request? Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s - elsif merge_request? - Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s - elsif tag? - Gitlab::Git::TAG_REF_PREFIX + ref.to_s else - raise ArgumentError, 'Invalid pipeline type!' + super end end -- cgit v1.2.1 From 78383c41bb51d50d970f0056e80f698960ed40dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 8 Dec 2018 20:39:11 +0100 Subject: Use build for testing HasRef --- spec/models/concerns/has_ref_spec.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/models/concerns/has_ref_spec.rb b/spec/models/concerns/has_ref_spec.rb index 14db197dfe0..8aed72d77a4 100644 --- a/spec/models/concerns/has_ref_spec.rb +++ b/spec/models/concerns/has_ref_spec.rb @@ -4,13 +4,13 @@ require 'spec_helper' describe HasRef do describe '#branch?' do - let(:pipeline) { create(:ci_pipeline) } + let(:build) { create(:ci_build) } - subject { pipeline.branch? } + subject { build.branch? } context 'is not a tag' do before do - pipeline.tag = false + build.tag = false end it 'return true when tag is set to false' do @@ -20,7 +20,7 @@ describe HasRef do context 'is not a tag' do before do - pipeline.tag = true + build.tag = true end it 'return false when tag is set to true' do @@ -30,10 +30,10 @@ describe HasRef do end describe '#git_ref' do - subject { pipeline.git_ref } + subject { build.git_ref } context 'when tag is true' do - let(:pipeline) { create(:ci_pipeline, tag: true) } + let(:build) { create(:ci_build, tag: true) } it 'returns a tag ref' do is_expected.to start_with(Gitlab::Git::TAG_REF_PREFIX) @@ -41,7 +41,7 @@ describe HasRef do end context 'when tag is false' do - let(:pipeline) { create(:ci_pipeline, tag: false) } + let(:build) { create(:ci_build, tag: false) } it 'returns a branch ref' do is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX) @@ -49,7 +49,7 @@ describe HasRef do end context 'when tag is nil' do - let(:pipeline) { create(:ci_pipeline, tag: nil) } + let(:build) { create(:ci_build, tag: nil) } it 'returns a branch ref' do is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX) -- cgit v1.2.1 From 426488b7218e85ce69868ae4628801af2322b74a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 8 Dec 2018 20:39:26 +0100 Subject: Use full ref when creating MR pipeline in specs --- spec/services/ci/create_pipeline_service_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index ccc6b0ef1c7..02fde579f6f 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -667,7 +667,7 @@ describe Ci::CreatePipelineService do stub_ci_pipeline_yaml_file(YAML.dump(config)) end - let(:ref_name) { 'feature' } + let(:ref_name) { 'refs/heads/feature' } context 'when source is merge request' do let(:source) { :merge_request } @@ -696,7 +696,7 @@ describe Ci::CreatePipelineService do let(:merge_request) do create(:merge_request, source_project: project, - source_branch: ref_name, + source_branch: Gitlab::Git.ref_name(ref_name), target_project: project, target_branch: 'master') end @@ -709,7 +709,7 @@ describe Ci::CreatePipelineService do end context 'when ref is tag' do - let(:ref_name) { 'v1.1.0' } + let(:ref_name) { 'refs/tags/v1.1.0' } it 'does not create a merge request pipeline' do expect(pipeline).not_to be_persisted @@ -721,7 +721,7 @@ describe Ci::CreatePipelineService do let(:merge_request) do create(:merge_request, source_project: project, - source_branch: ref_name, + source_branch: Gitlab::Git.ref_name(ref_name), target_project: target_project, target_branch: 'master') end @@ -786,7 +786,7 @@ describe Ci::CreatePipelineService do let(:merge_request) do create(:merge_request, source_project: project, - source_branch: ref_name, + source_branch: Gitlab::Git.ref_name(ref_name), target_project: project, target_branch: 'master') end @@ -839,7 +839,7 @@ describe Ci::CreatePipelineService do let(:merge_request) do create(:merge_request, source_project: project, - source_branch: ref_name, + source_branch: Gitlab::Git.ref_name(ref_name), target_project: project, target_branch: 'master') end -- cgit v1.2.1 From 15526e023daa4c19dc1b619e02946a5dc775c49c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 8 Dec 2018 21:25:33 +0100 Subject: Add CHANGELOG entry --- .../unreleased/security-master-secret-ci-variables-exposed.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/security-master-secret-ci-variables-exposed.yml diff --git a/changelogs/unreleased/security-master-secret-ci-variables-exposed.yml b/changelogs/unreleased/security-master-secret-ci-variables-exposed.yml new file mode 100644 index 00000000000..702181065f5 --- /dev/null +++ b/changelogs/unreleased/security-master-secret-ci-variables-exposed.yml @@ -0,0 +1,5 @@ +--- +title: Prevent leaking protected variables for ambiguous refs. +merge_request: +author: +type: security -- cgit v1.2.1 From 08bfec57c3e17225a93a33e464a898a14741d5c4 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Tue, 11 Dec 2018 17:20:06 +0100 Subject: Set URL rel attribute for broken URLs It's possible that URI fails to parse a link, but browsers still recognize given URL as a link, we should make sure that 'rel' attribute is set also in this case. --- changelogs/unreleased/security-master-url-rel.yml | 5 +++++ lib/banzai/filter/external_link_filter.rb | 12 ++++++------ spec/lib/banzai/filter/external_link_filter_spec.rb | 8 ++++---- 3 files changed, 15 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/security-master-url-rel.yml diff --git a/changelogs/unreleased/security-master-url-rel.yml b/changelogs/unreleased/security-master-url-rel.yml new file mode 100644 index 00000000000..75f599f6bcd --- /dev/null +++ b/changelogs/unreleased/security-master-url-rel.yml @@ -0,0 +1,5 @@ +--- +title: Set URL rel attribute for broken URLs. +merge_request: +author: +type: security diff --git a/lib/banzai/filter/external_link_filter.rb b/lib/banzai/filter/external_link_filter.rb index 2e6d742de27..4f60b6f84c6 100644 --- a/lib/banzai/filter/external_link_filter.rb +++ b/lib/banzai/filter/external_link_filter.rb @@ -9,11 +9,10 @@ module Banzai def call links.each do |node| uri = uri(node['href'].to_s) - next unless uri - node.set_attribute('href', uri.to_s) + node.set_attribute('href', uri.to_s) if uri - if SCHEMES.include?(uri.scheme) && external_url?(uri) + if SCHEMES.include?(uri&.scheme) && !internal_url?(uri) node.set_attribute('rel', 'nofollow noreferrer noopener') node.set_attribute('target', '_blank') end @@ -35,11 +34,12 @@ module Banzai doc.xpath(query) end - def external_url?(uri) + def internal_url?(uri) + return false if uri.nil? # Relative URLs miss a hostname - return false unless uri.hostname + return true unless uri.hostname - uri.hostname != internal_url.hostname + uri.hostname == internal_url.hostname end def internal_url diff --git a/spec/lib/banzai/filter/external_link_filter_spec.rb b/spec/lib/banzai/filter/external_link_filter_spec.rb index 2a3c0cd78b8..e6dae8d5382 100644 --- a/spec/lib/banzai/filter/external_link_filter_spec.rb +++ b/spec/lib/banzai/filter/external_link_filter_spec.rb @@ -49,16 +49,16 @@ describe Banzai::Filter::ExternalLinkFilter do end context 'for invalid urls' do - it 'skips broken hrefs' do + it 'adds rel and target attributes to broken hrefs' do doc = filter %q(

Google

) - expected = %q(

Google

) + expected = %q(

Google

) expect(doc.to_html).to eq(expected) end - it 'skips improperly formatted mailtos' do + it 'adds rel and target to improperly formatted mailtos' do doc = filter %q(

Email

) - expected = %q(

Email

) + expected = %q(

Email

) expect(doc.to_html).to eq(expected) end -- cgit v1.2.1 From a1d69ab6b86b93e600bdd90190f0a7d574992e91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Wed, 12 Dec 2018 19:28:31 +0100 Subject: Escape html entities when no label found --- changelogs/unreleased/54427-label-xss.yml | 5 +++++ lib/banzai/filter/label_reference_filter.rb | 6 +++++- spec/lib/banzai/filter/label_reference_filter_spec.rb | 18 ++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/54427-label-xss.yml diff --git a/changelogs/unreleased/54427-label-xss.yml b/changelogs/unreleased/54427-label-xss.yml new file mode 100644 index 00000000000..090d1832af2 --- /dev/null +++ b/changelogs/unreleased/54427-label-xss.yml @@ -0,0 +1,5 @@ +--- +title: Escape html entities in LabelReferenceFilter when no label found +merge_request: +author: +type: security diff --git a/lib/banzai/filter/label_reference_filter.rb b/lib/banzai/filter/label_reference_filter.rb index 04ec38209c7..f90a35952e5 100644 --- a/lib/banzai/filter/label_reference_filter.rb +++ b/lib/banzai/filter/label_reference_filter.rb @@ -29,7 +29,7 @@ module Banzai if label yield match, label.id, project, namespace, $~ else - match + escape_html_entities(match) end end end @@ -102,6 +102,10 @@ module Banzai CGI.unescapeHTML(text.to_s) end + def escape_html_entities(text) + CGI.escapeHTML(text.to_s) + end + def object_link_title(object, matches) # use title of wrapped element instead nil diff --git a/spec/lib/banzai/filter/label_reference_filter_spec.rb b/spec/lib/banzai/filter/label_reference_filter_spec.rb index 00257ed7904..9cfdb9e53a2 100644 --- a/spec/lib/banzai/filter/label_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/label_reference_filter_spec.rb @@ -236,6 +236,24 @@ describe Banzai::Filter::LabelReferenceFilter do end end + context 'References with html entities' do + let!(:label) { create(:label, name: '<html>', project: project) } + + it 'links to a valid reference' do + doc = reference_filter('See ~"<html>"') + + expect(doc.css('a').first.attr('href')).to eq urls + .project_issues_url(project, label_name: label.name) + expect(doc.text).to eq 'See ' + end + + it 'ignores invalid label names and escapes entities' do + act = %(Label #{Label.reference_prefix}"<non valid>") + + expect(reference_filter(act).to_html).to eq act + end + end + describe 'consecutive references' do let(:bug) { create(:label, name: 'bug', project: project) } let(:feature_proposal) { create(:label, name: 'feature proposal', project: project) } -- cgit v1.2.1 From 63c48f73803cf1c68d6c9af408f877ea61781118 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Mon, 10 Dec 2018 17:23:52 +0100 Subject: Replaced UrlValidator with PublicUrlValidator for import_url and remote mirror urls --- app/models/project.rb | 7 +++---- app/models/remote_mirror.rb | 2 +- .../security-fix-ssrf-import-url-remote-mirror.yml | 5 +++++ spec/models/project_spec.rb | 7 +++++++ spec/models/remote_mirror_spec.rb | 14 ++++++++++++++ 5 files changed, 30 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/security-fix-ssrf-import-url-remote-mirror.yml diff --git a/app/models/project.rb b/app/models/project.rb index 075c07f5c8e..f91ac015475 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -324,10 +324,9 @@ class Project < ActiveRecord::Base validates :namespace, presence: true validates :name, uniqueness: { scope: :namespace_id } - validates :import_url, url: { protocols: ->(project) { project.persisted? ? VALID_MIRROR_PROTOCOLS : VALID_IMPORT_PROTOCOLS }, - ports: ->(project) { project.persisted? ? VALID_MIRROR_PORTS : VALID_IMPORT_PORTS }, - allow_localhost: false, - enforce_user: true }, if: [:external_import?, :import_url_changed?] + validates :import_url, public_url: { protocols: ->(project) { project.persisted? ? VALID_MIRROR_PROTOCOLS : VALID_IMPORT_PROTOCOLS }, + ports: ->(project) { project.persisted? ? VALID_MIRROR_PORTS : VALID_IMPORT_PORTS }, + enforce_user: true }, if: [:external_import?, :import_url_changed?] validates :star_count, numericality: { greater_than_or_equal_to: 0 } validate :check_limit, on: :create validate :check_repository_path_availability, on: :update, if: ->(project) { project.renamed? } diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb index b7b4d0f1be9..327c6e7c7a3 100644 --- a/app/models/remote_mirror.rb +++ b/app/models/remote_mirror.rb @@ -17,7 +17,7 @@ class RemoteMirror < ActiveRecord::Base belongs_to :project, inverse_of: :remote_mirrors - validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true, enforce_user: true } + validates :url, presence: true, public_url: { protocols: %w(ssh git http https), allow_blank: true, enforce_user: true } before_save :set_new_remote_name, if: :mirror_url_changed? diff --git a/changelogs/unreleased/security-fix-ssrf-import-url-remote-mirror.yml b/changelogs/unreleased/security-fix-ssrf-import-url-remote-mirror.yml new file mode 100644 index 00000000000..7ba7aa21090 --- /dev/null +++ b/changelogs/unreleased/security-fix-ssrf-import-url-remote-mirror.yml @@ -0,0 +1,5 @@ +--- +title: Fix SSRF with import_url and remote mirror url +merge_request: +author: +type: security diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 9e5b06b745a..a9624b6a00e 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -314,6 +314,13 @@ describe Project do expect(project.errors[:import_url].first).to include('Requests to localhost are not allowed') end + it 'does not allow import_url pointing to the local network' do + project = build(:project, import_url: 'https://192.168.1.1') + + expect(project).to be_invalid + expect(project.errors[:import_url].first).to include('Requests to the local network are not allowed') + end + it "does not allow import_url with invalid ports for new projects" do project = build(:project, import_url: 'http://github.com:25/t.git') diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb index b12ca79847c..66a25ccb410 100644 --- a/spec/models/remote_mirror_spec.rb +++ b/spec/models/remote_mirror_spec.rb @@ -24,6 +24,20 @@ describe RemoteMirror do expect(remote_mirror).to be_invalid expect(remote_mirror.errors[:url].first).to include('Username needs to start with an alphanumeric character') end + + it 'does not allow url pointing to localhost' do + remote_mirror = build(:remote_mirror, url: 'http://127.0.0.2/t.git') + + expect(remote_mirror).to be_invalid + expect(remote_mirror.errors[:url].first).to include('Requests to loopback addresses are not allowed') + end + + it 'does not allow url pointing to the local network' do + remote_mirror = build(:remote_mirror, url: 'https://192.168.1.1') + + expect(remote_mirror).to be_invalid + expect(remote_mirror.errors[:url].first).to include('Requests to the local network are not allowed') + end end end -- cgit v1.2.1 From 08dbd93bd6e08bca179567a3c020b8fac5139b49 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 6 Dec 2018 17:04:34 +0100 Subject: Validate projects in MR build service This validates the correct abilities for both projects. Only `read_project` isn't enough: For the `source_project` we validate `create_merge_request_from` this also validates that the user has developer access to the project. For the `target_project` we validate `create_merge_reqeust_in` this also validates that the user has access to the project's repository. To avoid generating diffs for unrelated projects we also validate that the projects are in the same fork network now. --- app/services/merge_requests/build_service.rb | 24 +++++++--- .../security-bvl-fix-cross-project-mr-exposure.yml | 5 ++ ...ccess_private_repository_through_new_mr_spec.rb | 37 +++++++++++++++ spec/services/merge_requests/build_service_spec.rb | 55 ++++++++++++++++++++-- 4 files changed, 111 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/security-bvl-fix-cross-project-mr-exposure.yml create mode 100644 spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 36767621d74..48419da98ad 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -18,7 +18,7 @@ module MergeRequests merge_request.source_project = find_source_project merge_request.target_project = find_target_project merge_request.target_branch = find_target_branch - merge_request.can_be_created = branches_valid? + merge_request.can_be_created = projects_and_branches_valid? # compare branches only if branches are valid, otherwise # compare_branches may raise an error @@ -49,15 +49,19 @@ module MergeRequests to: :merge_request def find_source_project - return source_project if source_project.present? && can?(current_user, :read_project, source_project) + return source_project if source_project.present? && can?(current_user, :create_merge_request_from, source_project) project end def find_target_project - return target_project if target_project.present? && can?(current_user, :read_project, target_project) + return target_project if target_project.present? && can?(current_user, :create_merge_request_in, target_project) - project.default_merge_request_target + target_project = project.default_merge_request_target + + return target_project if target_project.present? && can?(current_user, :create_merge_request_in, target_project) + + project end def find_target_branch @@ -72,10 +76,11 @@ module MergeRequests params[:target_branch].present? end - def branches_valid? + def projects_and_branches_valid? + return false if source_project.nil? || target_project.nil? return false unless source_branch_specified? || target_branch_specified? - validate_branches + validate_projects_and_branches errors.blank? end @@ -94,7 +99,12 @@ module MergeRequests end end - def validate_branches + def validate_projects_and_branches + merge_request.validate_target_project + merge_request.validate_fork + + return if errors.any? + add_error('You must select source and target branch') unless branches_present? add_error('You must select different branches') if same_source_and_target? add_error("Source branch \"#{source_branch}\" does not exist") unless source_branch_exists? diff --git a/changelogs/unreleased/security-bvl-fix-cross-project-mr-exposure.yml b/changelogs/unreleased/security-bvl-fix-cross-project-mr-exposure.yml new file mode 100644 index 00000000000..11aae4428fb --- /dev/null +++ b/changelogs/unreleased/security-bvl-fix-cross-project-mr-exposure.yml @@ -0,0 +1,5 @@ +--- +title: Don't expose cross project repositories through diffs when creating merge reqeusts +merge_request: +author: +type: security diff --git a/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb b/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb new file mode 100644 index 00000000000..9318b5f1ebb --- /dev/null +++ b/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb @@ -0,0 +1,37 @@ +require 'spec_helper' + +describe 'Merge Request > Tries to access private repo of public project' do + let(:current_user) { create(:user) } + let(:private_project) do + create(:project, :public, :repository, + path: 'nothing-to-see-here', + name: 'nothing to see here', + repository_access_level: ProjectFeature::PRIVATE) + end + let(:owned_project) do + create(:project, :public, :repository, + namespace: current_user.namespace, + creator: current_user) + end + + context 'when the user enters the querystring info for the other project' do + let(:mr_path) do + project_new_merge_request_diffs_path( + owned_project, + merge_request: { + source_project_id: private_project.id, + source_branch: 'feature' + } + ) + end + + before do + sign_in current_user + visit mr_path + end + + it "does not mention the project the user can't see the repo of" do + expect(page).not_to have_content('nothing-to-see-here') + end + end +end diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index 1894d8c8d0e..536d0d345a4 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe MergeRequests::BuildService do using RSpec::Parameterized::TableSyntax include RepoHelpers + include ProjectForksHelper let(:project) { create(:project, :repository) } let(:source_project) { nil } @@ -49,7 +50,7 @@ describe MergeRequests::BuildService do describe '#execute' do it 'calls the compare service with the correct arguments' do - allow_any_instance_of(described_class).to receive(:branches_valid?).and_return(true) + allow_any_instance_of(described_class).to receive(:projects_and_branches_valid?).and_return(true) expect(CompareService).to receive(:new) .with(project, Gitlab::Git::BRANCH_REF_PREFIX + source_branch) .and_call_original @@ -393,11 +394,27 @@ describe MergeRequests::BuildService do end end + context 'target_project is set but repo is not accessible by current_user' do + let(:target_project) do + create(:project, :public, :repository, repository_access_level: ProjectFeature::PRIVATE) + end + + it 'sets target project correctly' do + expect(merge_request.target_project).to eq(project) + end + end + context 'source_project is set and accessible by current_user' do let(:source_project) { create(:project, :public, :repository)} let(:commits) { Commit.decorate([commit_1], project) } - it 'sets target project correctly' do + before do + # To create merge requests _from_ a project the user needs at least + # developer access + source_project.add_developer(user) + end + + it 'sets source project correctly' do expect(merge_request.source_project).to eq(source_project) end end @@ -406,11 +423,43 @@ describe MergeRequests::BuildService do let(:source_project) { create(:project, :private, :repository)} let(:commits) { Commit.decorate([commit_1], project) } - it 'sets target project correctly' do + it 'sets source project correctly' do + expect(merge_request.source_project).to eq(project) + end + end + + context 'source_project is set but the user cannot create merge requests from the project' do + let(:source_project) do + create(:project, :public, :repository, merge_requests_access_level: ProjectFeature::PRIVATE) + end + + it 'sets the source_project correctly' do expect(merge_request.source_project).to eq(project) end end + context 'target_project is not in the fork network of source_project' do + let(:target_project) { create(:project, :public, :repository) } + + it 'adds an error to the merge request' do + expect(merge_request.errors[:validate_fork]).to contain_exactly('Source project is not a fork of the target project') + end + end + + context 'target_project is in the fork network of source project but no longer accessible' do + let!(:project) { fork_project(target_project, user, namespace: user.namespace, repository: true) } + let(:source_project) { project } + let(:target_project) { create(:project, :public, :repository) } + + before do + target_project.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end + + it 'sets the target_project correctly' do + expect(merge_request.target_project).to eq(project) + end + end + context 'when specifying target branch in the description' do let(:description) { "A merge request targeting another branch\n\n/target_branch with-codeowners" } -- cgit v1.2.1 From 1653f7b1c68b2ea7da8df84ed459b9578e3dff8f Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Tue, 11 Dec 2018 16:15:10 -0200 Subject: Delete confidential issue todos for guests Fix leaking information of confidential issues on TODOs when user is downgraded to guest access. --- app/models/todo.rb | 5 +++++ app/services/groups/update_service.rb | 2 +- app/services/issues/update_service.rb | 2 +- app/services/members/base_service.rb | 6 ++++++ app/services/members/destroy_service.rb | 8 +------- app/services/members/update_service.rb | 9 +++++++++ app/services/projects/update_service.rb | 4 ++-- .../security-todos_not_redacted_for_guests.yml | 5 +++++ doc/workflow/todos.md | 3 +++ spec/services/groups/update_service_spec.rb | 2 +- spec/services/issues/update_service_spec.rb | 2 +- spec/services/members/destroy_service_spec.rb | 2 +- spec/services/members/update_service_spec.rb | 17 +++++++++++++++++ spec/services/projects/update_service_spec.rb | 4 ++-- 14 files changed, 55 insertions(+), 16 deletions(-) create mode 100644 changelogs/unreleased/security-todos_not_redacted_for_guests.yml diff --git a/app/models/todo.rb b/app/models/todo.rb index 7b64615f699..d9b86d941b6 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -4,6 +4,11 @@ class Todo < ActiveRecord::Base include Sortable include FromUnion + # Time to wait for todos being removed when not visible for user anymore. + # Prevents TODOs being removed by mistake, for example, removing access from a user + # and giving it back again. + WAIT_FOR_DELETE = 1.hour + ASSIGNED = 1 MENTIONED = 2 BUILD_FAILED = 3 diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 0bf0e967dcc..83ffc3dc8cd 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -31,7 +31,7 @@ module Groups def after_update if group.previous_changes.include?(:visibility_level) && group.private? # don't enqueue immediately to prevent todos removal in case of a mistake - TodosDestroyer::GroupPrivateWorker.perform_in(1.hour, group.id) + TodosDestroyer::GroupPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, group.id) end end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index a1d0cc0e568..e992d682c79 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -44,7 +44,7 @@ module Issues if issue.previous_changes.include?('confidential') # don't enqueue immediately to prevent todos removal in case of a mistake - TodosDestroyer::ConfidentialIssueWorker.perform_in(1.hour, issue.id) if issue.confidential? + TodosDestroyer::ConfidentialIssueWorker.perform_in(Todo::WAIT_FOR_DELETE, issue.id) if issue.confidential? create_confidentiality_note(issue) end diff --git a/app/services/members/base_service.rb b/app/services/members/base_service.rb index d734571f835..e78affff797 100644 --- a/app/services/members/base_service.rb +++ b/app/services/members/base_service.rb @@ -47,5 +47,11 @@ module Members raise "Unknown action '#{action}' on #{member}!" end end + + def enqueue_delete_todos(member) + type = member.is_a?(GroupMember) ? 'Group' : 'Project' + # don't enqueue immediately to prevent todos removal in case of a mistake + TodosDestroyer::EntityLeaveWorker.perform_in(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, type) + end end end diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index c186a5971dc..ae0c644e6c0 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -15,7 +15,7 @@ module Members notification_service.decline_access_request(member) end - enqeue_delete_todos(member) + enqueue_delete_todos(member) after_execute(member: member) @@ -24,12 +24,6 @@ module Members private - def enqeue_delete_todos(member) - type = member.is_a?(GroupMember) ? 'Group' : 'Project' - # don't enqueue immediately to prevent todos removal in case of a mistake - TodosDestroyer::EntityLeaveWorker.perform_in(1.hour, member.user_id, member.source_id, type) - end - def can_destroy_member?(member) can?(current_user, destroy_member_permission(member), member) end diff --git a/app/services/members/update_service.rb b/app/services/members/update_service.rb index 1f5618dae53..ff8d5c1d8c9 100644 --- a/app/services/members/update_service.rb +++ b/app/services/members/update_service.rb @@ -10,9 +10,18 @@ module Members if member.update(params) after_execute(action: permission, old_access_level: old_access_level, member: member) + + # Deletes only confidential issues todos for guests + enqueue_delete_todos(member) if downgrading_to_guest? end member end + + private + + def downgrading_to_guest? + params[:access_level] == Gitlab::Access::GUEST + end end end diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index 93e48fc0199..dd1b9680ece 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -61,9 +61,9 @@ module Projects if project.previous_changes.include?(:visibility_level) && project.private? # don't enqueue immediately to prevent todos removal in case of a mistake - TodosDestroyer::ProjectPrivateWorker.perform_in(1.hour, project.id) + TodosDestroyer::ProjectPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id) elsif (project_changed_feature_keys & todos_features_changes).present? - TodosDestroyer::PrivateFeaturesWorker.perform_in(1.hour, project.id) + TodosDestroyer::PrivateFeaturesWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id) end if project.previous_changes.include?('path') diff --git a/changelogs/unreleased/security-todos_not_redacted_for_guests.yml b/changelogs/unreleased/security-todos_not_redacted_for_guests.yml new file mode 100644 index 00000000000..be0ae9a7193 --- /dev/null +++ b/changelogs/unreleased/security-todos_not_redacted_for_guests.yml @@ -0,0 +1,5 @@ +--- +title: Delete confidential todos for user when downgraded to Guest +merge_request: +author: +type: security diff --git a/doc/workflow/todos.md b/doc/workflow/todos.md index f94d592d0db..830f17aa7f2 100644 --- a/doc/workflow/todos.md +++ b/doc/workflow/todos.md @@ -35,6 +35,9 @@ A Todo appears in your Todos dashboard when: - the author, or - have set it to automatically merge once pipeline succeeds. +NOTE: **Note:** +When an user no longer has access to a resource related to a Todo like an issue, merge request, project or group the related Todos, for security reasons, gets deleted within the next hour. The delete is delayed to prevent data loss in case user got their access revoked by mistake. + ### Directly addressed Todos > [Introduced][ce-7926] in GitLab 9.0. diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 84cfa53ea05..d87a7dd234d 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -56,7 +56,7 @@ describe Groups::UpdateService do create(:project, :private, group: internal_group) expect(TodosDestroyer::GroupPrivateWorker).to receive(:perform_in) - .with(1.hour, internal_group.id) + .with(Todo::WAIT_FOR_DELETE, internal_group.id) end it "changes permission level to private" do diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index bd519e7f077..ce20bf2bef6 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -77,7 +77,7 @@ describe Issues::UpdateService, :mailer do end it 'enqueues ConfidentialIssueWorker when an issue is made confidential' do - expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(1.hour, issue.id) + expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, issue.id) update_issue(confidential: true) end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 0a5220c7c61..5aa7165e135 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -22,7 +22,7 @@ describe Members::DestroyService do shared_examples 'a service destroying a member' do before do type = member.is_a?(GroupMember) ? 'Group' : 'Project' - expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(1.hour, member.user_id, member.source_id, type) + expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, type) end it 'destroys the member' do diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb index 6d19a95ffeb..599ed39ca37 100644 --- a/spec/services/members/update_service_spec.rb +++ b/spec/services/members/update_service_spec.rb @@ -20,11 +20,28 @@ describe Members::UpdateService do shared_examples 'a service updating a member' do it 'updates the member' do + expect(TodosDestroyer::EntityLeaveWorker).not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name) + updated_member = described_class.new(current_user, params).execute(member, permission: permission) expect(updated_member).to be_valid expect(updated_member.access_level).to eq(Gitlab::Access::MAINTAINER) end + + context 'when member is downgraded to guest' do + let(:params) do + { access_level: Gitlab::Access::GUEST } + end + + it 'schedules to delete confidential todos' do + expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once + + updated_member = described_class.new(current_user, params).execute(member, permission: permission) + + expect(updated_member).to be_valid + expect(updated_member.access_level).to eq(Gitlab::Access::GUEST) + end + end end before do diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index d58ff2cedc0..8adfc63222e 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -41,7 +41,7 @@ describe Projects::UpdateService do end it 'updates the project to private' do - expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(1.hour, project.id) + expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) @@ -191,7 +191,7 @@ describe Projects::UpdateService do context 'when changing feature visibility to private' do it 'updates the visibility correctly' do expect(TodosDestroyer::PrivateFeaturesWorker) - .to receive(:perform_in).with(1.hour, project.id) + .to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) result = update_project(project, user, project_feature_attributes: { issues_access_level: ProjectFeature::PRIVATE } -- cgit v1.2.1 From b7a9c26a5bd9b33de9193ba3fd88e3c5d4a2d996 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Fri, 14 Dec 2018 18:16:03 +0100 Subject: Fixed SSRF in project imports with LFS --- .../projects/lfs_pointers/lfs_download_service.rb | 35 ++++++++++--- .../lfs_pointers/lfs_download_service_spec.rb | 59 ++++++++++++++++++---- 2 files changed, 77 insertions(+), 17 deletions(-) diff --git a/app/services/projects/lfs_pointers/lfs_download_service.rb b/app/services/projects/lfs_pointers/lfs_download_service.rb index f9b9781ad5f..b5128443435 100644 --- a/app/services/projects/lfs_pointers/lfs_download_service.rb +++ b/app/services/projects/lfs_pointers/lfs_download_service.rb @@ -12,28 +12,43 @@ module Projects return if LfsObject.exists?(oid: oid) - sanitized_uri = Gitlab::UrlSanitizer.new(url) - Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url, protocols: VALID_PROTOCOLS) + sanitized_uri = sanitize_url!(url) with_tmp_file(oid) do |file| - size = download_and_save_file(file, sanitized_uri) - lfs_object = LfsObject.new(oid: oid, size: size, file: file) + download_and_save_file(file, sanitized_uri) + lfs_object = LfsObject.new(oid: oid, size: file.size, file: file) project.all_lfs_objects << lfs_object end + rescue Gitlab::UrlBlocker::BlockedUrlError => e + Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded: #{e.message}") rescue StandardError => e - Rails.logger.error("LFS file with oid #{oid} could't be downloaded from #{sanitized_uri.sanitized_url}: #{e.message}") + Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded from #{sanitized_uri.sanitized_url}: #{e.message}") end # rubocop: enable CodeReuse/ActiveRecord private + def sanitize_url!(url) + Gitlab::UrlSanitizer.new(url).tap do |sanitized_uri| + # Just validate that HTTP/HTTPS protocols are used. The + # subsequent Gitlab::HTTP.get call will do network checks + # based on the settings. + Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url, + protocols: VALID_PROTOCOLS) + end + end + def download_and_save_file(file, sanitized_uri) - IO.copy_stream(open(sanitized_uri.sanitized_url, headers(sanitized_uri)), file) # rubocop:disable Security/Open + response = Gitlab::HTTP.get(sanitized_uri.sanitized_url, headers(sanitized_uri)) do |fragment| + file.write(fragment) + end + + raise StandardError, "Received error code #{response.code}" unless response.success? end def headers(sanitized_uri) - {}.tap do |headers| + query_options.tap do |headers| credentials = sanitized_uri.credentials if credentials[:user].present? || credentials[:password].present? @@ -43,10 +58,14 @@ module Projects end end + def query_options + { stream_body: true } + end + def with_tmp_file(oid) create_tmp_storage_dir - File.open(File.join(tmp_storage_dir, oid), 'w') { |file| yield file } + File.open(File.join(tmp_storage_dir, oid), 'wb') { |file| yield file } end def create_tmp_storage_dir diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index d7d7f1874eb..95c9b6e63b8 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -4,17 +4,15 @@ describe Projects::LfsPointers::LfsDownloadService do let(:project) { create(:project) } let(:oid) { '9e548e25631dd9ce6b43afd6359ab76da2819d6a5b474e66118c7819e1d8b3e8' } let(:download_link) { "http://gitlab.com/#{oid}" } - let(:lfs_content) do - <<~HEREDOC - whatever - HEREDOC - end + let(:lfs_content) { SecureRandom.random_bytes(10) } subject { described_class.new(project) } before do allow(project).to receive(:lfs_enabled?).and_return(true) WebMock.stub_request(:get, download_link).to_return(body: lfs_content) + + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false) end describe '#execute' do @@ -32,7 +30,7 @@ describe Projects::LfsPointers::LfsDownloadService do it 'stores the content' do subject.execute(oid, download_link) - expect(File.read(LfsObject.first.file.file.file)).to eq lfs_content + expect(File.binread(LfsObject.first.file.file.file)).to eq lfs_content end end @@ -54,18 +52,61 @@ describe Projects::LfsPointers::LfsDownloadService do end end + context 'when localhost requests are allowed' do + let(:download_link) { 'http://192.168.2.120' } + + before do + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true) + end + + it 'downloads the file' do + expect(subject).to receive(:download_and_save_file).and_call_original + + expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.by(1) + end + end + context 'when a bad URL is used' do - where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2']) + where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2', 'http://192.168.2.120']) with_them do it 'does not download the file' do - expect(subject).not_to receive(:download_and_save_file) - expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count } end end end + context 'when the URL points to a redirected URL' do + context 'that is blocked' do + where(redirect_link: ['ftp://example.com', 'http://127.0.0.2', 'http://192.168.2.120']) + + with_them do + before do + WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link }) + end + + it 'does not follow the redirection' do + expect(Rails.logger).to receive(:error).with(/LFS file with oid #{oid} couldn't be downloaded/) + + expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count } + end + end + end + + context 'that is valid' do + let(:redirect_link) { "http://example.com/"} + + before do + WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link }) + WebMock.stub_request(:get, redirect_link).to_return(body: lfs_content) + end + + it 'follows the redirection' do + expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.from(0).to(1) + end + end + end + context 'when an lfs object with the same oid already exists' do before do create(:lfs_object, oid: 'oid') -- cgit v1.2.1 From 8772bdabb2f48e9868971d8349f6e36985bffec0 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Mon, 10 Dec 2018 13:58:34 +0000 Subject: Project guests no longer are able to see refs page Adds download_code authorization check to ProjectsController#refs action, to prevent a project guest from seeing branch, tags and commits information --- app/controllers/projects_controller.rb | 1 + .../security-refs-available-to-project-guest.yml | 5 +++++ spec/controllers/projects_controller_spec.rb | 24 ++++++++++++++++++---- 3 files changed, 26 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/security-refs-available-to-project-guest.yml diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 8bf93bfd68d..878816475b2 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -19,6 +19,7 @@ class ProjectsController < Projects::ApplicationController before_action :lfs_blob_ids, only: [:show], if: [:repo_exists?, :project_view_files?] before_action :project_export_enabled, only: [:export, :download_export, :remove_export, :generate_new_export] before_action :present_project, only: [:edit] + before_action :authorize_download_code!, only: [:refs] # Authorize before_action :authorize_admin_project!, only: [:edit, :update, :housekeeping, :download_export, :export, :remove_export, :generate_new_export] diff --git a/changelogs/unreleased/security-refs-available-to-project-guest.yml b/changelogs/unreleased/security-refs-available-to-project-guest.yml new file mode 100644 index 00000000000..eb6804c52d3 --- /dev/null +++ b/changelogs/unreleased/security-refs-available-to-project-guest.yml @@ -0,0 +1,5 @@ +--- +title: Project guests no longer are able to see refs page +merge_request: +author: +type: security diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index ea067a01295..4747d837273 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -621,10 +621,10 @@ describe ProjectsController do end describe "GET refs" do - let(:public_project) { create(:project, :public, :repository) } + let(:project) { create(:project, :public, :repository) } it 'gets a list of branches and tags' do - get :refs, params: { namespace_id: public_project.namespace, id: public_project, sort: 'updated_desc' } + get :refs, params: { namespace_id: project.namespace, id: project, sort: 'updated_desc' } parsed_body = JSON.parse(response.body) expect(parsed_body['Branches']).to include('master') @@ -634,7 +634,7 @@ describe ProjectsController do end it "gets a list of branches, tags and commits" do - get :refs, params: { namespace_id: public_project.namespace, id: public_project, ref: "123456" } + get :refs, params: { namespace_id: project.namespace, id: project, ref: "123456" } parsed_body = JSON.parse(response.body) expect(parsed_body["Branches"]).to include("master") @@ -649,7 +649,7 @@ describe ProjectsController do end it "gets a list of branches, tags and commits" do - get :refs, params: { namespace_id: public_project.namespace, id: public_project, ref: "123456" } + get :refs, params: { namespace_id: project.namespace, id: project, ref: "123456" } parsed_body = JSON.parse(response.body) expect(parsed_body["Branches"]).to include("master") @@ -657,6 +657,22 @@ describe ProjectsController do expect(parsed_body["Commits"]).to include("123456") end end + + context 'when private project' do + let(:project) { create(:project, :repository) } + + context 'as a guest' do + it 'renders forbidden' do + user = create(:user) + project.add_guest(user) + + sign_in(user) + get :refs, namespace_id: project.namespace, id: project + + expect(response).to have_gitlab_http_status(404) + end + end + end end describe 'POST #preview_markdown' do -- cgit v1.2.1 From 52feca595a3311fc12a6f35191a24ff61c33e440 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Fri, 7 Dec 2018 15:48:38 +0000 Subject: Adds validation to check if user can read project An issuable should not be available to a user if the project is not visible to that specific user --- app/policies/issuable_policy.rb | 2 +- ...s-access-to-mr-issue-when-removed-from-team.yml | 5 ++++ spec/models/event_spec.rb | 18 +++++++++++++-- spec/policies/issuable_policy_spec.rb | 27 ++++++++++++++++++++++ spec/services/issuable/bulk_update_service_spec.rb | 27 ++++++++++++++++++++++ spec/services/todo_service_spec.rb | 1 + 6 files changed, 77 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/security-53543-user-keeps-access-to-mr-issue-when-removed-from-team.yml diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index 6d8b575102e..ecb2797d1d9 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -11,7 +11,7 @@ class IssuablePolicy < BasePolicy @user && @subject.assignee_or_author?(@user) end - rule { assignee_or_author }.policy do + rule { can?(:guest_access) & assignee_or_author }.policy do enable :read_issue enable :update_issue enable :reopen_issue diff --git a/changelogs/unreleased/security-53543-user-keeps-access-to-mr-issue-when-removed-from-team.yml b/changelogs/unreleased/security-53543-user-keeps-access-to-mr-issue-when-removed-from-team.yml new file mode 100644 index 00000000000..ab12ba539c1 --- /dev/null +++ b/changelogs/unreleased/security-53543-user-keeps-access-to-mr-issue-when-removed-from-team.yml @@ -0,0 +1,5 @@ +--- +title: Issuable no longer is visible to users when project can't be viewed +merge_request: +author: +type: security diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index 81748681528..a64720f1876 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/event_spec.rb @@ -243,6 +243,20 @@ describe Event do expect(event.visible_to_user?(admin)).to eq true end end + + context 'private project' do + let(:project) { create(:project, :private) } + let(:target) { note_on_issue } + + it do + expect(event.visible_to_user?(non_member)).to eq false + expect(event.visible_to_user?(author)).to eq false + expect(event.visible_to_user?(assignee)).to eq false + expect(event.visible_to_user?(member)).to eq true + expect(event.visible_to_user?(guest)).to eq true + expect(event.visible_to_user?(admin)).to eq true + end + end end context 'merge request diff note event' do @@ -265,8 +279,8 @@ describe Event do it do expect(event.visible_to_user?(non_member)).to eq false - expect(event.visible_to_user?(author)).to eq true - expect(event.visible_to_user?(assignee)).to eq true + expect(event.visible_to_user?(author)).to eq false + expect(event.visible_to_user?(assignee)).to eq false expect(event.visible_to_user?(member)).to eq true expect(event.visible_to_user?(guest)).to eq false expect(event.visible_to_user?(admin)).to eq true diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb index d1bf98995e7..db3df760472 100644 --- a/spec/policies/issuable_policy_spec.rb +++ b/spec/policies/issuable_policy_spec.rb @@ -7,6 +7,33 @@ describe IssuablePolicy, models: true do let(:policies) { described_class.new(user, issue) } describe '#rules' do + context 'when user is author of issuable' do + let(:merge_request) { create(:merge_request, source_project: project, author: user) } + let(:policies) { described_class.new(user, merge_request) } + + context 'when user is able to read project' do + it 'enables user to read and update issuables' do + expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request) + end + end + + context 'when project is private' do + let(:project) { create(:project, :private) } + + context 'when user belongs to the projects team' do + it 'enables user to read and update issuables' do + project.add_maintainer(user) + + expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request) + end + end + + it 'disallows user from reading and updating issuables from that project' do + expect(policies).to be_disallowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request) + end + end + end + context 'when discussion is locked for the issuable' do let(:issue) { create(:issue, project: project, discussion_locked: true) } diff --git a/spec/services/issuable/bulk_update_service_spec.rb b/spec/services/issuable/bulk_update_service_spec.rb index f0b0f7956ce..ca366cdf1df 100644 --- a/spec/services/issuable/bulk_update_service_spec.rb +++ b/spec/services/issuable/bulk_update_service_spec.rb @@ -28,6 +28,33 @@ describe Issuable::BulkUpdateService do expect(project.issues.opened).to be_empty expect(project.issues.closed).not_to be_empty end + + context 'when issue for a different project is created' do + let(:private_project) { create(:project, :private) } + let(:issue) { create(:issue, project: private_project, author: user) } + + context 'when user has access to the project' do + it 'closes all issues passed' do + private_project.add_maintainer(user) + + bulk_update(issues + [issue], state_event: 'close') + + expect(project.issues.opened).to be_empty + expect(project.issues.closed).not_to be_empty + expect(private_project.issues.closed).not_to be_empty + end + end + + context 'when user does not have access to project' do + it 'only closes all issues that the user has access to' do + bulk_update(issues + [issue], state_event: 'close') + + expect(project.issues.opened).to be_empty + expect(project.issues.closed).not_to be_empty + expect(private_project.issues.closed).to be_empty + end + end + end end describe 'reopen issues' do diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index c52515aefd8..253f2e44d10 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -19,6 +19,7 @@ describe TodoService do before do project.add_guest(guest) project.add_developer(author) + project.add_developer(assignee) project.add_developer(member) project.add_developer(john_doe) project.add_developer(skipped) -- cgit v1.2.1 From c7ea28612a210811696dae50d6ca948c85566da2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 14 Dec 2018 16:36:33 +0100 Subject: Authorize read_build action when listing jobs --- lib/api/jobs.rb | 2 ++ spec/requests/api/jobs_spec.rb | 16 +++++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb index 80a5cbd6b19..3cfeb9a2784 100644 --- a/lib/api/jobs.rb +++ b/lib/api/jobs.rb @@ -38,6 +38,8 @@ module API end # rubocop: disable CodeReuse/ActiveRecord get ':id/jobs' do + authorize_read_builds! + builds = user_project.builds.order('id DESC') builds = filter_builds(builds, params[:scope]) diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb index 73131dba542..6deb842b0bc 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -142,10 +142,20 @@ describe API::Jobs do end context 'unauthorized user' do - let(:api_user) { nil } + context 'when user is not logged in' do + let(:api_user) { nil } - it 'does not return project jobs' do - expect(response).to have_gitlab_http_status(401) + it 'does not return project jobs' do + expect(response).to have_gitlab_http_status(401) + end + end + + context 'when user is guest' do + let(:api_user) { guest } + + it 'does not return project jobs' do + expect(response).to have_gitlab_http_status(403) + end end end -- cgit v1.2.1 From a1c77f2d34d979016499e4fa15b49e67d5666d63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 14 Dec 2018 16:42:04 +0100 Subject: Authorize read_build when listing pipeline jobs --- lib/api/jobs.rb | 2 ++ spec/requests/api/jobs_spec.rb | 16 +++++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb index 3cfeb9a2784..bd704f3bf25 100644 --- a/lib/api/jobs.rb +++ b/lib/api/jobs.rb @@ -59,6 +59,8 @@ module API # rubocop: disable CodeReuse/ActiveRecord get ':id/pipelines/:pipeline_id/jobs' do pipeline = user_project.ci_pipelines.find(params[:pipeline_id]) + authorize!(:read_build, pipeline) + builds = pipeline.builds builds = filter_builds(builds, params[:scope]) builds = builds.preload(:job_artifacts_archive, :job_artifacts, project: [:namespace]) diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb index 6deb842b0bc..97aa71bf231 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -251,10 +251,20 @@ describe API::Jobs do end context 'unauthorized user' do - let(:api_user) { nil } + context 'when user is not logged in' do + let(:api_user) { nil } - it 'does not return jobs' do - expect(response).to have_gitlab_http_status(401) + it 'does not return jobs' do + expect(response).to have_gitlab_http_status(401) + end + end + + context 'when user is guest' do + let(:api_user) { guest } + + it 'does not return jobs' do + expect(response).to have_gitlab_http_status(403) + end end end end -- cgit v1.2.1 From 89b856e76c5e77428535f169350443272a34e1d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 18 Dec 2018 14:36:26 +0100 Subject: Authorize read_pipeline before read_build --- lib/api/jobs.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb index bd704f3bf25..e2ab60f3855 100644 --- a/lib/api/jobs.rb +++ b/lib/api/jobs.rb @@ -59,6 +59,7 @@ module API # rubocop: disable CodeReuse/ActiveRecord get ':id/pipelines/:pipeline_id/jobs' do pipeline = user_project.ci_pipelines.find(params[:pipeline_id]) + authorize!(:read_pipeline, user_project) authorize!(:read_build, pipeline) builds = pipeline.builds -- cgit v1.2.1 From ccc227e6674e5ae42519776b82ce899193973496 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Thu, 20 Dec 2018 04:09:47 +0100 Subject: Move pipeline auth above pipeline assignment --- lib/api/jobs.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb index e2ab60f3855..45c694b6448 100644 --- a/lib/api/jobs.rb +++ b/lib/api/jobs.rb @@ -58,8 +58,8 @@ module API end # rubocop: disable CodeReuse/ActiveRecord get ':id/pipelines/:pipeline_id/jobs' do - pipeline = user_project.ci_pipelines.find(params[:pipeline_id]) authorize!(:read_pipeline, user_project) + pipeline = user_project.ci_pipelines.find(params[:pipeline_id]) authorize!(:read_build, pipeline) builds = pipeline.builds -- cgit v1.2.1 From 30c6db8f0354847c275335c120d7218c0098c41f Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Tue, 11 Dec 2018 14:28:06 +0800 Subject: Move embeddable? to model to be used outside view --- app/helpers/snippets_helper.rb | 8 ------- app/models/snippet.rb | 8 +++++++ app/views/shared/snippets/_header.html.haml | 2 +- spec/models/snippet_spec.rb | 37 +++++++++++++++++++++++++++++ 4 files changed, 46 insertions(+), 9 deletions(-) diff --git a/app/helpers/snippets_helper.rb b/app/helpers/snippets_helper.rb index c7d31f3469d..a20c47ed91a 100644 --- a/app/helpers/snippets_helper.rb +++ b/app/helpers/snippets_helper.rb @@ -130,12 +130,4 @@ module SnippetsHelper link_to external_snippet_icon('download'), download_url, class: 'btn', target: '_blank', title: 'Download', rel: 'noopener noreferrer' end - - def public_snippet? - if @snippet.project_id? - can?(nil, :read_project_snippet, @snippet) - else - can?(nil, :read_personal_snippet, @snippet) - end - end end diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 11856b55902..e623ee8161f 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -175,6 +175,14 @@ class Snippet < ActiveRecord::Base :visibility_level end + def embeddable? + if project_id? + Ability.allowed?(nil, :read_project_snippet, self) + else + Ability.allowed?(nil, :read_personal_snippet, self) + end + end + def notes_with_associations notes.includes(:author) end diff --git a/app/views/shared/snippets/_header.html.haml b/app/views/shared/snippets/_header.html.haml index 10bfc30492a..a43296aa806 100644 --- a/app/views/shared/snippets/_header.html.haml +++ b/app/views/shared/snippets/_header.html.haml @@ -30,7 +30,7 @@ - if @snippet.updated_at != @snippet.created_at = edited_time_ago_with_tooltip(@snippet, placement: 'bottom', html_class: 'snippet-edited-ago', exclude_author: true) - - if public_snippet? + - if @snippet.embeddable? .embed-snippet .input-group .input-group-prepend diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb index 7a7272ccb60..664dc3fa145 100644 --- a/spec/models/snippet_spec.rb +++ b/spec/models/snippet_spec.rb @@ -423,4 +423,41 @@ describe Snippet do expect(blob.data).to eq(snippet.content) end end + + describe '#embeddable?' do + context 'project snippet' do + [ + { project: :public, snippet: :public, embeddable: true }, + { project: :internal, snippet: :public, embeddable: false }, + { project: :private, snippet: :public, embeddable: false }, + { project: :public, snippet: :internal, embeddable: false }, + { project: :internal, snippet: :internal, embeddable: false }, + { project: :private, snippet: :internal, embeddable: false }, + { project: :public, snippet: :private, embeddable: false }, + { project: :internal, snippet: :private, embeddable: false }, + { project: :private, snippet: :private, embeddable: false } + ].each do |combination| + it 'only returns true when both project and snippet are public' do + project = create(:project, combination[:project]) + snippet = create(:project_snippet, combination[:snippet], project: project) + + expect(snippet.embeddable?).to eq(combination[:embeddable]) + end + end + end + + context 'personal snippet' do + [ + { snippet: :public, embeddable: true }, + { snippet: :internal, embeddable: false }, + { snippet: :private, embeddable: false } + ].each do |combination| + it 'only returns true when snippet is public' do + snippet = create(:personal_snippet, combination[:snippet]) + + expect(snippet.embeddable?).to eq(combination[:embeddable]) + end + end + end + end end -- cgit v1.2.1 From ed0d691e0dfba54cd8f03706afd011afe4063a7a Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Tue, 11 Dec 2018 14:32:25 +0800 Subject: Block private snippets from being embeddable --- app/controllers/projects/snippets_controller.rb | 9 ++++- app/controllers/snippets_controller.rb | 8 ++++- app/models/snippet.rb | 8 ++--- .../unreleased/security-48259-private-snippet.yml | 5 +++ .../projects/snippets_controller_spec.rb | 40 ++++++++++++++++++++++ spec/controllers/snippets_controller_spec.rb | 19 ++++++++++ 6 files changed, 82 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/security-48259-private-snippet.yml diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb index a44acb12bdf..255f1f3569a 100644 --- a/app/controllers/projects/snippets_controller.rb +++ b/app/controllers/projects/snippets_controller.rb @@ -75,7 +75,14 @@ class Projects::SnippetsController < Projects::ApplicationController format.json do render_blob_json(blob) end - format.js { render 'shared/snippets/show'} + + format.js do + if @snippet.embeddable? + render 'shared/snippets/show' + else + head :not_found + end + end end end diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb index dd9bf17cf0c..8ea5450b4e8 100644 --- a/app/controllers/snippets_controller.rb +++ b/app/controllers/snippets_controller.rb @@ -80,7 +80,13 @@ class SnippetsController < ApplicationController render_blob_json(blob) end - format.js { render 'shared/snippets/show' } + format.js do + if @snippet.embeddable? + render 'shared/snippets/show' + else + head :not_found + end + end end end diff --git a/app/models/snippet.rb b/app/models/snippet.rb index e623ee8161f..f9b23bbbf6c 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -176,11 +176,9 @@ class Snippet < ActiveRecord::Base end def embeddable? - if project_id? - Ability.allowed?(nil, :read_project_snippet, self) - else - Ability.allowed?(nil, :read_personal_snippet, self) - end + ability = project_id? ? :read_project_snippet : :read_personal_snippet + + Ability.allowed?(nil, ability, self) end def notes_with_associations diff --git a/changelogs/unreleased/security-48259-private-snippet.yml b/changelogs/unreleased/security-48259-private-snippet.yml new file mode 100644 index 00000000000..6cf1e5dc694 --- /dev/null +++ b/changelogs/unreleased/security-48259-private-snippet.yml @@ -0,0 +1,5 @@ +--- +title: Prevent private snippets from being embeddable +merge_request: +author: +type: security diff --git a/spec/controllers/projects/snippets_controller_spec.rb b/spec/controllers/projects/snippets_controller_spec.rb index 1a3fb4da15f..e4b78aff25d 100644 --- a/spec/controllers/projects/snippets_controller_spec.rb +++ b/spec/controllers/projects/snippets_controller_spec.rb @@ -379,6 +379,46 @@ describe Projects::SnippetsController do end end + describe "GET #show for embeddable content" do + let(:project_snippet) { create(:project_snippet, snippet_permission, project: project, author: user) } + + before do + sign_in(user) + + get :show, namespace_id: project.namespace, project_id: project, id: project_snippet.to_param, format: :js + end + + context 'when snippet is private' do + let(:snippet_permission) { :private } + + it 'responds with status 404' do + expect(response).to have_gitlab_http_status(404) + end + end + + context 'when snippet is public' do + let(:snippet_permission) { :public } + + it 'responds with status 200' do + expect(assigns(:snippet)).to eq(project_snippet) + expect(response).to have_gitlab_http_status(200) + end + end + + context 'when the project is private' do + let(:project) { create(:project_empty_repo, :private) } + + context 'when snippet is public' do + let(:project_snippet) { create(:project_snippet, :public, project: project, author: user) } + + it 'responds with status 404' do + expect(assigns(:snippet)).to eq(project_snippet) + expect(response).to have_gitlab_http_status(404) + end + end + end + end + describe 'GET #raw' do let(:project_snippet) do create( diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index 01a5161f775..af44598bfe4 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -80,6 +80,12 @@ describe SnippetsController do expect(assigns(:snippet)).to eq(personal_snippet) expect(response).to have_gitlab_http_status(200) end + + it 'responds with status 404 when embeddable content is requested' do + get :show, id: personal_snippet.to_param, format: :js + + expect(response).to have_gitlab_http_status(404) + end end end @@ -106,6 +112,12 @@ describe SnippetsController do expect(assigns(:snippet)).to eq(personal_snippet) expect(response).to have_gitlab_http_status(200) end + + it 'responds with status 404 when embeddable content is requested' do + get :show, id: personal_snippet.to_param, format: :js + + expect(response).to have_gitlab_http_status(404) + end end context 'when not signed in' do @@ -131,6 +143,13 @@ describe SnippetsController do expect(assigns(:snippet)).to eq(personal_snippet) expect(response).to have_gitlab_http_status(200) end + + it 'responds with status 200 when embeddable content is requested' do + get :show, id: personal_snippet.to_param, format: :js + + expect(assigns(:snippet)).to eq(personal_snippet) + expect(response).to have_gitlab_http_status(200) + end end context 'when not signed in' do -- cgit v1.2.1 From 5f03d26a194c25abef20b94c175ac4f587e821a2 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 11 Dec 2018 17:52:08 +0530 Subject: Escape label and milestone titles to prevent XSS --- app/assets/javascripts/gfm_auto_complete.js | 17 ++++++----- spec/features/issues/gfm_autocomplete_spec.rb | 44 +++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/gfm_auto_complete.js b/app/assets/javascripts/gfm_auto_complete.js index c14eb936930..8178821be3d 100644 --- a/app/assets/javascripts/gfm_auto_complete.js +++ b/app/assets/javascripts/gfm_auto_complete.js @@ -256,7 +256,7 @@ class GfmAutoComplete { displayTpl(value) { let tmpl = GfmAutoComplete.Loading.template; if (value.title != null) { - tmpl = GfmAutoComplete.Milestones.template; + tmpl = GfmAutoComplete.Milestones.templateFunction(value.title); } return tmpl; }, @@ -323,7 +323,7 @@ class GfmAutoComplete { searchKey: 'search', data: GfmAutoComplete.defaultLoadingData, displayTpl(value) { - let tmpl = GfmAutoComplete.Labels.template; + let tmpl = GfmAutoComplete.Labels.templateFunction(value.color, value.title); if (GfmAutoComplete.isLoading(value)) { tmpl = GfmAutoComplete.Loading.template; } @@ -588,9 +588,11 @@ GfmAutoComplete.Members = { }, }; GfmAutoComplete.Labels = { - template: - // eslint-disable-next-line no-template-curly-in-string - '
  • ${title}
    • ', + templateFunction(color, title) { + return `
  • ${_.escape(title)}
    • `; + }, }; // Issues, MergeRequests and Snippets GfmAutoComplete.Issues = { @@ -600,8 +602,9 @@ GfmAutoComplete.Issues = { }; // Milestones GfmAutoComplete.Milestones = { - // eslint-disable-next-line no-template-curly-in-string - template: '
  • ${title}
    • ', + templateFunction(title) { + return `
  • ${_.escape(title)}
    • `; + }, }; GfmAutoComplete.Loading = { template: diff --git a/spec/features/issues/gfm_autocomplete_spec.rb b/spec/features/issues/gfm_autocomplete_spec.rb index d7531d5fcd9..3b7a17ef355 100644 --- a/spec/features/issues/gfm_autocomplete_spec.rb +++ b/spec/features/issues/gfm_autocomplete_spec.rb @@ -3,6 +3,8 @@ require 'rails_helper' describe 'GFM autocomplete', :js do let(:issue_xss_title) { 'This will execute alert Date: Sun, 16 Dec 2018 15:38:36 +0100 Subject: Check for group admin permissions --- .../groups/settings/ci_cd_controller.rb | 6 +-- .../groups/settings/ci_cd_controller_spec.rb | 55 ++++++++++++++++++---- spec/features/group_variables_spec.rb | 2 +- spec/features/runners_spec.rb | 3 +- 4 files changed, 51 insertions(+), 15 deletions(-) diff --git a/app/controllers/groups/settings/ci_cd_controller.rb b/app/controllers/groups/settings/ci_cd_controller.rb index c1dcc463de7..f476f428fdb 100644 --- a/app/controllers/groups/settings/ci_cd_controller.rb +++ b/app/controllers/groups/settings/ci_cd_controller.rb @@ -4,7 +4,7 @@ module Groups module Settings class CiCdController < Groups::ApplicationController skip_cross_project_access_check :show - before_action :authorize_admin_pipeline! + before_action :authorize_admin_group! def show define_ci_variables @@ -26,8 +26,8 @@ module Groups .map { |variable| variable.present(current_user: current_user) } end - def authorize_admin_pipeline! - return render_404 unless can?(current_user, :admin_pipeline, group) + def authorize_admin_group! + return render_404 unless can?(current_user, :admin_group, group) end end end diff --git a/spec/controllers/groups/settings/ci_cd_controller_spec.rb b/spec/controllers/groups/settings/ci_cd_controller_spec.rb index b7f04f732b9..40673d10b91 100644 --- a/spec/controllers/groups/settings/ci_cd_controller_spec.rb +++ b/spec/controllers/groups/settings/ci_cd_controller_spec.rb @@ -5,30 +5,65 @@ describe Groups::Settings::CiCdController do let(:user) { create(:user) } before do - group.add_maintainer(user) sign_in(user) end describe 'GET #show' do - it 'renders show with 200 status code' do - get :show, params: { group_id: group } + context 'when user is owner' do + before do + group.add_owner(user) + end - expect(response).to have_gitlab_http_status(200) - expect(response).to render_template(:show) + it 'renders show with 200 status code' do + get :show, params: { group_id: group } + + expect(response).to have_gitlab_http_status(200) + expect(response).to render_template(:show) + end + end + + context 'when user is not owner' do + before do + group.add_maintainer(user) + end + + it 'renders a 404' do + get :show, params: { group_id: group } + + expect(response).to have_gitlab_http_status(404) + end end end describe 'PUT #reset_registration_token' do subject { put :reset_registration_token, params: { group_id: group } } - it 'resets runner registration token' do - expect { subject }.to change { group.reload.runners_token } + context 'when user is owner' do + before do + group.add_owner(user) + end + + it 'resets runner registration token' do + expect { subject }.to change { group.reload.runners_token } + end + + it 'redirects the user to admin runners page' do + subject + + expect(response).to redirect_to(group_settings_ci_cd_path) + end end - it 'redirects the user to admin runners page' do - subject + context 'when user is not owner' do + before do + group.add_maintainer(user) + end + + it 'renders a 404' do + subject - expect(response).to redirect_to(group_settings_ci_cd_path) + expect(response).to have_gitlab_http_status(404) + end end end end diff --git a/spec/features/group_variables_spec.rb b/spec/features/group_variables_spec.rb index 89e0cdd8ed7..57e3ddfb39c 100644 --- a/spec/features/group_variables_spec.rb +++ b/spec/features/group_variables_spec.rb @@ -7,7 +7,7 @@ describe 'Group variables', :js do let(:page_path) { group_settings_ci_cd_path(group) } before do - group.add_maintainer(user) + group.add_owner(user) gitlab_sign_in(user) visit page_path diff --git a/spec/features/runners_spec.rb b/spec/features/runners_spec.rb index cb7a912946c..09de983f669 100644 --- a/spec/features/runners_spec.rb +++ b/spec/features/runners_spec.rb @@ -259,8 +259,9 @@ describe 'Runners' do context 'group runners in group settings' do let(:group) { create(:group) } + before do - group.add_maintainer(user) + group.add_owner(user) end context 'group with no runners' do -- cgit v1.2.1 From 597e22c4049b574436cb2258387137b559ad5f9b Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Thu, 20 Dec 2018 12:07:04 +0000 Subject: Add changelog entry --- changelogs/unreleased/security-54377-label-milestone-name-xss.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/security-54377-label-milestone-name-xss.yml diff --git a/changelogs/unreleased/security-54377-label-milestone-name-xss.yml b/changelogs/unreleased/security-54377-label-milestone-name-xss.yml new file mode 100644 index 00000000000..76589b2eb4f --- /dev/null +++ b/changelogs/unreleased/security-54377-label-milestone-name-xss.yml @@ -0,0 +1,5 @@ +--- +title: Escape label and milestone titles to prevent XSS in GFM autocomplete +merge_request: 2693 +author: +type: security -- cgit v1.2.1 From 68d172daec63f70c8641da574d5a1a97c8167833 Mon Sep 17 00:00:00 2001 From: Raphael Nestler Date: Mon, 17 Dec 2018 16:26:53 +0100 Subject: Explain how to use kaniko with a registry with a custom certificate --- doc/ci/docker/using_kaniko.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/doc/ci/docker/using_kaniko.md b/doc/ci/docker/using_kaniko.md index 66f0d429165..aa6b387bc58 100644 --- a/doc/ci/docker/using_kaniko.md +++ b/doc/ci/docker/using_kaniko.md @@ -57,3 +57,26 @@ build: only: - tags ``` + +## Using a registry with a custom certificate + +When trying to push to a Docker registry that uses a certificate that is signed +by a custom CA, you might get the following error: + +```sh +$ /kaniko/executor --context $CI_PROJECT_DIR --dockerfile $CI_PROJECT_DIR/Dockerfile --no-push +INFO[0000] Downloading base image registry.gitlab.example.com/group/docker-image +error building image: getting stage builder for stage 0: Get https://registry.gitlab.example.com/v2/: x509: certificate signed by unknown authority +``` + +This can be solved by adding your CA's certificate to the kaniko certificate +store: + +```yaml + before_script: + - echo "{\"auths\":{\"$CI_REGISTRY\":{\"username\":\"$CI_REGISTRY_USER\",\"password\":\"$CI_REGISTRY_PASSWORD\"}}}" > /kaniko/.docker/config.json + - | + echo "-----BEGIN CERTIFICATE----- + ... + -----END CERTIFICATE-----" >> /kaniko/ssl/certs/ca-certificates.crt +``` -- cgit v1.2.1 From 1ed71dbea2cfb0618421fd17c5a261b47a3c3882 Mon Sep 17 00:00:00 2001 From: sofiane belaribi Date: Fri, 21 Dec 2018 13:49:20 +0000 Subject: Update mysql.md --- doc/ci/services/mysql.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/ci/services/mysql.md b/doc/ci/services/mysql.md index b76f9618fc9..2902c30c7c0 100644 --- a/doc/ci/services/mysql.md +++ b/doc/ci/services/mysql.md @@ -16,7 +16,7 @@ services: - mysql:latest variables: - # Configure mysql environment variables (https://hub.docker.com/r/_/mysql/) + # Configure mysql environment variables (https://hub.docker.com/_/mysql/) MYSQL_DATABASE: el_duderino MYSQL_ROOT_PASSWORD: mysql_strong_password ``` @@ -114,5 +114,5 @@ available [shared runners](../runners/README.md). Want to hack on it? Simply fork it, commit and push your changes. Within a few moments the changes will be picked by a public runner and the job will begin. -[hub-mysql]: https://hub.docker.com/r/_/mysql/ +[hub-mysql]: https://hub.docker.com/_/mysql/ [mysql-example-repo]: https://gitlab.com/gitlab-examples/mysql -- cgit v1.2.1 From e783ad5b7ad16409a49afd10fa859dd19115164b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 22 Dec 2018 13:53:17 +0100 Subject: Add CHANGELOG entry --- changelogs/unreleased/security-master-guests-jobs-api.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/security-master-guests-jobs-api.yml diff --git a/changelogs/unreleased/security-master-guests-jobs-api.yml b/changelogs/unreleased/security-master-guests-jobs-api.yml new file mode 100644 index 00000000000..83022e91aca --- /dev/null +++ b/changelogs/unreleased/security-master-guests-jobs-api.yml @@ -0,0 +1,5 @@ +--- +title: Authorize before reading job information via API. +merge_request: +author: +type: security -- cgit v1.2.1 From e264677bf1799f52c23cd602aaafad4fb53b36ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 24 Dec 2018 00:51:29 +0100 Subject: Add CHANGELOG entry --- .../security-master-group-cicd-settings-accessible-to-maintainer.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/security-master-group-cicd-settings-accessible-to-maintainer.yml diff --git a/changelogs/unreleased/security-master-group-cicd-settings-accessible-to-maintainer.yml b/changelogs/unreleased/security-master-group-cicd-settings-accessible-to-maintainer.yml new file mode 100644 index 00000000000..5586fa6cd8e --- /dev/null +++ b/changelogs/unreleased/security-master-group-cicd-settings-accessible-to-maintainer.yml @@ -0,0 +1,5 @@ +--- +title: Allow changing group CI/CD settings only for owners. +merge_request: +author: +type: security -- cgit v1.2.1 From 02f1d9cff0e8453c8e44533c28bfe2266040aa7a Mon Sep 17 00:00:00 2001 From: Gareth Davies Date: Tue, 25 Dec 2018 04:15:44 +0000 Subject: Updating link to correctly point to environment scope docs --- doc/user/project/clusters/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/project/clusters/index.md b/doc/user/project/clusters/index.md index 6bdafc60949..6f334af4fb7 100644 --- a/doc/user/project/clusters/index.md +++ b/doc/user/project/clusters/index.md @@ -60,7 +60,7 @@ new Kubernetes cluster to your project: **Sign in with Google** button. 1. From there on, choose your cluster's settings: - **Kubernetes cluster name** - The name you wish to give the cluster. - - **Environment scope** - The [associated environment](#setting-the-environment-scope) to this cluster. + - **Environment scope** - The [associated environment](#setting-the-environment-scope-premium) to this cluster. - **Google Cloud Platform project** - Choose the project you created in your GCP console that will host the Kubernetes cluster. Learn more about [Google Cloud Platform projects](https://cloud.google.com/resource-manager/docs/creating-managing-projects). -- cgit v1.2.1 From 2bfc317492f0a5918c4861203f3eedf71b2627aa Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Fri, 21 Dec 2018 12:16:15 +0100 Subject: Use directories.new when getting S3 directory Calling `Fog::Storage::AWS::Directories#get` requires the ListAllMyBuckets permission, but we can avoid that extra query and permission by initializing the directory with a specific bucket: https://stackoverflow.com/a/12288581/1992201 --- changelogs/unreleased/s3-directories-get.yml | 6 ++++++ lib/gitlab/cleanup/remote_uploads.rb | 2 +- spec/lib/gitlab/cleanup/remote_uploads_spec.rb | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/s3-directories-get.yml diff --git a/changelogs/unreleased/s3-directories-get.yml b/changelogs/unreleased/s3-directories-get.yml new file mode 100644 index 00000000000..9f76af2bb09 --- /dev/null +++ b/changelogs/unreleased/s3-directories-get.yml @@ -0,0 +1,6 @@ +--- +title: Allow 'rake gitlab:cleanup:remote_upload_files' to read bucket files without + having permissions to see all buckets. +merge_request: 23981 +author: +type: fixed diff --git a/lib/gitlab/cleanup/remote_uploads.rb b/lib/gitlab/cleanup/remote_uploads.rb index eba1faacc3a..03298d960a4 100644 --- a/lib/gitlab/cleanup/remote_uploads.rb +++ b/lib/gitlab/cleanup/remote_uploads.rb @@ -67,7 +67,7 @@ module Gitlab end def remote_directory - connection.directories.get(configuration['remote_directory']) + connection.directories.new(key: configuration['remote_directory']) end def connection diff --git a/spec/lib/gitlab/cleanup/remote_uploads_spec.rb b/spec/lib/gitlab/cleanup/remote_uploads_spec.rb index 8d03baeb07b..35642cd6e50 100644 --- a/spec/lib/gitlab/cleanup/remote_uploads_spec.rb +++ b/spec/lib/gitlab/cleanup/remote_uploads_spec.rb @@ -25,7 +25,7 @@ describe Gitlab::Cleanup::RemoteUploads do expect(::Fog::Storage).to receive(:new).and_return(connection) - expect(connection).to receive(:directories).and_return(double(get: directory)) + expect(connection).to receive(:directories).and_return(double(new: directory)) expect(directory).to receive(:files).and_return(remote_files) end -- cgit v1.2.1 From 5d550fa5a2d780fecef328525d16b3288606696f Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Fri, 28 Dec 2018 09:51:27 +0000 Subject: Update CHANGELOG.md for 11.6.1 [ci skip] --- CHANGELOG.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b4fa22ad70e..a1c928aedf3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,31 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 11.6.1 (2018-12-28) + +### Security (15 changes) + +- Escape label and milestone titles to prevent XSS in GFM autocomplete. !2740 +- Prevent private snippets from being embeddable. +- Add subresources removal to member destroy service. +- Escape html entities in LabelReferenceFilter when no label found. +- Allow changing group CI/CD settings only for owners. +- Authorize before reading job information via API. +- Prevent leaking protected variables for ambiguous refs. +- Ensure that build token is only used when running. +- Issuable no longer is visible to users when project can't be viewed. +- Don't expose cross project repositories through diffs when creating merge reqeusts. +- Fix SSRF with import_url and remote mirror url. +- Fix persistent symlink in project import. +- Set URL rel attribute for broken URLs. +- Project guests no longer are able to see refs page. +- Delete confidential todos for user when downgraded to Guest. + +### Other (1 change) + +- Fix due date test. !23845 + + ## 11.6.0 (2018-12-22) ### Security (24 changes, 1 of them is from the community) -- cgit v1.2.1 From 441dee4c315cb728e61ae5e5e42f36d782ef8e29 Mon Sep 17 00:00:00 2001 From: Mark Lapierre Date: Fri, 28 Dec 2018 12:39:07 -0500 Subject: Log text_filter arg of find_element --- qa/qa/support/page/logging.rb | 7 +++++-- qa/spec/page/logging_spec.rb | 9 +++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/qa/qa/support/page/logging.rb b/qa/qa/support/page/logging.rb index df3b794b14b..cfccbb910b7 100644 --- a/qa/qa/support/page/logging.rb +++ b/qa/qa/support/page/logging.rb @@ -37,8 +37,11 @@ module QA exists end - def find_element(name, wait: Capybara.default_max_wait_time) - log("finding :#{name} (wait: #{wait})") + def find_element(name, text_filter = nil, wait: Capybara.default_max_wait_time) + msg = ["finding :#{name}"] + msg << %Q(with text_filter "#{text_filter}") if text_filter + msg << "(wait: #{wait})" + log(msg.compact.join(' ')) element = super diff --git a/qa/spec/page/logging_spec.rb b/qa/spec/page/logging_spec.rb index a54ff424f53..f108a5ca318 100644 --- a/qa/spec/page/logging_spec.rb +++ b/qa/spec/page/logging_spec.rb @@ -47,6 +47,15 @@ describe QA::Support::Page::Logging do it 'logs find_element' do expect { subject.find_element(:element) } + .to output(/finding :element/).to_stdout_from_any_process + expect { subject.find_element(:element) } + .to output(/found :element/).to_stdout_from_any_process + end + + it 'logs find_element with text_filter' do + expect { subject.find_element(:element, 'foo') } + .to output(/finding :element with text_filter "foo"/).to_stdout_from_any_process + expect { subject.find_element(:element, 'foo') } .to output(/found :element/).to_stdout_from_any_process end -- cgit v1.2.1 From 1c91d042b596e3c0940df3c6167f2e628c102f61 Mon Sep 17 00:00:00 2001 From: Jacopo Date: Mon, 31 Dec 2018 10:46:11 +0100 Subject: Renames Milestone sort into Milestone due date --- app/helpers/sorting_helper.rb | 2 +- .../55369-update-milestone-sort-to-say-say-milestone-due-date.yml | 5 +++++ locale/gitlab.pot | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/55369-update-milestone-sort-to-say-say-milestone-due-date.yml diff --git a/app/helpers/sorting_helper.rb b/app/helpers/sorting_helper.rb index 6ac1f42c321..02762897c89 100644 --- a/app/helpers/sorting_helper.rb +++ b/app/helpers/sorting_helper.rb @@ -234,7 +234,7 @@ module SortingHelper end def sort_title_milestone - s_('SortOptions|Milestone') + s_('SortOptions|Milestone due date') end def sort_title_milestone_later diff --git a/changelogs/unreleased/55369-update-milestone-sort-to-say-say-milestone-due-date.yml b/changelogs/unreleased/55369-update-milestone-sort-to-say-say-milestone-due-date.yml new file mode 100644 index 00000000000..7476b9caa93 --- /dev/null +++ b/changelogs/unreleased/55369-update-milestone-sort-to-say-say-milestone-due-date.yml @@ -0,0 +1,5 @@ +--- +title: Renames Milestone sort into Milestone due date +merge_request: 24080 +author: Jacopo Beschi @jacopo-beschi +type: changed diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 88eff3b6645..8d90d89b575 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -6239,7 +6239,7 @@ msgstr "" msgid "SortOptions|Least popular" msgstr "" -msgid "SortOptions|Milestone" +msgid "SortOptions|Milestone due date" msgstr "" msgid "SortOptions|Milestone due later" -- cgit v1.2.1 From 5542f3d3cc2753516e75d0ea5290ab018e2c12e1 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 31 Dec 2018 23:11:09 -0800 Subject: Upgrade GitLab QA image to Ruby 2.5 GitLab is now shipping with Ruby 2.5.3, so we should make the version consistent for the QA image as well. --- qa/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qa/Dockerfile b/qa/Dockerfile index 9956ced0ef6..ca7f9accb70 100644 --- a/qa/Dockerfile +++ b/qa/Dockerfile @@ -1,4 +1,4 @@ -FROM ruby:2.4-stretch +FROM ruby:2.5-stretch LABEL maintainer "Grzegorz Bizon " ENV DEBIAN_FRONTEND noninteractive -- cgit v1.2.1 From 1b87f8d7637ce7ba58c9391ea69d35515cb78074 Mon Sep 17 00:00:00 2001 From: Takuya Noguchi Date: Tue, 1 Jan 2019 19:23:37 +0900 Subject: Remove gem install bundler from Docker-based Ruby environments bundler gem has been included in the Docker official Ruby image since 2.1. Signed-off-by: Takuya Noguchi --- ...emove-gem-install-bundler-from-docker-based-ruby-environments.yml | 5 +++++ doc/api/lint.md | 2 +- doc/api/templates/gitlab_ci_ymls.md | 2 +- doc/ci/caching/index.md | 1 - lib/gitlab/ci/templates/Ruby.gitlab-ci.yml | 1 - spec/controllers/projects/ci/lints_controller_spec.rb | 1 - spec/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml | 1 - spec/lib/gitlab/ci/config/external/file/local_spec.rb | 1 - spec/lib/gitlab/ci/config/external/file/remote_spec.rb | 1 - spec/lib/gitlab/ci/config/external/processor_spec.rb | 2 -- spec/lib/gitlab/ci/config_spec.rb | 1 - spec/support/gitlab_stubs/gitlab_ci.yml | 3 +-- 12 files changed, 8 insertions(+), 13 deletions(-) create mode 100644 changelogs/unreleased/55838-remove-gem-install-bundler-from-docker-based-ruby-environments.yml diff --git a/changelogs/unreleased/55838-remove-gem-install-bundler-from-docker-based-ruby-environments.yml b/changelogs/unreleased/55838-remove-gem-install-bundler-from-docker-based-ruby-environments.yml new file mode 100644 index 00000000000..08f60d205df --- /dev/null +++ b/changelogs/unreleased/55838-remove-gem-install-bundler-from-docker-based-ruby-environments.yml @@ -0,0 +1,5 @@ +--- +title: Remove gem install bundler from Docker-based Ruby environments +merge_request: 24093 +author: Takuya Noguchi +type: other diff --git a/doc/api/lint.md b/doc/api/lint.md index bd5a216a99d..c37a8bff749 100644 --- a/doc/api/lint.md +++ b/doc/api/lint.md @@ -13,7 +13,7 @@ POST /lint | `content` | string | yes | the .gitlab-ci.yaml content| ```bash -curl --header "Content-Type: application/json" https://gitlab.example.com/api/v4/ci/lint --data '{"content": "{ \"image\": \"ruby:2.1\", \"services\": [\"postgres\"], \"before_script\": [\"gem install bundler\", \"bundle install\", \"bundle exec rake db:create\"], \"variables\": {\"DB_NAME\": \"postgres\"}, \"types\": [\"test\", \"deploy\", \"notify\"], \"rspec\": { \"script\": \"rake spec\", \"tags\": [\"ruby\", \"postgres\"], \"only\": [\"branches\"]}}"}' +curl --header "Content-Type: application/json" https://gitlab.example.com/api/v4/ci/lint --data '{"content": "{ \"image\": \"ruby:2.6\", \"services\": [\"postgres\"], \"before_script\": [\"bundle install\", \"bundle exec rake db:create\"], \"variables\": {\"DB_NAME\": \"postgres\"}, \"types\": [\"test\", \"deploy\", \"notify\"], \"rspec\": { \"script\": \"rake spec\", \"tags\": [\"ruby\", \"postgres\"], \"only\": [\"branches\"]}}"}' ``` Be sure to copy paste the exact contents of `.gitlab-ci.yml` as YAML is very picky about indentation and spaces. diff --git a/doc/api/templates/gitlab_ci_ymls.md b/doc/api/templates/gitlab_ci_ymls.md index 8481323994b..11ec7360e06 100644 --- a/doc/api/templates/gitlab_ci_ymls.md +++ b/doc/api/templates/gitlab_ci_ymls.md @@ -120,6 +120,6 @@ Example response: ```json { "name": "Ruby", - "content": "# This file is a template, and might need editing before it works on your project.\n# Official language image. Look for the different tagged releases at:\n# https://hub.docker.com/r/library/ruby/tags/\nimage: \"ruby:2.3\"\n\n# Pick zero or more services to be used on all builds.\n# Only needed when using a docker container to run your tests in.\n# Check out: http://docs.gitlab.com/ce/ci/docker/using_docker_images.html#what-is-service\nservices:\n - mysql:latest\n - redis:latest\n - postgres:latest\n\nvariables:\n POSTGRES_DB: database_name\n\n# Cache gems in between builds\ncache:\n paths:\n - vendor/ruby\n\n# This is a basic example for a gem or script which doesn't use\n# services such as redis or postgres\nbefore_script:\n - ruby -v # Print out ruby version for debugging\n # Uncomment next line if your rails app needs a JS runtime:\n # - apt-get update -q && apt-get install nodejs -yqq\n - gem install bundler --no-document # Bundler is not installed with the image\n - bundle install -j $(nproc) --path vendor # Install dependencies into ./vendor/ruby\n\n# Optional - Delete if not using `rubocop`\nrubocop:\n script:\n - rubocop\n\nrspec:\n script:\n - rspec spec\n\nrails:\n variables:\n DATABASE_URL: \"postgresql://postgres:postgres@postgres:5432/$POSTGRES_DB\"\n script:\n - bundle exec rake db:migrate\n - bundle exec rake db:seed\n - bundle exec rake test\n" + "content": "# This file is a template, and might need editing before it works on your project.\n# Official language image. Look for the different tagged releases at:\n# https://hub.docker.com/r/library/ruby/tags/\nimage: \"ruby:2.5\"\n\n# Pick zero or more services to be used on all builds.\n# Only needed when using a docker container to run your tests in.\n# Check out: http://docs.gitlab.com/ce/ci/docker/using_docker_images.html#what-is-a-service\nservices:\n - mysql:latest\n - redis:latest\n - postgres:latest\n\nvariables:\n POSTGRES_DB: database_name\n\n# Cache gems in between builds\ncache:\n paths:\n - vendor/ruby\n\n# This is a basic example for a gem or script which doesn't use\n# services such as redis or postgres\nbefore_script:\n - ruby -v # Print out ruby version for debugging\n # Uncomment next line if your rails app needs a JS runtime:\n # - apt-get update -q && apt-get install nodejs -yqq\n - bundle install -j $(nproc) --path vendor # Install dependencies into ./vendor/ruby\n\n# Optional - Delete if not using `rubocop`\nrubocop:\n script:\n - rubocop\n\nrspec:\n script:\n - rspec spec\n\nrails:\n variables:\n DATABASE_URL: \"postgresql://postgres:postgres@postgres:5432/$POSTGRES_DB\"\n script:\n - rails db:migrate\n - rails db:seed\n - rails test\n\n# This deploy job uses a simple deploy flow to Heroku, other providers, e.g. AWS Elastic Beanstalk\n# are supported too: https://github.com/travis-ci/dpl\ndeploy:\n type: deploy\n environment: production\n script:\n - gem install dpl\n - dpl --provider=heroku --app=$HEROKU_APP_NAME --api-key=$HEROKU_PRODUCTION_KEY\n" } ``` diff --git a/doc/ci/caching/index.md b/doc/ci/caching/index.md index 4626ccfe93d..495ec099111 100644 --- a/doc/ci/caching/index.md +++ b/doc/ci/caching/index.md @@ -300,7 +300,6 @@ cache: before_script: - ruby -v # Print out ruby version for debugging - - gem install bundler --no-document # Bundler is not installed with the image - bundle install -j $(nproc) --path vendor # Install dependencies into ./vendor/ruby rspec: diff --git a/lib/gitlab/ci/templates/Ruby.gitlab-ci.yml b/lib/gitlab/ci/templates/Ruby.gitlab-ci.yml index 51d6628c63b..0d12cbc6460 100644 --- a/lib/gitlab/ci/templates/Ruby.gitlab-ci.yml +++ b/lib/gitlab/ci/templates/Ruby.gitlab-ci.yml @@ -24,7 +24,6 @@ before_script: - ruby -v # Print out ruby version for debugging # Uncomment next line if your rails app needs a JS runtime: # - apt-get update -q && apt-get install nodejs -yqq - - gem install bundler --no-document # Bundler is not installed with the image - bundle install -j $(nproc) --path vendor # Install dependencies into ./vendor/ruby # Optional - Delete if not using `rubocop` diff --git a/spec/controllers/projects/ci/lints_controller_spec.rb b/spec/controllers/projects/ci/lints_controller_spec.rb index 82c1374aa4f..cfa010c2d1c 100644 --- a/spec/controllers/projects/ci/lints_controller_spec.rb +++ b/spec/controllers/projects/ci/lints_controller_spec.rb @@ -51,7 +51,6 @@ describe Projects::Ci::LintsController do - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs - ruby -v - which ruby - - gem install bundler --no-ri --no-rdoc - bundle install --jobs $(nproc) "${FLAGS[@]}" HEREDOC end diff --git a/spec/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml b/spec/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml index 0bab94a7c2e..1e88cd120aa 100644 --- a/spec/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml +++ b/spec/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml @@ -2,7 +2,6 @@ before_script: - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs - ruby -v - which ruby - - gem install bundler --no-ri --no-rdoc - bundle install --jobs $(nproc) "${FLAGS[@]}" rspec: diff --git a/spec/lib/gitlab/ci/config/external/file/local_spec.rb b/spec/lib/gitlab/ci/config/external/file/local_spec.rb index 541deb13b97..645db642e29 100644 --- a/spec/lib/gitlab/ci/config/external/file/local_spec.rb +++ b/spec/lib/gitlab/ci/config/external/file/local_spec.rb @@ -44,7 +44,6 @@ describe Gitlab::Ci::Config::External::File::Local do - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs - ruby -v - which ruby - - gem install bundler --no-ri --no-rdoc - bundle install --jobs $(nproc) "${FLAGS[@]}" HEREDOC end diff --git a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb index 7c1a1c38736..eaf621e4140 100644 --- a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb +++ b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb @@ -11,7 +11,6 @@ describe Gitlab::Ci::Config::External::File::Remote do - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs - ruby -v - which ruby - - gem install bundler --no-ri --no-rdoc - bundle install --jobs $(nproc) "${FLAGS[@]}" HEREDOC end diff --git a/spec/lib/gitlab/ci/config/external/processor_spec.rb b/spec/lib/gitlab/ci/config/external/processor_spec.rb index 1a05f716247..dbd28e9745c 100644 --- a/spec/lib/gitlab/ci/config/external/processor_spec.rb +++ b/spec/lib/gitlab/ci/config/external/processor_spec.rb @@ -51,7 +51,6 @@ describe Gitlab::Ci::Config::External::Processor do - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs - ruby -v - which ruby - - gem install bundler --no-ri --no-rdoc - bundle install --jobs $(nproc) "${FLAGS[@]}" rspec: @@ -86,7 +85,6 @@ describe Gitlab::Ci::Config::External::Processor do - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs - ruby -v - which ruby - - gem install bundler --no-ri --no-rdoc - bundle install --jobs $(nproc) "${FLAGS[@]}" HEREDOC end diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb index 975e11e8cc1..ea6f1e20014 100644 --- a/spec/lib/gitlab/ci/config_spec.rb +++ b/spec/lib/gitlab/ci/config_spec.rb @@ -170,7 +170,6 @@ describe Gitlab::Ci::Config do before_script_values = [ "apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs", "ruby -v", "which ruby", - "gem install bundler --no-ri --no-rdoc", "bundle install --jobs $(nproc) \"${FLAGS[@]}\"" ] variables = { diff --git a/spec/support/gitlab_stubs/gitlab_ci.yml b/spec/support/gitlab_stubs/gitlab_ci.yml index e55a61b2b94..f3755e52b2c 100644 --- a/spec/support/gitlab_stubs/gitlab_ci.yml +++ b/spec/support/gitlab_stubs/gitlab_ci.yml @@ -1,9 +1,8 @@ -image: ruby:2.1 +image: ruby:2.6 services: - postgres before_script: - - gem install bundler - bundle install - bundle exec rake db:create -- cgit v1.2.1 From 92eff44f0b94bbb7d871eeff326ff7d9c369e149 Mon Sep 17 00:00:00 2001 From: Constance Okoghenun Date: Wed, 2 Jan 2019 05:42:31 +0100 Subject: Made discussion filters only visible in merge request discussions tab Discussion filters will be hidden on Commits, Pipelines, and Changes tabs on merge requests page. This does not affect its behavior on issues page --- .../notes/components/discussion_filter.vue | 20 ++++++++++-- app/assets/javascripts/notes/constants.js | 1 + ...vity-filter-dropdown-in-discussion-tab-only.yml | 5 +++ .../notes/components/discussion_filter_spec.js | 36 ++++++++++++++++++++-- 4 files changed, 58 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/54206-show-the-activity-filter-dropdown-in-discussion-tab-only.yml diff --git a/app/assets/javascripts/notes/components/discussion_filter.vue b/app/assets/javascripts/notes/components/discussion_filter.vue index 86c114a761a..f5c410211b6 100644 --- a/app/assets/javascripts/notes/components/discussion_filter.vue +++ b/app/assets/javascripts/notes/components/discussion_filter.vue @@ -2,7 +2,11 @@ import $ from 'jquery'; import { mapGetters, mapActions } from 'vuex'; import Icon from '~/vue_shared/components/icon.vue'; -import { DISCUSSION_FILTERS_DEFAULT_VALUE, HISTORY_ONLY_FILTER_VALUE } from '../constants'; +import { + DISCUSSION_FILTERS_DEFAULT_VALUE, + HISTORY_ONLY_FILTER_VALUE, + DISCUSSION_TAB_LABEL, +} from '../constants'; export default { components: { @@ -23,6 +27,7 @@ export default { return { currentValue: this.selectedValue, defaultValue: DISCUSSION_FILTERS_DEFAULT_VALUE, + displayFilters: true, }; }, computed: { @@ -32,6 +37,14 @@ export default { return this.filters.find(filter => filter.value === this.currentValue); }, }, + created() { + if (window.mrTabs) { + const { eventHub, currentTab } = window.mrTabs; + + eventHub.$on('MergeRequestTabChange', this.toggleFilters); + this.toggleFilters(currentTab); + } + }, mounted() { this.toggleCommentsForm(); }, @@ -51,12 +64,15 @@ export default { toggleCommentsForm() { this.setCommentsDisabled(this.currentValue === HISTORY_ONLY_FILTER_VALUE); }, + toggleFilters(tab) { + this.displayFilters = tab === DISCUSSION_TAB_LABEL; + }, }, };