Merge branch '4873-fixes-invalid-external-location' into 'master'

Resolve "Error message is not helping when an include is not a valid URL or local path"

Closes #4873 and #4874

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

ee/lib/gitlab/ci/external/file/base.rb

+module Gitlab
+  module Ci
+    module External
+      module File
+        class Base
+          YAML_WHITELIST_EXTENSION = /(yml|yaml)$/i.freeze
+
+          def initialize(location, opts = {})
+            @location = location
+          end
+
+          def valid?
+            location.match(YAML_WHITELIST_EXTENSION) && content
+          end
+
+          def content
+            raise NotImplementedError, 'content must be implemented and return a string or nil'
+          end
+
+          def error_message
+            raise NotImplementedError, 'error_message must be implemented and return a string'
+          end
+        end
+      end
+    end
+  end
+end

ee/lib/gitlab/ci/external/file/local.rb

@@ -2,27 +2,28 @@ module Gitlab
   module Ci
     module External
       module File
-        class Local
+        class Local < Base
           attr_reader :location, :project, :sha
 
           def initialize(location, opts = {})
-            @location = location
+            super
+
             @project = opts.fetch(:project)
             @sha = opts.fetch(:sha)
           end
 
-          def valid?
-            local_file_content
+          def content
+            @content ||= fetch_local_content
           end
 
-          def content
-            local_file_content
+          def error_message
+            "Local file '#{location}' is not valid."
           end
 
           private
 
-          def local_file_content
-            @local_file_content ||= project.repository.blob_data_at(sha, location)
+          def fetch_local_content
+            project.repository.blob_data_at(sha, location)
           end
         end
       end

ee/lib/gitlab/ci/external/file/remote.rb

@@ -2,29 +2,25 @@ module Gitlab
   module Ci
     module External
       module File
-        class Remote
+        class Remote < Base
           include Gitlab::Utils::StrongMemoize
           attr_reader :location
 
-          def initialize(location, opts = {})
-            @location = location
-          end
-
-          def valid?
-            ::Gitlab::UrlSanitizer.valid?(location) && content
-          end
-
           def content
             return @content if defined?(@content)
 
             @content = strong_memoize(:content) do
               begin
                 HTTParty.get(location)
-              rescue HTTParty::Error, Timeout::Error
-                false
+              rescue HTTParty::Error, Timeout::Error, SocketError
+                nil
               end
             end
           end
+
+          def error_message
+            "Remote file '#{location}' is not valid."
+          end
         end
       end
     end

ee/lib/gitlab/ci/external/mapper.rb

@@ -17,10 +17,8 @@ module Gitlab
         attr_reader :locations, :project, :sha
 
         def build_external_file(location)
-          remote_file = Gitlab::Ci::External::File::Remote.new(location)
-
-          if remote_file.valid?
-            remote_file
+          if ::Gitlab::UrlSanitizer.valid?(location)
+            Gitlab::Ci::External::File::Remote.new(location)
           else
             options = { project: project, sha: sha }
             Gitlab::Ci::External::File::Local.new(location, options)

ee/lib/gitlab/ci/external/processor.rb

@@ -28,7 +28,7 @@ module Gitlab
 
         def validate_external_file(external_file)
           unless external_file.valid?
-            raise FileError, "External file: '#{external_file.location}' should be a valid local or remote file"
+            raise FileError, external_file.error_message
           end
         end
 

spec/ee/spec/lib/ee/gitlab/ci/config_spec.rb

@@ -46,7 +46,7 @@ describe EE::Gitlab::Ci::Config do
 
     context "when gitlab_ci_yml has valid 'include' defined" do
       before do
-        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content)
+        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:fetch_local_content).and_return(local_file_content)
         WebMock.stub_request(:get, remote_location).to_return(body: remote_file_content)
       end
 
@@ -85,7 +85,7 @@ describe EE::Gitlab::Ci::Config do
       it 'raises error YamlProcessor validationError' do
         expect { config }.to raise_error(
           ::Gitlab::Ci::YamlProcessor::ValidationError,
-          "External file: 'invalid' should be a valid local or remote file"
+          "Local file 'invalid' is not valid."
         )
       end
     end

spec/ee/spec/lib/gitlab/ci/external/file/local_spec.rb

@@ -4,12 +4,12 @@ describe Gitlab::Ci::External::File::Local do
   let(:project) { create(:project, :repository) }
   let(:local_file) { described_class.new(location, { project: project, sha: '12345' }) }
 
-  describe "#valid?" do
+  describe '#valid?' do
     context 'when is a valid local path' do
       let(:location) { '/vendor/gitlab-ci-yml/existent-file.yml' }
 
       before do
-        allow_any_instance_of(described_class).to receive(:local_file_content).and_return("image: 'ruby2:2'")
+        allow_any_instance_of(described_class).to receive(:fetch_local_content).and_return("image: 'ruby2:2'")
       end
 
       it 'should return true' do
@@ -25,29 +25,52 @@ describe Gitlab::Ci::External::File::Local do
       end
     end
 
-    describe "#content" do
+    context 'when is not a yaml file' do
+      let(:location) { '/config/application.rb' }
+
+      it 'should return false' do
+        expect(local_file.valid?).to be_falsy
+      end
+    end
+  end
+
+  describe '#content' do
+    context 'with a a valid file' do
       let(:local_file_content) do
         <<~HEREDOC
-      before_script:
-        - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs
-        - ruby -v
-        - which ruby
-        - gem install bundler --no-ri --no-rdoc
-        - bundle install --jobs $(nproc)  "${FLAGS[@]}"
+          before_script:
+            - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs
+            - ruby -v
+            - which ruby
+            - gem install bundler --no-ri --no-rdoc
+            - bundle install --jobs $(nproc)  "${FLAGS[@]}"
         HEREDOC
       end
+      let(:location) { '/vendor/gitlab-ci-yml/existent-file.yml' }
+
+      before do
+        allow_any_instance_of(described_class).to receive(:fetch_local_content).and_return(local_file_content)
+      end
 
-      context 'with a local file' do
-        let(:location) { '/vendor/gitlab-ci-yml/non-existent-file.yml' }
+      it 'should return the content of the file' do
+        expect(local_file.content).to eq(local_file_content)
+      end
+    end
 
-        before do
-          allow_any_instance_of(described_class).to receive(:local_file_content).and_return(local_file_content)
-        end
+    context 'with an invalid file' do
+      let(:location) { '/vendor/gitlab-ci-yml/non-existent-file.yml' }
 
-        it 'should return the content of the file' do
-          expect(local_file.content).to eq(local_file_content)
-        end
+      it 'should be nil' do
+        expect(local_file.content).to be_nil
       end
     end
   end
+
+  describe '#error_message' do
+    let(:location) { '/vendor/gitlab-ci-yml/non-existent-file.yml' }
+
+    it 'should return an error message' do
+      expect(local_file.error_message).to eq("Local file '#{location}' is not valid.")
+    end
+  end
 end

spec/ee/spec/lib/gitlab/ci/external/file/remote_spec.rb

@@ -25,7 +25,7 @@ describe Gitlab::Ci::External::File::Remote do
       end
     end
 
-    context 'when is not a valid remote url' do
+    context 'with an irregular url' do
       let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
 
       it 'should return false' do
@@ -42,6 +42,14 @@ describe Gitlab::Ci::External::File::Remote do
         expect(remote_file.valid?).to be_falsy
       end
     end
+
+    context 'when is not a yaml file' do
+      let(:location) { 'https://asdasdasdaj48ggerexample.com' }
+
+      it 'should be falsy' do
+        expect(remote_file.valid?).to be_falsy
+      end
+    end
   end
 
   describe "#content" do
@@ -64,5 +72,25 @@ describe Gitlab::Ci::External::File::Remote do
         expect(remote_file.content).to be_falsy
       end
     end
+
+    context 'with an invalid remote url' do
+      let(:location) { 'https://asdasdasdaj48ggerexample.com' }
+
+      before do
+        WebMock.stub_request(:get, location).to_raise(SocketError.new('Some HTTP error'))
+      end
+
+      it 'should be nil' do
+        expect(remote_file.content).to be_nil
+      end
+    end
+  end
+
+  describe "#error_message" do
+    let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
+
+    it 'should return an error message' do
+      expect(remote_file.error_message).to eq("Remote file '#{location}' is not valid.")
+    end
   end
 end

spec/ee/spec/lib/gitlab/ci/external/processor_spec.rb

@@ -19,18 +19,23 @@ describe Gitlab::Ci::External::Processor do
       it 'should raise an error' do
         expect { processor.perform }.to raise_error(
           described_class::FileError,
-          "External file: '/vendor/gitlab-ci-yml/non-existent-file.yml' should be a valid local or remote file"
+          "Local file '/vendor/gitlab-ci-yml/non-existent-file.yml' is not valid."
         )
       end
     end
 
     context 'when an invalid remote file is defined' do
-      let(:values) { { include: 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml', image: 'ruby:2.2' } }
+      let(:remote_file) { 'http://doesntexist.com/.gitlab-ci-1.yml' }
+      let(:values) { { include: remote_file, image: 'ruby:2.2' } }
+
+      before do
+        WebMock.stub_request(:get, remote_file).to_raise(SocketError.new('Some HTTP error'))
+      end
 
       it 'should raise an error' do
         expect { processor.perform }.to raise_error(
           described_class::FileError,
-          "External file: 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' should be a valid local or remote file"
+          "Remote file '#{remote_file}' is not valid."
         )
       end
     end
@@ -85,7 +90,7 @@ describe Gitlab::Ci::External::Processor do
       end
 
       before do
-        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content)
+        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:fetch_local_content).and_return(local_file_content)
       end
 
       it 'should append the file to the values' do
@@ -124,7 +129,7 @@ describe Gitlab::Ci::External::Processor do
 
       before do
         local_file_content = File.read("#{Rails.root}/spec/ee/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml")
-        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content)
+        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:fetch_local_content).and_return(local_file_content)
         WebMock.stub_request(:get, remote_file).to_return(body: remote_file_content)
       end
 
@@ -143,7 +148,7 @@ describe Gitlab::Ci::External::Processor do
       let(:local_file_content) { 'invalid content file ////' }
 
       before do
-        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content)
+        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:fetch_local_content).and_return(local_file_content)
       end
 
       it 'should raise an error' do