Merge branch '57905-etag-caching-probably-broken-since-11-5-0' into 'master'

Fix ETag caching not being used for AJAX requests

Closes #57905

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

app/controllers/application_controller.rb

@@ -43,7 +43,10 @@ class ApplicationController < ActionController::Base
     :git_import_enabled?, :gitlab_project_import_enabled?,
     :manifest_import_enabled?
 
+  # Adds `no-store` to the DEFAULT_CACHE_CONTROL, to prevent security
+  # concerns due to caching private data.
   DEFAULT_GITLAB_CACHE_CONTROL = "#{ActionDispatch::Http::Cache::Response::DEFAULT_CACHE_CONTROL}, no-store".freeze
+  DEFAULT_GITLAB_CONTROL_NO_CACHE = "#{DEFAULT_GITLAB_CACHE_CONTROL}, no-cache".freeze
 
   rescue_from Encoding::CompatibilityError do |exception|
     log_exception(exception)
@@ -235,9 +238,9 @@ class ApplicationController < ActionController::Base
   end
 
   def no_cache_headers
-    response.headers["Cache-Control"] = "no-cache, no-store, max-age=0, must-revalidate"
-    response.headers["Pragma"] = "no-cache"
-    response.headers["Expires"] = "Fri, 01 Jan 1990 00:00:00 GMT"
+    headers['Cache-Control'] = DEFAULT_GITLAB_CONTROL_NO_CACHE
+    headers['Pragma'] = 'no-cache' # HTTP 1.0 compatibility
+    headers['Expires'] = 'Fri, 01 Jan 1990 00:00:00 GMT'
   end
 
   def default_headers
@@ -247,10 +250,16 @@ class ApplicationController < ActionController::Base
     headers['X-Content-Type-Options'] = 'nosniff'
 
     if current_user
-      # Adds `no-store` to the DEFAULT_CACHE_CONTROL, to prevent security
-      # concerns due to caching private data.
-      headers['Cache-Control'] = DEFAULT_GITLAB_CACHE_CONTROL
-      headers["Pragma"] = "no-cache" # HTTP 1.0 compatibility
+      headers['Cache-Control'] = default_cache_control
+      headers['Pragma'] = 'no-cache' # HTTP 1.0 compatibility
+    end
+  end
+
+  def default_cache_control
+    if request.xhr?
+      ActionDispatch::Http::Cache::Response::DEFAULT_CACHE_CONTROL
+    else
+      DEFAULT_GITLAB_CACHE_CONTROL
     end
   end
 

app/models/ci/pipeline.rb

@@ -652,9 +652,9 @@ module Ci
     def all_merge_requests
       @all_merge_requests ||=
         if merge_request?
-          project.merge_requests.where(id: merge_request_id)
+          MergeRequest.where(id: merge_request_id)
         else
-          project.merge_requests.where(source_branch: ref)
+          MergeRequest.where(source_project_id: project_id, source_branch: ref)
         end
     end
 

app/workers/expire_pipeline_cache_worker.rb

@@ -37,9 +37,9 @@ class ExpirePipelineCacheWorker
     Gitlab::Routing.url_helpers.project_new_merge_request_path(project, format: :json)
   end
 
-  def each_pipelines_merge_request_path(project, pipeline)
+  def each_pipelines_merge_request_path(pipeline)
     pipeline.all_merge_requests.each do |merge_request|
-      path = Gitlab::Routing.url_helpers.pipelines_project_merge_request_path(project, merge_request, format: :json)
+      path = Gitlab::Routing.url_helpers.pipelines_project_merge_request_path(merge_request.target_project, merge_request, format: :json)
 
       yield(path)
     end
@@ -59,7 +59,7 @@ class ExpirePipelineCacheWorker
     store.touch(project_pipeline_path(project, pipeline))
     store.touch(commit_pipelines_path(project, pipeline.commit)) unless pipeline.commit.nil?
     store.touch(new_merge_request_pipelines_path(project))
-    each_pipelines_merge_request_path(project, pipeline) do |path|
+    each_pipelines_merge_request_path(pipeline) do |path|
       store.touch(path)
     end
   end

changelogs/unreleased/57905-etag-caching-probably-broken-since-11-5-0.yml

+---
+title: Fix ETag caching not being used for AJAX requests
+merge_request: 25400
+author:
+type: fixed

spec/controllers/application_controller_spec.rb

@@ -665,6 +665,14 @@ describe ApplicationController do
 
         expect(response.headers['Cache-Control']).to eq 'max-age=0, private, must-revalidate, no-store'
       end
+
+      it 'does not set the "no-store" header for XHR requests' do
+        sign_in(user)
+
+        get :index, xhr: true
+
+        expect(response.headers['Cache-Control']).to eq 'max-age=0, private, must-revalidate'
+      end
     end
   end
 end

spec/models/ci/pipeline_spec.rb

 require 'spec_helper'
 
 describe Ci::Pipeline, :mailer do
+  include ProjectForksHelper
+
   let(:user) { create(:user) }
   set(:project) { create(:project) }
 
@@ -2114,66 +2116,81 @@ describe Ci::Pipeline, :mailer do
 
   describe "#all_merge_requests" do
     let(:project) { create(:project) }
-    let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master') }
 
-    it "returns all merge requests having the same source branch" do
-      merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref)
+    shared_examples 'a method that returns all merge requests for a given pipeline' do
+      let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: pipeline_project, ref: 'master') }
 
-      expect(pipeline.all_merge_requests).to eq([merge_request])
-    end
+      it "returns all merge requests having the same source branch" do
+        merge_request = create(:merge_request, source_project: pipeline_project, target_project: project, source_branch: pipeline.ref)
 
-    it "doesn't return merge requests having a different source branch" do
-      create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'master')
-
-      expect(pipeline.all_merge_requests).to be_empty
-    end
-
-    context 'when there is a merge request pipeline' do
-      let(:source_branch) { 'feature' }
-      let(:target_branch) { 'master' }
-
-      let!(:pipeline) do
-        create(:ci_pipeline,
-               source: :merge_request,
-               project: project,
-               ref: source_branch,
-               merge_request: merge_request)
+        expect(pipeline.all_merge_requests).to eq([merge_request])
       end
 
-      let(:merge_request) do
-        create(:merge_request,
-               source_project: project,
-               source_branch: source_branch,
-               target_project: project,
-               target_branch: target_branch)
-      end
+      it "doesn't return merge requests having a different source branch" do
+        create(:merge_request, source_project: pipeline_project, target_project: project, source_branch: 'feature', target_branch: 'master')
 
-      it 'returns an associated merge request' do
-        expect(pipeline.all_merge_requests).to eq([merge_request])
+        expect(pipeline.all_merge_requests).to be_empty
       end
 
-      context 'when there is another merge request pipeline that targets a different branch' do
-        let(:target_branch_2) { 'merge-test' }
+      context 'when there is a merge request pipeline' do
+        let(:source_branch) { 'feature' }
+        let(:target_branch) { 'master' }
 
-        let!(:pipeline_2) do
+        let!(:pipeline) do
           create(:ci_pipeline,
                  source: :merge_request,
-                 project: project,
+                 project: pipeline_project,
                  ref: source_branch,
-                 merge_request: merge_request_2)
+                 merge_request: merge_request)
         end
 
-        let(:merge_request_2) do
+        let(:merge_request) do
           create(:merge_request,
-                 source_project: project,
+                 source_project: pipeline_project,
                  source_branch: source_branch,
                  target_project: project,
-                 target_branch: target_branch_2)
+                 target_branch: target_branch)
         end
 
-        it 'does not return an associated merge request' do
-          expect(pipeline.all_merge_requests).not_to include(merge_request_2)
+        it 'returns an associated merge request' do
+          expect(pipeline.all_merge_requests).to eq([merge_request])
         end
+
+        context 'when there is another merge request pipeline that targets a different branch' do
+          let(:target_branch_2) { 'merge-test' }
+
+          let!(:pipeline_2) do
+            create(:ci_pipeline,
+                   source: :merge_request,
+                   project: pipeline_project,
+                   ref: source_branch,
+                   merge_request: merge_request_2)
+          end
+
+          let(:merge_request_2) do
+            create(:merge_request,
+                   source_project: pipeline_project,
+                   source_branch: source_branch,
+                   target_project: project,
+                   target_branch: target_branch_2)
+          end
+
+          it 'does not return an associated merge request' do
+            expect(pipeline.all_merge_requests).not_to include(merge_request_2)
+          end
+        end
+      end
+    end
+
+    it_behaves_like 'a method that returns all merge requests for a given pipeline' do
+      let(:pipeline_project) { project }
+    end
+
+    context 'for a fork' do
+      let(:fork) { fork_project(project) }
+
+      it_behaves_like 'a method that returns all merge requests for a given pipeline' do
+        let(:pipeline_project) { fork }
       end
     end
   end