Merge branch 'sh-use-system-path-for-appearance-logos' into 'master'

Use system paths for appearance logos

Closes gitlab-ee#6778

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

7 files changed, 70 insertions, 7 deletions

diff --git a/app/helpers/appearances_helper.rb b/app/helpers/appearances_helper.rb
index 3f69af50f25..473c90c882c 100644
--- a/app/helpers/appearances_helper.rb
+++ b/app/helpers/appearances_helper.rb
@@ -11,7 +11,7 @@ module AppearancesHelper
   end
 
   def brand_image
-    image_tag(current_appearance.logo) if current_appearance&.logo?
+    image_tag(current_appearance.logo_path) if current_appearance&.logo?
   end
 
   def brand_text
@@ -28,7 +28,7 @@ module AppearancesHelper
 
   def brand_header_logo
     if current_appearance&.header_logo?
-      image_tag current_appearance.header_logo
+      image_tag current_appearance.header_logo_path
     else
       render 'shared/logo.svg'
     end
diff --git a/app/helpers/emails_helper.rb b/app/helpers/emails_helper.rb
index e4c46ceeaa2..fa5d3ae474a 100644
--- a/app/helpers/emails_helper.rb
+++ b/app/helpers/emails_helper.rb
@@ -58,7 +58,7 @@ module EmailsHelper
   def header_logo
     if current_appearance&.header_logo?
       image_tag(
-        current_appearance.header_logo,
+        current_appearance.header_logo_path,
         style: 'height: 50px'
       )
     else
diff --git a/app/models/appearance.rb b/app/models/appearance.rb
index bffba3e13fa..e114c435b67 100644
--- a/app/models/appearance.rb
+++ b/app/models/appearance.rb
@@ -28,4 +28,32 @@ class Appearance < ActiveRecord::Base
       errors.add(:single_appearance_row, 'Only 1 appearances row can exist')
     end
   end
+
+  def logo_path
+    logo_system_path(logo, 'logo')
+  end
+
+  def header_logo_path
+    logo_system_path(header_logo, 'header_logo')
+  end
+
+  def favicon_path
+    logo_system_path(favicon, 'favicon')
+  end
+
+  private
+
+  def logo_system_path(logo, mount_type)
+    return unless logo&.upload
+
+    # If we're using a CDN, we need to use the full URL
+    asset_host = ActionController::Base.asset_host
+    local_path = Gitlab::Routing.url_helpers.appearance_upload_path(
+      filename: logo.filename,
+      id: logo.upload.model_id,
+      model: 'appearance',
+      mounted_as: mount_type)
+
+    Gitlab::Utils.append_path(asset_host, local_path)
+  end
 end
diff --git a/app/views/admin/appearances/_form.html.haml b/app/views/admin/appearances/_form.html.haml
index cb67079853e..544f09048f5 100644
--- a/app/views/admin/appearances/_form.html.haml
+++ b/app/views/admin/appearances/_form.html.haml
@@ -8,7 +8,7 @@
     = f.label :header_logo, 'Header logo', class: 'col-sm-2 col-form-label pt-0'
     .col-sm-10
       - if @appearance.header_logo?
-        = image_tag @appearance.header_logo_url, class: 'appearance-light-logo-preview'
+        = image_tag @appearance.header_logo_path, class: 'appearance-light-logo-preview'
         - if @appearance.persisted?
           %br
           = link_to 'Remove header logo', header_logos_admin_appearances_path, data: { confirm: "Header logo will be removed. Are you sure?"}, method: :delete, class: "btn btn-inverted btn-remove btn-sm remove-logo"
@@ -25,7 +25,7 @@
     = f.label :favicon, 'Favicon', class: 'col-sm-2 col-form-label pt-0'
     .col-sm-10
       - if @appearance.favicon?
-        = image_tag @appearance.favicon_url, class: 'appearance-light-logo-preview'
+        = image_tag @appearance.favicon_path, class: 'appearance-light-logo-preview'
         - if @appearance.persisted?
           %br
           = link_to 'Remove favicon', favicon_admin_appearances_path, data: { confirm: "Favicon will be removed. Are you sure?"}, method: :delete, class: "btn btn-inverted btn-remove btn-sm remove-logo"
@@ -54,7 +54,7 @@
     = f.label :logo, class: 'col-sm-2 col-form-label pt-0'
     .col-sm-10
       - if @appearance.logo?
-        = image_tag @appearance.logo_url, class: 'appearance-logo-preview'
+        = image_tag @appearance.logo_path, class: 'appearance-logo-preview'
         - if @appearance.persisted?
           %br
           = link_to 'Remove logo', logo_admin_appearances_path, data: { confirm: "Logo will be removed. Are you sure?"}, method: :delete, class: "btn btn-inverted btn-remove btn-sm remove-logo"
diff --git a/config/routes/uploads.rb b/config/routes/uploads.rb
index 6becadd57ae..b594f55f8a0 100644
--- a/config/routes/uploads.rb
+++ b/config/routes/uploads.rb
@@ -17,7 +17,8 @@ scope path: :uploads do
   # Appearance
   get "-/system/:model/:mounted_as/:id/:filename",
       to:           "uploads#show",
-      constraints:  { model: /appearance/, mounted_as: /logo|header_logo|favicon/, filename: /.+/ }
+      constraints:  { model: /appearance/, mounted_as: /logo|header_logo|favicon/, filename: /.+/ },
+      as: 'appearance_upload'
 
   # Project markdown uploads
   get ":namespace_id/:project_id/:secret/:filename",
diff --git a/spec/factories/appearances.rb b/spec/factories/appearances.rb
index 18c7453bd1b..dd5129229d4 100644
--- a/spec/factories/appearances.rb
+++ b/spec/factories/appearances.rb
@@ -15,6 +15,10 @@ FactoryBot.define do
     header_logo { fixture_file_upload('spec/fixtures/dk.png') }
   end
 
+  trait :with_favicon do
+    favicon { fixture_file_upload('spec/fixtures/dk.png') }
+  end
+
   trait :with_logos do
     with_logo
     with_header_logo
diff --git a/spec/models/appearance_spec.rb b/spec/models/appearance_spec.rb
index 35415030154..ec2e7d672f0 100644
--- a/spec/models/appearance_spec.rb
+++ b/spec/models/appearance_spec.rb
@@ -26,4 +26,34 @@ describe Appearance do
       let(:uploader_class) { AttachmentUploader }
     end
   end
+
+  shared_examples 'logo paths' do |logo_type|
+    let(:appearance) { create(:appearance, "with_#{logo_type}".to_sym) }
+    let(:filename) { 'dk.png' }
+    let(:expected_path) { "/uploads/-/system/appearance/#{logo_type}/#{appearance.id}/#{filename}" }
+
+    it 'returns nil when there is no upload' do
+      expect(subject.send("#{logo_type}_path")).to be_nil
+    end
+
+    it 'returns a local path using the system route' do
+      expect(appearance.send("#{logo_type}_path")).to eq(expected_path)
+    end
+
+    describe 'with asset host configured' do
+      let(:asset_host) { 'https://gitlab-assets.example.com' }
+
+      before do
+        allow(ActionController::Base).to receive(:asset_host) { asset_host }
+      end
+
+      it 'returns a full URL with the system path' do
+        expect(appearance.send("#{logo_type}_path")).to eq("#{asset_host}#{expected_path}")
+      end
+    end
+  end
+
+  %i(logo header_logo favicon).each do |logo_type|
+    it_behaves_like 'logo paths', logo_type
+  end
 end