From c13f712c77e0837760e79293b6fb41c734741e77 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Fri, 18 Aug 2017 17:43:23 -0700 Subject: [PATCH 01/57] implement Repository#== so that with_repo_branch_commit can properly short-circuit --- app/models/repository.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/models/repository.rb b/app/models/repository.rb index c1e4fcf94a4..1c3080c0efd 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -60,6 +60,12 @@ class Repository @project = project end + def equals(other) + @disk_path == other.disk_path + end + + alias_method :==, :equals + def raw_repository return nil unless full_path From 00b440443808fba04fc55f7e77c61f79e29c21ea Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Mon, 21 Aug 2017 15:20:44 -0700 Subject: [PATCH 02/57] skip the branch fetch if we already have the sha --- app/models/repository.rb | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 1c3080c0efd..61fab9764e8 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -991,23 +991,27 @@ class Repository end def with_repo_branch_commit(start_repository, start_branch_name) - return yield(nil) if start_repository.empty_repo? + tmp_ref = nil + return yield nil if start_repository.empty_repo? - branch_name_or_sha = + branch_commit = if start_repository == self - start_branch_name + commit(start_branch_name) else - tmp_ref = fetch_ref( - start_repository.path_to_repo, - "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}", - "refs/tmp/#{SecureRandom.hex}/head" - ) + sha = start_repository.find_branch(start_branch_name).target + commit(sha) || + begin + tmp_ref = fetch_ref( + start_repository.path_to_repo, + "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}", + "refs/tmp/#{SecureRandom.hex}/head" + ) - start_repository.commit(start_branch_name).sha + commit(start_repository.commit(start_branch_name).sha) + end end - yield(commit(branch_name_or_sha)) - + yield branch_commit ensure rugged.references.delete(tmp_ref) if tmp_ref end From 5f811894a8ba0d85298cc695c360f171d30c193c Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 23 Aug 2017 20:25:44 +0800 Subject: [PATCH 03/57] Remove unwanted refs after importing a project Because we don't need them, and they would slow down the repository, keeping unneeded objects as well. We could also consider doing this in regular housekeeping. --- app/models/project.rb | 2 +- app/services/projects/housekeeping_service.rb | 2 ++ .../projects/import_export/cleanup_service.rb | 33 +++++++++++++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 app/services/projects/import_export/cleanup_service.rb diff --git a/app/models/project.rb b/app/models/project.rb index 37f4dd08355..72da4e8eb2e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -372,7 +372,7 @@ class Project < ActiveRecord::Base if Gitlab::ImportSources.importer_names.include?(project.import_type) && project.repo_exists? project.run_after_commit do begin - Projects::HousekeepingService.new(project).execute + Projects::ImportExport::CleanupService.new(project).execute rescue Projects::HousekeepingService::LeaseTaken => e Rails.logger.info("Could not perform housekeeping for project #{project.full_path} (#{project.id}): #{e}") end diff --git a/app/services/projects/housekeeping_service.rb b/app/services/projects/housekeeping_service.rb index d66ef676088..dcef8b66215 100644 --- a/app/services/projects/housekeeping_service.rb +++ b/app/services/projects/housekeeping_service.rb @@ -26,6 +26,8 @@ module Projects lease_uuid = try_obtain_lease raise LeaseTaken unless lease_uuid.present? + yield if block_given? + execute_gitlab_shell_gc(lease_uuid) end diff --git a/app/services/projects/import_export/cleanup_service.rb b/app/services/projects/import_export/cleanup_service.rb new file mode 100644 index 00000000000..54d7fb88a91 --- /dev/null +++ b/app/services/projects/import_export/cleanup_service.rb @@ -0,0 +1,33 @@ +module Projects + module ImportExport + class CleanupService + RESERVED_REFS_REGEXP = + %r{\Arefs/(?:heads|tags|merge\-requests|keep\-around|environments)/} + + attr_reader :project + + def initialize(project) + @project = project + end + + # This could raise Projects::HousekeepingService::LeaseTaken + def execute + Projects::HousekeepingService.new(project).execute do + garbage_refs.each(&rugged.references.method(:delete)) + end + end + + private + + def garbage_refs + @garbage_refs ||= rugged.references.reject do |ref| + ref.name =~ RESERVED_REFS_REGEXP + end + end + + def rugged + @rugged ||= project.repository.rugged + end + end + end +end From 140ac8d2ad81f03f67dddcb565458e9baee79755 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 23 Aug 2017 21:51:21 +0800 Subject: [PATCH 04/57] Add changelog and tests --- .../projects/import_export/cleanup_service.rb | 5 +- .../36807-gc-unwanted-refs-after-import.yml | 5 ++ spec/models/project_spec.rb | 13 ++++- .../projects/housekeeping_service_spec.rb | 13 +++++ .../import_export/cleanup_service_spec.rb | 55 +++++++++++++++++++ 5 files changed, 88 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/36807-gc-unwanted-refs-after-import.yml create mode 100644 spec/services/projects/import_export/cleanup_service_spec.rb diff --git a/app/services/projects/import_export/cleanup_service.rb b/app/services/projects/import_export/cleanup_service.rb index 54d7fb88a91..75eaee0cb7b 100644 --- a/app/services/projects/import_export/cleanup_service.rb +++ b/app/services/projects/import_export/cleanup_service.rb @@ -1,8 +1,11 @@ module Projects module ImportExport class CleanupService + RESERVED_REFS_NAMES = + %w[heads tags merge-requests keep-around environments] RESERVED_REFS_REGEXP = - %r{\Arefs/(?:heads|tags|merge\-requests|keep\-around|environments)/} + %r{\Arefs/(?:#{ + RESERVED_REFS_NAMES.map(&Regexp.method(:escape)).join('|')})/}x attr_reader :project diff --git a/changelogs/unreleased/36807-gc-unwanted-refs-after-import.yml b/changelogs/unreleased/36807-gc-unwanted-refs-after-import.yml new file mode 100644 index 00000000000..a37de4325bb --- /dev/null +++ b/changelogs/unreleased/36807-gc-unwanted-refs-after-import.yml @@ -0,0 +1,5 @@ +--- +title: Remove unwanted refs after importing a project +merge_request: 13766 +author: +type: other diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 2e613c44357..130c0739033 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1563,10 +1563,18 @@ describe Project do describe 'project import state transitions' do context 'state transition: [:started] => [:finished]' do - let(:housekeeping_service) { spy } + let(:cleanup_service) { spy(:cleanup_service) } + let(:housekeeping_service) { spy(:housekeeping_service) } before do - allow(Projects::HousekeepingService).to receive(:new) { housekeeping_service } + allow(Projects::ImportExport::CleanupService) + .to receive(:new) { cleanup_service } + + allow(cleanup_service) + .to receive(:execute) { housekeeping_service.execute } + + allow(Projects::HousekeepingService) + .to receive(:new) { housekeeping_service } end it 'resets project import_error' do @@ -1581,6 +1589,7 @@ describe Project do project.import_finish + expect(cleanup_service).to have_received(:execute) expect(housekeeping_service).to have_received(:execute) end diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb index 385f56e447f..6e916a523fe 100644 --- a/spec/services/projects/housekeeping_service_spec.rb +++ b/spec/services/projects/housekeeping_service_spec.rb @@ -23,6 +23,12 @@ describe Projects::HousekeepingService do expect(project.reload.pushes_since_gc).to eq(0) end + it 'yields the block if given' do + expect do |b| + subject.execute(&b) + end.to yield_with_no_args + end + context 'when no lease can be obtained' do before do expect(subject).to receive(:try_obtain_lease).and_return(false) @@ -39,6 +45,13 @@ describe Projects::HousekeepingService do expect { subject.execute }.to raise_error(Projects::HousekeepingService::LeaseTaken) end.not_to change { project.pushes_since_gc } end + + it 'does not yield' do + expect do |b| + expect { subject.execute(&b) } + .to raise_error(Projects::HousekeepingService::LeaseTaken) + end.not_to yield_with_no_args + end end end diff --git a/spec/services/projects/import_export/cleanup_service_spec.rb b/spec/services/projects/import_export/cleanup_service_spec.rb new file mode 100644 index 00000000000..b46efc40a2f --- /dev/null +++ b/spec/services/projects/import_export/cleanup_service_spec.rb @@ -0,0 +1,55 @@ +require 'spec_helper' + +describe Projects::ImportExport::CleanupService do + subject { described_class.new(project) } + + let(:project) { create(:project, :repository) } + let(:repository) { project.repository } + let(:sha) { project.commit.sha } + let(:housekeeping_service) { double(:housekeeping_service) } + + describe '#execute' do + before do + allow(Projects::HousekeepingService) + .to receive(:new).with(project).and_return(housekeeping_service) + + allow(housekeeping_service) + .to receive(:execute).and_yield + end + + it 'performs housekeeping' do + subject.execute + + expect(housekeeping_service).to have_received(:execute) + end + + context 'with some refs in refs/pull/**/*' do + before do + repository.write_ref('refs/pull/1/head', sha) + repository.write_ref('refs/pull/1/merge', sha) + + subject.execute + end + + it 'removes refs/pull/**/*' do + expect(repository.rugged.references.map(&:name)) + .not_to include(%r{\Arefs/pull/}) + end + end + + described_class::RESERVED_REFS_NAMES.each do |name| + context "with a ref in refs/#{name}/tmp" do + before do + repository.write_ref("refs/#{name}/tmp", sha) + + subject.execute + end + + it "does not remove refs/#{name}/tmp" do + expect(repository.rugged.references.map(&:name)) + .to include("refs/#{name}/tmp") + end + end + end + end +end From bea29327af6d14bea58f82304115735fd0b13d3f Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 24 Aug 2017 17:26:18 +0800 Subject: [PATCH 05/57] Just use methods over constants, so that we could take the advantage that if they're not used, we could garbage collect those data, reducing memory footprint. --- .../projects/import_export/cleanup_service.rb | 20 +++++++++++++------ .../import_export/cleanup_service_spec.rb | 2 +- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/app/services/projects/import_export/cleanup_service.rb b/app/services/projects/import_export/cleanup_service.rb index 75eaee0cb7b..2e6bfb99189 100644 --- a/app/services/projects/import_export/cleanup_service.rb +++ b/app/services/projects/import_export/cleanup_service.rb @@ -1,11 +1,15 @@ module Projects module ImportExport class CleanupService - RESERVED_REFS_NAMES = + def self.reserved_refs_names %w[heads tags merge-requests keep-around environments] - RESERVED_REFS_REGEXP = - %r{\Arefs/(?:#{ - RESERVED_REFS_NAMES.map(&Regexp.method(:escape)).join('|')})/}x + end + + def self.reserved_refs_regexp + names = reserved_refs_names.map(&Regexp.method(:escape)).join('|') + + %r{\Arefs/(?:#{names})/} + end attr_reader :project @@ -23,8 +27,12 @@ module Projects private def garbage_refs - @garbage_refs ||= rugged.references.reject do |ref| - ref.name =~ RESERVED_REFS_REGEXP + @garbage_refs ||= begin + reserved_refs_regexp = self.class.reserved_refs_regexp + + rugged.references.reject do |ref| + ref.name =~ reserved_refs_regexp + end end end diff --git a/spec/services/projects/import_export/cleanup_service_spec.rb b/spec/services/projects/import_export/cleanup_service_spec.rb index b46efc40a2f..108d0ea2ecc 100644 --- a/spec/services/projects/import_export/cleanup_service_spec.rb +++ b/spec/services/projects/import_export/cleanup_service_spec.rb @@ -37,7 +37,7 @@ describe Projects::ImportExport::CleanupService do end end - described_class::RESERVED_REFS_NAMES.each do |name| + described_class.reserved_refs_names.each do |name| context "with a ref in refs/#{name}/tmp" do before do repository.write_ref("refs/#{name}/tmp", sha) From cc2c737dea3c90d465e423e102fb634c531bdcc9 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 24 Aug 2017 17:30:54 +0800 Subject: [PATCH 06/57] Just use @project directly --- app/services/projects/import_export/cleanup_service.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/services/projects/import_export/cleanup_service.rb b/app/services/projects/import_export/cleanup_service.rb index 2e6bfb99189..25bff3468a9 100644 --- a/app/services/projects/import_export/cleanup_service.rb +++ b/app/services/projects/import_export/cleanup_service.rb @@ -11,15 +11,13 @@ module Projects %r{\Arefs/(?:#{names})/} end - attr_reader :project - def initialize(project) @project = project end # This could raise Projects::HousekeepingService::LeaseTaken def execute - Projects::HousekeepingService.new(project).execute do + Projects::HousekeepingService.new(@project).execute do garbage_refs.each(&rugged.references.method(:delete)) end end @@ -37,7 +35,7 @@ module Projects end def rugged - @rugged ||= project.repository.rugged + @rugged ||= @project.repository.rugged end end end From 5c31c72048d418d88d59abbdb117ec5413eb5f83 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 24 Aug 2017 17:36:16 +0800 Subject: [PATCH 07/57] Use block rather than just b --- spec/services/projects/housekeeping_service_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb index 6e916a523fe..9386c110385 100644 --- a/spec/services/projects/housekeeping_service_spec.rb +++ b/spec/services/projects/housekeeping_service_spec.rb @@ -24,8 +24,8 @@ describe Projects::HousekeepingService do end it 'yields the block if given' do - expect do |b| - subject.execute(&b) + expect do |block| + subject.execute(&block) end.to yield_with_no_args end @@ -47,8 +47,8 @@ describe Projects::HousekeepingService do end it 'does not yield' do - expect do |b| - expect { subject.execute(&b) } + expect do |block| + expect { subject.execute(&block) } .to raise_error(Projects::HousekeepingService::LeaseTaken) end.not_to yield_with_no_args end From 932d32515a72bc80e021474100f677d954f3822e Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 24 Aug 2017 18:58:31 +0800 Subject: [PATCH 08/57] Move to Projects::HousecleaningService --- app/models/project.rb | 2 +- .../projects/housecleaning_service.rb | 40 ++++++++++++++++++ .../projects/import_export/cleanup_service.rb | 42 ------------------- spec/models/project_spec.rb | 10 ++--- ..._spec.rb => housecleaning_service_spec.rb} | 2 +- 5 files changed, 47 insertions(+), 49 deletions(-) create mode 100644 app/services/projects/housecleaning_service.rb delete mode 100644 app/services/projects/import_export/cleanup_service.rb rename spec/services/projects/{import_export/cleanup_service_spec.rb => housecleaning_service_spec.rb} (96%) diff --git a/app/models/project.rb b/app/models/project.rb index 72da4e8eb2e..c0060504d74 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -372,7 +372,7 @@ class Project < ActiveRecord::Base if Gitlab::ImportSources.importer_names.include?(project.import_type) && project.repo_exists? project.run_after_commit do begin - Projects::ImportExport::CleanupService.new(project).execute + Projects::HousecleaningService.new(project).execute rescue Projects::HousekeepingService::LeaseTaken => e Rails.logger.info("Could not perform housekeeping for project #{project.full_path} (#{project.id}): #{e}") end diff --git a/app/services/projects/housecleaning_service.rb b/app/services/projects/housecleaning_service.rb new file mode 100644 index 00000000000..d5cf8478e13 --- /dev/null +++ b/app/services/projects/housecleaning_service.rb @@ -0,0 +1,40 @@ +module Projects + class HousecleaningService + def self.reserved_refs_names + %w[heads tags merge-requests keep-around environments] + end + + def self.reserved_refs_regexp + names = reserved_refs_names.map(&Regexp.method(:escape)).join('|') + + %r{\Arefs/(?:#{names})/} + end + + def initialize(project) + @project = project + end + + # This could raise Projects::HousekeepingService::LeaseTaken + def execute + Projects::HousekeepingService.new(@project).execute do + garbage_refs.each(&rugged.references.method(:delete)) + end + end + + private + + def garbage_refs + @garbage_refs ||= begin + reserved_refs_regexp = self.class.reserved_refs_regexp + + rugged.references.reject do |ref| + ref.name =~ reserved_refs_regexp + end + end + end + + def rugged + @rugged ||= @project.repository.rugged + end + end +end diff --git a/app/services/projects/import_export/cleanup_service.rb b/app/services/projects/import_export/cleanup_service.rb deleted file mode 100644 index 25bff3468a9..00000000000 --- a/app/services/projects/import_export/cleanup_service.rb +++ /dev/null @@ -1,42 +0,0 @@ -module Projects - module ImportExport - class CleanupService - def self.reserved_refs_names - %w[heads tags merge-requests keep-around environments] - end - - def self.reserved_refs_regexp - names = reserved_refs_names.map(&Regexp.method(:escape)).join('|') - - %r{\Arefs/(?:#{names})/} - end - - def initialize(project) - @project = project - end - - # This could raise Projects::HousekeepingService::LeaseTaken - def execute - Projects::HousekeepingService.new(@project).execute do - garbage_refs.each(&rugged.references.method(:delete)) - end - end - - private - - def garbage_refs - @garbage_refs ||= begin - reserved_refs_regexp = self.class.reserved_refs_regexp - - rugged.references.reject do |ref| - ref.name =~ reserved_refs_regexp - end - end - end - - def rugged - @rugged ||= @project.repository.rugged - end - end - end -end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 130c0739033..7631207b1d0 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1563,14 +1563,14 @@ describe Project do describe 'project import state transitions' do context 'state transition: [:started] => [:finished]' do - let(:cleanup_service) { spy(:cleanup_service) } + let(:housecleaning_service) { spy(:housecleaning_service) } let(:housekeeping_service) { spy(:housekeeping_service) } before do - allow(Projects::ImportExport::CleanupService) - .to receive(:new) { cleanup_service } + allow(Projects::HousecleaningService) + .to receive(:new) { housecleaning_service } - allow(cleanup_service) + allow(housecleaning_service) .to receive(:execute) { housekeeping_service.execute } allow(Projects::HousekeepingService) @@ -1589,7 +1589,7 @@ describe Project do project.import_finish - expect(cleanup_service).to have_received(:execute) + expect(housecleaning_service).to have_received(:execute) expect(housekeeping_service).to have_received(:execute) end diff --git a/spec/services/projects/import_export/cleanup_service_spec.rb b/spec/services/projects/housecleaning_service_spec.rb similarity index 96% rename from spec/services/projects/import_export/cleanup_service_spec.rb rename to spec/services/projects/housecleaning_service_spec.rb index 108d0ea2ecc..3dd7906dc9a 100644 --- a/spec/services/projects/import_export/cleanup_service_spec.rb +++ b/spec/services/projects/housecleaning_service_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Projects::ImportExport::CleanupService do +describe Projects::HousecleaningService do subject { described_class.new(project) } let(:project) { create(:project, :repository) } From 9e203582b367a1b84035572261a79b62e22bfeaa Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Wed, 23 Aug 2017 01:51:53 +0900 Subject: [PATCH 09/57] Improve AutocompleteController#user.json performance --- app/models/user.rb | 2 +- .../improve-autocomplete-user-performance.yml | 5 +++ lib/gitlab/sql/pattern.rb | 29 +++++++++++++++++ .../filtered_search/dropdown_author_spec.rb | 12 +++---- spec/lib/gitlab/sql/pattern_spec.rb | 31 +++++++++++++++++++ spec/models/user_spec.rb | 17 ++++++++++ 6 files changed, 89 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/improve-autocomplete-user-performance.yml create mode 100644 lib/gitlab/sql/pattern.rb create mode 100644 spec/lib/gitlab/sql/pattern_spec.rb diff --git a/app/models/user.rb b/app/models/user.rb index fbd08bc4d0a..e5a84ce4080 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -303,7 +303,7 @@ class User < ActiveRecord::Base # Returns an ActiveRecord::Relation. def search(query) table = arel_table - pattern = "%#{query}%" + pattern = Gitlab::SQL::Pattern.new(query).to_sql order = <<~SQL CASE diff --git a/changelogs/unreleased/improve-autocomplete-user-performance.yml b/changelogs/unreleased/improve-autocomplete-user-performance.yml new file mode 100644 index 00000000000..5a7153771ff --- /dev/null +++ b/changelogs/unreleased/improve-autocomplete-user-performance.yml @@ -0,0 +1,5 @@ +--- +title: Improve performance for AutocompleteController#users.json +merge_request: 13754 +author: Hiroyuki Sato +type: changed diff --git a/lib/gitlab/sql/pattern.rb b/lib/gitlab/sql/pattern.rb new file mode 100644 index 00000000000..47ea19994a2 --- /dev/null +++ b/lib/gitlab/sql/pattern.rb @@ -0,0 +1,29 @@ +module Gitlab + module SQL + class Pattern + MIN_CHARS_FOR_PARTIAL_MATCHING = 3 + + attr_reader :query + + def initialize(query) + @query = query + end + + def to_sql + if exact_matching? + query + else + "%#{query}%" + end + end + + def exact_matching? + !partial_matching? + end + + def partial_matching? + @query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING + end + end + end +end diff --git a/spec/features/issues/filtered_search/dropdown_author_spec.rb b/spec/features/issues/filtered_search/dropdown_author_spec.rb index 975dc035f2d..3cec59050ab 100644 --- a/spec/features/issues/filtered_search/dropdown_author_spec.rb +++ b/spec/features/issues/filtered_search/dropdown_author_spec.rb @@ -6,7 +6,7 @@ describe 'Dropdown author', js: true do let!(:project) { create(:project) } let!(:user) { create(:user, name: 'administrator', username: 'root') } let!(:user_john) { create(:user, name: 'John', username: 'th0mas') } - let!(:user_jacob) { create(:user, name: 'Jacob', username: 'otter32') } + let!(:user_jacob) { create(:user, name: 'Jacob', username: 'ooter32') } let(:filtered_search) { find('.filtered-search') } let(:js_dropdown_author) { '#js-dropdown-author' } @@ -82,31 +82,31 @@ describe 'Dropdown author', js: true do end it 'filters by name' do - send_keys_to_filtered_search('ja') + send_keys_to_filtered_search('jac') expect(dropdown_author_size).to eq(1) end it 'filters by case insensitive name' do - send_keys_to_filtered_search('Ja') + send_keys_to_filtered_search('Jac') expect(dropdown_author_size).to eq(1) end it 'filters by username with symbol' do - send_keys_to_filtered_search('@ot') + send_keys_to_filtered_search('@oot') expect(dropdown_author_size).to eq(2) end it 'filters by username without symbol' do - send_keys_to_filtered_search('ot') + send_keys_to_filtered_search('oot') expect(dropdown_author_size).to eq(2) end it 'filters by case insensitive username without symbol' do - send_keys_to_filtered_search('OT') + send_keys_to_filtered_search('OOT') expect(dropdown_author_size).to eq(2) end diff --git a/spec/lib/gitlab/sql/pattern_spec.rb b/spec/lib/gitlab/sql/pattern_spec.rb new file mode 100644 index 00000000000..cbafe36de06 --- /dev/null +++ b/spec/lib/gitlab/sql/pattern_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe Gitlab::SQL::Pattern do + describe '#to_sql' do + subject(:to_sql) { described_class.new(query).to_sql } + + context 'when a query is shorter than 3 chars' do + let(:query) { '12' } + + it 'returns exact matching pattern' do + expect(to_sql).to eq('12') + end + end + + context 'when a query is equal to 3 chars' do + let(:query) { '123' } + + it 'returns partial matching pattern' do + expect(to_sql).to eq('%123%') + end + end + + context 'when a query is longer than 3 chars' do + let(:query) { '1234' } + + it 'returns partial matching pattern' do + expect(to_sql).to eq('%1234%') + end + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 9a9e255f874..50abd7af429 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -789,6 +789,7 @@ describe User do describe '.search' do let!(:user) { create(:user, name: 'user', username: 'usern', email: 'email@gmail.com') } let!(:user2) { create(:user, name: 'user name', username: 'username', email: 'someemail@gmail.com') } + let!(:user3) { create(:user, name: 'us', username: 'se', email: 'foo@gmail.com') } describe 'name matching' do it 'returns users with a matching name with exact match first' do @@ -802,6 +803,14 @@ describe User do it 'returns users with a matching name regardless of the casing' do expect(described_class.search(user2.name.upcase)).to eq([user2]) end + + it 'returns users with a exact matching name shorter than 3 chars' do + expect(described_class.search(user3.name)).to eq([user3]) + end + + it 'returns users with a exact matching name shorter than 3 chars regardless of the casing' do + expect(described_class.search(user3.name.upcase)).to eq([user3]) + end end describe 'email matching' do @@ -830,6 +839,14 @@ describe User do it 'returns users with a matching username regardless of the casing' do expect(described_class.search(user2.username.upcase)).to eq([user2]) end + + it 'returns users with a exact matching username shorter than 3 chars' do + expect(described_class.search(user3.username)).to eq([user3]) + end + + it 'returns users with a exact matching username shorter than 3 chars regardless of the casing' do + expect(described_class.search(user3.username.upcase)).to eq([user3]) + end end end From d1054bd3ddb48c15b6a3a53f8c57974208094106 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 25 Aug 2017 22:38:07 +0800 Subject: [PATCH 10/57] Resolve feedback on the MR: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13766 * Rename AfterImportService * Use constants * Move Git operations to Gitlab::Git::Repository * Use Regexp.union --- app/models/project.rb | 6 +-- app/services/projects/after_import_service.rb | 32 +++++++++++++++ .../projects/housecleaning_service.rb | 40 ------------------- lib/gitlab/git/repository.rb | 11 +++++ ...e_spec.rb => after_import_service_spec.rb} | 4 +- 5 files changed, 46 insertions(+), 47 deletions(-) create mode 100644 app/services/projects/after_import_service.rb delete mode 100644 app/services/projects/housecleaning_service.rb rename spec/services/projects/{housecleaning_service_spec.rb => after_import_service_spec.rb} (93%) diff --git a/app/models/project.rb b/app/models/project.rb index c0060504d74..f86f75fbbdc 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -371,11 +371,7 @@ class Project < ActiveRecord::Base if Gitlab::ImportSources.importer_names.include?(project.import_type) && project.repo_exists? project.run_after_commit do - begin - Projects::HousecleaningService.new(project).execute - rescue Projects::HousekeepingService::LeaseTaken => e - Rails.logger.info("Could not perform housekeeping for project #{project.full_path} (#{project.id}): #{e}") - end + Projects::AfterImportService.new(project).execute end end end diff --git a/app/services/projects/after_import_service.rb b/app/services/projects/after_import_service.rb new file mode 100644 index 00000000000..bbada7e2b1c --- /dev/null +++ b/app/services/projects/after_import_service.rb @@ -0,0 +1,32 @@ +module Projects + class AfterImportService + RESERVED_REFS_NAMES = + %w[heads tags merge-requests keep-around environments].freeze + + RESERVED_REFS_REGEXP = + %r{\Arefs/(?:#{Regexp.union(*RESERVED_REFS_NAMES)})/} + + def initialize(project) + @project = project + end + + def execute + Projects::HousekeepingService.new(@project).execute do + repository.delete_refs(garbage_refs) + end + rescue Projects::HousekeepingService::LeaseTaken => e + Rails.logger.info( + "Could not perform housekeeping for project #{@project.full_path} (#{@project.id}): #{e}") + end + + private + + def garbage_refs + @garbage_refs ||= repository.all_ref_names_except(RESERVED_REFS_REGEXP) + end + + def repository + @repository ||= @project.repository + end + end +end diff --git a/app/services/projects/housecleaning_service.rb b/app/services/projects/housecleaning_service.rb deleted file mode 100644 index d5cf8478e13..00000000000 --- a/app/services/projects/housecleaning_service.rb +++ /dev/null @@ -1,40 +0,0 @@ -module Projects - class HousecleaningService - def self.reserved_refs_names - %w[heads tags merge-requests keep-around environments] - end - - def self.reserved_refs_regexp - names = reserved_refs_names.map(&Regexp.method(:escape)).join('|') - - %r{\Arefs/(?:#{names})/} - end - - def initialize(project) - @project = project - end - - # This could raise Projects::HousekeepingService::LeaseTaken - def execute - Projects::HousekeepingService.new(@project).execute do - garbage_refs.each(&rugged.references.method(:delete)) - end - end - - private - - def garbage_refs - @garbage_refs ||= begin - reserved_refs_regexp = self.class.reserved_refs_regexp - - rugged.references.reject do |ref| - ref.name =~ reserved_refs_regexp - end - end - end - - def rugged - @rugged ||= @project.repository.rugged - end - end -end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index eb3731ba35a..f6d30ad7534 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -232,6 +232,13 @@ module Gitlab branch_names + tag_names end + # Returns an Array of all ref names, except when it's matching pattern + # + # regexp - The pattern for ref names we don't want + def all_ref_names_except(regexp) + rugged.references.reject { |ref| ref.name =~ regexp }.map(&:name) + end + # Discovers the default branch based on the repository's available branches # # - If no branches are present, returns nil @@ -577,6 +584,10 @@ module Gitlab rugged.branches.delete(branch_name) end + def delete_refs(ref_names) + ref_names.each(&rugged.references.method(:delete)) + end + # Create a new branch named **ref+ based on **stat_point+, HEAD by default # # Examples: diff --git a/spec/services/projects/housecleaning_service_spec.rb b/spec/services/projects/after_import_service_spec.rb similarity index 93% rename from spec/services/projects/housecleaning_service_spec.rb rename to spec/services/projects/after_import_service_spec.rb index 3dd7906dc9a..540d2191b2d 100644 --- a/spec/services/projects/housecleaning_service_spec.rb +++ b/spec/services/projects/after_import_service_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Projects::HousecleaningService do +describe Projects::AfterImportService do subject { described_class.new(project) } let(:project) { create(:project, :repository) } @@ -37,7 +37,7 @@ describe Projects::HousecleaningService do end end - described_class.reserved_refs_names.each do |name| + described_class::RESERVED_REFS_NAMES.each do |name| context "with a ref in refs/#{name}/tmp" do before do repository.write_ref("refs/#{name}/tmp", sha) From 081e2fce8240f86f991b0bd3653ef6756f2cf9aa Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 25 Aug 2017 23:00:06 +0800 Subject: [PATCH 11/57] Try to make reserved ref names more obvious So that whenever we want to reserve more, we're aware, and don't mess it up. --- app/models/environment.rb | 2 +- app/models/merge_request.rb | 2 +- app/models/repository.rb | 18 +++++++++++++++--- app/services/projects/after_import_service.rb | 5 +---- .../projects/after_import_service_spec.rb | 2 +- 5 files changed, 19 insertions(+), 10 deletions(-) diff --git a/app/models/environment.rb b/app/models/environment.rb index e9ebf0637f3..435eeaf0e2e 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -114,7 +114,7 @@ class Environment < ActiveRecord::Base end def ref_path - "refs/environments/#{Shellwords.shellescape(name)}" + "refs/#{Repository::REF_ENVIRONMENTS}/#{Shellwords.shellescape(name)}" end def formatted_external_url diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index f028d2395c1..8361039f301 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -797,7 +797,7 @@ class MergeRequest < ActiveRecord::Base end def ref_path - "refs/merge-requests/#{iid}/head" + "refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/head" end def ref_fetched? diff --git a/app/models/repository.rb b/app/models/repository.rb index c1e4fcf94a4..c247fb840ce 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1,6 +1,18 @@ require 'securerandom' class Repository + REF_MERGE_REQUEST = 'merge-requests' + REF_KEEP_AROUND = 'keep-around' + REF_ENVIRONMENTS = 'environments' + + RESERVED_REFS_NAMES = %W[ + heads + tags + #{REF_ENVIRONMENTS} + #{REF_KEEP_AROUND} + #{REF_ENVIRONMENTS} + ].freeze + include Gitlab::ShellAdapter include RepositoryMirroring @@ -228,10 +240,10 @@ class Repository begin write_ref(keep_around_ref_name(sha), sha) rescue Rugged::ReferenceError => ex - Rails.logger.error "Unable to create keep-around reference for repository #{path}: #{ex}" + Rails.logger.error "Unable to create #{REF_KEEP_AROUND} reference for repository #{path}: #{ex}" rescue Rugged::OSError => ex raise unless ex.message =~ /Failed to create locked file/ && ex.message =~ /File exists/ - Rails.logger.error "Unable to create keep-around reference for repository #{path}: #{ex}" + Rails.logger.error "Unable to create #{REF_KEEP_AROUND} reference for repository #{path}: #{ex}" end end @@ -1152,7 +1164,7 @@ class Repository end def keep_around_ref_name(sha) - "refs/keep-around/#{sha}" + "refs/#{REF_KEEP_AROUND}/#{sha}" end def repository_event(event, tags = {}) diff --git a/app/services/projects/after_import_service.rb b/app/services/projects/after_import_service.rb index bbada7e2b1c..107856885f3 100644 --- a/app/services/projects/after_import_service.rb +++ b/app/services/projects/after_import_service.rb @@ -1,10 +1,7 @@ module Projects class AfterImportService - RESERVED_REFS_NAMES = - %w[heads tags merge-requests keep-around environments].freeze - RESERVED_REFS_REGEXP = - %r{\Arefs/(?:#{Regexp.union(*RESERVED_REFS_NAMES)})/} + %r{\Arefs/(?:#{Regexp.union(*Repository::RESERVED_REFS_NAMES)})/} def initialize(project) @project = project diff --git a/spec/services/projects/after_import_service_spec.rb b/spec/services/projects/after_import_service_spec.rb index 540d2191b2d..c6678fc1f5c 100644 --- a/spec/services/projects/after_import_service_spec.rb +++ b/spec/services/projects/after_import_service_spec.rb @@ -37,7 +37,7 @@ describe Projects::AfterImportService do end end - described_class::RESERVED_REFS_NAMES.each do |name| + Repository::RESERVED_REFS_NAMES.each do |name| context "with a ref in refs/#{name}/tmp" do before do repository.write_ref("refs/#{name}/tmp", sha) From 866aab7f2a92f9929a5c5811d3d3c23c11184b26 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Sat, 26 Aug 2017 22:32:55 +0900 Subject: [PATCH 12/57] Fix escape characters was not sanitized --- lib/gitlab/sql/pattern.rb | 9 +++++++-- spec/lib/gitlab/sql/pattern_spec.rb | 24 ++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/sql/pattern.rb b/lib/gitlab/sql/pattern.rb index 47ea19994a2..46c973d8a11 100644 --- a/lib/gitlab/sql/pattern.rb +++ b/lib/gitlab/sql/pattern.rb @@ -11,9 +11,9 @@ module Gitlab def to_sql if exact_matching? - query + sanitized_query else - "%#{query}%" + "%#{sanitized_query}%" end end @@ -24,6 +24,11 @@ module Gitlab def partial_matching? @query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING end + + def sanitized_query + # Note: ActiveRecord::Base.sanitize_sql_like is a protected method + ActiveRecord::Base.__send__(:sanitize_sql_like, query) + end end end end diff --git a/spec/lib/gitlab/sql/pattern_spec.rb b/spec/lib/gitlab/sql/pattern_spec.rb index cbafe36de06..d0412f37098 100644 --- a/spec/lib/gitlab/sql/pattern_spec.rb +++ b/spec/lib/gitlab/sql/pattern_spec.rb @@ -12,6 +12,14 @@ describe Gitlab::SQL::Pattern do end end + context 'when a query with a escape character is shorter than 3 chars' do + let(:query) { '_2' } + + it 'returns sanitized exact matching pattern' do + expect(to_sql).to eq('\_2') + end + end + context 'when a query is equal to 3 chars' do let(:query) { '123' } @@ -20,6 +28,14 @@ describe Gitlab::SQL::Pattern do end end + context 'when a query with a escape character is equal to 3 chars' do + let(:query) { '_23' } + + it 'returns partial matching pattern' do + expect(to_sql).to eq('%\_23%') + end + end + context 'when a query is longer than 3 chars' do let(:query) { '1234' } @@ -27,5 +43,13 @@ describe Gitlab::SQL::Pattern do expect(to_sql).to eq('%1234%') end end + + context 'when a query with a escape character is longer than 3 chars' do + let(:query) { '_234' } + + it 'returns sanitized partial matching pattern' do + expect(to_sql).to eq('%\_234%') + end + end end end From 3a4da8ce8b57aa430720de75397a38c2be0fc6c0 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 28 Aug 2017 18:51:23 +0800 Subject: [PATCH 13/57] Fix tests --- app/models/repository.rb | 6 +++--- spec/models/project_spec.rb | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index c247fb840ce..b917eb4c302 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1,9 +1,9 @@ require 'securerandom' class Repository - REF_MERGE_REQUEST = 'merge-requests' - REF_KEEP_AROUND = 'keep-around' - REF_ENVIRONMENTS = 'environments' + REF_MERGE_REQUEST = 'merge-requests'.freeze + REF_KEEP_AROUND = 'keep-around'.freeze + REF_ENVIRONMENTS = 'environments'.freeze RESERVED_REFS_NAMES = %W[ heads diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 7631207b1d0..11717ba39e8 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1563,14 +1563,14 @@ describe Project do describe 'project import state transitions' do context 'state transition: [:started] => [:finished]' do - let(:housecleaning_service) { spy(:housecleaning_service) } + let(:after_import_service) { spy(:after_import_service) } let(:housekeeping_service) { spy(:housekeeping_service) } before do - allow(Projects::HousecleaningService) - .to receive(:new) { housecleaning_service } + allow(Projects::AfterImportService) + .to receive(:new) { after_import_service } - allow(housecleaning_service) + allow(after_import_service) .to receive(:execute) { housekeeping_service.execute } allow(Projects::HousekeepingService) @@ -1589,7 +1589,7 @@ describe Project do project.import_finish - expect(housecleaning_service).to have_received(:execute) + expect(after_import_service).to have_received(:execute) expect(housekeeping_service).to have_received(:execute) end From 54b3908cd1fac68318007e6800305ece6013727e Mon Sep 17 00:00:00 2001 From: winh Date: Thu, 17 Aug 2017 10:42:18 +0200 Subject: [PATCH 14/57] Make notification dropdowns consistent --- app/assets/stylesheets/pages/notifications.scss | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/assets/stylesheets/pages/notifications.scss b/app/assets/stylesheets/pages/notifications.scss index bdf07a99daf..c28b1e68008 100644 --- a/app/assets/stylesheets/pages/notifications.scss +++ b/app/assets/stylesheets/pages/notifications.scss @@ -14,3 +14,7 @@ font-size: 18px; } } + +.notification-form { + @include new-style-dropdown; +} From 53fd93220c19aec495212e848979293da41164ba Mon Sep 17 00:00:00 2001 From: winh Date: Tue, 15 Aug 2017 22:39:19 +0200 Subject: [PATCH 15/57] Make file template dropdowns consistent --- app/assets/stylesheets/pages/editor.scss | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/assets/stylesheets/pages/editor.scss b/app/assets/stylesheets/pages/editor.scss index f6b8c8ee2bc..d3cd4d507de 100644 --- a/app/assets/stylesheets/pages/editor.scss +++ b/app/assets/stylesheets/pages/editor.scss @@ -204,6 +204,8 @@ .gitlab-ci-yml-selector, .dockerfile-selector, .template-type-selector { + @include new-style-dropdown; + display: inline-block; vertical-align: top; font-family: $regular_font; From b7316ffdff561cc16a003c5540f2c6e627818a2e Mon Sep 17 00:00:00 2001 From: Kai Kontio Date: Mon, 28 Aug 2017 11:47:09 +0000 Subject: [PATCH 16/57] Update backup_restore.md documentation regarding S3 and IAM profiles. https://gitlab.com/gitlab-org/gitlab-ce/commit/7f8ef19c411eceec207fef5d8459fbeaa40bf464 most likely broke the old configuration format where empty string aws_access_key_id & aws_secret_access_key were still OK. From 9.5 onwards moving backups to S3 using IAM profiles will fail if they are configured. --- doc/raketasks/backup_restore.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/doc/raketasks/backup_restore.md b/doc/raketasks/backup_restore.md index 10f5ab3370d..ae69d7f92f2 100644 --- a/doc/raketasks/backup_restore.md +++ b/doc/raketasks/backup_restore.md @@ -144,9 +144,8 @@ gitlab_rails['backup_upload_connection'] = { 'region' => 'eu-west-1', 'aws_access_key_id' => 'AKIAKIAKI', 'aws_secret_access_key' => 'secret123' - # If using an IAM Profile, leave aws_access_key_id & aws_secret_access_key empty - # ie. 'aws_access_key_id' => '', - # 'use_iam_profile' => 'true' + # If using an IAM Profile, don't configure aws_access_key_id & aws_secret_access_key + # 'use_iam_profile' => true } gitlab_rails['backup_upload_remote_directory'] = 'my.s3.bucket' ``` From 998afa5f74558be215a924d95aa131a69831ca43 Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Wed, 1 Mar 2017 14:35:48 +0100 Subject: [PATCH 17/57] API: Respect the 'If-Unmodified-Since' for delete endpoints --- lib/api/access_requests.rb | 3 +++ lib/api/award_emoji.rb | 1 + lib/api/boards.rb | 1 + lib/api/broadcast_messages.rb | 1 + lib/api/deploy_keys.rb | 2 ++ lib/api/environments.rb | 1 + lib/api/groups.rb | 2 ++ lib/api/helpers.rb | 8 ++++++++ lib/api/issues.rb | 2 ++ lib/api/labels.rb | 2 ++ lib/api/members.rb | 3 ++- lib/api/merge_requests.rb | 2 ++ lib/api/notes.rb | 2 ++ lib/api/project_hooks.rb | 2 ++ lib/api/project_snippets.rb | 2 ++ lib/api/projects.rb | 2 ++ lib/api/runners.rb | 2 ++ lib/api/services.rb | 2 ++ lib/api/snippets.rb | 1 + lib/api/system_hooks.rb | 2 ++ lib/api/triggers.rb | 2 ++ lib/api/users.rb | 28 ++++++++++++++++++++++++++++ 22 files changed, 72 insertions(+), 1 deletion(-) diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb index cdacf9839e5..0c5b8862d79 100644 --- a/lib/api/access_requests.rb +++ b/lib/api/access_requests.rb @@ -67,6 +67,9 @@ module API end delete ":id/access_requests/:user_id" do source = find_source(source_type, params[:id]) + member = source.public_send(:requesters).find_by!(user_id: params[:user_id]) + + check_unmodified_since(member.updated_at) status 204 ::Members::DestroyService.new(source, current_user, params) diff --git a/lib/api/award_emoji.rb b/lib/api/award_emoji.rb index 5a028fc9d0b..51a8587d26e 100644 --- a/lib/api/award_emoji.rb +++ b/lib/api/award_emoji.rb @@ -85,6 +85,7 @@ module API end delete "#{endpoint}/:award_id" do award = awardable.award_emoji.find(params[:award_id]) + check_unmodified_since(award.updated_at) unauthorized! unless award.user == current_user || current_user.admin? diff --git a/lib/api/boards.rb b/lib/api/boards.rb index 5a2d7a681e3..d36df77dc6c 100644 --- a/lib/api/boards.rb +++ b/lib/api/boards.rb @@ -124,6 +124,7 @@ module API authorize!(:admin_list, user_project) list = board_lists.find(params[:list_id]) + check_unmodified_since(list.updated_at) service = ::Boards::Lists::DestroyService.new(user_project, current_user) diff --git a/lib/api/broadcast_messages.rb b/lib/api/broadcast_messages.rb index 9980aec4752..352972b584a 100644 --- a/lib/api/broadcast_messages.rb +++ b/lib/api/broadcast_messages.rb @@ -90,6 +90,7 @@ module API end delete ':id' do message = find_message + check_unmodified_since(message.updated_at) status 204 message.destroy diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index 42e7c1486b0..971cc816454 100644 --- a/lib/api/deploy_keys.rb +++ b/lib/api/deploy_keys.rb @@ -125,6 +125,8 @@ module API key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id]) not_found!('Deploy Key') unless key + check_unmodified_since(key.updated_at) + status 204 key.destroy end diff --git a/lib/api/environments.rb b/lib/api/environments.rb index c774a5c6685..3fc423ae79a 100644 --- a/lib/api/environments.rb +++ b/lib/api/environments.rb @@ -78,6 +78,7 @@ module API authorize! :update_environment, user_project environment = user_project.environments.find(params[:environment_id]) + check_unmodified_since(environment.updated_at) status 204 environment.destroy diff --git a/lib/api/groups.rb b/lib/api/groups.rb index e56427304a6..c9b32a85487 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -117,6 +117,8 @@ module API delete ":id" do group = find_group!(params[:id]) authorize! :admin_group, group + + check_unmodified_since(group.updated_at) status 204 ::Groups::DestroyService.new(group, current_user).execute diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index b56fd2388b3..1c74a14d91c 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -11,6 +11,14 @@ module API declared(params, options).to_h.symbolize_keys end + def check_unmodified_since(last_modified) + if_unmodified_since = Time.parse(headers['If-Unmodified-Since']) if headers.key?('If-Unmodified-Since') rescue nil + + if if_unmodified_since && if_unmodified_since < last_modified + render_api_error!('412 Precondition Failed', 412) + end + end + def current_user return @current_user if defined?(@current_user) diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 4cec1145f3a..cee9898d3a6 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -230,6 +230,8 @@ module API not_found!('Issue') unless issue authorize!(:destroy_issue, issue) + check_unmodified_since(issue.updated_at) + status 204 issue.destroy end diff --git a/lib/api/labels.rb b/lib/api/labels.rb index 4520c98d951..45fa57fdf55 100644 --- a/lib/api/labels.rb +++ b/lib/api/labels.rb @@ -56,6 +56,8 @@ module API label = user_project.labels.find_by(title: params[:name]) not_found!('Label') unless label + check_unmodified_since(label.updated_at) + status 204 label.destroy end diff --git a/lib/api/members.rb b/lib/api/members.rb index bb970b7cd54..5634f123eca 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -94,7 +94,8 @@ module API delete ":id/members/:user_id" do source = find_source(source_type, params[:id]) # Ensure that memeber exists - source.members.find_by!(user_id: params[:user_id]) + member = source.members.find_by!(user_id: params[:user_id]) + check_unmodified_since(member.updated_at) status 204 ::Members::DestroyService.new(source, current_user, declared_params).execute diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 8810d4e441d..c6fecc1aa6c 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -164,6 +164,8 @@ module API merge_request = find_project_merge_request(params[:merge_request_iid]) authorize!(:destroy_merge_request, merge_request) + check_unmodified_since(merge_request.updated_at) + status 204 merge_request.destroy end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 4e4e473994b..58d71787aca 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -129,7 +129,9 @@ module API end delete ":id/#{noteables_str}/:noteable_id/notes/:note_id" do note = user_project.notes.find(params[:note_id]) + authorize! :admin_note, note + check_unmodified_since(note.updated_at) status 204 ::Notes::DestroyService.new(user_project, current_user).execute(note) diff --git a/lib/api/project_hooks.rb b/lib/api/project_hooks.rb index 649dd891f56..74d736fda59 100644 --- a/lib/api/project_hooks.rb +++ b/lib/api/project_hooks.rb @@ -96,6 +96,8 @@ module API delete ":id/hooks/:hook_id" do hook = user_project.hooks.find(params.delete(:hook_id)) + check_unmodified_since(hook.updated_at) + status 204 hook.destroy end diff --git a/lib/api/project_snippets.rb b/lib/api/project_snippets.rb index f3d905b0068..645162d564d 100644 --- a/lib/api/project_snippets.rb +++ b/lib/api/project_snippets.rb @@ -116,6 +116,8 @@ module API not_found!('Snippet') unless snippet authorize! :admin_project_snippet, snippet + check_unmodified_since(snippet.updated_at) + status 204 snippet.destroy end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 15c3832b032..eab0ca0b3c9 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -334,6 +334,8 @@ module API desc 'Remove a project' delete ":id" do authorize! :remove_project, user_project + check_unmodified_since(user_project.updated_at) + ::Projects::DestroyService.new(user_project, current_user, {}).async_execute accepted! diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 31f940fe96b..e3b2eb904b7 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -77,7 +77,9 @@ module API end delete ':id' do runner = get_runner(params[:id]) + authenticate_delete_runner!(runner) + check_unmodified_since(runner.updated_at) status 204 runner.destroy! diff --git a/lib/api/services.rb b/lib/api/services.rb index 843c05ae32e..4fef3383e5e 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -655,6 +655,8 @@ module API end delete ":id/services/:service_slug" do service = user_project.find_or_initialize_service(params[:service_slug].underscore) + # Todo, not sure + check_unmodified_since(service.updated_at) attrs = service_attributes(service).inject({}) do |hash, key| hash.merge!(key => nil) diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb index 35ece56c65c..7107b3d669c 100644 --- a/lib/api/snippets.rb +++ b/lib/api/snippets.rb @@ -122,6 +122,7 @@ module API return not_found!('Snippet') unless snippet authorize! :destroy_personal_snippet, snippet + check_unmodified_since(snippet.updated_at) status 204 snippet.destroy diff --git a/lib/api/system_hooks.rb b/lib/api/system_hooks.rb index c0179037440..64066a75b15 100644 --- a/lib/api/system_hooks.rb +++ b/lib/api/system_hooks.rb @@ -66,6 +66,8 @@ module API hook = SystemHook.find_by(id: params[:id]) not_found!('System hook') unless hook + check_unmodified_since(hook.updated_at) + status 204 hook.destroy end diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index edfdb63d183..4ae70c65759 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -140,6 +140,8 @@ module API trigger = user_project.triggers.find(params.delete(:trigger_id)) return not_found!('Trigger') unless trigger + check_unmodified_since(trigger.updated_at) + status 204 trigger.destroy end diff --git a/lib/api/users.rb b/lib/api/users.rb index e2019d6d512..942bb72cf97 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -230,7 +230,12 @@ module API key = user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key +<<<<<<< HEAD status 204 +======= + check_unmodified_since(key.updated_at) + +>>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints key.destroy end @@ -287,7 +292,14 @@ module API email = user.emails.find_by(id: params[:email_id]) not_found!('Email') unless email +<<<<<<< HEAD Emails::DestroyService.new(user, email: email.email).execute +======= + check_unmodified_since(email.updated_at) + + email.destroy + user.update_secondary_emails! +>>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints end desc 'Delete a user. Available only for admins.' do @@ -299,11 +311,18 @@ module API end delete ":id" do authenticated_as_admin! + user = User.find_by(id: params[:id]) not_found!('User') unless user +<<<<<<< HEAD status 204 user.delete_async(deleted_by: current_user, params: params) +======= + check_unmodified_since(user.updated_at) + + ::Users::DestroyService.new(current_user).execute(user) +>>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints end desc 'Block a user. Available only for admins.' @@ -481,6 +500,8 @@ module API key = current_user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key + check_unmodified_since(key.updated_at) + status 204 key.destroy end @@ -533,6 +554,7 @@ module API email = current_user.emails.find_by(id: params[:email_id]) not_found!('Email') unless email +<<<<<<< HEAD status 204 Emails::DestroyService.new(current_user, email: email.email).execute end @@ -550,6 +572,12 @@ module API .reorder(last_activity_on: :asc) present paginate(activities), with: Entities::UserActivity +======= + check_unmodified_since(email.updated_at) + + email.destroy + current_user.update_secondary_emails! +>>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints end end end From e80313f9ee5b3495a8713e6ddae111bc8106155b Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Thu, 2 Mar 2017 13:14:13 +0100 Subject: [PATCH 18/57] Conditionally destroy a ressource --- lib/api/access_requests.rb | 11 ++++---- lib/api/award_emoji.rb | 4 +-- lib/api/boards.rb | 11 ++++---- lib/api/broadcast_messages.rb | 4 +-- lib/api/deploy_keys.rb | 5 +--- lib/api/environments.rb | 4 +-- lib/api/groups.rb | 7 +++-- lib/api/helpers.rb | 17 +++++++++--- lib/api/issues.rb | 4 +-- lib/api/labels.rb | 5 +--- lib/api/members.rb | 7 +++-- lib/api/merge_requests.rb | 4 +-- lib/api/notes.rb | 6 ++--- lib/api/project_hooks.rb | 5 +--- lib/api/project_snippets.rb | 4 +-- lib/api/projects.rb | 5 ++-- lib/api/runner.rb | 6 +++-- lib/api/runners.rb | 7 ++--- lib/api/services.rb | 4 +-- lib/api/snippets.rb | 4 +-- lib/api/system_hooks.rb | 5 +--- lib/api/triggers.rb | 5 +--- lib/api/users.rb | 47 ++++++++++------------------------ spec/requests/api/tags_spec.rb | 4 +-- 24 files changed, 71 insertions(+), 114 deletions(-) diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb index 0c5b8862d79..4fa9b2b2494 100644 --- a/lib/api/access_requests.rb +++ b/lib/api/access_requests.rb @@ -67,13 +67,12 @@ module API end delete ":id/access_requests/:user_id" do source = find_source(source_type, params[:id]) - member = source.public_send(:requesters).find_by!(user_id: params[:user_id]) + member = source.requesters.find_by!(user_id: params[:user_id]) - check_unmodified_since(member.updated_at) - - status 204 - ::Members::DestroyService.new(source, current_user, params) - .execute(:requesters) + destroy_conditionally!(member) do + ::Members::DestroyService.new(source, current_user, params) + .execute(:requesters) + end end end end diff --git a/lib/api/award_emoji.rb b/lib/api/award_emoji.rb index 51a8587d26e..8e3851640da 100644 --- a/lib/api/award_emoji.rb +++ b/lib/api/award_emoji.rb @@ -85,12 +85,10 @@ module API end delete "#{endpoint}/:award_id" do award = awardable.award_emoji.find(params[:award_id]) - check_unmodified_since(award.updated_at) unauthorized! unless award.user == current_user || current_user.admin? - status 204 - award.destroy + destroy_conditionally!(award) end end end diff --git a/lib/api/boards.rb b/lib/api/boards.rb index d36df77dc6c..0d11c5fc971 100644 --- a/lib/api/boards.rb +++ b/lib/api/boards.rb @@ -122,14 +122,13 @@ module API end delete "/lists/:list_id" do authorize!(:admin_list, user_project) - list = board_lists.find(params[:list_id]) - check_unmodified_since(list.updated_at) - service = ::Boards::Lists::DestroyService.new(user_project, current_user) - - unless service.execute(list) - render_api_error!({ error: 'List could not be deleted!' }, 400) + destroy_conditionally!(list) do |list| + service = ::Boards::Lists::DestroyService.new(user_project, current_user) + unless service.execute(list) + render_api_error!({ error: 'List could not be deleted!' }, 400) + end end end end diff --git a/lib/api/broadcast_messages.rb b/lib/api/broadcast_messages.rb index 352972b584a..0b45621ce7b 100644 --- a/lib/api/broadcast_messages.rb +++ b/lib/api/broadcast_messages.rb @@ -90,10 +90,8 @@ module API end delete ':id' do message = find_message - check_unmodified_since(message.updated_at) - status 204 - message.destroy + destroy_conditionally!(message) end end end diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index 971cc816454..f405c341398 100644 --- a/lib/api/deploy_keys.rb +++ b/lib/api/deploy_keys.rb @@ -125,10 +125,7 @@ module API key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id]) not_found!('Deploy Key') unless key - check_unmodified_since(key.updated_at) - - status 204 - key.destroy + destroy_conditionally!(key) end end end diff --git a/lib/api/environments.rb b/lib/api/environments.rb index 3fc423ae79a..e33269f9483 100644 --- a/lib/api/environments.rb +++ b/lib/api/environments.rb @@ -78,10 +78,8 @@ module API authorize! :update_environment, user_project environment = user_project.environments.find(params[:environment_id]) - check_unmodified_since(environment.updated_at) - status 204 - environment.destroy + destroy_conditionally!(environment) end desc 'Stops an existing environment' do diff --git a/lib/api/groups.rb b/lib/api/groups.rb index c9b32a85487..ee2ad27837b 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -117,11 +117,10 @@ module API delete ":id" do group = find_group!(params[:id]) authorize! :admin_group, group - - check_unmodified_since(group.updated_at) - status 204 - ::Groups::DestroyService.new(group, current_user).execute + destroy_conditionally!(group) do |group| + ::Groups::DestroyService.new(group, current_user).execute + end end desc 'Get a list of projects in this group.' do diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 1c74a14d91c..8d4f8c01903 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -11,14 +11,25 @@ module API declared(params, options).to_h.symbolize_keys end - def check_unmodified_since(last_modified) - if_unmodified_since = Time.parse(headers['If-Unmodified-Since']) if headers.key?('If-Unmodified-Since') rescue nil + def check_unmodified_since!(last_modified) + if_unmodified_since = Time.parse(headers['If-Unmodified-Since']) rescue nil - if if_unmodified_since && if_unmodified_since < last_modified + if if_unmodified_since && last_modified > if_unmodified_since render_api_error!('412 Precondition Failed', 412) end end + def destroy_conditionally!(resource, last_update_field: :updated_at) + check_unmodified_since!(resource.public_send(last_update_field)) + + status 204 + if block_given? + yield resource + else + resource.destroy + end + end + def current_user return @current_user if defined?(@current_user) diff --git a/lib/api/issues.rb b/lib/api/issues.rb index cee9898d3a6..6503629e2a2 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -230,10 +230,8 @@ module API not_found!('Issue') unless issue authorize!(:destroy_issue, issue) - check_unmodified_since(issue.updated_at) - status 204 - issue.destroy + destroy_conditionally!(issue) end desc 'List merge requests closing issue' do diff --git a/lib/api/labels.rb b/lib/api/labels.rb index 45fa57fdf55..c0cf618ee8d 100644 --- a/lib/api/labels.rb +++ b/lib/api/labels.rb @@ -56,10 +56,7 @@ module API label = user_project.labels.find_by(title: params[:name]) not_found!('Label') unless label - check_unmodified_since(label.updated_at) - - status 204 - label.destroy + destroy_conditionally!(label) end desc 'Update an existing label. At least one optional parameter is required.' do diff --git a/lib/api/members.rb b/lib/api/members.rb index 5634f123eca..a5d3d7f25a0 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -93,12 +93,11 @@ module API end delete ":id/members/:user_id" do source = find_source(source_type, params[:id]) - # Ensure that memeber exists member = source.members.find_by!(user_id: params[:user_id]) - check_unmodified_since(member.updated_at) - status 204 - ::Members::DestroyService.new(source, current_user, declared_params).execute + destroy_conditionally!(member) do + ::Members::DestroyService.new(source, current_user, declared_params).execute + end end end end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index c6fecc1aa6c..969c6064662 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -164,10 +164,8 @@ module API merge_request = find_project_merge_request(params[:merge_request_iid]) authorize!(:destroy_merge_request, merge_request) - check_unmodified_since(merge_request.updated_at) - status 204 - merge_request.destroy + destroy_conditionally!(merge_request) end params do diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 58d71787aca..e116448c15b 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -131,10 +131,10 @@ module API note = user_project.notes.find(params[:note_id]) authorize! :admin_note, note - check_unmodified_since(note.updated_at) - status 204 - ::Notes::DestroyService.new(user_project, current_user).execute(note) + destroy_conditionally!(note) do |note| + ::Notes::DestroyService.new(user_project, current_user).execute(note) + end end end end diff --git a/lib/api/project_hooks.rb b/lib/api/project_hooks.rb index 74d736fda59..5b457bbe639 100644 --- a/lib/api/project_hooks.rb +++ b/lib/api/project_hooks.rb @@ -96,10 +96,7 @@ module API delete ":id/hooks/:hook_id" do hook = user_project.hooks.find(params.delete(:hook_id)) - check_unmodified_since(hook.updated_at) - - status 204 - hook.destroy + destroy_conditionally!(hook) end end end diff --git a/lib/api/project_snippets.rb b/lib/api/project_snippets.rb index 645162d564d..704e8c6718d 100644 --- a/lib/api/project_snippets.rb +++ b/lib/api/project_snippets.rb @@ -116,10 +116,8 @@ module API not_found!('Snippet') unless snippet authorize! :admin_project_snippet, snippet - check_unmodified_since(snippet.updated_at) - status 204 - snippet.destroy + destroy_conditionally!(snippet) end desc 'Get a raw project snippet' diff --git a/lib/api/projects.rb b/lib/api/projects.rb index eab0ca0b3c9..36fe3f243b9 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -334,9 +334,10 @@ module API desc 'Remove a project' delete ":id" do authorize! :remove_project, user_project - check_unmodified_since(user_project.updated_at) - ::Projects::DestroyService.new(user_project, current_user, {}).async_execute + destroy_conditionally!(user_project) do + ::Projects::DestroyService.new(user_project, current_user, {}).async_execute + end accepted! end diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 88fc62d33df..daa8ddbe251 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -45,8 +45,10 @@ module API end delete '/' do authenticate_runner! - status 204 - Ci::Runner.find_by_token(params[:token]).destroy + + runner = Ci::Runner.find_by_token(params[:token]) + + destroy_conditionally!(runner) end desc 'Validates authentication credentials' do diff --git a/lib/api/runners.rb b/lib/api/runners.rb index e3b2eb904b7..68c2120cc15 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -79,10 +79,8 @@ module API runner = get_runner(params[:id]) authenticate_delete_runner!(runner) - check_unmodified_since(runner.updated_at) - status 204 - runner.destroy! + destroy_conditionally!(runner) end end @@ -137,8 +135,7 @@ module API runner = runner_project.runner forbidden!("Only one project associated with the runner. Please remove the runner instead") if runner.projects.count == 1 - status 204 - runner_project.destroy + destroy_conditionally!(runner_project) end end diff --git a/lib/api/services.rb b/lib/api/services.rb index 4fef3383e5e..2b5ef75b6bf 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -655,8 +655,8 @@ module API end delete ":id/services/:service_slug" do service = user_project.find_or_initialize_service(params[:service_slug].underscore) - # Todo, not sure - check_unmodified_since(service.updated_at) + # Todo: Check if this done the right way + check_unmodified_since!(service.updated_at) attrs = service_attributes(service).inject({}) do |hash, key| hash.merge!(key => nil) diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb index 7107b3d669c..00eb7c60f16 100644 --- a/lib/api/snippets.rb +++ b/lib/api/snippets.rb @@ -122,10 +122,8 @@ module API return not_found!('Snippet') unless snippet authorize! :destroy_personal_snippet, snippet - check_unmodified_since(snippet.updated_at) - status 204 - snippet.destroy + destroy_conditionally!(snippet) end desc 'Get a raw snippet' do diff --git a/lib/api/system_hooks.rb b/lib/api/system_hooks.rb index 64066a75b15..6b6a03e3300 100644 --- a/lib/api/system_hooks.rb +++ b/lib/api/system_hooks.rb @@ -66,10 +66,7 @@ module API hook = SystemHook.find_by(id: params[:id]) not_found!('System hook') unless hook - check_unmodified_since(hook.updated_at) - - status 204 - hook.destroy + destroy_conditionally!(hook) end end end diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 4ae70c65759..c9fee7e5193 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -140,10 +140,7 @@ module API trigger = user_project.triggers.find(params.delete(:trigger_id)) return not_found!('Trigger') unless trigger - check_unmodified_since(trigger.updated_at) - - status 204 - trigger.destroy + destroy_conditionally!(trigger) end end end diff --git a/lib/api/users.rb b/lib/api/users.rb index 942bb72cf97..d7c7b9ae9c1 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -230,13 +230,7 @@ module API key = user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key -<<<<<<< HEAD - status 204 -======= - check_unmodified_since(key.updated_at) - ->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints - key.destroy + destroy_conditionally!(key) end desc 'Add an email address to a specified user. Available only for admins.' do @@ -292,14 +286,11 @@ module API email = user.emails.find_by(id: params[:email_id]) not_found!('Email') unless email -<<<<<<< HEAD - Emails::DestroyService.new(user, email: email.email).execute -======= - check_unmodified_since(email.updated_at) + destroy_conditionally!(email) do |email| + Emails::DestroyService.new(current_user, email: email.email).execute + end - email.destroy user.update_secondary_emails! ->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints end desc 'Delete a user. Available only for admins.' do @@ -315,14 +306,9 @@ module API user = User.find_by(id: params[:id]) not_found!('User') unless user -<<<<<<< HEAD - status 204 - user.delete_async(deleted_by: current_user, params: params) -======= - check_unmodified_since(user.updated_at) - - ::Users::DestroyService.new(current_user).execute(user) ->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints + destroy_conditionally!(user) do + user.delete_async(deleted_by: current_user, params: params) + end end desc 'Block a user. Available only for admins.' @@ -500,10 +486,7 @@ module API key = current_user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key - check_unmodified_since(key.updated_at) - - status 204 - key.destroy + destroy_conditionally!(key) end desc "Get the currently authenticated user's email addresses" do @@ -554,9 +537,11 @@ module API email = current_user.emails.find_by(id: params[:email_id]) not_found!('Email') unless email -<<<<<<< HEAD - status 204 - Emails::DestroyService.new(current_user, email: email.email).execute + destroy_conditionally!(email) do |email| + Emails::DestroyService.new(current_user, email: email.email).execute + end + + current_user.update_secondary_emails! end desc 'Get a list of user activities' @@ -572,12 +557,6 @@ module API .reorder(last_activity_on: :asc) present paginate(activities), with: Entities::UserActivity -======= - check_unmodified_since(email.updated_at) - - email.destroy - current_user.update_secondary_emails! ->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints end end end diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb index 9884c1ec206..54900c75eb8 100644 --- a/spec/requests/api/tags_spec.rb +++ b/spec/requests/api/tags_spec.rb @@ -283,7 +283,7 @@ describe API::Tags do it_behaves_like '404 response' do let(:request) { delete api(route, current_user) } - let(:message) { 'No such tag' } + let(:message) { '404 Tag Not Found' } end end @@ -394,7 +394,7 @@ describe API::Tags do it_behaves_like '404 response' do let(:request) { put api(route, current_user), description: new_description } - let(:message) { 'Tag does not exist' } + let(:message) { '404 Tag Not Found' } end end From f0f3f38576c0691e6d0e751c962382beea998afb Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Thu, 2 Mar 2017 14:21:36 +0100 Subject: [PATCH 19/57] Use commit date for branches and tags --- lib/api/branches.rb | 15 +++++++++++---- lib/api/tags.rb | 15 +++++++++++---- spec/requests/api/tags_spec.rb | 2 +- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/lib/api/branches.rb b/lib/api/branches.rb index d3dbf941298..b87f7cdbad1 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -125,11 +125,18 @@ module API delete ':id/repository/branches/:branch', requirements: BRANCH_ENDPOINT_REQUIREMENTS do authorize_push_project - result = DeleteBranchService.new(user_project, current_user) - .execute(params[:branch]) + branch = user_project.repository.find_branch(params[:branch]) + not_found!('Branch') unless branch - if result[:status] != :success - render_api_error!(result[:message], result[:return_code]) + commit = user_project.repository.commit(branch.dereferenced_target) + + destroy_conditionally!(commit, last_update_field: :authored_date) do + result = DeleteBranchService.new(user_project, current_user) + .execute(params[:branch]) + + if result[:status] != :success + render_api_error!(result[:message], result[:return_code]) + end end end diff --git a/lib/api/tags.rb b/lib/api/tags.rb index 1333747cced..81b17935b81 100644 --- a/lib/api/tags.rb +++ b/lib/api/tags.rb @@ -65,11 +65,18 @@ module API delete ':id/repository/tags/:tag_name', requirements: TAG_ENDPOINT_REQUIREMENTS do authorize_push_project - result = ::Tags::DestroyService.new(user_project, current_user) - .execute(params[:tag_name]) + tag = user_project.repository.find_tag(params[:tag_name]) + not_found!('Tag') unless tag - if result[:status] != :success - render_api_error!(result[:message], result[:return_code]) + commit = user_project.repository.commit(tag.dereferenced_target) + + destroy_conditionally!(commit, last_update_field: :authored_date) do + result = ::Tags::DestroyService.new(user_project, current_user) + .execute(params[:tag_name]) + + if result[:status] != :success + render_api_error!(result[:message], result[:return_code]) + end end end diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb index 54900c75eb8..6ac0caa7fe8 100644 --- a/spec/requests/api/tags_spec.rb +++ b/spec/requests/api/tags_spec.rb @@ -394,7 +394,7 @@ describe API::Tags do it_behaves_like '404 response' do let(:request) { put api(route, current_user), description: new_description } - let(:message) { '404 Tag Not Found' } + let(:message) { 'Tag does not exist' } end end From dcd4ea473cab20eee05995ecaca043da98ca5a8d Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Thu, 24 Aug 2017 10:41:54 +0200 Subject: [PATCH 20/57] Update remaining endpoints --- lib/api/group_variables.rb | 3 +-- lib/api/helpers.rb | 2 +- lib/api/pipeline_schedules.rb | 3 +-- lib/api/projects.rb | 1 - lib/api/protected_branches.rb | 4 +--- lib/api/services.rb | 14 +++++++------- lib/api/users.rb | 7 +++++-- lib/api/variables.rb | 1 + spec/requests/api/pipeline_schedules_spec.rb | 3 +-- 9 files changed, 18 insertions(+), 20 deletions(-) diff --git a/lib/api/group_variables.rb b/lib/api/group_variables.rb index f64da4ab77b..25152f30998 100644 --- a/lib/api/group_variables.rb +++ b/lib/api/group_variables.rb @@ -88,8 +88,7 @@ module API variable = user_group.variables.find_by(key: params[:key]) not_found!('GroupVariable') unless variable - status 204 - variable.destroy + destroy_conditionally!(variable) end end end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 8d4f8c01903..84980864151 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -14,7 +14,7 @@ module API def check_unmodified_since!(last_modified) if_unmodified_since = Time.parse(headers['If-Unmodified-Since']) rescue nil - if if_unmodified_since && last_modified > if_unmodified_since + if if_unmodified_since && last_modified && last_modified > if_unmodified_since render_api_error!('412 Precondition Failed', 412) end end diff --git a/lib/api/pipeline_schedules.rb b/lib/api/pipeline_schedules.rb index dbeaf9e17ef..e3123ef4e2d 100644 --- a/lib/api/pipeline_schedules.rb +++ b/lib/api/pipeline_schedules.rb @@ -117,8 +117,7 @@ module API not_found!('PipelineSchedule') unless pipeline_schedule authorize! :admin_pipeline_schedule, pipeline_schedule - status :accepted - present pipeline_schedule.destroy, with: Entities::PipelineScheduleDetails + destroy_conditionally!(pipeline_schedule) end end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 36fe3f243b9..78e25b6da03 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -1,7 +1,6 @@ require_dependency 'declarative_policy' module API - # Projects API class Projects < Grape::API include PaginationParams diff --git a/lib/api/protected_branches.rb b/lib/api/protected_branches.rb index dccf4fa27a7..15fcb9e8e27 100644 --- a/lib/api/protected_branches.rb +++ b/lib/api/protected_branches.rb @@ -76,9 +76,7 @@ module API delete ':id/protected_branches/:name', requirements: BRANCH_ENDPOINT_REQUIREMENTS do protected_branch = user_project.protected_branches.find_by!(name: params[:name]) - protected_branch.destroy - - status 204 + destroy_conditionally!(protected_branch) end end end diff --git a/lib/api/services.rb b/lib/api/services.rb index 2b5ef75b6bf..ff9ddd44439 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -655,15 +655,15 @@ module API end delete ":id/services/:service_slug" do service = user_project.find_or_initialize_service(params[:service_slug].underscore) - # Todo: Check if this done the right way - check_unmodified_since!(service.updated_at) - attrs = service_attributes(service).inject({}) do |hash, key| - hash.merge!(key => nil) - end + destroy_conditionally!(service) do + attrs = service_attributes(service).inject({}) do |hash, key| + hash.merge!(key => nil) + end - unless service.update_attributes(attrs.merge(active: false)) - render_api_error!('400 Bad Request', 400) + unless service.update_attributes(attrs.merge(active: false)) + render_api_error!('400 Bad Request', 400) + end end end diff --git a/lib/api/users.rb b/lib/api/users.rb index d7c7b9ae9c1..96f47bb618a 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -408,8 +408,11 @@ module API requires :impersonation_token_id, type: Integer, desc: 'The ID of the impersonation token' end delete ':impersonation_token_id' do - status 204 - find_impersonation_token.revoke! + token = find_impersonation_token + + destroy_conditionally!(token) do + token.revoke! + end end end end diff --git a/lib/api/variables.rb b/lib/api/variables.rb index 7c0fdd3d1be..da71787abab 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -88,6 +88,7 @@ module API variable = user_project.variables.find_by(key: params[:key]) not_found!('Variable') unless variable + # Variables don't have any timestamp. Therfore, destroy unconditionally. status 204 variable.destroy end diff --git a/spec/requests/api/pipeline_schedules_spec.rb b/spec/requests/api/pipeline_schedules_spec.rb index 1fc0ec528b9..150a391bb8d 100644 --- a/spec/requests/api/pipeline_schedules_spec.rb +++ b/spec/requests/api/pipeline_schedules_spec.rb @@ -267,8 +267,7 @@ describe API::PipelineSchedules do delete api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", master) end.to change { project.pipeline_schedules.count }.by(-1) - expect(response).to have_http_status(:accepted) - expect(response).to match_response_schema('pipeline_schedule') + expect(response).to have_http_status(204) end it 'responds with 404 Not Found if requesting non-existing pipeline_schedule' do From 915dd57fe26fb228f30ae08edc4905b24d4c4339 Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Thu, 24 Aug 2017 18:03:39 +0200 Subject: [PATCH 21/57] Add tests for the unmodified header --- spec/requests/api/award_emoji_spec.rb | 16 +++++ spec/requests/api/boards_spec.rb | 4 ++ spec/requests/api/branches_spec.rb | 4 ++ spec/requests/api/broadcast_messages_spec.rb | 4 ++ spec/requests/api/deploy_keys_spec.rb | 4 ++ spec/requests/api/environments_spec.rb | 4 ++ spec/requests/api/group_variables_spec.rb | 4 ++ spec/requests/api/groups_spec.rb | 4 ++ spec/requests/api/issues_spec.rb | 4 ++ spec/requests/api/labels_spec.rb | 5 ++ spec/requests/api/members_spec.rb | 4 ++ spec/requests/api/merge_requests_spec.rb | 4 ++ spec/requests/api/notes_spec.rb | 12 ++++ spec/requests/api/pipeline_schedules_spec.rb | 4 ++ spec/requests/api/project_hooks_spec.rb | 4 ++ spec/requests/api/project_snippets_spec.rb | 7 +- spec/requests/api/projects_spec.rb | 69 +++++++++++++------ spec/requests/api/protected_branches_spec.rb | 4 ++ spec/requests/api/runner_spec.rb | 5 ++ spec/requests/api/runners_spec.rb | 12 ++++ spec/requests/api/snippets_spec.rb | 4 ++ spec/requests/api/system_hooks_spec.rb | 4 ++ spec/requests/api/tags_spec.rb | 4 ++ spec/requests/api/triggers_spec.rb | 4 ++ spec/requests/api/users_spec.rb | 24 +++++++ .../requests/api/status_shared_examples.rb | 11 +++ 26 files changed, 206 insertions(+), 23 deletions(-) diff --git a/spec/requests/api/award_emoji_spec.rb b/spec/requests/api/award_emoji_spec.rb index 1dd9f3f6ddc..593068b8cd7 100644 --- a/spec/requests/api/award_emoji_spec.rb +++ b/spec/requests/api/award_emoji_spec.rb @@ -253,6 +253,10 @@ describe API::AwardEmoji do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/issues/#{issue.iid}/award_emoji/#{award_emoji.id}", user) } + end end context 'when the awardable is a Merge Request' do @@ -269,6 +273,10 @@ describe API::AwardEmoji do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/award_emoji/#{downvote.id}", user) } + end end context 'when the awardable is a Snippet' do @@ -282,6 +290,10 @@ describe API::AwardEmoji do expect(response).to have_http_status(204) end.to change { snippet.award_emoji.count }.from(1).to(0) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/snippets/#{snippet.id}/award_emoji/#{award.id}", user) } + end end end @@ -295,5 +307,9 @@ describe API::AwardEmoji do expect(response).to have_http_status(204) end.to change { note.award_emoji.count }.from(1).to(0) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/issues/#{issue.iid}/notes/#{note.id}/award_emoji/#{rocket.id}", user) } + end end end diff --git a/spec/requests/api/boards_spec.rb b/spec/requests/api/boards_spec.rb index 43b381c2219..f698d5dddb3 100644 --- a/spec/requests/api/boards_spec.rb +++ b/spec/requests/api/boards_spec.rb @@ -195,6 +195,10 @@ describe API::Boards do expect(response).to have_http_status(204) end + + it_behaves_like '412 response' do + let(:request) { api("#{base_url}/#{dev_list.id}", owner) } + end end end end diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb index 5a2e1b2cf2d..b1e011de604 100644 --- a/spec/requests/api/branches_spec.rb +++ b/spec/requests/api/branches_spec.rb @@ -499,6 +499,10 @@ describe API::Branches do expect(response).to have_gitlab_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/repository/branches/#{branch_name}", user) } + end end describe 'DELETE /projects/:id/repository/merged_branches' do diff --git a/spec/requests/api/broadcast_messages_spec.rb b/spec/requests/api/broadcast_messages_spec.rb index 67989689799..b043a333d33 100644 --- a/spec/requests/api/broadcast_messages_spec.rb +++ b/spec/requests/api/broadcast_messages_spec.rb @@ -171,6 +171,10 @@ describe API::BroadcastMessages do expect(response).to have_http_status(403) end + it_behaves_like '412 response' do + let(:request) { api("/broadcast_messages/#{message.id}", admin) } + end + it 'deletes the broadcast message for admins' do expect do delete api("/broadcast_messages/#{message.id}", admin) diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb index e497ec333a2..684877c33c0 100644 --- a/spec/requests/api/deploy_keys_spec.rb +++ b/spec/requests/api/deploy_keys_spec.rb @@ -190,6 +190,10 @@ describe API::DeployKeys do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", admin) } + end end describe 'POST /projects/:id/deploy_keys/:key_id/enable' do diff --git a/spec/requests/api/environments_spec.rb b/spec/requests/api/environments_spec.rb index 87716c6fe3a..2361809e0e1 100644 --- a/spec/requests/api/environments_spec.rb +++ b/spec/requests/api/environments_spec.rb @@ -138,6 +138,10 @@ describe API::Environments do expect(response).to have_http_status(404) expect(json_response['message']).to eq('404 Not found') end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/environments/#{environment.id}", user) } + end end context 'a non member' do diff --git a/spec/requests/api/group_variables_spec.rb b/spec/requests/api/group_variables_spec.rb index 2179790d098..93b9cf85c1d 100644 --- a/spec/requests/api/group_variables_spec.rb +++ b/spec/requests/api/group_variables_spec.rb @@ -200,6 +200,10 @@ describe API::GroupVariables do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/groups/#{group.id}/variables/#{variable.key}", user) } + end end context 'authorized user with invalid permissions' do diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index a7557c7fb22..39d76cdbc74 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -488,6 +488,10 @@ describe API::Groups do expect(response).to have_http_status(204) end + it_behaves_like '412 response' do + let(:request) { api("/groups/#{group1.id}", user1) } + end + it "does not remove a group if not an owner" do user4 = create(:user) group1.add_master(user4) diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 7d120e4a234..58d89451073 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -1304,6 +1304,10 @@ describe API::Issues, :mailer do expect(response).to have_http_status(204) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/issues/#{issue.iid}", owner) } + end end context 'when issue does not exist' do diff --git a/spec/requests/api/labels_spec.rb b/spec/requests/api/labels_spec.rb index 5a4257d1009..b231fdea2a3 100644 --- a/spec/requests/api/labels_spec.rb +++ b/spec/requests/api/labels_spec.rb @@ -189,6 +189,11 @@ describe API::Labels do delete api("/projects/#{project.id}/labels", user) expect(response).to have_http_status(400) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/labels", user) } + let(:params) { { name: 'label1' } } + end end describe 'PUT /projects/:id/labels' do diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb index 06aca698c91..d3bae8d2888 100644 --- a/spec/requests/api/members_spec.rb +++ b/spec/requests/api/members_spec.rb @@ -284,6 +284,10 @@ describe API::Members do expect(response).to have_http_status(204) end.to change { source.members.count }.by(-1) end + + it_behaves_like '412 response' do + let(:request) { api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master) } + end end it 'returns 404 if member does not exist' do diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 0db645863fb..9027090aabd 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -698,6 +698,10 @@ describe API::MergeRequests do expect(response).to have_gitlab_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user) } + end end end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 75e5062a99c..f5882c0c74a 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -390,6 +390,10 @@ describe API::Notes do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/issues/#{issue.iid}/notes/#{issue_note.id}", user) } + end end context 'when noteable is a Snippet' do @@ -410,6 +414,10 @@ describe API::Notes do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/snippets/#{snippet.id}/notes/#{snippet_note.id}", user) } + end end context 'when noteable is a Merge Request' do @@ -430,6 +438,10 @@ describe API::Notes do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/notes/#{merge_request_note.id}", user) } + end end end end diff --git a/spec/requests/api/pipeline_schedules_spec.rb b/spec/requests/api/pipeline_schedules_spec.rb index 150a391bb8d..b6a5a7ffbb5 100644 --- a/spec/requests/api/pipeline_schedules_spec.rb +++ b/spec/requests/api/pipeline_schedules_spec.rb @@ -275,6 +275,10 @@ describe API::PipelineSchedules do expect(response).to have_http_status(:not_found) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", master) } + end end context 'authenticated user with invalid permissions' do diff --git a/spec/requests/api/project_hooks_spec.rb b/spec/requests/api/project_hooks_spec.rb index 2829c243af3..ac3bab09c4c 100644 --- a/spec/requests/api/project_hooks_spec.rb +++ b/spec/requests/api/project_hooks_spec.rb @@ -212,5 +212,9 @@ describe API::ProjectHooks, 'ProjectHooks' do expect(response).to have_http_status(404) expect(WebHook.exists?(hook.id)).to be_truthy end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/hooks/#{hook.id}", user) } + end end end diff --git a/spec/requests/api/project_snippets_spec.rb b/spec/requests/api/project_snippets_spec.rb index 2b541f5719e..0ea95f487ac 100644 --- a/spec/requests/api/project_snippets_spec.rb +++ b/spec/requests/api/project_snippets_spec.rb @@ -228,9 +228,6 @@ describe API::ProjectSnippets do let(:snippet) { create(:project_snippet, author: admin) } it 'deletes snippet' do - admin = create(:admin) - snippet = create(:project_snippet, author: admin) - delete api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin) expect(response).to have_http_status(204) @@ -242,6 +239,10 @@ describe API::ProjectSnippets do expect(response).to have_http_status(404) expect(json_response['message']).to eq('404 Snippet Not Found') end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin) } + end end describe 'GET /projects/:project_id/snippets/:id/raw' do diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index a89a58ff713..46550243d06 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1029,6 +1029,10 @@ describe API::Projects do delete api("/projects/#{project.id}/snippets/1234", user) expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/snippets/#{snippet.id}", user) } + end end describe 'GET /projects/:id/snippets/:snippet_id/raw' do @@ -1104,25 +1108,33 @@ describe API::Projects do project_fork_target.group.add_developer user2 end + context 'for a forked project' do + before do + post api("/projects/#{project_fork_target.id}/fork/#{project_fork_source.id}", admin) + project_fork_target.reload + expect(project_fork_target.forked_from_project).not_to be_nil + expect(project_fork_target.forked?).to be_truthy + end + + it 'makes forked project unforked' do + delete api("/projects/#{project_fork_target.id}/fork", admin) + + expect(response).to have_http_status(204) + project_fork_target.reload + expect(project_fork_target.forked_from_project).to be_nil + expect(project_fork_target.forked?).not_to be_truthy + end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project_fork_target.id}/fork", admin) } + end + end + it 'is forbidden to non-owner users' do delete api("/projects/#{project_fork_target.id}/fork", user2) expect(response).to have_http_status(403) end - it 'makes forked project unforked' do - post api("/projects/#{project_fork_target.id}/fork/#{project_fork_source.id}", admin) - project_fork_target.reload - expect(project_fork_target.forked_from_project).not_to be_nil - expect(project_fork_target.forked?).to be_truthy - - delete api("/projects/#{project_fork_target.id}/fork", admin) - - expect(response).to have_http_status(204) - project_fork_target.reload - expect(project_fork_target.forked_from_project).to be_nil - expect(project_fork_target.forked?).not_to be_truthy - end - it 'is idempotent if not forked' do expect(project_fork_target.forked_from_project).to be_nil delete api("/projects/#{project_fork_target.id}/fork", admin) @@ -1188,14 +1200,23 @@ describe API::Projects do end describe 'DELETE /projects/:id/share/:group_id' do - it 'returns 204 when deleting a group share' do - group = create(:group, :public) - create(:project_group_link, group: group, project: project) + context 'for a valid group' do + let(:group) { create(:group, :public) } - delete api("/projects/#{project.id}/share/#{group.id}", user) + before do + create(:project_group_link, group: group, project: project) + end - expect(response).to have_http_status(204) - expect(project.project_group_links).to be_empty + it 'returns 204 when deleting a group share' do + delete api("/projects/#{project.id}/share/#{group.id}", user) + + expect(response).to have_http_status(204) + expect(project.project_group_links).to be_empty + end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/share/#{group.id}", user) } + end end it 'returns a 400 when group id is not an integer' do @@ -1519,6 +1540,10 @@ describe API::Projects do expect(json_response['message']).to eql('202 Accepted') end + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}", user) } + end + it 'does not remove a project if not an owner' do user3 = create(:user) project.team << [user3, :developer] @@ -1549,6 +1574,10 @@ describe API::Projects do delete api('/projects/1328', admin) expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}", admin) } + end end end diff --git a/spec/requests/api/protected_branches_spec.rb b/spec/requests/api/protected_branches_spec.rb index 1aa8a95780e..07d7f96bd70 100644 --- a/spec/requests/api/protected_branches_spec.rb +++ b/spec/requests/api/protected_branches_spec.rb @@ -213,6 +213,10 @@ describe API::ProtectedBranches do expect(response).to have_gitlab_http_status(204) end + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/protected_branches/#{branch_name}", user) } + end + it "returns 404 if branch does not exist" do delete api("/projects/#{project.id}/protected_branches/barfoo", user) diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index e9ee3dd679d..993164aa8fe 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -149,6 +149,11 @@ describe API::Runner do expect(response).to have_http_status 204 expect(Ci::Runner.count).to eq(0) end + + it_behaves_like '412 response' do + let(:request) { api('/runners') } + let(:params) { { token: runner.token } } + end end end diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index c8ff25f70fa..244895a417e 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -279,6 +279,10 @@ describe API::Runners do expect(response).to have_http_status(204) end.to change { Ci::Runner.shared.count }.by(-1) end + + it_behaves_like '412 response' do + let(:request) { api("/runners/#{shared_runner.id}", admin) } + end end context 'when runner is not shared' do @@ -332,6 +336,10 @@ describe API::Runners do expect(response).to have_http_status(204) end.to change { Ci::Runner.specific.count }.by(-1) end + + it_behaves_like '412 response' do + let(:request) { api("/runners/#{specific_runner.id}", user) } + end end end @@ -463,6 +471,10 @@ describe API::Runners do expect(response).to have_http_status(204) end.to change { project.runners.count }.by(-1) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/runners/#{two_projects_runner.id}", user) } + end end context 'when runner have one associated projects' do diff --git a/spec/requests/api/snippets_spec.rb b/spec/requests/api/snippets_spec.rb index d09b8bc42f1..351dd3276d0 100644 --- a/spec/requests/api/snippets_spec.rb +++ b/spec/requests/api/snippets_spec.rb @@ -270,6 +270,10 @@ describe API::Snippets do expect(response).to have_http_status(404) expect(json_response['message']).to eq('404 Snippet Not Found') end + + it_behaves_like '412 response' do + let(:request) { api("/snippets/#{public_snippet.id}", user) } + end end describe "GET /snippets/:id/user_agent_detail" do diff --git a/spec/requests/api/system_hooks_spec.rb b/spec/requests/api/system_hooks_spec.rb index f65b475fe44..216d278ad21 100644 --- a/spec/requests/api/system_hooks_spec.rb +++ b/spec/requests/api/system_hooks_spec.rb @@ -102,5 +102,9 @@ describe API::SystemHooks do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/hooks/#{hook.id}", admin) } + end end end diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb index 6ac0caa7fe8..0bf7863bdc8 100644 --- a/spec/requests/api/tags_spec.rb +++ b/spec/requests/api/tags_spec.rb @@ -278,6 +278,10 @@ describe API::Tags do expect(response).to have_gitlab_http_status(204) end + it_behaves_like '412 response' do + let(:request) { api(route, current_user) } + end + context 'when tag does not exist' do let(:tag_name) { 'unknown' } diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index 1e206fd2a9e..d1ed57b1825 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -293,6 +293,10 @@ describe API::Triggers do expect(response).to have_http_status(404) end + + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/triggers/#{trigger.id}", user) } + end end context 'authenticated user with invalid permissions' do diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 49739a1601a..5fef4437997 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -733,6 +733,10 @@ describe API::Users do end.to change { user.keys.count }.by(-1) end + it_behaves_like '412 response' do + let(:request) { api("/users/#{user.id}/keys/#{key.id}", admin) } + end + it 'returns 404 error if user not found' do user.keys << key user.save @@ -838,6 +842,10 @@ describe API::Users do end.to change { user.emails.count }.by(-1) end + it_behaves_like '412 response' do + let(:request) { api("/users/#{user.id}/emails/#{email.id}", admin) } + end + it 'returns 404 error if user not found' do user.emails << email user.save @@ -876,6 +884,10 @@ describe API::Users do expect { Namespace.find(namespace.id) }.to raise_error ActiveRecord::RecordNotFound end + it_behaves_like '412 response' do + let(:request) { api("/users/#{user.id}", admin) } + end + it "does not delete for unauthenticated user" do Sidekiq::Testing.inline! { delete api("/users/#{user.id}") } expect(response).to have_http_status(401) @@ -1116,6 +1128,10 @@ describe API::Users do end.to change { user.keys.count}.by(-1) end + it_behaves_like '412 response' do + let(:request) { api("/user/keys/#{key.id}", user) } + end + it "returns 404 if key ID not found" do delete api("/user/keys/42", user) @@ -1239,6 +1255,10 @@ describe API::Users do end.to change { user.emails.count}.by(-1) end + it_behaves_like '412 response' do + let(:request) { api("/user/emails/#{email.id}", user) } + end + it "returns 404 if email ID not found" do delete api("/user/emails/42", user) @@ -1551,6 +1571,10 @@ describe API::Users do expect(json_response['message']).to eq('403 Forbidden') end + it_behaves_like '412 response' do + let(:request) { api("/users/#{user.id}/impersonation_tokens/#{impersonation_token.id}", admin) } + end + it 'revokes a impersonation token' do delete api("/users/#{user.id}/impersonation_tokens/#{impersonation_token.id}", admin) diff --git a/spec/support/shared_examples/requests/api/status_shared_examples.rb b/spec/support/shared_examples/requests/api/status_shared_examples.rb index 226277411d6..1bffd1e49db 100644 --- a/spec/support/shared_examples/requests/api/status_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/status_shared_examples.rb @@ -40,3 +40,14 @@ shared_examples_for '404 response' do end end end + +shared_examples_for '412 response' do + let(:params) { nil } + before do + delete request, params, { 'HTTP_IF_UNMODIFIED_SINCE' => '1990-01-12T00:00:48-0600' } + end + + it 'returns 412' do + expect(response).to have_gitlab_http_status(412) + end +end From 8bd9fb4cc4f8689a23c68e1b7f893ee59907069f Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Fri, 25 Aug 2017 14:31:09 +0200 Subject: [PATCH 22/57] Add changelog and doc --- changelogs/unreleased/api-delete-respect-headers.yml | 5 +++++ doc/api/README.md | 1 + 2 files changed, 6 insertions(+) create mode 100644 changelogs/unreleased/api-delete-respect-headers.yml diff --git a/changelogs/unreleased/api-delete-respect-headers.yml b/changelogs/unreleased/api-delete-respect-headers.yml new file mode 100644 index 00000000000..cfc8fbfdf91 --- /dev/null +++ b/changelogs/unreleased/api-delete-respect-headers.yml @@ -0,0 +1,5 @@ +--- +title: 'API: Respect the "If-Unmodified-Since" header when delting a resource' +merge_request: 9621 +author: Robert Schilling +type: added diff --git a/doc/api/README.md b/doc/api/README.md index 266b5f018d9..c2a08dcff07 100644 --- a/doc/api/README.md +++ b/doc/api/README.md @@ -263,6 +263,7 @@ The following table shows the possible return codes for API requests. | `404 Not Found` | A resource could not be accessed, e.g., an ID for a resource could not be found. | | `405 Method Not Allowed` | The request is not supported. | | `409 Conflict` | A conflicting resource already exists, e.g., creating a project with a name that already exists. | +| `412` | Indicates the request was denied. May happen if the `If-Unmodified-Since` header is provided when trying to delete a resource, which was modified in between. | | `422 Unprocessable` | The entity could not be processed. | | `500 Server Error` | While handling the request something went wrong server-side. | From 87b51c5981db3b1b9831b01ca6e74127d57dc2d9 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Tue, 29 Aug 2017 07:14:41 +0900 Subject: [PATCH 23/57] Move the logic to a concern --- app/models/user.rb | 3 ++- lib/gitlab/sql/pattern.rb | 39 ++++++++++++----------------- spec/lib/gitlab/sql/pattern_spec.rb | 16 ++++++------ 3 files changed, 26 insertions(+), 32 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index e5a84ce4080..70787de4b40 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -5,6 +5,7 @@ class User < ActiveRecord::Base include Gitlab::ConfigHelper include Gitlab::CurrentSettings + include Gitlab::SQL::Pattern include Avatarable include Referable include Sortable @@ -303,7 +304,7 @@ class User < ActiveRecord::Base # Returns an ActiveRecord::Relation. def search(query) table = arel_table - pattern = Gitlab::SQL::Pattern.new(query).to_sql + pattern = User.to_pattern(query) order = <<~SQL CASE diff --git a/lib/gitlab/sql/pattern.rb b/lib/gitlab/sql/pattern.rb index 46c973d8a11..26bfeeeee67 100644 --- a/lib/gitlab/sql/pattern.rb +++ b/lib/gitlab/sql/pattern.rb @@ -1,33 +1,26 @@ module Gitlab module SQL - class Pattern + module Pattern + extend ActiveSupport::Concern + MIN_CHARS_FOR_PARTIAL_MATCHING = 3 - attr_reader :query - - def initialize(query) - @query = query - end - - def to_sql - if exact_matching? - sanitized_query - else - "%#{sanitized_query}%" + class_methods do + def to_pattern(query) + if exact_matching?(query) + sanitize_sql_like(query) + else + "%#{sanitize_sql_like(query)}%" + end end - end - def exact_matching? - !partial_matching? - end + def exact_matching?(query) + query.length < MIN_CHARS_FOR_PARTIAL_MATCHING + end - def partial_matching? - @query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING - end - - def sanitized_query - # Note: ActiveRecord::Base.sanitize_sql_like is a protected method - ActiveRecord::Base.__send__(:sanitize_sql_like, query) + def partial_matching?(query) + query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING + end end end end diff --git a/spec/lib/gitlab/sql/pattern_spec.rb b/spec/lib/gitlab/sql/pattern_spec.rb index d0412f37098..224336421be 100644 --- a/spec/lib/gitlab/sql/pattern_spec.rb +++ b/spec/lib/gitlab/sql/pattern_spec.rb @@ -1,14 +1,14 @@ require 'spec_helper' describe Gitlab::SQL::Pattern do - describe '#to_sql' do - subject(:to_sql) { described_class.new(query).to_sql } + describe '#to_pattern' do + subject(:to_pattern) { User.to_pattern(query) } context 'when a query is shorter than 3 chars' do let(:query) { '12' } it 'returns exact matching pattern' do - expect(to_sql).to eq('12') + expect(to_pattern).to eq('12') end end @@ -16,7 +16,7 @@ describe Gitlab::SQL::Pattern do let(:query) { '_2' } it 'returns sanitized exact matching pattern' do - expect(to_sql).to eq('\_2') + expect(to_pattern).to eq('\_2') end end @@ -24,7 +24,7 @@ describe Gitlab::SQL::Pattern do let(:query) { '123' } it 'returns partial matching pattern' do - expect(to_sql).to eq('%123%') + expect(to_pattern).to eq('%123%') end end @@ -32,7 +32,7 @@ describe Gitlab::SQL::Pattern do let(:query) { '_23' } it 'returns partial matching pattern' do - expect(to_sql).to eq('%\_23%') + expect(to_pattern).to eq('%\_23%') end end @@ -40,7 +40,7 @@ describe Gitlab::SQL::Pattern do let(:query) { '1234' } it 'returns partial matching pattern' do - expect(to_sql).to eq('%1234%') + expect(to_pattern).to eq('%1234%') end end @@ -48,7 +48,7 @@ describe Gitlab::SQL::Pattern do let(:query) { '_234' } it 'returns sanitized partial matching pattern' do - expect(to_sql).to eq('%\_234%') + expect(to_pattern).to eq('%\_234%') end end end From ee4820a5268d02fb7ed142511d1a194f06629a1e Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Mon, 28 Aug 2017 17:13:22 +0200 Subject: [PATCH 24/57] Add a spec when ressource is not modified --- lib/api/projects.rb | 6 ++--- spec/requests/api/projects_spec.rb | 2 ++ .../requests/api/status_shared_examples.rb | 22 +++++++++++++++---- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 78e25b6da03..78d900984ac 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -365,8 +365,7 @@ module API authorize! :remove_fork_project, user_project if user_project.forked? - status 204 - user_project.forked_project_link.destroy + destroy_conditionally!(user_project.forked_project_link) else not_modified! end @@ -410,8 +409,7 @@ module API link = user_project.project_group_links.find_by(group_id: params[:group_id]) not_found!('Group Link') unless link - status 204 - link.destroy + destroy_conditionally!(link) end desc 'Upload a file' diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 46550243d06..4490e50702b 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1541,6 +1541,7 @@ describe API::Projects do end it_behaves_like '412 response' do + let(:success_status) { 202 } let(:request) { api("/projects/#{project.id}", user) } end @@ -1576,6 +1577,7 @@ describe API::Projects do end it_behaves_like '412 response' do + let(:success_status) { 202 } let(:request) { api("/projects/#{project.id}", admin) } end end diff --git a/spec/support/shared_examples/requests/api/status_shared_examples.rb b/spec/support/shared_examples/requests/api/status_shared_examples.rb index 1bffd1e49db..7d7f66adeab 100644 --- a/spec/support/shared_examples/requests/api/status_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/status_shared_examples.rb @@ -43,11 +43,25 @@ end shared_examples_for '412 response' do let(:params) { nil } - before do - delete request, params, { 'HTTP_IF_UNMODIFIED_SINCE' => '1990-01-12T00:00:48-0600' } + let(:success_status) { 204 } + + context 'for a modified ressource' do + before do + delete request, params, { 'HTTP_IF_UNMODIFIED_SINCE' => '1990-01-12T00:00:48-0600' } + end + + it 'returns 412' do + expect(response).to have_gitlab_http_status(412) + end end - it 'returns 412' do - expect(response).to have_gitlab_http_status(412) + context 'for an unmodified ressource' do + before do + delete request, params, { 'HTTP_IF_UNMODIFIED_SINCE' => Time.now } + end + + it 'returns accepted' do + expect(response).to have_gitlab_http_status(success_status) + end end end From 9954dafda58a0d5d856fdbbef01633e674638aa1 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 29 Aug 2017 16:23:12 +0800 Subject: [PATCH 25/57] Just use a block --- lib/gitlab/git/repository.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index f6d30ad7534..adf15473eb6 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -585,7 +585,7 @@ module Gitlab end def delete_refs(ref_names) - ref_names.each(&rugged.references.method(:delete)) + ref_names.each { |ref| rugged.references.delete(ref) } end # Create a new branch named **ref+ based on **stat_point+, HEAD by default From 12633b46b6884dda4ffd87b14b4b52725acd6ec1 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Tue, 29 Aug 2017 18:00:03 +0900 Subject: [PATCH 26/57] Refactor --- lib/gitlab/sql/pattern.rb | 10 +++------- spec/lib/gitlab/sql/pattern_spec.rb | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/gitlab/sql/pattern.rb b/lib/gitlab/sql/pattern.rb index 26bfeeeee67..b42bc67ccfc 100644 --- a/lib/gitlab/sql/pattern.rb +++ b/lib/gitlab/sql/pattern.rb @@ -7,17 +7,13 @@ module Gitlab class_methods do def to_pattern(query) - if exact_matching?(query) - sanitize_sql_like(query) - else + if partial_matching?(query) "%#{sanitize_sql_like(query)}%" + else + sanitize_sql_like(query) end end - def exact_matching?(query) - query.length < MIN_CHARS_FOR_PARTIAL_MATCHING - end - def partial_matching?(query) query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING end diff --git a/spec/lib/gitlab/sql/pattern_spec.rb b/spec/lib/gitlab/sql/pattern_spec.rb index 224336421be..9d7b2136dab 100644 --- a/spec/lib/gitlab/sql/pattern_spec.rb +++ b/spec/lib/gitlab/sql/pattern_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::SQL::Pattern do - describe '#to_pattern' do + describe '.to_pattern' do subject(:to_pattern) { User.to_pattern(query) } context 'when a query is shorter than 3 chars' do From 5eab624d3ce7500b3cc19230b5ce86125b88015b Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 18 Aug 2017 14:26:23 +0200 Subject: [PATCH 27/57] Improve migrations using triggers This adds a bunch of checks to migrations that may create or drop triggers. Dropping triggers/functions is done using "IF EXISTS" so we don't throw an error if the object in question has already been dropped. We now also raise a custom error (message) when the user does not have TRIGGER privileges. This should prevent the schema from entering an inconsistent state while also providing the user with enough information on how to solve the problem. The recommendation of using SUPERUSER permissions is a bit extreme but we require this anyway (Omnibus also configures users with this permission). Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/36633 --- .../unreleased/check-trigger-permissions.yml | 5 +++ lib/gitlab/database.rb | 8 +++++ lib/gitlab/database/grant.rb | 34 ++++++++++++++++++ lib/gitlab/database/migration_helpers.rb | 36 ++++++++++++++++--- spec/lib/gitlab/database/grant_spec.rb | 30 ++++++++++++++++ .../gitlab/database/migration_helpers_spec.rb | 32 ++++++++++++++--- 6 files changed, 137 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/check-trigger-permissions.yml create mode 100644 lib/gitlab/database/grant.rb create mode 100644 spec/lib/gitlab/database/grant_spec.rb diff --git a/changelogs/unreleased/check-trigger-permissions.yml b/changelogs/unreleased/check-trigger-permissions.yml new file mode 100644 index 00000000000..e0809cea9bf --- /dev/null +++ b/changelogs/unreleased/check-trigger-permissions.yml @@ -0,0 +1,5 @@ +--- +title: Improve migrations using triggers +merge_request: +author: +type: fixed diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index e001d25e7b7..a6ec75da385 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -9,6 +9,14 @@ module Gitlab ActiveRecord::Base.configurations[Rails.env] end + def self.username + config['username'] || ENV['USER'] + end + + def self.database_name + config['database'] + end + def self.adapter_name config['adapter'] end diff --git a/lib/gitlab/database/grant.rb b/lib/gitlab/database/grant.rb new file mode 100644 index 00000000000..aee3981e79a --- /dev/null +++ b/lib/gitlab/database/grant.rb @@ -0,0 +1,34 @@ +module Gitlab + module Database + # Model that can be used for querying permissions of a SQL user. + class Grant < ActiveRecord::Base + self.table_name = + if Database.postgresql? + 'information_schema.role_table_grants' + else + 'mysql.user' + end + + def self.scope_to_current_user + if Database.postgresql? + where('grantee = user') + else + where("CONCAT(User, '@', Host) = current_user()") + end + end + + # Returns true if the current user can create and execute triggers on the + # given table. + def self.create_and_execute_trigger?(table) + priv = + if Database.postgresql? + where(privilege_type: 'TRIGGER', table_name: table) + else + where(Trigger_priv: 'Y') + end + + priv.scope_to_current_user.any? + end + end + end +end diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 5e2c6cc5cad..fb14798efe6 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -358,6 +358,8 @@ module Gitlab raise 'rename_column_concurrently can not be run inside a transaction' end + check_trigger_permissions!(table) + old_col = column_for(table, old) new_type = type || old_col.type @@ -430,6 +432,8 @@ module Gitlab def cleanup_concurrent_column_rename(table, old, new) trigger_name = rename_trigger_name(table, old, new) + check_trigger_permissions!(table) + if Database.postgresql? remove_rename_triggers_for_postgresql(table, trigger_name) else @@ -485,14 +489,14 @@ module Gitlab # Removes the triggers used for renaming a PostgreSQL column concurrently. def remove_rename_triggers_for_postgresql(table, trigger) - execute("DROP TRIGGER #{trigger} ON #{table}") - execute("DROP FUNCTION #{trigger}()") + execute("DROP TRIGGER IF EXISTS #{trigger} ON #{table}") + execute("DROP FUNCTION IF EXISTS #{trigger}()") end # Removes the triggers used for renaming a MySQL column concurrently. def remove_rename_triggers_for_mysql(trigger) - execute("DROP TRIGGER #{trigger}_insert") - execute("DROP TRIGGER #{trigger}_update") + execute("DROP TRIGGER IF EXISTS #{trigger}_insert") + execute("DROP TRIGGER IF EXISTS #{trigger}_update") end # Returns the (base) name to use for triggers when renaming columns. @@ -625,6 +629,30 @@ module Gitlab conn.llen("queue:#{queue_name}") end end + + def check_trigger_permissions!(table) + unless Grant.create_and_execute_trigger?(table) + dbname = Database.database_name + user = Database.username + + raise <<-EOF +Your database user is not allowed to create, drop, or execute triggers on the +table #{table}. + +If you are using PostgreSQL you can solve this by logging in to the GitLab +database (#{dbname}) using a super user and running: + + ALTER #{user} WITH SUPERUSER + +For MySQL you instead need to run: + + GRANT ALL PRIVILEGES ON *.* TO #{user}@'%' + +Both queries will grant the user super user permissions, ensuring you don't run +into similar problems in the future (e.g. when new tables are created). + EOF + end + end end end end diff --git a/spec/lib/gitlab/database/grant_spec.rb b/spec/lib/gitlab/database/grant_spec.rb new file mode 100644 index 00000000000..651da3e8476 --- /dev/null +++ b/spec/lib/gitlab/database/grant_spec.rb @@ -0,0 +1,30 @@ +require 'spec_helper' + +describe Gitlab::Database::Grant do + describe '.scope_to_current_user' do + it 'scopes the relation to the current user' do + user = Gitlab::Database.username + column = Gitlab::Database.postgresql? ? :grantee : :User + names = described_class.scope_to_current_user.pluck(column).uniq + + expect(names).to eq([user]) + end + end + + describe '.create_and_execute_trigger' do + it 'returns true when the user can create and execute a trigger' do + # We assume the DB/user is set up correctly so that triggers can be + # created, which is necessary anyway for other tests to work. + expect(described_class.create_and_execute_trigger?('users')).to eq(true) + end + + it 'returns false when the user can not create and/or execute a trigger' do + allow(described_class).to receive(:scope_to_current_user) + .and_return(described_class.none) + + result = described_class.create_and_execute_trigger?('kittens') + + expect(result).to eq(false) + end + end +end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index c25fd459dd7..1bcdc369c44 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -450,6 +450,8 @@ describe Gitlab::Database::MigrationHelpers do it 'renames a column concurrently' do allow(Gitlab::Database).to receive(:postgresql?).and_return(false) + expect(model).to receive(:check_trigger_permissions!).with(:users) + expect(model).to receive(:install_rename_triggers_for_mysql) .with(trigger_name, 'users', 'old', 'new') @@ -477,6 +479,8 @@ describe Gitlab::Database::MigrationHelpers do it 'renames a column concurrently' do allow(Gitlab::Database).to receive(:postgresql?).and_return(true) + expect(model).to receive(:check_trigger_permissions!).with(:users) + expect(model).to receive(:install_rename_triggers_for_postgresql) .with(trigger_name, 'users', 'old', 'new') @@ -506,6 +510,8 @@ describe Gitlab::Database::MigrationHelpers do it 'cleans up the renaming procedure for PostgreSQL' do allow(Gitlab::Database).to receive(:postgresql?).and_return(true) + expect(model).to receive(:check_trigger_permissions!).with(:users) + expect(model).to receive(:remove_rename_triggers_for_postgresql) .with(:users, /trigger_.{12}/) @@ -517,6 +523,8 @@ describe Gitlab::Database::MigrationHelpers do it 'cleans up the renaming procedure for MySQL' do allow(Gitlab::Database).to receive(:postgresql?).and_return(false) + expect(model).to receive(:check_trigger_permissions!).with(:users) + expect(model).to receive(:remove_rename_triggers_for_mysql) .with(/trigger_.{12}/) @@ -573,8 +581,8 @@ describe Gitlab::Database::MigrationHelpers do describe '#remove_rename_triggers_for_postgresql' do it 'removes the function and trigger' do - expect(model).to receive(:execute).with('DROP TRIGGER foo ON bar') - expect(model).to receive(:execute).with('DROP FUNCTION foo()') + expect(model).to receive(:execute).with('DROP TRIGGER IF EXISTS foo ON bar') + expect(model).to receive(:execute).with('DROP FUNCTION IF EXISTS foo()') model.remove_rename_triggers_for_postgresql('bar', 'foo') end @@ -582,8 +590,8 @@ describe Gitlab::Database::MigrationHelpers do describe '#remove_rename_triggers_for_mysql' do it 'removes the triggers' do - expect(model).to receive(:execute).with('DROP TRIGGER foo_insert') - expect(model).to receive(:execute).with('DROP TRIGGER foo_update') + expect(model).to receive(:execute).with('DROP TRIGGER IF EXISTS foo_insert') + expect(model).to receive(:execute).with('DROP TRIGGER IF EXISTS foo_update') model.remove_rename_triggers_for_mysql('foo') end @@ -890,4 +898,20 @@ describe Gitlab::Database::MigrationHelpers do end end end + + describe '#check_trigger_permissions!' do + it 'does nothing when the user has the correct permissions' do + expect { model.check_trigger_permissions!('users') } + .not_to raise_error(RuntimeError) + end + + it 'raises RuntimeError when the user does not have the correct permissions' do + allow(Gitlab::Database::Grant).to receive(:create_and_execute_trigger?) + .with('kittens') + .and_return(false) + + expect { model.check_trigger_permissions!('kittens') } + .to raise_error(RuntimeError, /Your database user is not allowed/) + end + end end From 11da029edb0bf09eb906301bd0692ed1d74d25eb Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Mon, 28 Aug 2017 13:50:21 +0200 Subject: [PATCH 28/57] Move GPG signed commits docs to new location Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/36804 --- app/views/profiles/gpg_keys/index.html.haml | 2 +- .../commit/_signature_badge.html.haml | 2 +- doc/README.md | 3 +- doc/user/project/gpg_signed_commits/index.md | 246 +----------------- .../profile_settings_gpg_keys_paste_pub.png | Bin .../profile_settings_gpg_keys_single_key.png | Bin .../project_signed_and_unsigned_commits.png | Bin ...ect_signed_commit_unverified_signature.png | Bin ...oject_signed_commit_verified_signature.png | Bin .../repository/gpg_signed_commits/index.md | 245 +++++++++++++++++ doc/user/project/repository/index.md | 4 +- 11 files changed, 253 insertions(+), 249 deletions(-) rename doc/user/project/{ => repository}/gpg_signed_commits/img/profile_settings_gpg_keys_paste_pub.png (100%) rename doc/user/project/{ => repository}/gpg_signed_commits/img/profile_settings_gpg_keys_single_key.png (100%) rename doc/user/project/{ => repository}/gpg_signed_commits/img/project_signed_and_unsigned_commits.png (100%) rename doc/user/project/{ => repository}/gpg_signed_commits/img/project_signed_commit_unverified_signature.png (100%) rename doc/user/project/{ => repository}/gpg_signed_commits/img/project_signed_commit_verified_signature.png (100%) create mode 100644 doc/user/project/repository/gpg_signed_commits/index.md diff --git a/app/views/profiles/gpg_keys/index.html.haml b/app/views/profiles/gpg_keys/index.html.haml index 720a97cddb7..8dbb8aef31b 100644 --- a/app/views/profiles/gpg_keys/index.html.haml +++ b/app/views/profiles/gpg_keys/index.html.haml @@ -12,7 +12,7 @@ Add a GPG key %p.profile-settings-content Before you can add a GPG key you need to - = link_to 'generate it.', help_page_path('user/project/gpg_signed_commits/index.md') + = link_to 'generate it.', help_page_path('user/project/repository/gpg_signed_commits/index.md') = render 'form' %hr %h5 diff --git a/app/views/projects/commit/_signature_badge.html.haml b/app/views/projects/commit/_signature_badge.html.haml index a3783b31b86..d06b29db838 100644 --- a/app/views/projects/commit/_signature_badge.html.haml +++ b/app/views/projects/commit/_signature_badge.html.haml @@ -12,7 +12,7 @@ %span.monospace= signature.gpg_key_primary_keyid - = link_to('Learn more about signing commits', help_page_path('user/project/gpg_signed_commits/index.md'), class: 'gpg-popover-help-link') + = link_to('Learn more about signing commits', help_page_path('user/project/repository/gpg_signed_commits/index.md'), class: 'gpg-popover-help-link') %button{ class: css_classes, data: { toggle: 'popover', html: 'true', placement: 'auto top', title: title, content: content } } = label diff --git a/doc/README.md b/doc/README.md index 76d4c3e51fe..63ba8ff03e9 100644 --- a/doc/README.md +++ b/doc/README.md @@ -77,6 +77,8 @@ Manage your [repositories](user/project/repository/index.md) from the UI (user i - [Create a branch](user/project/repository/web_editor.md#create-a-new-branch) - [Protected branches](user/project/protected_branches.md#protected-branches) - [Delete merged branches](user/project/repository/branches/index.md#delete-merged-branches) +- Commits + - [Signing commits](user/project/repository/gpg_signed_commits/index.md): use GPG to sign your commits. ### Issues and Merge Requests (MRs) @@ -98,7 +100,6 @@ Manage your [repositories](user/project/repository/index.md) from the UI (user i - [Git](topics/git/index.md): Getting started with Git, branching strategies, Git LFS, advanced use. - [Git cheatsheet](https://gitlab.com/gitlab-com/marketing/raw/master/design/print/git-cheatsheet/print-pdf/git-cheatsheet.pdf): Download a PDF describing the most used Git operations. - [GitLab Flow](workflow/gitlab_flow.md): explore the best of Git with the GitLab Flow strategy. -- [Signing commits](user/project/gpg_signed_commits/index.md): use GPG to sign your commits. ### Migrate and import your projects from other platforms diff --git a/doc/user/project/gpg_signed_commits/index.md b/doc/user/project/gpg_signed_commits/index.md index 3ea2203c895..261eedeb412 100644 --- a/doc/user/project/gpg_signed_commits/index.md +++ b/doc/user/project/gpg_signed_commits/index.md @@ -1,245 +1 @@ -# Signing commits with GPG - -> [Introduced][ce-9546] in GitLab 9.5. - -GitLab can show whether a commit is verified or not when signed with a GPG key. -All you need to do is upload the public GPG key in your profile settings. - -GPG verified tags are not supported yet. - -## Getting started with GPG - -Here are a few guides to get you started with GPG: - -- [Git Tools - Signing Your Work](https://git-scm.com/book/en/v2/Git-Tools-Signing-Your-Work) -- [Managing OpenPGP Keys](https://riseup.net/en/security/message-security/openpgp/gpg-keys) -- [OpenPGP Best Practices](https://riseup.net/en/security/message-security/openpgp/best-practices) -- [Creating a new GPG key with subkeys](https://www.void.gr/kargig/blog/2013/12/02/creating-a-new-gpg-key-with-subkeys/) (advanced) - -## How GitLab handles GPG - -GitLab uses its own keyring to verify the GPG signature. It does not access any -public key server. - -In order to have a commit verified on GitLab the corresponding public key needs -to be uploaded to GitLab. For a signature to be verified two prerequisites need -to be met: - -1. The public key needs to be added your GitLab account -1. One of the emails in the GPG key matches your **primary** email - -## Generating a GPG key - -If you don't already have a GPG key, the following steps will help you get -started: - -1. [Install GPG](https://www.gnupg.org/download/index.html) for your operating system -1. Generate the private/public key pair with the following command: - - ```sh - gpg --full-gen-key - ``` - - This will spawn a series of questions. - -1. The first question is which algorithm can be used. Select the kind you want - or press Enter to choose the default (RSA and RSA): - - ``` - Please select what kind of key you want: - (1) RSA and RSA (default) - (2) DSA and Elgamal - (3) DSA (sign only) - (4) RSA (sign only) - Your selection? 1 - ``` - -1. The next question is key length. We recommend to choose the highest value - which is `4096`: - - ``` - RSA keys may be between 1024 and 4096 bits long. - What keysize do you want? (2048) 4096 - Requested keysize is 4096 bits - ``` -1. Next, you need to specify the validity period of your key. This is something - subjective, and you can use the default value which is to never expire: - - ``` - Please specify how long the key should be valid. - 0 = key does not expire - = key expires in n days - w = key expires in n weeks - m = key expires in n months - y = key expires in n years - Key is valid for? (0) 0 - Key does not expire at all - ``` - -1. Confirm that the answers you gave were correct by typing `y`: - - ``` - Is this correct? (y/N) y - ``` - -1. Enter you real name, the email address to be associated with this key (should - match the primary email address you use in GitLab) and an optional comment - (press Enter to skip): - - ``` - GnuPG needs to construct a user ID to identify your key. - - Real name: Mr. Robot - Email address: mr@robot.sh - Comment: - You selected this USER-ID: - "Mr. Robot " - - Change (N)ame, (C)omment, (E)mail or (O)kay/(Q)uit? O - ``` - -1. Pick a strong password when asked and type it twice to confirm. -1. Use the following command to list the private GPG key you just created: - - ``` - gpg --list-secret-keys mr@robot.sh - ``` - - Replace `mr@robot.sh` with the email address you entered above. - -1. Copy the GPG key ID that starts with `sec`. In the following example, that's - `0x30F2B65B9246B6CA`: - - ``` - sec rsa4096/0x30F2B65B9246B6CA 2017-08-18 [SC] - D5E4F29F3275DC0CDA8FFC8730F2B65B9246B6CA - uid [ultimate] Mr. Robot - ssb rsa4096/0xB7ABC0813E4028C0 2017-08-18 [E] - ``` - -1. Export the public key of that ID (replace your key ID from the previous step): - - ``` - gpg --armor --export 0x30F2B65B9246B6CA - ``` - -1. Finally, copy the public key and [add it in your profile settings](#adding-a-gpg-key-to-your-account) - -## Adding a GPG key to your account - ->**Note:** -Once you add a key, you cannot edit it, only remove it. In case the paste -didn't work, you'll have to remove the offending key and re-add it. - -You can add a GPG key in your profile's settings: - -1. On the upper right corner, click on your avatar and go to your **Settings**. - - ![Settings dropdown](../../profile/img/profile_settings_dropdown.png) - -1. Navigate to the **GPG keys** tab and paste your _public_ key in the 'Key' - box. - - ![Paste GPG public key](img/profile_settings_gpg_keys_paste_pub.png) - -1. Finally, click on **Add key** to add it to GitLab. You will be able to see - its fingerprint, the corresponding email address and creation date. - - ![GPG key single page](img/profile_settings_gpg_keys_single_key.png) - -## Associating your GPG key with Git - -After you have [created your GPG key](#generating-a-gpg-key) and [added it to -your account](#adding-a-gpg-key-to-your-account), it's time to tell Git which -key to use. - -1. Use the following command to list the private GPG key you just created: - - ``` - gpg --list-secret-keys mr@robot.sh - ``` - - Replace `mr@robot.sh` with the email address you entered above. - -1. Copy the GPG key ID that starts with `sec`. In the following example, that's - `0x30F2B65B9246B6CA`: - - ``` - sec rsa4096/0x30F2B65B9246B6CA 2017-08-18 [SC] - D5E4F29F3275DC0CDA8FFC8730F2B65B9246B6CA - uid [ultimate] Mr. Robot - ssb rsa4096/0xB7ABC0813E4028C0 2017-08-18 [E] - ``` - -1. Tell Git to use that key to sign the commits: - - ``` - git config --global user.signingkey 0x30F2B65B9246B6CA - ``` - - Replace `0x30F2B65B9246B6CA` with your GPG key ID. - -## Signing commits - -After you have [created your GPG key](#generating-a-gpg-key) and [added it to -your account](#adding-a-gpg-key-to-your-account), you can start signing your -commits: - -1. Commit like you used to, the only difference is the addition of the `-S` flag: - - ``` - git commit -S -m "My commit msg" - ``` - -1. Enter the passphrase of your GPG key when asked. -1. Push to GitLab and check that your commits [are verified](#verifying-commits). - -If you don't want to type the `-S` flag every time you commit, you can tell Git -to sign your commits automatically: - -``` -git config --global commit.gpgsign true -``` - -## Verifying commits - -1. Within a project or [merge request](../merge_requests/index.md), navigate to - the **Commits** tab. Signed commits will show a badge containing either - "Verified" or "Unverified", depending on the verification status of the GPG - signature. - - ![Signed and unsigned commits](img/project_signed_and_unsigned_commits.png) - -1. By clicking on the GPG badge, details of the signature are displayed. - - ![Signed commit with verified signature](img/project_signed_commit_verified_signature.png) - - ![Signed commit with verified signature](img/project_signed_commit_unverified_signature.png) - -## Revoking a GPG key - -Revoking a key **unverifies** already signed commits. Commits that were -verified by using this key will change to an unverified state. Future commits -will also stay unverified once you revoke this key. This action should be used -in case your key has been compromised. - -To revoke a GPG key: - -1. On the upper right corner, click on your avatar and go to your **Settings**. -1. Navigate to the **GPG keys** tab. -1. Click on **Revoke** besides the GPG key you want to delete. - -## Removing a GPG key - -Removing a key **does not unverify** already signed commits. Commits that were -verified by using this key will stay verified. Only unpushed commits will stay -unverified once you remove this key. To unverify already signed commits, you need -to [revoke the associated GPG key](#revoking-a-gpg-key) from your account. - -To remove a GPG key from your account: - -1. On the upper right corner, click on your avatar and go to your **Settings**. -1. Navigate to the **GPG keys** tab. -1. Click on the trash icon besides the GPG key you want to delete. - -[ce-9546]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546 +This document was moved to [another location](../repository/gpg_signed_commits/index.md). diff --git a/doc/user/project/gpg_signed_commits/img/profile_settings_gpg_keys_paste_pub.png b/doc/user/project/repository/gpg_signed_commits/img/profile_settings_gpg_keys_paste_pub.png similarity index 100% rename from doc/user/project/gpg_signed_commits/img/profile_settings_gpg_keys_paste_pub.png rename to doc/user/project/repository/gpg_signed_commits/img/profile_settings_gpg_keys_paste_pub.png diff --git a/doc/user/project/gpg_signed_commits/img/profile_settings_gpg_keys_single_key.png b/doc/user/project/repository/gpg_signed_commits/img/profile_settings_gpg_keys_single_key.png similarity index 100% rename from doc/user/project/gpg_signed_commits/img/profile_settings_gpg_keys_single_key.png rename to doc/user/project/repository/gpg_signed_commits/img/profile_settings_gpg_keys_single_key.png diff --git a/doc/user/project/gpg_signed_commits/img/project_signed_and_unsigned_commits.png b/doc/user/project/repository/gpg_signed_commits/img/project_signed_and_unsigned_commits.png similarity index 100% rename from doc/user/project/gpg_signed_commits/img/project_signed_and_unsigned_commits.png rename to doc/user/project/repository/gpg_signed_commits/img/project_signed_and_unsigned_commits.png diff --git a/doc/user/project/gpg_signed_commits/img/project_signed_commit_unverified_signature.png b/doc/user/project/repository/gpg_signed_commits/img/project_signed_commit_unverified_signature.png similarity index 100% rename from doc/user/project/gpg_signed_commits/img/project_signed_commit_unverified_signature.png rename to doc/user/project/repository/gpg_signed_commits/img/project_signed_commit_unverified_signature.png diff --git a/doc/user/project/gpg_signed_commits/img/project_signed_commit_verified_signature.png b/doc/user/project/repository/gpg_signed_commits/img/project_signed_commit_verified_signature.png similarity index 100% rename from doc/user/project/gpg_signed_commits/img/project_signed_commit_verified_signature.png rename to doc/user/project/repository/gpg_signed_commits/img/project_signed_commit_verified_signature.png diff --git a/doc/user/project/repository/gpg_signed_commits/index.md b/doc/user/project/repository/gpg_signed_commits/index.md new file mode 100644 index 00000000000..ff419d714f9 --- /dev/null +++ b/doc/user/project/repository/gpg_signed_commits/index.md @@ -0,0 +1,245 @@ +# Signing commits with GPG + +> [Introduced][ce-9546] in GitLab 9.5. + +GitLab can show whether a commit is verified or not when signed with a GPG key. +All you need to do is upload the public GPG key in your profile settings. + +GPG verified tags are not supported yet. + +## Getting started with GPG + +Here are a few guides to get you started with GPG: + +- [Git Tools - Signing Your Work](https://git-scm.com/book/en/v2/Git-Tools-Signing-Your-Work) +- [Managing OpenPGP Keys](https://riseup.net/en/security/message-security/openpgp/gpg-keys) +- [OpenPGP Best Practices](https://riseup.net/en/security/message-security/openpgp/best-practices) +- [Creating a new GPG key with subkeys](https://www.void.gr/kargig/blog/2013/12/02/creating-a-new-gpg-key-with-subkeys/) (advanced) + +## How GitLab handles GPG + +GitLab uses its own keyring to verify the GPG signature. It does not access any +public key server. + +In order to have a commit verified on GitLab the corresponding public key needs +to be uploaded to GitLab. For a signature to be verified two prerequisites need +to be met: + +1. The public key needs to be added your GitLab account +1. One of the emails in the GPG key matches your **primary** email + +## Generating a GPG key + +If you don't already have a GPG key, the following steps will help you get +started: + +1. [Install GPG](https://www.gnupg.org/download/index.html) for your operating system +1. Generate the private/public key pair with the following command: + + ```sh + gpg --full-gen-key + ``` + + This will spawn a series of questions. + +1. The first question is which algorithm can be used. Select the kind you want + or press Enter to choose the default (RSA and RSA): + + ``` + Please select what kind of key you want: + (1) RSA and RSA (default) + (2) DSA and Elgamal + (3) DSA (sign only) + (4) RSA (sign only) + Your selection? 1 + ``` + +1. The next question is key length. We recommend to choose the highest value + which is `4096`: + + ``` + RSA keys may be between 1024 and 4096 bits long. + What keysize do you want? (2048) 4096 + Requested keysize is 4096 bits + ``` +1. Next, you need to specify the validity period of your key. This is something + subjective, and you can use the default value which is to never expire: + + ``` + Please specify how long the key should be valid. + 0 = key does not expire + = key expires in n days + w = key expires in n weeks + m = key expires in n months + y = key expires in n years + Key is valid for? (0) 0 + Key does not expire at all + ``` + +1. Confirm that the answers you gave were correct by typing `y`: + + ``` + Is this correct? (y/N) y + ``` + +1. Enter you real name, the email address to be associated with this key (should + match the primary email address you use in GitLab) and an optional comment + (press Enter to skip): + + ``` + GnuPG needs to construct a user ID to identify your key. + + Real name: Mr. Robot + Email address: mr@robot.sh + Comment: + You selected this USER-ID: + "Mr. Robot " + + Change (N)ame, (C)omment, (E)mail or (O)kay/(Q)uit? O + ``` + +1. Pick a strong password when asked and type it twice to confirm. +1. Use the following command to list the private GPG key you just created: + + ``` + gpg --list-secret-keys mr@robot.sh + ``` + + Replace `mr@robot.sh` with the email address you entered above. + +1. Copy the GPG key ID that starts with `sec`. In the following example, that's + `0x30F2B65B9246B6CA`: + + ``` + sec rsa4096/0x30F2B65B9246B6CA 2017-08-18 [SC] + D5E4F29F3275DC0CDA8FFC8730F2B65B9246B6CA + uid [ultimate] Mr. Robot + ssb rsa4096/0xB7ABC0813E4028C0 2017-08-18 [E] + ``` + +1. Export the public key of that ID (replace your key ID from the previous step): + + ``` + gpg --armor --export 0x30F2B65B9246B6CA + ``` + +1. Finally, copy the public key and [add it in your profile settings](#adding-a-gpg-key-to-your-account) + +## Adding a GPG key to your account + +>**Note:** +Once you add a key, you cannot edit it, only remove it. In case the paste +didn't work, you'll have to remove the offending key and re-add it. + +You can add a GPG key in your profile's settings: + +1. On the upper right corner, click on your avatar and go to your **Settings**. + + ![Settings dropdown](../../../profile/img/profile_settings_dropdown.png) + +1. Navigate to the **GPG keys** tab and paste your _public_ key in the 'Key' + box. + + ![Paste GPG public key](img/profile_settings_gpg_keys_paste_pub.png) + +1. Finally, click on **Add key** to add it to GitLab. You will be able to see + its fingerprint, the corresponding email address and creation date. + + ![GPG key single page](img/profile_settings_gpg_keys_single_key.png) + +## Associating your GPG key with Git + +After you have [created your GPG key](#generating-a-gpg-key) and [added it to +your account](#adding-a-gpg-key-to-your-account), it's time to tell Git which +key to use. + +1. Use the following command to list the private GPG key you just created: + + ``` + gpg --list-secret-keys mr@robot.sh + ``` + + Replace `mr@robot.sh` with the email address you entered above. + +1. Copy the GPG key ID that starts with `sec`. In the following example, that's + `0x30F2B65B9246B6CA`: + + ``` + sec rsa4096/0x30F2B65B9246B6CA 2017-08-18 [SC] + D5E4F29F3275DC0CDA8FFC8730F2B65B9246B6CA + uid [ultimate] Mr. Robot + ssb rsa4096/0xB7ABC0813E4028C0 2017-08-18 [E] + ``` + +1. Tell Git to use that key to sign the commits: + + ``` + git config --global user.signingkey 0x30F2B65B9246B6CA + ``` + + Replace `0x30F2B65B9246B6CA` with your GPG key ID. + +## Signing commits + +After you have [created your GPG key](#generating-a-gpg-key) and [added it to +your account](#adding-a-gpg-key-to-your-account), you can start signing your +commits: + +1. Commit like you used to, the only difference is the addition of the `-S` flag: + + ``` + git commit -S -m "My commit msg" + ``` + +1. Enter the passphrase of your GPG key when asked. +1. Push to GitLab and check that your commits [are verified](#verifying-commits). + +If you don't want to type the `-S` flag every time you commit, you can tell Git +to sign your commits automatically: + +``` +git config --global commit.gpgsign true +``` + +## Verifying commits + +1. Within a project or [merge request](../../merge_requests/index.md), navigate to + the **Commits** tab. Signed commits will show a badge containing either + "Verified" or "Unverified", depending on the verification status of the GPG + signature. + + ![Signed and unsigned commits](img/project_signed_and_unsigned_commits.png) + +1. By clicking on the GPG badge, details of the signature are displayed. + + ![Signed commit with verified signature](img/project_signed_commit_verified_signature.png) + + ![Signed commit with verified signature](img/project_signed_commit_unverified_signature.png) + +## Revoking a GPG key + +Revoking a key **unverifies** already signed commits. Commits that were +verified by using this key will change to an unverified state. Future commits +will also stay unverified once you revoke this key. This action should be used +in case your key has been compromised. + +To revoke a GPG key: + +1. On the upper right corner, click on your avatar and go to your **Settings**. +1. Navigate to the **GPG keys** tab. +1. Click on **Revoke** besides the GPG key you want to delete. + +## Removing a GPG key + +Removing a key **does not unverify** already signed commits. Commits that were +verified by using this key will stay verified. Only unpushed commits will stay +unverified once you remove this key. To unverify already signed commits, you need +to [revoke the associated GPG key](#revoking-a-gpg-key) from your account. + +To remove a GPG key from your account: + +1. On the upper right corner, click on your avatar and go to your **Settings**. +1. Navigate to the **GPG keys** tab. +1. Click on the trash icon besides the GPG key you want to delete. + +[ce-9546]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9546 diff --git a/doc/user/project/repository/index.md b/doc/user/project/repository/index.md index 5e5ae880518..235af83353d 100644 --- a/doc/user/project/repository/index.md +++ b/doc/user/project/repository/index.md @@ -22,7 +22,7 @@ you to [connect with GitLab via SSH](../../../ssh/README.md). ## Files -## Create and edit files +### Create and edit files Host your codebase in GitLab repositories by pushing your files to GitLab. You can either use the user interface (UI), or connect your local computer @@ -111,6 +111,8 @@ right from the UI. - **Revert a commit:** Easily [revert a commit](../merge_requests/revert_changes.md#reverting-a-commit) from the UI to a selected branch. +- **Sign a commit:** +Use GPG to [sign your commits](gpg_signed_commits/index.md). ## Repository size From 573f73f22a53ac6d72d4670f925eceb28f3cc8fc Mon Sep 17 00:00:00 2001 From: winh Date: Wed, 16 Aug 2017 13:15:40 +0200 Subject: [PATCH 29/57] Make issuable dashboard dropdowns consistent --- app/assets/stylesheets/framework/dropdowns.scss | 5 +++++ app/assets/stylesheets/framework/filters.scss | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index a45d5a6dca0..4cf2f46c6d3 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -771,6 +771,11 @@ &::before { top: 16px; } + + &.dropdown-menu-user-link::before { + top: 50%; + transform: translateY(-50%); + } } } } diff --git a/app/assets/stylesheets/framework/filters.scss b/app/assets/stylesheets/framework/filters.scss index a5d33d410fb..e4e095eaabf 100644 --- a/app/assets/stylesheets/framework/filters.scss +++ b/app/assets/stylesheets/framework/filters.scss @@ -478,3 +478,7 @@ padding: 8px 16px; text-align: center; } + +.issues-details-filters { + @include new-style-dropdown; +} From 6d9fb6b0b779d7065f0c36ec15a42d0d459abbeb Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 29 Aug 2017 21:11:38 +0800 Subject: [PATCH 30/57] It doesn't seem that rubocop is complaining for me --- app/models/repository.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 0f67395b88b..9fe39172b19 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -60,12 +60,10 @@ class Repository @project = project end - def equals(other) + def ==(other) @disk_path == other.disk_path end - alias_method :==, :equals - def raw_repository return nil unless full_path From 8cbc9c4ed032e7a4f73aeb86a0bec152a1c7b696 Mon Sep 17 00:00:00 2001 From: Travis Miller Date: Sat, 19 Aug 2017 12:49:39 -0500 Subject: [PATCH 31/57] Add time stats documentation to issue and merge request end points --- doc/api/issues.md | 56 ++++++++++++++++++++++++++++++++++- doc/api/merge_requests.md | 62 ++++++++++++++++++++++++++++++++++----- 2 files changed, 110 insertions(+), 8 deletions(-) diff --git a/doc/api/issues.md b/doc/api/issues.md index f30ed08d0fa..14635114a31 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -101,6 +101,12 @@ Example response: "user_notes_count": 1, "due_date": "2016-07-22", "web_url": "http://example.com/example/example/issues/6", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + }, "confidential": false } ] @@ -198,6 +204,12 @@ Example response: "user_notes_count": 1, "due_date": null, "web_url": "http://example.com/example/example/issues/1", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + }, "confidential": false } ] @@ -296,6 +308,12 @@ Example response: "user_notes_count": 1, "due_date": "2016-07-22", "web_url": "http://example.com/example/example/issues/1", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + }, "confidential": false } ] @@ -372,6 +390,12 @@ Example response: "user_notes_count": 1, "due_date": null, "web_url": "http://example.com/example/example/issues/1", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + }, "confidential": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", @@ -440,6 +464,12 @@ Example response: "user_notes_count": 0, "due_date": null, "web_url": "http://example.com/example/example/issues/14", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + }, "confidential": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", @@ -509,6 +539,12 @@ Example response: "user_notes_count": 0, "due_date": "2016-07-22", "web_url": "http://example.com/example/example/issues/15", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + }, "confidential": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", @@ -601,6 +637,12 @@ Example response: }, "due_date": null, "web_url": "http://example.com/example/example/issues/11", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + }, "confidential": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", @@ -672,6 +714,12 @@ Example response: }, "due_date": null, "web_url": "http://example.com/example/example/issues/11", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + }, "confidential": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", @@ -1001,7 +1049,13 @@ Example response: "user_notes_count": 1, "should_remove_source_branch": null, "force_remove_source_branch": false, - "web_url": "https://gitlab.example.com/gitlab-org/gitlab-test/merge_requests/6432" + "web_url": "https://gitlab.example.com/gitlab-org/gitlab-test/merge_requests/6432", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + } } ] ``` diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 802e5362d70..4f67aa4b9d4 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -92,7 +92,13 @@ Parameters: "user_notes_count": 1, "should_remove_source_branch": true, "force_remove_source_branch": false, - "web_url": "http://example.com/example/example/merge_requests/1" + "web_url": "http://example.com/example/example/merge_requests/1", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + } } ] ``` @@ -181,7 +187,13 @@ Parameters: "user_notes_count": 1, "should_remove_source_branch": true, "force_remove_source_branch": false, - "web_url": "http://example.com/example/example/merge_requests/1" + "web_url": "http://example.com/example/example/merge_requests/1", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + } } ] ``` @@ -250,7 +262,13 @@ Parameters: "user_notes_count": 1, "should_remove_source_branch": true, "force_remove_source_branch": false, - "web_url": "http://example.com/example/example/merge_requests/1" + "web_url": "http://example.com/example/example/merge_requests/1", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + } } ``` @@ -356,6 +374,12 @@ Parameters: "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + } "changes": [ { "old_path": "VERSION", @@ -442,7 +466,13 @@ POST /projects/:id/merge_requests "user_notes_count": 0, "should_remove_source_branch": true, "force_remove_source_branch": false, - "web_url": "http://example.com/example/example/merge_requests/1" + "web_url": "http://example.com/example/example/merge_requests/1", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + } } ``` @@ -519,7 +549,13 @@ Must include at least one non-required attribute from above. "user_notes_count": 1, "should_remove_source_branch": true, "force_remove_source_branch": false, - "web_url": "http://example.com/example/example/merge_requests/1" + "web_url": "http://example.com/example/example/merge_requests/1", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + } } ``` @@ -617,7 +653,13 @@ Parameters: "user_notes_count": 1, "should_remove_source_branch": true, "force_remove_source_branch": false, - "web_url": "http://example.com/example/example/merge_requests/1" + "web_url": "http://example.com/example/example/merge_requests/1", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + } } ``` @@ -687,7 +729,13 @@ Parameters: "user_notes_count": 1, "should_remove_source_branch": true, "force_remove_source_branch": false, - "web_url": "http://example.com/example/example/merge_requests/1" + "web_url": "http://example.com/example/example/merge_requests/1", + "time_stats": { + "time_estimate": 0, + "total_time_spent": 0, + "human_time_estimate": null, + "human_total_time_spent": null + } } ``` From 6782b406f77d237e58c23a3bce444e3cd4dde849 Mon Sep 17 00:00:00 2001 From: Travis Miller Date: Sat, 19 Aug 2017 12:56:17 -0500 Subject: [PATCH 32/57] Add time stats to API schema for issue and merge request end points --- spec/fixtures/api/schemas/public_api/v4/issues.json | 8 +++++++- .../api/schemas/public_api/v4/merge_requests.json | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/spec/fixtures/api/schemas/public_api/v4/issues.json b/spec/fixtures/api/schemas/public_api/v4/issues.json index bd6bfc03199..8acd9488215 100644 --- a/spec/fixtures/api/schemas/public_api/v4/issues.json +++ b/spec/fixtures/api/schemas/public_api/v4/issues.json @@ -78,7 +78,13 @@ "downvotes": { "type": "integer" }, "due_date": { "type": ["date", "null"] }, "confidential": { "type": "boolean" }, - "web_url": { "type": "uri" } + "web_url": { "type": "uri" }, + "time_stats": { + "time_estimate": { "type": "integer" }, + "total_time_spent": { "type": "integer" }, + "human_time_estimate": { "type": ["string", "null"] }, + "human_total_time_spent": { "type": ["string", "null"] } + } }, "required": [ "id", "iid", "project_id", "title", "description", diff --git a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json index 60aa47c1259..31b3f4ba946 100644 --- a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json +++ b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json @@ -72,7 +72,13 @@ "user_notes_count": { "type": "integer" }, "should_remove_source_branch": { "type": ["boolean", "null"] }, "force_remove_source_branch": { "type": ["boolean", "null"] }, - "web_url": { "type": "uri" } + "web_url": { "type": "uri" }, + "time_stats": { + "time_estimate": { "type": "integer" }, + "total_time_spent": { "type": "integer" }, + "human_time_estimate": { "type": ["string", "null"] }, + "human_total_time_spent": { "type": ["string", "null"] } + } }, "required": [ "id", "iid", "project_id", "title", "description", From 2531fb3be9542c08231a6540a14b12d907b151e5 Mon Sep 17 00:00:00 2001 From: Travis Miller Date: Mon, 28 Aug 2017 19:02:52 -0500 Subject: [PATCH 33/57] Add missing N+1 test to issues spec --- spec/requests/api/issues_spec.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 7d120e4a234..03b1d14828d 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -509,6 +509,18 @@ describe API::Issues, :mailer do describe "GET /projects/:id/issues" do let(:base_url) { "/projects/#{project.id}" } + it 'avoids N+1 queries' do + control_count = ActiveRecord::QueryRecorder.new do + get api("/projects/#{project.id}/issues", user) + end.count + + create(:issue, author: user, project: project) + + expect do + get api("/projects/#{project.id}/issues", user) + end.not_to exceed_query_limit(control_count) + end + it 'returns 404 when project does not exist' do get api('/projects/1000/issues', non_member) From ce1ce82045f168143ccc143f5200ea9da820d990 Mon Sep 17 00:00:00 2001 From: Travis Miller Date: Mon, 21 Aug 2017 17:42:03 -0500 Subject: [PATCH 34/57] Resolve new N+1 by adding preloads and metadata to issues end points --- lib/api/entities.rb | 22 ++++++++++++++++++++-- lib/api/issues.rb | 22 +++++++++++++++++++--- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index e8dd61e493f..e18c69b7a3d 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -320,7 +320,10 @@ module API end class IssueBasic < ProjectEntity - expose :label_names, as: :labels + expose :labels do |issue, options| + # Avoids an N+1 query since labels are preloaded + issue.labels.map(&:title).sort + end expose :milestone, using: Entities::Milestone expose :assignees, :author, using: Entities::UserBasic @@ -329,7 +332,22 @@ module API end expose :user_notes_count - expose :upvotes, :downvotes + expose :upvotes do |issue, options| + if options[:issuable_metadata] + # Avoids an N+1 query when metadata is included + options[:issuable_metadata][issue.id].upvotes + else + issue.upvotes + end + end + expose :downvotes do |issue, options| + if options[:issuable_metadata] + # Avoids an N+1 query when metadata is included + options[:issuable_metadata][issue.id].downvotes + else + issue.downvotes + end + end expose :due_date expose :confidential diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 4cec1145f3a..64a425eb96e 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -4,6 +4,8 @@ module API before { authenticate! } + helpers ::Gitlab::IssuableMetadata + helpers do def find_issues(args = {}) args = params.merge(args) @@ -13,6 +15,7 @@ module API args[:label_name] = args.delete(:labels) issues = IssuesFinder.new(current_user, args).execute + .preload(:assignees, :labels, :notes) issues.reorder(args[:order_by] => args[:sort]) end @@ -65,7 +68,11 @@ module API get do issues = find_issues - present paginate(issues), with: Entities::IssueBasic, current_user: current_user + options = { with: Entities::IssueBasic, + current_user: current_user } + options[:issuable_metadata] = issuable_meta_data(issues, 'Issue') + + present paginate(issues), options end end @@ -86,7 +93,11 @@ module API issues = find_issues(group_id: group.id) - present paginate(issues), with: Entities::IssueBasic, current_user: current_user + options = { with: Entities::IssueBasic, + current_user: current_user } + options[:issuable_metadata] = issuable_meta_data(issues, 'Issue') + + present paginate(issues), options end end @@ -109,7 +120,12 @@ module API issues = find_issues(project_id: project.id) - present paginate(issues), with: Entities::IssueBasic, current_user: current_user, project: user_project + options = { with: Entities::IssueBasic, + current_user: current_user, + project: user_project } + options[:issuable_metadata] = issuable_meta_data(issues, 'Issue') + + present paginate(issues), options end desc 'Get a single project issue' do From 749c389345cf382b740277db62f7d4b849902d60 Mon Sep 17 00:00:00 2001 From: Travis Miller Date: Mon, 28 Aug 2017 19:02:26 -0500 Subject: [PATCH 35/57] Add time stats to issue and merge request API end points --- lib/api/entities.rb | 22 +++++++++++++++++++++- lib/api/issues.rb | 28 +++++++++++++++++----------- lib/api/merge_requests.rb | 2 +- 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index e18c69b7a3d..803b48dd88a 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -354,6 +354,10 @@ module API expose :web_url do |issue, options| Gitlab::UrlBuilder.build(issue) end + + expose :time_stats, using: 'API::Entities::IssuableTimeStats' do |issue| + issue + end end class Issue < IssueBasic @@ -383,10 +387,22 @@ module API end class IssuableTimeStats < Grape::Entity + format_with(:time_tracking_formatter) do |time_spent| + Gitlab::TimeTrackingFormatter.output(time_spent) + end + expose :time_estimate expose :total_time_spent expose :human_time_estimate - expose :human_total_time_spent + + with_options(format_with: :time_tracking_formatter) do + expose :total_time_spent, as: :human_total_time_spent + end + + def total_time_spent + # Avoids an N+1 query since timelogs are preloaded + object.timelogs.map(&:time_spent).sum + end end class ExternalIssue < Grape::Entity @@ -436,6 +452,10 @@ module API expose :web_url do |merge_request, options| Gitlab::UrlBuilder.build(merge_request) end + + expose :time_stats, using: 'API::Entities::IssuableTimeStats' do |merge_request| + merge_request + end end class MergeRequest < MergeRequestBasic diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 64a425eb96e..c30e430ae85 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -15,7 +15,7 @@ module API args[:label_name] = args.delete(:labels) issues = IssuesFinder.new(current_user, args).execute - .preload(:assignees, :labels, :notes) + .preload(:assignees, :labels, :notes, :timelogs) issues.reorder(args[:order_by] => args[:sort]) end @@ -68,9 +68,11 @@ module API get do issues = find_issues - options = { with: Entities::IssueBasic, - current_user: current_user } - options[:issuable_metadata] = issuable_meta_data(issues, 'Issue') + options = { + with: Entities::IssueBasic, + current_user: current_user, + issuable_metadata: issuable_meta_data(issues, 'Issue') + } present paginate(issues), options end @@ -93,9 +95,11 @@ module API issues = find_issues(group_id: group.id) - options = { with: Entities::IssueBasic, - current_user: current_user } - options[:issuable_metadata] = issuable_meta_data(issues, 'Issue') + options = { + with: Entities::IssueBasic, + current_user: current_user, + issuable_metadata: issuable_meta_data(issues, 'Issue') + } present paginate(issues), options end @@ -120,10 +124,12 @@ module API issues = find_issues(project_id: project.id) - options = { with: Entities::IssueBasic, - current_user: current_user, - project: user_project } - options[:issuable_metadata] = issuable_meta_data(issues, 'Issue') + options = { + with: Entities::IssueBasic, + current_user: current_user, + project: user_project, + issuable_metadata: issuable_meta_data(issues, 'Issue') + } present paginate(issues), options end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 8810d4e441d..a3284afb745 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -21,7 +21,7 @@ module API return merge_requests if args[:view] == 'simple' merge_requests - .preload(:notes, :author, :assignee, :milestone, :merge_request_diff, :labels) + .preload(:notes, :author, :assignee, :milestone, :merge_request_diff, :labels, :timelogs) end params :merge_requests_params do From 54b0f57b2e286680b7b2dc2757c31536858478ce Mon Sep 17 00:00:00 2001 From: Travis Miller Date: Sat, 19 Aug 2017 13:03:34 -0500 Subject: [PATCH 36/57] Add changelog --- ...28453-add-time-estimate-time-spent-to-api-issue-output.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/28453-add-time-estimate-time-spent-to-api-issue-output.yml diff --git a/changelogs/unreleased/28453-add-time-estimate-time-spent-to-api-issue-output.yml b/changelogs/unreleased/28453-add-time-estimate-time-spent-to-api-issue-output.yml new file mode 100644 index 00000000000..129cf505a3f --- /dev/null +++ b/changelogs/unreleased/28453-add-time-estimate-time-spent-to-api-issue-output.yml @@ -0,0 +1,4 @@ +--- +title: Add time stats to Issue and Merge Request API +merge_request: 13335 +author: @travismiller From e6630d7f7276dc5cec2a02e32dc74a0a060886ab Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 29 Aug 2017 22:42:02 +0800 Subject: [PATCH 37/57] Make sure inspect doesn't generate crazy string --- app/models/repository.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/models/repository.rb b/app/models/repository.rb index 9fe39172b19..04a76c365be 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -79,6 +79,10 @@ class Repository ) end + def inspect + "#<#{self.class.name}:#{@disk_path}>" + end + # # Git repository can contains some hidden refs like: # /refs/notes/* From 9d3ee1ff139650e064a41fd90d34cd3f558c1099 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 29 Aug 2017 22:42:34 +0800 Subject: [PATCH 38/57] Further break with_repo_branch_commit into parts So it's more clear what could happen. Also add more tests about the behaviour. --- app/models/repository.rb | 43 +++++++++++++++++++--------------- spec/models/repository_spec.rb | 36 +++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 20 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 04a76c365be..93017dfedf9 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1000,29 +1000,22 @@ class Repository end def with_repo_branch_commit(start_repository, start_branch_name) - tmp_ref = nil return yield nil if start_repository.empty_repo? - branch_commit = - if start_repository == self - commit(start_branch_name) + if start_repository == self + yield commit(start_branch_name) + else + sha = start_repository.commit(start_branch_name).sha + + if branch_commit = commit(sha) + yield branch_commit else - sha = start_repository.find_branch(start_branch_name).target - commit(sha) || - begin - tmp_ref = fetch_ref( - start_repository.path_to_repo, - "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}", - "refs/tmp/#{SecureRandom.hex}/head" - ) - - commit(start_repository.commit(start_branch_name).sha) - end + with_repo_tmp_commit( + start_repository, start_branch_name, sha) do |tmp_commit| + yield tmp_commit + end end - - yield branch_commit - ensure - rugged.references.delete(tmp_ref) if tmp_ref + end end def add_remote(name, url) @@ -1231,4 +1224,16 @@ class Repository .commits_by_message(query, revision: ref, path: path, limit: limit, offset: offset) .map { |c| commit(c) } end + + def with_repo_tmp_commit(start_repository, start_branch_name, sha) + tmp_ref = fetch_ref( + start_repository.path_to_repo, + "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}", + "refs/tmp/#{SecureRandom.hex}/head" + ) + + yield commit(sha) + ensure + rugged.references.delete(tmp_ref) if tmp_ref + end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 462e92b8b62..8d9a86d952b 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -923,13 +923,16 @@ describe Repository, models: true do describe '#update_branch_with_hooks' do let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature let(:new_rev) { 'a74ae73c1ccde9b974a70e82b901588071dc142a' } # commit whose parent is old_rev + let(:updating_ref) { 'refs/heads/feature' } + let(:target_project) { project } + let(:target_repository) { target_project.repository } context 'when pre hooks were successful' do before do service = Gitlab::Git::HooksService.new expect(Gitlab::Git::HooksService).to receive(:new).and_return(service) expect(service).to receive(:execute) - .with(committer, repository, old_rev, new_rev, 'refs/heads/feature') + .with(committer, target_repository, old_rev, new_rev, updating_ref) .and_yield(service).and_return(true) end @@ -960,6 +963,37 @@ describe Repository, models: true do expect(repository.find_branch('feature').dereferenced_target.id).to eq(new_rev) end end + + context 'when target project does not have the commit' do + let(:target_project) { create(:project, :empty_repo) } + let(:old_rev) { Gitlab::Git::BLANK_SHA } + let(:new_rev) { project.commit('feature').sha } + let(:updating_ref) { 'refs/heads/master' } + + it 'fetch_ref and create the branch' do + expect(target_project.repository).to receive(:fetch_ref) + .and_call_original + + GitOperationService.new(committer, target_repository) + .with_branch( + 'master', + start_project: project, + start_branch_name: 'feature') { new_rev } + + expect(target_repository.branch_names).to contain_exactly('master') + end + end + + context 'when target project already has the commit' do + let(:target_project) { create(:project, :repository) } + + it 'does not fetch_ref and just pass the commit' do + expect(target_repository).not_to receive(:fetch_ref) + + GitOperationService.new(committer, target_repository) + .with_branch('feature', start_project: project) { new_rev } + end + end end context 'when temporary ref failed to be created from other project' do From 67c042e4a5603a39494c3c7e407161348d7e85f3 Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Tue, 29 Aug 2017 16:49:43 +0200 Subject: [PATCH 39/57] Respect the default visibility level when creating a group --- lib/api/groups.rb | 4 +++- spec/requests/api/groups_spec.rb | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/api/groups.rb b/lib/api/groups.rb index e56427304a6..15266e9d4e5 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -7,7 +7,9 @@ module API helpers do params :optional_params_ce do optional :description, type: String, desc: 'The description of the group' - optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, desc: 'The visibility of the group' + optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, + default: Gitlab::VisibilityLevel.string_level(Gitlab::CurrentSettings.current_application_settings.default_group_visibility), + desc: 'The visibility of the group' optional :lfs_enabled, type: Boolean, desc: 'Enable/disable LFS for the projects in this group' optional :request_access_enabled, type: Boolean, desc: 'Allow users to request member access' optional :share_with_group_lock, type: Boolean, desc: 'Prevent sharing a project with another group within this group' diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index a7557c7fb22..c6bc0c25e99 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -444,6 +444,7 @@ describe API::Groups do expect(json_response["name"]).to eq(group[:name]) expect(json_response["path"]).to eq(group[:path]) expect(json_response["request_access_enabled"]).to eq(group[:request_access_enabled]) + expect(json_response["visibility"]).to eq(Gitlab::VisibilityLevel.string_level(Gitlab::CurrentSettings.current_application_settings.default_group_visibility)) end it "creates a nested group", :nested_groups do From 1488d1f77c83969f42269c9cb0b7c285b4232899 Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Tue, 29 Aug 2017 17:15:04 +0200 Subject: [PATCH 40/57] Add changelog --- .../37198-api-doesn-t-respect-default-group-visibility.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/37198-api-doesn-t-respect-default-group-visibility.yml diff --git a/changelogs/unreleased/37198-api-doesn-t-respect-default-group-visibility.yml b/changelogs/unreleased/37198-api-doesn-t-respect-default-group-visibility.yml new file mode 100644 index 00000000000..ef83dc1d10a --- /dev/null +++ b/changelogs/unreleased/37198-api-doesn-t-respect-default-group-visibility.yml @@ -0,0 +1,5 @@ +--- +title: 'API: Respect default group visibility when creating a group' +merge_request: 13903 +author: Robert Schilling +type: fixed From a0814a8dfb83f4f0acf74072623a2a4fea85f23f Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 28 Aug 2017 15:11:34 -0500 Subject: [PATCH 41/57] Better align fallback image emojis Fix https://gitlab.com/gitlab-org/gitlab-ce/issues/37147 --- app/assets/stylesheets/framework/typography.scss | 2 +- changelogs/unreleased/37147-fix-fallback-emoji-alignment.yml | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/37147-fix-fallback-emoji-alignment.yml diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss index 71eec0e1a5e..368cc71c5a6 100644 --- a/app/assets/stylesheets/framework/typography.scss +++ b/app/assets/stylesheets/framework/typography.scss @@ -10,7 +10,7 @@ color: $md-link-color; } - img { + img:not(.emoji) { /*max-width: 100%;*/ margin: 0 0 8px; } diff --git a/changelogs/unreleased/37147-fix-fallback-emoji-alignment.yml b/changelogs/unreleased/37147-fix-fallback-emoji-alignment.yml new file mode 100644 index 00000000000..34161e63c81 --- /dev/null +++ b/changelogs/unreleased/37147-fix-fallback-emoji-alignment.yml @@ -0,0 +1,5 @@ +--- +title: Better align fallback image emojis +merge_request: +author: +type: fixed From 934d09698cce2f913d4ca36e4b5c223e1ea75e85 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 29 Aug 2017 10:09:47 -0500 Subject: [PATCH 42/57] Remove commented out code --- app/assets/stylesheets/framework/typography.scss | 1 - 1 file changed, 1 deletion(-) diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss index 368cc71c5a6..b2423bc1a66 100644 --- a/app/assets/stylesheets/framework/typography.scss +++ b/app/assets/stylesheets/framework/typography.scss @@ -11,7 +11,6 @@ } img:not(.emoji) { - /*max-width: 100%;*/ margin: 0 0 8px; } From f41464f878726de7490ea2457060ff0b93dcd138 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Tue, 29 Aug 2017 18:19:59 +0100 Subject: [PATCH 43/57] Fixes the margin of the top buttons of the pipeline page --- app/assets/javascripts/pipelines/components/pipelines.vue | 4 +++- app/assets/stylesheets/pages/pipelines.scss | 4 ++++ changelogs/unreleased/34990-top-buttons-misaligned.yml | 5 +++++ 3 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/34990-top-buttons-misaligned.yml diff --git a/app/assets/javascripts/pipelines/components/pipelines.vue b/app/assets/javascripts/pipelines/components/pipelines.vue index 5df317a76bf..010063a0240 100644 --- a/app/assets/javascripts/pipelines/components/pipelines.vue +++ b/app/assets/javascripts/pipelines/components/pipelines.vue @@ -139,7 +139,9 @@ };