Merge branch 'fix-diff-patch-public-mr' into 'master'

Fix downloading of patches on public merge requests when user logged out

### What does this MR do?

This MR makes it possible to download a diff patch on a public merge request when a user is logged out.

### Why was this MR needed?

An Error 500 would result when a user attempted to click on the "Email Patches" or "Plain Diff" button:

```
NoMethodError - undefined method `id' for nil:NilClass:
  lib/gitlab/backend/shell_env.rb:9:in `set_env'
  lib/gitlab/satellite/action.rb:20:in `in_locked_and_timed_satellite'
  lib/gitlab/satellite/merge_action.rb:49:in `diff_in_satellite'
  app/models/merge_request.rb:219:in `to_diff'
  app/controllers/projects/merge_requests_controller.rb:42:in `block (2 levels) in show'
```

### What are the relevant issue numbers?

* Closes #1225
* Closes #1854 (dup)
* Closes #1858 (dup)

See merge request !872

6 files changed, 47 insertions, 3 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 6ba09e952bf..f353a444fba 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 v 7.13.0 (unreleased)
   - Fix invalid timestamps in RSS feeds (Rowan Wookey)
   - Fix error when deleting a user who has projects (Stan Hu)
+  - Fix downloading of patches on public merge requests when user logged out (Stan Hu)
   - Update maintenance documentation to explain no need to recompile asssets for omnibus installations (Stan Hu)
   - Support commenting on diffs in side-by-side mode (Stan Hu)
   - Fix JavaScript error when clicking on the comment button on a diff line that has a comment already (Stan Hu)
diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature
index d043badbc46..35ffa9fc6e1 100644
--- a/features/project/merge_requests.feature
+++ b/features/project/merge_requests.feature
@@ -41,6 +41,18 @@ Feature: Project Merge Requests
     And I submit new merge request "Wiki Feature"
     Then I should see merge request "Wiki Feature"
 
+  Scenario: I download a diff on a public merge request
+    Given public project "Community"
+    And "John Doe" owns public project "Community"
+    And project "Community" has "Bug CO-01" open merge request with diffs inside
+    Given I logout directly
+    And I visit merge request page "Bug CO-01"
+    And I click on "Email Patches"
+    Then I should see a patch diff
+    And I visit merge request page "Bug CO-01"
+    And I click on "Plain Diff"
+    Then I should see a patch diff
+
   @javascript
   Scenario: I comment on a merge request
     Given I visit merge request page "Bug NS-04"
diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb
index 5684d661527..f11edb659d5 100644
--- a/features/steps/project/merge_requests.rb
+++ b/features/steps/project/merge_requests.rb
@@ -6,6 +6,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
   include SharedPaths
   include SharedMarkdown
   include SharedDiffNote
+  include SharedUser
 
   step 'I click link "New Merge Request"' do
     click_link "New Merge Request"
@@ -108,6 +109,15 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
            author: project.users.first)
   end
 
+  step 'project "Community" has "Bug CO-01" open merge request with diffs inside' do
+    project = Project.find_by(name: "Community")
+    create(:merge_request_with_diffs,
+           title: "Bug CO-01",
+           source_project: project,
+           target_project: project,
+           author: project.users.first)
+  end
+
   step 'I switch to the diff tab' do
     visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request)
   end
@@ -326,6 +336,18 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     expect(page).to have_content 'Target branch changed from master to feature'
   end
 
+  step 'I click on "Email Patches"' do
+    click_link "Email Patches"
+  end
+
+  step 'I click on "Plain Diff"' do
+    click_link "Plain Diff"
+  end
+
+  step 'I should see a patch diff' do
+    expect(page).to have_content('diff --git')
+  end
+
   def merge_request
     @merge_request ||= MergeRequest.find_by!(title: "Bug NS-05")
   end
diff --git a/features/steps/shared/paths.rb b/features/steps/shared/paths.rb
index 88ab9d0ab43..4cc01443c8b 100644
--- a/features/steps/shared/paths.rb
+++ b/features/steps/shared/paths.rb
@@ -357,6 +357,11 @@ module SharedPaths
     visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr)
   end
 
+  step 'I visit merge request page "Bug CO-01"' do
+    mr = MergeRequest.find_by(title: "Bug CO-01")
+    visit namespace_project_merge_request_path(mr.target_project.namespace, mr.target_project, mr)
+  end
+
   step 'I visit project "Shop" merge requests page' do
     visit namespace_project_merge_requests_path(project.namespace, project)
   end
diff --git a/lib/gitlab/backend/shell_env.rb b/lib/gitlab/backend/shell_env.rb
index 044afb27f3f..17ec029eed4 100644
--- a/lib/gitlab/backend/shell_env.rb
+++ b/lib/gitlab/backend/shell_env.rb
@@ -6,7 +6,9 @@ module Gitlab
 
     def set_env(user)
       # Set GL_ID env variable
-      ENV['GL_ID'] = "user-#{user.id}"
+      if user
+        ENV['GL_ID'] = "user-#{user.id}"
+      end
     end
 
     def reset_env
diff --git a/lib/gitlab/satellite/action.rb b/lib/gitlab/satellite/action.rb
index 4890ccf21e6..489070f1a3f 100644
--- a/lib/gitlab/satellite/action.rb
+++ b/lib/gitlab/satellite/action.rb
@@ -39,8 +39,10 @@ module Gitlab
       def prepare_satellite!(repo)
         project.satellite.clear_and_update!
 
-        repo.config['user.name'] = user.name
-        repo.config['user.email'] = user.email
+        if user
+          repo.config['user.name'] = user.name
+          repo.config['user.email'] = user.email
+        end
       end
 
       def default_options(options = {})