diff options
| -rw-r--r-- | app/models/application_record.rb | 6 | ||||
| -rw-r--r-- | app/models/gpg_signature.rb | 7 | ||||
| -rw-r--r-- | app/services/error_tracking/list_projects_service.rb | 44 | ||||
| -rw-r--r-- | changelogs/unreleased/bvl-fix-race-condition-creating-signature.yml | 5 | ||||
| -rw-r--r-- | lib/gitlab/gpg/commit.rb | 7 | ||||
| -rw-r--r-- | spec/models/application_record_spec.rb | 15 | ||||
| -rw-r--r-- | spec/models/gpg_signature_spec.rb | 35 | ||||
| -rw-r--r-- | spec/services/error_tracking/list_projects_service_spec.rb | 149 |
8 files changed, 263 insertions, 5 deletions
diff --git a/app/models/application_record.rb b/app/models/application_record.rb index c4e310e638d..a3d662d8250 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -7,6 +7,12 @@ class ApplicationRecord < ActiveRecord::Base where(id: ids) end + def self.safe_find_or_create_by!(*args) + safe_find_or_create_by(*args).tap do |record| + record.validate! unless record.persisted? + end + end + def self.safe_find_or_create_by(*args) transaction(requires_new: true) do find_or_create_by(*args) diff --git a/app/models/gpg_signature.rb b/app/models/gpg_signature.rb index 0816778deae..7f9ff7bbda6 100644 --- a/app/models/gpg_signature.rb +++ b/app/models/gpg_signature.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class GpgSignature < ActiveRecord::Base +class GpgSignature < ApplicationRecord include ShaAttribute sha_attribute :commit_sha @@ -33,6 +33,11 @@ class GpgSignature < ActiveRecord::Base ) end + def self.safe_create!(attributes) + create_with(attributes) + .safe_find_or_create_by!(commit_sha: attributes[:commit_sha]) + end + def gpg_key=(model) case model when GpgKey diff --git a/app/services/error_tracking/list_projects_service.rb b/app/services/error_tracking/list_projects_service.rb new file mode 100644 index 00000000000..c6e8be0f2be --- /dev/null +++ b/app/services/error_tracking/list_projects_service.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module ErrorTracking + class ListProjectsService < ::BaseService + def execute + return error('access denied') unless can_read? + + setting = project_error_tracking_setting + + unless setting.valid? + return error(setting.errors.full_messages.join(', '), :bad_request) + end + + begin + result = setting.list_sentry_projects + rescue Sentry::Client::Error => e + return error(e.message, :bad_request) + rescue Sentry::Client::SentryError => e + return error(e.message, :unprocessable_entity) + end + + success(projects: result[:projects]) + end + + private + + def project_error_tracking_setting + (project.error_tracking_setting || project.build_error_tracking_setting).tap do |setting| + setting.api_url = ErrorTracking::ProjectErrorTrackingSetting.build_api_url_from( + api_host: params[:api_host], + organization_slug: nil, + project_slug: nil + ) + + setting.token = params[:token] + setting.enabled = true + end + end + + def can_read? + can?(current_user, :read_sentry_issue, project) + end + end +end diff --git a/changelogs/unreleased/bvl-fix-race-condition-creating-signature.yml b/changelogs/unreleased/bvl-fix-race-condition-creating-signature.yml new file mode 100644 index 00000000000..307b4f526bb --- /dev/null +++ b/changelogs/unreleased/bvl-fix-race-condition-creating-signature.yml @@ -0,0 +1,5 @@ +--- +title: Avoid race conditions when creating GpgSignature +merge_request: 24939 +author: +type: fixed diff --git a/lib/gitlab/gpg/commit.rb b/lib/gitlab/gpg/commit.rb index 4fbb87385c3..5ff415b6126 100644 --- a/lib/gitlab/gpg/commit.rb +++ b/lib/gitlab/gpg/commit.rb @@ -88,9 +88,10 @@ module Gitlab def create_cached_signature! using_keychain do |gpg_key| - signature = GpgSignature.new(attributes(gpg_key)) - signature.save! unless Gitlab::Database.read_only? - signature + attributes = attributes(gpg_key) + break GpgSignature.new(attributes) if Gitlab::Database.read_only? + + GpgSignature.safe_create!(attributes) end end diff --git a/spec/models/application_record_spec.rb b/spec/models/application_record_spec.rb index ca23f581fdc..fd25132ed3a 100644 --- a/spec/models/application_record_spec.rb +++ b/spec/models/application_record_spec.rb @@ -11,7 +11,7 @@ describe ApplicationRecord do end end - describe '#safe_find_or_create_by' do + describe '.safe_find_or_create_by' do it 'creates the user avoiding race conditions' do expect(Suggestion).to receive(:find_or_create_by).and_raise(ActiveRecord::RecordNotUnique) allow(Suggestion).to receive(:find_or_create_by).and_call_original @@ -20,4 +20,17 @@ describe ApplicationRecord do .to change { Suggestion.count }.by(1) end end + + describe '.safe_find_or_create_by!' do + it 'creates a record using safe_find_or_create_by' do + expect(Suggestion).to receive(:find_or_create_by).and_call_original + + expect(Suggestion.safe_find_or_create_by!(build(:suggestion).attributes)) + .to be_a(Suggestion) + end + + it 'raises a validation error if the record was not persisted' do + expect { Suggestion.find_or_create_by!(note: nil) }.to raise_error(ActiveRecord::RecordInvalid) + end + end end diff --git a/spec/models/gpg_signature_spec.rb b/spec/models/gpg_signature_spec.rb index cdd7dea2064..e90319c39b1 100644 --- a/spec/models/gpg_signature_spec.rb +++ b/spec/models/gpg_signature_spec.rb @@ -23,6 +23,41 @@ RSpec.describe GpgSignature do it { is_expected.to validate_presence_of(:gpg_key_primary_keyid) } end + describe '.safe_create!' do + let(:attributes) do + { + commit_sha: commit_sha, + project: project, + gpg_key_primary_keyid: gpg_key.keyid + } + end + + it 'finds a signature by commit sha if it existed' do + gpg_signature + + expect(described_class.safe_create!(commit_sha: commit_sha)).to eq(gpg_signature) + end + + it 'creates a new signature if it was not found' do + expect { described_class.safe_create!(attributes) }.to change { described_class.count }.by(1) + end + + it 'assigns the correct attributes when creating' do + signature = described_class.safe_create!(attributes) + + expect(signature.project).to eq(project) + expect(signature.commit_sha).to eq(commit_sha) + expect(signature.gpg_key_primary_keyid).to eq(gpg_key.keyid) + end + + it 'does not raise an error in case of a race condition' do + expect(described_class).to receive(:find_or_create_by).and_raise(ActiveRecord::RecordNotUnique) + allow(described_class).to receive(:find_or_create_by).and_call_original + + described_class.safe_create!(attributes) + end + end + describe '#commit' do it 'fetches the commit through the project' do expect_any_instance_of(Project).to receive(:commit).with(commit_sha).and_return(commit) diff --git a/spec/services/error_tracking/list_projects_service_spec.rb b/spec/services/error_tracking/list_projects_service_spec.rb new file mode 100644 index 00000000000..ee9c59e3f65 --- /dev/null +++ b/spec/services/error_tracking/list_projects_service_spec.rb @@ -0,0 +1,149 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ErrorTracking::ListProjectsService do + set(:user) { create(:user) } + set(:project) { create(:project) } + + let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project' } + let(:token) { 'test-token' } + let(:new_api_host) { 'https://gitlab.com/' } + let(:new_token) { 'new-token' } + let(:params) { ActionController::Parameters.new(api_host: new_api_host, token: new_token) } + + let(:error_tracking_setting) do + create(:project_error_tracking_setting, api_url: sentry_url, token: token, project: project) + end + + subject { described_class.new(project, user, params) } + + before do + project.add_reporter(user) + end + + describe '#execute' do + let(:result) { subject.execute } + + context 'with authorized user' do + before do + expect(project).to receive(:error_tracking_setting).at_least(:once) + .and_return(error_tracking_setting) + end + + context 'set model attributes to new values' do + let(:new_api_url) { new_api_host + 'api/0/projects/' } + + before do + expect(error_tracking_setting).to receive(:list_sentry_projects) + .and_return({ projects: [] }) + end + + it 'uses new api_url and token' do + subject.execute + + expect(error_tracking_setting.api_url).to eq(new_api_url) + expect(error_tracking_setting.token).to eq(new_token) + error_tracking_setting.reload + expect(error_tracking_setting.api_url).to eq(sentry_url) + expect(error_tracking_setting.token).to eq(token) + end + end + + context 'sentry client raises exception' do + before do + expect(error_tracking_setting).to receive(:list_sentry_projects) + .and_raise(Sentry::Client::Error, 'Sentry response error: 500') + end + + it 'returns error response' do + expect(result[:message]).to eq('Sentry response error: 500') + expect(result[:http_status]).to eq(:bad_request) + end + end + + context 'with invalid url' do + let(:params) do + ActionController::Parameters.new( + api_host: 'https://localhost', + token: new_token + ) + end + + before do + error_tracking_setting.enabled = false + end + + it 'returns error' do + expect(result[:message]).to start_with('Api url is blocked') + expect(error_tracking_setting).not_to be_valid + end + end + + context 'when list_sentry_projects returns projects' do + let(:projects) { [:list, :of, :projects] } + + before do + expect(error_tracking_setting) + .to receive(:list_sentry_projects).and_return(projects: projects) + end + + it 'returns the projects' do + expect(result).to eq(status: :success, projects: projects) + end + end + end + + context 'with unauthorized user' do + before do + project.add_guest(user) + end + + it 'returns error' do + expect(result).to include(status: :error, message: 'access denied') + end + end + + context 'with error tracking disabled' do + before do + expect(project).to receive(:error_tracking_setting).at_least(:once) + .and_return(error_tracking_setting) + expect(error_tracking_setting) + .to receive(:list_sentry_projects).and_return(projects: []) + + error_tracking_setting.enabled = false + end + + it 'ignores enabled flag' do + expect(result).to include(status: :success, projects: []) + end + end + + context 'error_tracking_setting is nil' do + let(:error_tracking_setting) { build(:project_error_tracking_setting) } + let(:new_api_url) { new_api_host + 'api/0/projects/' } + + before do + expect(project).to receive(:build_error_tracking_setting).once + .and_return(error_tracking_setting) + + expect(error_tracking_setting).to receive(:list_sentry_projects) + .and_return(projects: [:project1, :project2]) + end + + it 'builds a new error_tracking_setting' do + expect(project.error_tracking_setting).to be_nil + + expect(result[:projects]).to eq([:project1, :project2]) + + expect(error_tracking_setting.api_url).to eq(new_api_url) + expect(error_tracking_setting.token).to eq(new_token) + expect(error_tracking_setting.enabled).to be true + expect(error_tracking_setting.persisted?).to be false + expect(error_tracking_setting.project_id).not_to be_nil + + expect(project.error_tracking_setting).to be_nil + end + end + end +end |