Merge branch '34102-online-view-of-artifacts-fe' into 'master'

Add external link for online artifacts

Closes #34102

See merge request gitlab-org/gitlab-ce!14399

15 files changed, 277 insertions, 41 deletions

diff --git a/app/assets/javascripts/build_artifacts.js b/app/assets/javascripts/build_artifacts.js
index bd479700fd3..19388f1f9ae 100644
--- a/app/assets/javascripts/build_artifacts.js
+++ b/app/assets/javascripts/build_artifacts.js
@@ -1,9 +1,12 @@
 /* eslint-disable func-names, space-before-function-paren, wrap-iife, prefer-arrow-callback, no-unused-vars, no-return-assign, max-len */
+import { visitUrl } from './lib/utils/url_utility';
+import { convertPermissionToBoolean } from './lib/utils/common_utils';
 
 window.BuildArtifacts = (function() {
   function BuildArtifacts() {
     this.disablePropagation();
     this.setupEntryClick();
+    this.setupTooltips();
   }
 
   BuildArtifacts.prototype.disablePropagation = function() {
@@ -17,9 +20,28 @@ window.BuildArtifacts = (function() {
 
   BuildArtifacts.prototype.setupEntryClick = function() {
     return $('.tree-holder').on('click', 'tr[data-link]', function(e) {
-      return window.location = this.dataset.link;
+      visitUrl(this.dataset.link, convertPermissionToBoolean(this.dataset.externalLink));
     });
   };
 
+  BuildArtifacts.prototype.setupTooltips = function() {
+    $('.js-artifact-tree-tooltip').tooltip({
+      placement: 'bottom',
+      // Stop the tooltip from hiding when we stop hovering the element directly
+      // We handle all the showing/hiding below
+      trigger: 'manual',
+    });
+
+    // We want the tooltip to show if you hover anywhere on the row
+    // But be placed below and in the middle of the file name
+    $('.js-artifact-tree-row')
+      .on('mouseenter', (e) => {
+        $(e.currentTarget).find('.js-artifact-tree-tooltip').tooltip('show');
+      })
+      .on('mouseleave', (e) => {
+        $(e.currentTarget).find('.js-artifact-tree-tooltip').tooltip('hide');
+      });
+  };
+
   return BuildArtifacts;
 })();
diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js
index 3328ff9cc23..78c7a094127 100644
--- a/app/assets/javascripts/lib/utils/url_utility.js
+++ b/app/assets/javascripts/lib/utils/url_utility.js
@@ -1,4 +1,5 @@
 /* eslint-disable func-names, space-before-function-paren, wrap-iife, no-var, no-param-reassign, no-cond-assign, one-var, one-var-declaration-per-line, no-void, guard-for-in, no-restricted-syntax, prefer-template, quotes, max-len */
+
 var base;
 var w = window;
 if (w.gl == null) {
@@ -86,6 +87,21 @@ w.gl.utils.getLocationHash = function(url) {
 
 w.gl.utils.refreshCurrentPage = () => gl.utils.visitUrl(document.location.href);
 
-w.gl.utils.visitUrl = (url) => {
-  document.location.href = url;
+// eslint-disable-next-line import/prefer-default-export
+export function visitUrl(url, external = false) {
+  if (external) {
+    // Simulate `target="blank" rel="noopener noreferrer"`
+    // See https://mathiasbynens.github.io/rel-noopener/
+    const otherWindow = window.open();
+    otherWindow.opener = null;
+    otherWindow.location = url;
+  } else {
+    document.location.href = url;
+  }
+}
+
+window.gl = window.gl || {};
+window.gl.utils = {
+  ...(window.gl.utils || {}),
+  visitUrl,
 };
diff --git a/app/assets/stylesheets/pages/tree.scss b/app/assets/stylesheets/pages/tree.scss
index 224eee90a3f..e2f6e511c86 100644
--- a/app/assets/stylesheets/pages/tree.scss
+++ b/app/assets/stylesheets/pages/tree.scss
@@ -169,6 +169,14 @@
       }
     }
 
+    .tree-item-file-external-link {
+      margin-right: 4px;
+
+      span {
+        text-decoration: inherit;
+      }
+    }
+
     .tree_commit {
       max-width: 320px;
 
diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb
index eb010923466..0837451cc49 100644
--- a/app/controllers/projects/artifacts_controller.rb
+++ b/app/controllers/projects/artifacts_controller.rb
@@ -29,13 +29,17 @@ class Projects::ArtifactsController < Projects::ApplicationController
     blob = @entry.blob
     conditionally_expand_blob(blob)
 
-    respond_to do |format|
-      format.html do
-        render 'file'
-      end
-
-      format.json do
-        render_blob_json(blob)
+    if blob.external_link?(build)
+      redirect_to blob.external_url(@project, build)
+    else
+      respond_to do |format|
+        format.html do
+          render 'file'
+        end
+
+        format.json do
+          render_blob_json(blob)
+        end
       end
     end
   end
diff --git a/app/models/ci/artifact_blob.rb b/app/models/ci/artifact_blob.rb
index b35febc9ac5..8b66531ec7b 100644
--- a/app/models/ci/artifact_blob.rb
+++ b/app/models/ci/artifact_blob.rb
@@ -2,6 +2,8 @@ module Ci
   class ArtifactBlob
     include BlobLike
 
+    EXTENTIONS_SERVED_BY_PAGES = %w[.html .htm .txt .json].freeze
+
     attr_reader :entry
 
     def initialize(entry)
@@ -17,6 +19,7 @@ module Ci
     def size
       entry.metadata[:size]
     end
+    alias_method :external_size, :size
 
     def data
       "Build artifact #{path}"
@@ -30,6 +33,27 @@ module Ci
       :build_artifact
     end
 
-    alias_method :external_size, :size
+    def external_url(project, job)
+      return unless external_link?(job)
+
+      components = project.full_path_components
+      components << "-/jobs/#{job.id}/artifacts/file/#{path}"
+      artifact_path = components[1..-1].join('/')
+
+      "#{pages_config.protocol}://#{components[0]}.#{pages_config.host}/#{artifact_path}"
+    end
+
+    def external_link?(job)
+      pages_config.enabled &&
+        pages_config.artifacts_server &&
+        EXTENTIONS_SERVED_BY_PAGES.include?(File.extname(name)) &&
+        job.project.public?
+    end
+
+    private
+
+    def pages_config
+      Gitlab.config.pages
+    end
   end
 end
diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb
index f5048d17d80..12e93be2104 100644
--- a/app/models/concerns/routable.rb
+++ b/app/models/concerns/routable.rb
@@ -106,6 +106,10 @@ module Routable
     RequestStore[full_path_key] ||= uncached_full_path
   end
 
+  def full_path_components
+    full_path.split('/')
+  end
+
   def expires_full_path_cache
     RequestStore.delete(full_path_key) if RequestStore.active?
     @full_path = nil
diff --git a/app/views/projects/artifacts/_tree_file.html.haml b/app/views/projects/artifacts/_tree_file.html.haml
index 8edb9be049a..a97ddb3c377 100644
--- a/app/views/projects/artifacts/_tree_file.html.haml
+++ b/app/views/projects/artifacts/_tree_file.html.haml
@@ -1,10 +1,17 @@
+- blob = file.blob
 - path_to_file = file_project_job_artifacts_path(@project, @build, path: file.path)
+- external_link = blob.external_link?(@build)
 
-%tr.tree-item{ 'data-link' => path_to_file }
-  - blob = file.blob
+%tr.tree-item.js-artifact-tree-row{ data: { link: path_to_file, external_link: "#{external_link}" } }
   %td.tree-item-file-name
     = tree_icon('file', blob.mode, blob.name)
-    = link_to path_to_file do
-      %span.str-truncated= blob.name
+    - if external_link
+      = link_to path_to_file, class: 'tree-item-file-external-link js-artifact-tree-tooltip',
+          target: '_blank', rel: 'noopener noreferrer', title: _('Opens in a new window') do
+        %span.str-truncated>= blob.name
+      = icon('external-link', class: 'js-artifact-tree-external-icon')
+    - else
+      = link_to path_to_file do
+        %span.str-truncated= blob.name
   %td
     = number_to_human_size(blob.size, precision: 2)
diff --git a/changelogs/unreleased/34102-online-view-of-artifacts-fe.yml b/changelogs/unreleased/34102-online-view-of-artifacts-fe.yml
new file mode 100644
index 00000000000..ce83b140eb6
--- /dev/null
+++ b/changelogs/unreleased/34102-online-view-of-artifacts-fe.yml
@@ -0,0 +1,5 @@
+---
+title: Add online view of HTML artifacts for public projects
+merge_request: 14399
+author:
+type: added
diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example
index 88771c5f5bb..1069c7be5f0 100644
--- a/config/gitlab.yml.example
+++ b/config/gitlab.yml.example
@@ -164,6 +164,7 @@ production: &base
     host: example.com
     port: 80 # Set to 443 if you serve the pages with HTTPS
     https: false # Set to true if you serve the pages with HTTPS
+    artifacts_server: true
     # external_http: ["1.1.1.1:80", "[2001::1]:80"] # If defined, enables custom domain support in GitLab Pages
     # external_https: ["1.1.1.1:443", "[2001::1]:443"] # If defined, enables custom domain and certificate support in GitLab Pages
 
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index a23b3208dab..a4b7c1a3919 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -316,15 +316,16 @@ Settings.registry['path']            = Settings.absolute(Settings.registry['path
 # Pages
 #
 Settings['pages'] ||= Settingslogic.new({})
-Settings.pages['enabled']         = false if Settings.pages['enabled'].nil?
-Settings.pages['path']            = Settings.absolute(Settings.pages['path'] || File.join(Settings.shared['path'], "pages"))
-Settings.pages['https']           = false if Settings.pages['https'].nil?
-Settings.pages['host']            ||= "example.com"
-Settings.pages['port']            ||= Settings.pages.https ? 443 : 80
-Settings.pages['protocol']        ||= Settings.pages.https ? "https" : "http"
-Settings.pages['url']             ||= Settings.__send__(:build_pages_url)
-Settings.pages['external_http']   ||= false unless Settings.pages['external_http'].present?
-Settings.pages['external_https']  ||= false unless Settings.pages['external_https'].present?
+Settings.pages['enabled']           = false if Settings.pages['enabled'].nil?
+Settings.pages['path']              = Settings.absolute(Settings.pages['path'] || File.join(Settings.shared['path'], "pages"))
+Settings.pages['https']             = false if Settings.pages['https'].nil?
+Settings.pages['host']              ||= "example.com"
+Settings.pages['port']              ||= Settings.pages.https ? 443 : 80
+Settings.pages['protocol']          ||= Settings.pages.https ? "https" : "http"
+Settings.pages['url']               ||= Settings.__send__(:build_pages_url)
+Settings.pages['external_http']     ||= false unless Settings.pages['external_http'].present?
+Settings.pages['external_https']    ||= false unless Settings.pages['external_https'].present?
+Settings.pages['artifacts_server']  ||= Settings.pages['enabled'] if Settings.pages['artifacts_server'].nil?
 
 #
 # Git LFS
diff --git a/doc/user/project/pipelines/img/job_artifacts_browser.png b/doc/user/project/pipelines/img/job_artifacts_browser.png
index 145fe156bbb..d3d8de5ac60 100644
--- a/doc/user/project/pipelines/img/job_artifacts_browser.png
+++ b/doc/user/project/pipelines/img/job_artifacts_browser.png
diff --git a/doc/user/project/pipelines/job_artifacts.md b/doc/user/project/pipelines/job_artifacts.md
index 4e93e680fd2..9ef6f9185c9 100644
--- a/doc/user/project/pipelines/job_artifacts.md
+++ b/doc/user/project/pipelines/job_artifacts.md
@@ -50,6 +50,10 @@ For more examples on artifacts, follow the [artifacts reference in
 With GitLab 9.2, PDFs, images, videos and other formats can be previewed
 directly in the job artifacts browser without the need to download them.
 
+>**Note:**
+With [GitLab 10.1][ce-14399], HTML files in a public project can be previewed
+directly in a new tab without the need to download them.
+
 After a job finishes, if you visit the job's specific page, there are three
 buttons. You can download the artifacts archive or browse its contents, whereas
 the **Keep** button appears only if you have set an [expiry date] to the
@@ -64,7 +68,8 @@ archive. If your artifacts contained directories, then you are also able to
 browse inside them.
 
 Below you can see how browsing looks like. In this case we have browsed inside
-the archive and at this point there is one directory and one HTML file.
+the archive and at this point there is one directory, a couple files, and
+one HTML file that you can view directly online (opens in a new tab).
 
 ![Job artifacts browser](img/job_artifacts_browser.png)
 
@@ -158,3 +163,4 @@ information in the UI.
 
 
 [expiry date]: ../../../ci/yaml/README.md#artifacts-expire_in
+[ce-14399]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14399
diff --git a/spec/controllers/projects/artifacts_controller_spec.rb b/spec/controllers/projects/artifacts_controller_spec.rb
index caa63e7bd22..d0992719171 100644
--- a/spec/controllers/projects/artifacts_controller_spec.rb
+++ b/spec/controllers/projects/artifacts_controller_spec.rb
@@ -1,8 +1,8 @@
 require 'spec_helper'
 
 describe Projects::ArtifactsController do
-  let(:user) { create(:user) }
-  let(:project) { create(:project, :repository) }
+  set(:user) { create(:user) }
+  set(:project) { create(:project, :repository, :public) }
 
   let(:pipeline) do
     create(:ci_pipeline,
@@ -15,7 +15,7 @@ describe Projects::ArtifactsController do
   let(:job) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
 
   before do
-    project.team << [user, :developer]
+    project.add_developer(user)
 
     sign_in(user)
   end
@@ -47,19 +47,67 @@ describe Projects::ArtifactsController do
   end
 
   describe 'GET file' do
-    context 'when the file exists' do
-      it 'renders the file view' do
-        get :file, namespace_id: project.namespace, project_id: project, job_id: job, path: 'ci_artifacts.txt'
+    before do
+      allow(Gitlab.config.pages).to receive(:enabled).and_return(true)
+    end
 
-        expect(response).to render_template('projects/artifacts/file')
+    context 'when the file is served by GitLab Pages' do
+      before do
+        allow(Gitlab.config.pages).to receive(:artifacts_server).and_return(true)
+      end
+
+      context 'when the file exists' do
+        it 'renders the file view' do
+          get :file, namespace_id: project.namespace, project_id: project, job_id: job, path: 'ci_artifacts.txt'
+
+          expect(response).to have_http_status(302)
+        end
+      end
+
+      context 'when the file does not exist' do
+        it 'responds Not Found' do
+          get :file, namespace_id: project.namespace, project_id: project, job_id: job, path: 'unknown'
+
+          expect(response).to be_not_found
+        end
       end
     end
 
-    context 'when the file does not exist' do
-      it 'responds Not Found' do
-        get :file, namespace_id: project.namespace, project_id: project, job_id: job, path: 'unknown'
+    context 'when the file is served through Rails' do
+      context 'when the file exists' do
+        it 'renders the file view' do
+          get :file, namespace_id: project.namespace, project_id: project, job_id: job, path: 'ci_artifacts.txt'
 
-        expect(response).to be_not_found
+          expect(response).to have_http_status(:ok)
+          expect(response).to render_template('projects/artifacts/file')
+        end
+      end
+
+      context 'when the file does not exist' do
+        it 'responds Not Found' do
+          get :file, namespace_id: project.namespace, project_id: project, job_id: job, path: 'unknown'
+
+          expect(response).to be_not_found
+        end
+      end
+    end
+
+    context 'when the project is private' do
+      let(:private_project) { create(:project, :repository, :private) }
+      let(:pipeline) { create(:ci_pipeline, project: private_project) }
+      let(:job) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
+
+      before do
+        private_project.add_developer(user)
+
+        allow(Gitlab.config.pages).to receive(:artifacts_server).and_return(true)
+      end
+
+      it 'does not redirect the request' do
+        get :file, namespace_id: private_project.namespace, project_id: private_project, job_id: job, path: 'ci_artifacts.txt'
+
+        expect(response).to have_http_status(:ok)
+        expect(response).to render_template('projects/artifacts/file')
       end
     end
   end
diff --git a/spec/features/projects/artifacts/browse_spec.rb b/spec/features/projects/artifacts/browse_spec.rb
index 42b47cb3301..cb69aff8d5f 100644
--- a/spec/features/projects/artifacts/browse_spec.rb
+++ b/spec/features/projects/artifacts/browse_spec.rb
@@ -4,16 +4,15 @@ feature 'Browse artifact', :js do
   let(:project) { create(:project, :public) }
   let(:pipeline) { create(:ci_empty_pipeline, project: project) }
   let(:job) { create(:ci_build, :artifacts, pipeline: pipeline) }
+  let(:browse_url) do
+    browse_path('other_artifacts_0.1.2')
+  end
 
   def browse_path(path)
     browse_project_job_artifacts_path(project, job, path)
   end
 
   context 'when visiting old URL' do
-    let(:browse_url) do
-      browse_path('other_artifacts_0.1.2')
-    end
-
     before do
       visit browse_url.sub('/-/jobs', '/builds')
     end
@@ -22,4 +21,47 @@ feature 'Browse artifact', :js do
       expect(page.current_path).to eq(browse_url)
     end
   end
+
+  context 'when browsing a directory with an text file' do
+    let(:txt_entry) { job.artifacts_metadata_entry('other_artifacts_0.1.2/doc_sample.txt') }
+
+    before do
+      allow(Gitlab.config.pages).to receive(:enabled).and_return(true)
+      allow(Gitlab.config.pages).to receive(:artifacts_server).and_return(true)
+    end
+
+    context 'when the project is public' do
+      it "shows external link icon and styles" do
+        visit browse_url
+
+        link = first('.tree-item-file-external-link')
+
+        expect(page).to have_link('doc_sample.txt', href: file_project_job_artifacts_path(project, job, path: txt_entry.blob.path))
+        expect(link[:target]).to eq('_blank')
+        expect(link[:rel]).to include('noopener')
+        expect(link[:rel]).to include('noreferrer')
+        expect(page).to have_selector('.js-artifact-tree-external-icon')
+      end
+    end
+
+    context 'when the project is private' do
+      let!(:private_project) { create(:project, :private) }
+      let(:pipeline) { create(:ci_empty_pipeline, project: private_project) }
+      let(:job) { create(:ci_build, :artifacts, pipeline: pipeline) }
+      let(:user) { create(:user) }
+
+      before do
+        private_project.add_developer(user)
+
+        sign_in(user)
+      end
+
+      it 'shows internal link styles' do
+        visit browse_project_job_artifacts_path(private_project, job, 'other_artifacts_0.1.2')
+
+        expect(page).to have_link('doc_sample.txt')
+        expect(page).not_to have_selector('.js-artifact-tree-external-icon')
+      end
+    end
+  end
 end
diff --git a/spec/models/ci/artifact_blob_spec.rb b/spec/models/ci/artifact_blob_spec.rb
index a10a8af5303..d5ba088af53 100644
--- a/spec/models/ci/artifact_blob_spec.rb
+++ b/spec/models/ci/artifact_blob_spec.rb
@@ -1,7 +1,8 @@
 require 'spec_helper'
 
 describe Ci::ArtifactBlob do
-  let(:build) { create(:ci_build, :artifacts) }
+  set(:project) { create(:project, :public) }
+  set(:build) { create(:ci_build, :artifacts, project: project) }
   let(:entry) { build.artifacts_metadata_entry('other_artifacts_0.1.2/another-subdirectory/banana_sample.gif') }
 
   subject { described_class.new(entry) }
@@ -41,4 +42,51 @@ describe Ci::ArtifactBlob do
       expect(subject.external_storage).to eq(:build_artifact)
     end
   end
+
+  describe '#external_url' do
+    before do
+      allow(Gitlab.config.pages).to receive(:enabled).and_return(true)
+      allow(Gitlab.config.pages).to receive(:artifacts_server).and_return(true)
+    end
+
+    context '.gif extension' do
+      it 'returns nil' do
+        expect(subject.external_url(build.project, build)).to be_nil
+      end
+    end
+
+    context 'txt extensions' do
+      let(:entry) { build.artifacts_metadata_entry('other_artifacts_0.1.2/doc_sample.txt') }
+
+      it 'returns a URL' do
+        url = subject.external_url(build.project, build)
+
+        expect(url).not_to be_nil
+        expect(url).to start_with("http")
+        expect(url).to match Gitlab.config.pages.host
+        expect(url).to end_with(entry.path)
+      end
+    end
+  end
+
+  describe '#external_link?' do
+    before do
+      allow(Gitlab.config.pages).to receive(:enabled).and_return(true)
+      allow(Gitlab.config.pages).to receive(:artifacts_server).and_return(true)
+    end
+
+    context 'gif extensions' do
+      it 'returns false' do
+        expect(subject.external_link?(build)).to be false
+      end
+    end
+
+    context 'txt extensions' do
+      let(:entry) { build.artifacts_metadata_entry('other_artifacts_0.1.2/doc_sample.txt') }
+
+      it 'returns true' do
+        expect(subject.external_link?(build)).to be true
+      end
+    end
+  end
 end