Add latest changes from gitlab-org/gitlab@master

10 files changed, 91 insertions, 21 deletions

diff --git a/app/assets/javascripts/diff.js b/app/assets/javascripts/diff.js
index 23eb470503e..65816495432 100644
--- a/app/assets/javascripts/diff.js
+++ b/app/assets/javascripts/diff.js
@@ -12,9 +12,13 @@ const UNFOLD_COUNT = 20;
 let isBound = false;
 
 export default class Diff {
-  constructor() {
+  constructor({ mergeRequestEventHub } = {}) {
     const $diffFile = $('.files .diff-file');
 
+    if (mergeRequestEventHub) {
+      this.mrHub = mergeRequestEventHub;
+    }
+
     $diffFile.each((index, file) => {
       if (!$.data(file, 'singleFileDiff')) {
         $.data(file, 'singleFileDiff', new SingleFileDiff(file));
@@ -34,7 +38,8 @@ export default class Diff {
       $(document)
         .on('click', '.js-unfold', this.handleClickUnfold.bind(this))
         .on('click', '.diff-line-num a', this.handleClickLineNum.bind(this))
-        .on('mousedown', 'td.line_content.parallel', this.handleParallelLineDown.bind(this));
+        .on('mousedown', 'td.line_content.parallel', this.handleParallelLineDown.bind(this))
+        .on('click', '.inline-parallel-buttons a', ($e) => this.viewTypeSwitch($e));
       isBound = true;
     }
 
@@ -135,6 +140,20 @@ export default class Diff {
   diffViewType() {
     return $('.inline-parallel-buttons a.active').data('viewType');
   }
+  viewTypeSwitch(event) {
+    const click = event.originalEvent;
+    const diffSource = new URL(click.target.getAttribute('href'), document.location.href);
+
+    if (this.mrHub) {
+      click.preventDefault();
+      click.stopPropagation();
+
+      diffSource.pathname = `${diffSource.pathname}.json`;
+
+      this.mrHub.$emit('diff:switch-view-type', { source: diffSource.toString() });
+    }
+  }
+
   // eslint-disable-next-line class-methods-use-this
   lineNumbers(line) {
     const children = line.find('.diff-line-num').toArray();
diff --git a/app/assets/javascripts/init_diff_stats_dropdown.js b/app/assets/javascripts/init_diff_stats_dropdown.js
index 27df761a103..8413fe92f89 100644
--- a/app/assets/javascripts/init_diff_stats_dropdown.js
+++ b/app/assets/javascripts/init_diff_stats_dropdown.js
@@ -4,7 +4,7 @@ import { stickyMonitor } from './lib/utils/sticky';
 
 export const initDiffStatsDropdown = (stickyTop) => {
   if (stickyTop) {
-    stickyMonitor(document.querySelector('.js-diff-files-changed'), stickyTop);
+    stickyMonitor(document.querySelector('.js-diff-files-changed'), stickyTop, false);
   }
 
   const el = document.querySelector('.js-diff-stats-dropdown');
diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js
index 5a1410ceeba..46ee8fecfc5 100644
--- a/app/assets/javascripts/merge_request_tabs.js
+++ b/app/assets/javascripts/merge_request_tabs.js
@@ -1,4 +1,4 @@
-/* eslint-disable no-new, class-methods-use-this */
+/* eslint-disable class-methods-use-this */
 import $ from 'jquery';
 import Vue from 'vue';
 import { createAlert } from '~/flash';
@@ -134,8 +134,8 @@ function destroyPipelines(app) {
   return null;
 }
 
-function loadDiffs({ url, sticky }) {
-  return axios.get(`${url}.json${location.search}`).then(({ data }) => {
+function loadDiffs({ url, sticky, tabs }) {
+  return axios.get(url).then(({ data }) => {
     const $container = $('#diffs');
     $container.html(data.html);
     initDiffStatsDropdown(sticky);
@@ -143,7 +143,9 @@ function loadDiffs({ url, sticky }) {
     localTimeAgo(document.querySelectorAll('#diffs .js-timeago'));
     syntaxHighlight($('#diffs .js-syntax-highlight'));
 
-    new Diff();
+    tabs.createDiff();
+    tabs.setHubToDiff();
+
     scrollToContainer('#diffs');
 
     $('.diff-file').each((i, el) => {
@@ -204,6 +206,7 @@ export default class MergeRequestTabs {
 
     this.currentTab = null;
     this.diffsLoaded = false;
+    this.diffsClass = null;
     this.commitsLoaded = false;
     this.fixedLayoutPref = null;
     this.eventHub = createEventHub();
@@ -211,6 +214,7 @@ export default class MergeRequestTabs {
 
     this.setUrl = setUrl !== undefined ? setUrl : true;
     this.setCurrentAction = this.setCurrentAction.bind(this);
+    this.switchViewType = this.switchViewType.bind(this);
     this.tabShown = this.tabShown.bind(this);
     this.clickTab = this.clickTab.bind(this);
 
@@ -230,11 +234,13 @@ export default class MergeRequestTabs {
       this.tabShown(action, location.href);
       this.eventHub.$emit('MergeRequestTabChange', action);
     });
+    this.eventHub.$on('diff:switch-view-type', this.switchViewType);
   }
 
   // Used in tests
   unbindEvents() {
     $('.merge-request-tabs a[data-toggle="tabvue"]').off('click', this.clickTab);
+    this.eventHub.$off('diff:switch-view-type', this.switchViewType);
   }
 
   storeScroll() {
@@ -341,7 +347,7 @@ export default class MergeRequestTabs {
             in practice, this only occurs when comparing commits in
             the new merge request form page.
           */
-          this.loadDiff(href);
+          this.loadDiff({ endpoint: href, strip: true });
         }
         // this.hideSidebar();
         this.expandViewContainer();
@@ -503,17 +509,20 @@ export default class MergeRequestTabs {
   }
 
   // load the diff tab content from the backend
-  loadDiff(source) {
+  loadDiff({ endpoint, strip = true }) {
     if (this.diffsLoaded) {
       document.dispatchEvent(new CustomEvent('scroll'));
       return;
     }
 
+    // We extract pathname for the current Changes tab anchor href
+    // some pages like MergeRequestsController#new has query parameters on that anchor
+    const diffUrl = strip ? `${parseUrlPathname(endpoint)}.json${location.search}` : endpoint;
+
     loadDiffs({
-      // We extract pathname for the current Changes tab anchor href
-      // some pages like MergeRequestsController#new has query parameters on that anchor
-      url: parseUrlPathname(source),
+      url: diffUrl,
       sticky: computeTopOffset(this.mergeRequestTabs),
+      tabs: this,
     })
       .then(() => {
         if (this.isDiffAction(this.currentAction)) {
@@ -528,6 +537,21 @@ export default class MergeRequestTabs {
         });
       });
   }
+  switchViewType({ source }) {
+    this.diffsLoaded = false;
+
+    this.loadDiff({ endpoint: source, strip: false });
+  }
+  createDiff() {
+    if (!this.diffsClass) {
+      this.diffsClass = new Diff({ mergeRequestEventHub: this.eventHub });
+    }
+  }
+  setHubToDiff() {
+    if (this.diffsClass) {
+      this.diffsClass.mrHub = this.eventHub;
+    }
+  }
 
   diffViewType() {
     return $('.js-diff-view-buttons button.active').data('viewType');
diff --git a/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js b/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js
index a4e3ddfc506..d4734b8842d 100644
--- a/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js
+++ b/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js
@@ -4,14 +4,12 @@ import $ from 'jquery';
 import IssuableForm from 'ee_else_ce/issuable/issuable_form';
 import IssuableLabelSelector from '~/issuable/issuable_label_selector';
 import ShortcutsNavigation from '~/behaviors/shortcuts/shortcuts_navigation';
-import Diff from '~/diff';
 import GLForm from '~/gl_form';
 import LabelsSelect from '~/labels/labels_select';
 import IssuableTemplateSelectors from '~/issuable/issuable_template_selectors';
 import { mountMilestoneDropdown } from '~/sidebar/mount_sidebar';
 
 export default () => {
-  new Diff();
   new ShortcutsNavigation();
   new GLForm($('.merge-request-form'));
   new IssuableForm($('.merge-request-form'));
diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb
index 53c358f4eba..0dca5b18a24 100644
--- a/app/models/ci/job_artifact.rb
+++ b/app/models/ci/job_artifact.rb
@@ -14,6 +14,8 @@ module Ci
     include EachBatch
     include Gitlab::Utils::StrongMemoize
 
+    enum accessibility: { public: 0, private: 1 }, _suffix: true
+
     NON_ERASABLE_FILE_TYPES = %w[trace].freeze
 
     REPORT_FILE_TYPES = {
@@ -346,6 +348,12 @@ module Ci
       end
     end
 
+    def public_access?
+      return true unless Feature.enabled?(:non_public_artifacts, type: :development)
+
+      public_accessibility?
+    end
+
     private
 
     def store_file_in_transaction!
diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb
index 5ef926ef2e3..ca0b51e1385 100644
--- a/app/policies/ci/build_policy.rb
+++ b/app/policies/ci/build_policy.rb
@@ -59,7 +59,7 @@ module Ci
       @subject.debug_mode?
     end
 
-    condition(:project_read_build, scope: :subject) do
+    condition(:can_read_project_build, scope: :subject) do
       can?(:read_build, @subject.project)
     end
 
@@ -71,7 +71,7 @@ module Ci
       can?(:developer_access, @subject.project)
     end
 
-    rule { project_read_build }.enable :read_build_trace
+    rule { can_read_project_build }.enable :read_build_trace
     rule { debug_mode & ~project_update_build }.prevent :read_build_trace
 
     # Authorizing the user to access to protected entities.
@@ -114,7 +114,7 @@ module Ci
       prevent :create_build_service_proxy
     end
 
-    rule { project_read_build }.enable :read_job_artifacts
+    rule { can_read_project_build }.enable :read_job_artifacts
     rule { ~artifacts_public & ~project_developer }.prevent :read_job_artifacts
   end
 end
diff --git a/app/policies/ci/job_artifact_policy.rb b/app/policies/ci/job_artifact_policy.rb
index e25c7311565..61c935af8ba 100644
--- a/app/policies/ci/job_artifact_policy.rb
+++ b/app/policies/ci/job_artifact_policy.rb
@@ -3,5 +3,20 @@
 module Ci
   class JobArtifactPolicy < BasePolicy
     delegate { @subject.job.project }
+
+    condition(:public_access, scope: :subject) do
+      @subject.public_access?
+    end
+
+    condition(:can_read_project_build, scope: :subject) do
+      can?(:read_build, @subject.job.project)
+    end
+
+    condition(:has_access_to_project) do
+      can?(:developer_access, @subject.job.project)
+    end
+
+    rule { can_read_project_build }.enable :read_job_artifacts
+    rule { ~public_access & ~has_access_to_project }.prevent :read_job_artifacts
   end
 end
diff --git a/app/serializers/ci/downloadable_artifact_entity.rb b/app/serializers/ci/downloadable_artifact_entity.rb
index 1f3885f0715..2e8aafcee43 100644
--- a/app/serializers/ci/downloadable_artifact_entity.rb
+++ b/app/serializers/ci/downloadable_artifact_entity.rb
@@ -8,7 +8,7 @@ module Ci
       artifacts = pipeline.downloadable_artifacts
 
       if Feature.enabled?(:non_public_artifacts)
-        artifacts = artifacts.select { |artifact| can?(request.current_user, :read_job_artifacts, artifact.job) }
+        artifacts = artifacts.select { |artifact| can?(request.current_user, :read_job_artifacts, artifact) }
       end
 
       BuildArtifactEntity.represent(artifacts, options.merge(project: pipeline.project))
diff --git a/app/serializers/merge_requests/pipeline_entity.rb b/app/serializers/merge_requests/pipeline_entity.rb
index 76e75a8ca6d..445914fe01c 100644
--- a/app/serializers/merge_requests/pipeline_entity.rb
+++ b/app/serializers/merge_requests/pipeline_entity.rb
@@ -30,7 +30,7 @@ class MergeRequests::PipelineEntity < Grape::Entity
       rel = pipeline.downloadable_artifacts
 
       if Feature.enabled?(:non_public_artifacts, type: :development)
-        rel = rel.select { |artifact| can?(request.current_user, :read_job_artifacts, artifact.job) }
+        rel = rel.select { |artifact| can?(request.current_user, :read_job_artifacts, artifact) }
       end
 
       BuildArtifactEntity.represent(rel, options.merge(project: pipeline.project))
diff --git a/app/services/ci/job_artifacts/create_service.rb b/app/services/ci/job_artifacts/create_service.rb
index ee9982cf3ab..6e2ba76682f 100644
--- a/app/services/ci/job_artifacts/create_service.rb
+++ b/app/services/ci/job_artifacts/create_service.rb
@@ -92,7 +92,8 @@ module Ci
             file: artifacts_file,
             file_type: params[:artifact_type],
             file_format: params[:artifact_format],
-            file_sha256: artifacts_file.sha256
+            file_sha256: artifacts_file.sha256,
+            accessibility: accessibility(params)
           )
         )
 
@@ -102,7 +103,8 @@ module Ci
                                   file: metadata_file,
                                   file_type: :metadata,
                                   file_format: :gzip,
-                                  file_sha256: metadata_file.sha256
+                                  file_sha256: metadata_file.sha256,
+                                  accessibility: accessibility(params)
                                 )
                               )
                             end
@@ -110,6 +112,10 @@ module Ci
         [artifact, artifact_metadata]
       end
 
+      def accessibility(params)
+        params[:accessibility] || 'public'
+      end
+
       def parse_artifact(artifact)
         case artifact.file_type
         when 'dotenv' then parse_dotenv_artifact(artifact)