Merge branch 'ce-to-ee-2018-07-03' into 'master'

CE upstream - 2018-07-03 21:01 UTC

See merge request gitlab-org/gitlab-ee!6368

app/controllers/sessions_controller.rb

@@ -63,7 +63,11 @@ class SessionsController < Devise::SessionsController
     return unless captcha_enabled?
     return unless Gitlab::Recaptcha.load_configurations!
 
-    unless verify_recaptcha
+    if verify_recaptcha
+      increment_successful_login_captcha_counter
+    else
+      increment_failed_login_captcha_counter
+
       self.resource = resource_class.new
       flash[:alert] = 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.'
       flash.delete :recaptcha_error
@@ -72,6 +76,20 @@ class SessionsController < Devise::SessionsController
     end
   end
 
+  def increment_failed_login_captcha_counter
+    Gitlab::Metrics.counter(
+      :failed_login_captcha_total,
+      'Number of failed CAPTCHA attempts for logins'.freeze
+    ).increment
+  end
+
+  def increment_successful_login_captcha_counter
+    Gitlab::Metrics.counter(
+      :successful_login_captcha_total,
+      'Number of successful CAPTCHA attempts for logins'.freeze
+    ).increment
+  end
+
   def log_failed_login
     Gitlab::AppLogger.info("Failed Login: username=#{user_params[:login]} ip=#{request.remote_ip}")
   end

app/uploaders/file_uploader.rb

@@ -81,6 +81,13 @@ class FileUploader < GitlabUploader
     apply_context!(uploader_context)
   end
 
+  def initialize_copy(from)
+    super
+
+    @secret = self.class.generate_secret
+    @upload = nil # calling record_upload would delete the old upload if set
+  end
+
   # enforce the usage of Hashed storage when storing to
   # remote store as the FileMover doesn't support OS
   def base_dir(store = nil)
@@ -144,6 +151,27 @@ class FileUploader < GitlabUploader
     @secret ||= self.class.generate_secret
   end
 
+  # return a new uploader with a file copy on another project
+  def self.copy_to(uploader, to_project)
+    moved = uploader.dup.tap do |u|
+      u.model = to_project
+    end
+
+    moved.copy_file(uploader.file)
+    moved
+  end
+
+  def copy_file(file)
+    to_path = if file_storage?
+                File.join(self.class.root, store_path)
+              else
+                store_path
+              end
+
+    self.file = file.copy_to(to_path)
+    record_upload # after_store is not triggered
+  end
+
   private
 
   def apply_context!(uploader_context)

changelogs/unreleased/sh-fix-move-issue-with-object-storage.yml

+---
+title: Implement upload copy when moving an issue with upload on object storage 
+merge_request: 20191
+author:
+type: fixed

doc/administration/monitoring/prometheus/gitlab_metrics.md

@@ -49,6 +49,8 @@ The following metrics are available:
 | filesystem_circuitbreaker         | Gauge     | 9.5   | Whether or not the circuit for a certain shard is broken or not |
 | circuitbreaker_storage_check_duration_seconds | Histogram | 10.3 | Time a single storage probe took |
 | upload_file_does_not_exist                    | Counter   | 10.7  | Number of times an upload record could not find its file |
+| failed_login_captcha_total        | Gauge | 11.0 | Counter of failed CAPTCHA attempts during login |
+| successful_login_captcha_total    | Gauge | 11.0 | Counter of successful CAPTCHA attempts during login |
 
 ## Sidekiq Metrics available
 

lib/gitlab/gfm/uploads_rewriter.rb

@@ -23,11 +23,8 @@ module Gitlab
           file = find_file(@source_project, $~[:secret], $~[:file])
           break markdown unless file.try(:exists?)
 
-          new_uploader = FileUploader.new(target_project)
-          with_link_in_tmp_dir(file.file) do |open_tmp_file|
-            new_uploader.store!(open_tmp_file)
-          end
-          new_uploader.markdown_link
+          moved = FileUploader.copy_to(file, target_project)
+          moved.markdown_link
         end
       end
 
@@ -48,20 +45,7 @@ module Gitlab
       def find_file(project, secret, file)
         uploader = FileUploader.new(project, secret: secret)
         uploader.retrieve_from_store!(file)
-        uploader.file
-      end
-
-      # Because the uploaders use 'move_to_store' we must have a temporary
-      # file that is allowed to be (re)moved.
-      def with_link_in_tmp_dir(file)
-        dir = Dir.mktmpdir('UploadsRewriter', File.dirname(file))
-        # The filename matters to Carrierwave so we make sure to preserve it
-        tmp_file = File.join(dir, File.basename(file))
-        File.link(file, tmp_file)
-        # Open the file to placate Carrierwave
-        File.open(tmp_file) { |open_file| yield open_file }
-      ensure
-        FileUtils.rm_rf(dir)
+        uploader
       end
     end
   end

spec/controllers/sessions_controller_spec.rb

@@ -93,6 +93,12 @@ describe SessionsController do
         it 'displays an error when the reCAPTCHA is not solved' do
           # Without this, `verify_recaptcha` arbitraily returns true in test env
           Recaptcha.configuration.skip_verify_env.delete('test')
+          counter = double(:counter)
+
+          expect(counter).to receive(:increment)
+          expect(Gitlab::Metrics).to receive(:counter)
+                                      .with(:failed_login_captcha_total, anything)
+                                      .and_return(counter)
 
           post(:create, user: user_params)
 
@@ -104,6 +110,13 @@ describe SessionsController do
         it 'successfully logs in a user when reCAPTCHA is solved' do
           # Avoid test ordering issue and ensure `verify_recaptcha` returns true
           Recaptcha.configuration.skip_verify_env << 'test'
+          counter = double(:counter)
+
+          expect(counter).to receive(:increment)
+          expect(Gitlab::Metrics).to receive(:counter)
+                                      .with(:successful_login_captcha_total, anything)
+                                      .and_return(counter)
+          expect(Gitlab::Metrics).to receive(:counter).and_call_original
 
           post(:create, user: user_params)
 

spec/lib/gitlab/gfm/uploads_rewriter_spec.rb

@@ -20,37 +20,55 @@ describe Gitlab::Gfm::UploadsRewriter do
       "Text and #{image_uploader.markdown_link} and #{zip_uploader.markdown_link}"
     end
 
-    describe '#rewrite' do
-      let!(:new_text) { rewriter.rewrite(new_project) }
+    shared_examples "files are accessible" do
+      describe '#rewrite' do
+        let!(:new_text) { rewriter.rewrite(new_project) }
 
-      let(:old_files) { [image_uploader, zip_uploader].map(&:file) }
-      let(:new_files) do
-        described_class.new(new_text, new_project, user).files
-      end
+        let(:old_files) { [image_uploader, zip_uploader] }
+        let(:new_files) do
+          described_class.new(new_text, new_project, user).files
+        end
 
-      let(:old_paths) { old_files.map(&:path) }
-      let(:new_paths) { new_files.map(&:path) }
+        let(:old_paths) { old_files.map(&:path) }
+        let(:new_paths) { new_files.map(&:path) }
 
-      it 'rewrites content' do
-        expect(new_text).not_to eq text
-        expect(new_text.length).to eq text.length
-      end
+        it 'rewrites content' do
+          expect(new_text).not_to eq text
+          expect(new_text.length).to eq text.length
+        end
 
-      it 'copies files' do
-        expect(new_files).to all(exist)
-        expect(old_paths).not_to match_array new_paths
-        expect(old_paths).to all(include(old_project.disk_path))
-        expect(new_paths).to all(include(new_project.disk_path))
-      end
+        it 'copies files' do
+          expect(new_files).to all(exist)
+          expect(old_paths).not_to match_array new_paths
+          expect(old_paths).to all(include(old_project.disk_path))
+          expect(new_paths).to all(include(new_project.disk_path))
+        end
 
-      it 'does not remove old files' do
-        expect(old_files).to all(exist)
+        it 'does not remove old files' do
+          expect(old_files).to all(exist)
+        end
+
+        it 'generates a new secret for each file' do
+          expect(new_paths).not_to include image_uploader.secret
+          expect(new_paths).not_to include zip_uploader.secret
+        end
       end
+    end
 
-      it 'generates a new secret for each file' do
-        expect(new_paths).not_to include image_uploader.secret
-        expect(new_paths).not_to include zip_uploader.secret
+    context "file are stored locally" do
+      include_examples "files are accessible"
+    end
+
+    context "files are stored remotely" do
+      before do
+        stub_uploads_object_storage(FileUploader)
+
+        old_files.each do |file|
+          file.migrate!(ObjectStorage::Store::REMOTE)
+        end
       end
+
+      include_examples "files are accessible"
     end
 
     describe '#needs_rewrite?' do

spec/uploaders/file_uploader_spec.rb

@@ -80,6 +80,50 @@ describe FileUploader do
     end
   end
 
+  describe 'copy_to' do
+    shared_examples 'returns a valid uploader' do
+      describe 'returned uploader' do
+        let(:new_project) { create(:project) }
+        let(:moved) { described_class.copy_to(subject, new_project) }
+
+        it 'generates a new secret' do
+          expect(subject).to be
+          expect(described_class).to receive(:generate_secret).once.and_call_original
+          expect(moved).to be
+        end
+
+        it 'create new upload' do
+          expect(moved.upload).not_to eq(subject.upload)
+        end
+
+        it 'copies the file' do
+          expect(subject.file).to exist
+          expect(moved.file).to exist
+          expect(subject.file).not_to eq(moved.file)
+          expect(subject.object_store).to eq(moved.object_store)
+        end
+      end
+    end
+
+    context 'files are stored locally' do
+      before do
+        subject.store!(fixture_file_upload('spec/fixtures/dk.png'))
+      end
+
+      include_examples 'returns a valid uploader'
+    end
+
+    context 'files are stored remotely' do
+      before do
+        stub_uploads_object_storage
+        subject.store!(fixture_file_upload('spec/fixtures/dk.png'))
+        subject.migrate!(ObjectStorage::Store::REMOTE)
+      end
+
+      include_examples 'returns a valid uploader'
+    end
+  end
+
   describe '#secret' do
     it 'generates a secret if none is provided' do
       expect(described_class).to receive(:generate_secret).and_return('secret')