From 68acc453afc364674f94b32888d401b9a540a4b3 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Fri, 26 Aug 2022 14:40:34 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-2-stable-ee --- app/models/integrations/zentao.rb | 4 + app/models/issue.rb | 8 +- lib/gitlab/set_cache.rb | 4 + lib/gitlab/zentao/client.rb | 50 +++++++-- spec/lib/gitlab/reactive_cache_set_cache_spec.rb | 14 +++ spec/lib/gitlab/zentao/client_spec.rb | 127 +++++++++++++++++++---- spec/models/integrations/zentao_spec.rb | 20 ++++ spec/models/issue_spec.rb | 14 ++- 8 files changed, 209 insertions(+), 32 deletions(-) diff --git a/app/models/integrations/zentao.rb b/app/models/integrations/zentao.rb index 53194089296..459756c865b 100644 --- a/app/models/integrations/zentao.rb +++ b/app/models/integrations/zentao.rb @@ -69,6 +69,10 @@ module Integrations } end + def client_url + api_url.presence || url + end + def self.to_param name.demodulize.downcase end diff --git a/app/models/issue.rb b/app/models/issue.rb index cae42115bef..56ffb9c1bb2 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -444,7 +444,13 @@ class Issue < ApplicationRecord return to_branch_name unless project.repository.branch_exists?(to_branch_name) start_counting_from = 2 - Uniquify.new(start_counting_from).string(-> (counter) { "#{to_branch_name}-#{counter}" }) do |suggested_branch_name| + + branch_name_generator = -> (counter) do + suffix = counter > 5 ? SecureRandom.hex(8) : counter + "#{to_branch_name}-#{suffix}" + end + + Uniquify.new(start_counting_from).string(branch_name_generator) do |suggested_branch_name| project.repository.branch_exists?(suggested_branch_name) end end diff --git a/lib/gitlab/set_cache.rb b/lib/gitlab/set_cache.rb index feb2c3c1d7d..896e7e3f65e 100644 --- a/lib/gitlab/set_cache.rb +++ b/lib/gitlab/set_cache.rb @@ -68,6 +68,10 @@ module Gitlab with { |redis| redis.ttl(cache_key(key)) } end + def count(key) + with { |redis| redis.scard(cache_key(key)) } + end + private def with(&blk) diff --git a/lib/gitlab/zentao/client.rb b/lib/gitlab/zentao/client.rb index 4da4631eecf..0de4ca3a09b 100644 --- a/lib/gitlab/zentao/client.rb +++ b/lib/gitlab/zentao/client.rb @@ -5,6 +5,10 @@ module Gitlab class Client Error = Class.new(StandardError) ConfigError = Class.new(Error) + RequestError = Class.new(Error) + + CACHE_MAX_SET_SIZE = 5_000 + CACHE_TTL = 1.month.freeze attr_reader :integration @@ -29,11 +33,21 @@ module Gitlab end def fetch_issues(params = {}) - get("products/#{zentao_product_xid}/issues", params) + get("products/#{zentao_product_xid}/issues", params).tap do |response| + mark_issues_as_seen_in_product(response['issues']) + end end def fetch_issue(issue_id) - raise Gitlab::Zentao::Client::Error, 'invalid issue id' unless issue_id_pattern.match(issue_id) + raise Error, 'invalid issue id' unless issue_id_pattern.match(issue_id) + + # Only return issues that are associated with the product configured in + # the integration. Due to a lack of available data in the ZenTao APIs, we + # can only determine if an issue belongs to a product if the issue was + # previously returned in the `#fetch_issues` call. + # + # See https://gitlab.com/gitlab-org/gitlab/-/issues/360372#note_1016963713 + raise RequestError unless issue_seen_in_product?(issue_id) get("issues/#{issue_id}") end @@ -48,17 +62,15 @@ module Gitlab options = { headers: headers, query: params } response = Gitlab::HTTP.get(url(path), options) - raise Gitlab::Zentao::Client::Error, 'request error' unless response.success? + raise RequestError unless response.success? Gitlab::Json.parse(response.body) rescue JSON::ParserError - raise Gitlab::Zentao::Client::Error, 'invalid response format' + raise Error, 'invalid response format' end def url(path) - host = integration.api_url.presence || integration.url - - URI.parse(Gitlab::Utils.append_path(host, "api.php/v1/#{path}")) + URI.parse(Gitlab::Utils.append_path(integration.client_url, "api.php/v1/#{path}")) end def headers @@ -71,6 +83,30 @@ module Gitlab def zentao_product_xid integration.zentao_product_xid end + + def issue_ids_cache_key + @issue_ids_cache_key ||= [ + :zentao_product_issues, + OpenSSL::Digest::SHA256.hexdigest(integration.client_url), + zentao_product_xid + ].join(':') + end + + def issue_ids_cache + @issue_ids_cache ||= ::Gitlab::SetCache.new(expires_in: CACHE_TTL) + end + + def mark_issues_as_seen_in_product(issues) + return unless issues && issue_ids_cache.count(issue_ids_cache_key) < CACHE_MAX_SET_SIZE + + ids = issues.map { _1['id'] } + + issue_ids_cache.write(issue_ids_cache_key, ids) + end + + def issue_seen_in_product?(id) + issue_ids_cache.include?(issue_ids_cache_key, id) + end end end end diff --git a/spec/lib/gitlab/reactive_cache_set_cache_spec.rb b/spec/lib/gitlab/reactive_cache_set_cache_spec.rb index f405b2ad86e..207ac1c0eaa 100644 --- a/spec/lib/gitlab/reactive_cache_set_cache_spec.rb +++ b/spec/lib/gitlab/reactive_cache_set_cache_spec.rb @@ -72,4 +72,18 @@ RSpec.describe Gitlab::ReactiveCacheSetCache, :clean_gitlab_redis_cache do it { is_expected.to be(true) } end end + + describe 'count' do + subject { cache.count(cache_prefix) } + + it { is_expected.to be(0) } + + context 'item added' do + before do + cache.write(cache_prefix, 'test_item') + end + + it { is_expected.to be(1) } + end + end end diff --git a/spec/lib/gitlab/zentao/client_spec.rb b/spec/lib/gitlab/zentao/client_spec.rb index 135f13e6265..b17ad867f0d 100644 --- a/spec/lib/gitlab/zentao/client_spec.rb +++ b/spec/lib/gitlab/zentao/client_spec.rb @@ -2,17 +2,21 @@ require 'spec_helper' -RSpec.describe Gitlab::Zentao::Client do - subject(:integration) { described_class.new(zentao_integration) } +RSpec.describe Gitlab::Zentao::Client, :clean_gitlab_redis_cache do + subject(:client) { described_class.new(zentao_integration) } let(:zentao_integration) { create(:zentao_integration) } def mock_get_products_url - integration.send(:url, "products/#{zentao_integration.zentao_product_xid}") + client.send(:url, "products/#{zentao_integration.zentao_product_xid}") + end + + def mock_fetch_issues_url + client.send(:url, "products/#{zentao_integration.zentao_product_xid}/issues") end def mock_fetch_issue_url(issue_id) - integration.send(:url, "issues/#{issue_id}") + client.send(:url, "issues/#{issue_id}") end let(:mock_headers) do @@ -29,13 +33,13 @@ RSpec.describe Gitlab::Zentao::Client do let(:zentao_integration) { nil } it 'raises ConfigError' do - expect { integration }.to raise_error(described_class::ConfigError) + expect { client }.to raise_error(described_class::ConfigError) end end context 'integration is provided' do it 'is initialized successfully' do - expect { integration }.not_to raise_error + expect { client }.not_to raise_error end end end @@ -50,7 +54,7 @@ RSpec.describe Gitlab::Zentao::Client do end it 'fetches the product' do - expect(integration.fetch_product(zentao_integration.zentao_product_xid)).to eq mock_response + expect(client.fetch_product(zentao_integration.zentao_product_xid)).to eq mock_response end end @@ -62,8 +66,8 @@ RSpec.describe Gitlab::Zentao::Client do it 'fetches the empty product' do expect do - integration.fetch_product(zentao_integration.zentao_product_xid) - end.to raise_error(Gitlab::Zentao::Client::Error, 'request error') + client.fetch_product(zentao_integration.zentao_product_xid) + end.to raise_error(Gitlab::Zentao::Client::RequestError) end end @@ -75,7 +79,7 @@ RSpec.describe Gitlab::Zentao::Client do it 'fetches the empty product' do expect do - integration.fetch_product(zentao_integration.zentao_product_xid) + client.fetch_product(zentao_integration.zentao_product_xid) end.to raise_error(Gitlab::Zentao::Client::Error, 'invalid response format') end end @@ -89,7 +93,7 @@ RSpec.describe Gitlab::Zentao::Client do end it 'responds with success' do - expect(integration.ping[:success]).to eq true + expect(client.ping[:success]).to eq true end end @@ -100,7 +104,69 @@ RSpec.describe Gitlab::Zentao::Client do end it 'responds with unsuccess' do - expect(integration.ping[:success]).to eq false + expect(client.ping[:success]).to eq false + end + end + end + + describe '#fetch_issues' do + let(:mock_response) { { 'issues' => [{ 'id' => 'story-1' }, { 'id' => 'bug-11' }] } } + + before do + WebMock.stub_request(:get, mock_fetch_issues_url) + .with(mock_headers).to_return(status: 200, body: mock_response.to_json) + end + + it 'returns the response' do + expect(client.fetch_issues).to eq(mock_response) + end + + describe 'marking the issues as seen in the product' do + let(:cache) { ::Gitlab::SetCache.new } + let(:cache_key) do + [ + :zentao_product_issues, + OpenSSL::Digest::SHA256.hexdigest(zentao_integration.client_url), + zentao_integration.zentao_product_xid + ].join(':') + end + + it 'adds issue ids to the cache' do + expect { client.fetch_issues }.to change { cache.read(cache_key) } + .from(be_empty) + .to match_array(%w[bug-11 story-1]) + end + + it 'does not add issue ids to the cache if max set size has been reached' do + cache.write(cache_key, %w[foo bar]) + stub_const("#{described_class}::CACHE_MAX_SET_SIZE", 1) + + client.fetch_issues + + expect(cache.read(cache_key)).to match_array(%w[foo bar]) + end + + it 'does not duplicate issue ids in the cache' do + client.fetch_issues + client.fetch_issues + + expect(cache.read(cache_key)).to match_array(%w[bug-11 story-1]) + end + + it 'touches the cache ttl every time issues are fetched' do + fresh_ttl = 1.month.to_i + + freeze_time do + client.fetch_issues + + expect(cache.ttl(cache_key)).to eq(fresh_ttl) + end + + travel_to(1.minute.from_now) do + client.fetch_issues + + expect(cache.ttl(cache_key)).to eq(fresh_ttl) + end end end end @@ -109,9 +175,9 @@ RSpec.describe Gitlab::Zentao::Client do context 'with invalid id' do let(:invalid_ids) { ['story', 'story-', '-', '123', ''] } - it 'returns empty object' do + it 'raises Error' do invalid_ids.each do |id| - expect { integration.fetch_issue(id) } + expect { client.fetch_issue(id) } .to raise_error(Gitlab::Zentao::Client::Error, 'invalid issue id') end end @@ -120,12 +186,31 @@ RSpec.describe Gitlab::Zentao::Client do context 'with valid id' do let(:valid_ids) { %w[story-1 bug-23] } - it 'fetches current issue' do - valid_ids.each do |id| - WebMock.stub_request(:get, mock_fetch_issue_url(id)) - .with(mock_headers).to_return(status: 200, body: { issue: { id: id } }.to_json) + context 'when issue has been seen on the index' do + before do + issues_body = { issues: valid_ids.map { { id: _1 } } }.to_json + + WebMock.stub_request(:get, mock_fetch_issues_url) + .with(mock_headers).to_return(status: 200, body: issues_body) + + client.fetch_issues + end + + it 'fetches the issue' do + valid_ids.each do |id| + WebMock.stub_request(:get, mock_fetch_issue_url(id)) + .with(mock_headers).to_return(status: 200, body: { issue: { id: id } }.to_json) + + expect(client.fetch_issue(id).dig('issue', 'id')).to eq id + end + end + end - expect(integration.fetch_issue(id).dig('issue', 'id')).to eq id + context 'when issue has not been seen on the index' do + it 'raises RequestError' do + valid_ids.each do |id| + expect { client.fetch_issue(id) }.to raise_error(Gitlab::Zentao::Client::RequestError) + end end end end @@ -135,7 +220,7 @@ RSpec.describe Gitlab::Zentao::Client do context 'api url' do shared_examples 'joins api_url correctly' do it 'verify url' do - expect(integration.send(:url, "products/1").to_s) + expect(client.send(:url, "products/1").to_s) .to eq("https://jihudemo.zentao.net/zentao/api.php/v1/products/1") end end @@ -157,7 +242,7 @@ RSpec.describe Gitlab::Zentao::Client do let(:zentao_integration) { create(:zentao_integration, url: 'https://jihudemo.zentao.net') } it 'joins url correctly' do - expect(integration.send(:url, "products/1").to_s) + expect(client.send(:url, "products/1").to_s) .to eq("https://jihudemo.zentao.net/api.php/v1/products/1") end end diff --git a/spec/models/integrations/zentao_spec.rb b/spec/models/integrations/zentao_spec.rb index 4ef977ba3d2..1a32453819d 100644 --- a/spec/models/integrations/zentao_spec.rb +++ b/spec/models/integrations/zentao_spec.rb @@ -81,4 +81,24 @@ RSpec.describe Integrations::Zentao do expect(zentao_integration.help).not_to be_empty end end + + describe '#client_url' do + subject(:integration) { build(:zentao_integration, api_url: api_url, url: 'url').client_url } + + context 'when api_url is set' do + let(:api_url) { 'api_url' } + + it 'returns the api_url' do + is_expected.to eq(api_url) + end + end + + context 'when api_url is not set' do + let(:api_url) { '' } + + it 'returns the url' do + is_expected.to eq('url') + end + end + end end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 89c440dc49c..90e87a7ed2e 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -670,14 +670,22 @@ RSpec.describe Issue do end describe '#to_branch_name exists ending with -index' do - before do + it 'returns #to_branch_name ending with max index + 1' do allow(repository).to receive(:branch_exists?).and_return(true) allow(repository).to receive(:branch_exists?).with("#{subject.to_branch_name}-3").and_return(false) - end - it 'returns #to_branch_name ending with max index + 1' do expect(subject.suggested_branch_name).to eq("#{subject.to_branch_name}-3") end + + context 'when branch name still exists after 5 attempts' do + it 'returns #to_branch_name ending with random characters' do + allow(repository).to receive(:branch_exists?).with(subject.to_branch_name).and_return(true) + allow(repository).to receive(:branch_exists?).with(/#{subject.to_branch_name}-\d/).and_return(true) + allow(repository).to receive(:branch_exists?).with(/#{subject.to_branch_name}-\h{8}/).and_return(false) + + expect(subject.suggested_branch_name).to match(/#{subject.to_branch_name}-\h{8}/) + end + end end end -- cgit v1.2.3