Always return full avatar URL for private/internal groups/projects when asset host is set

8 files changed, 82 insertions, 131 deletions

diff --git a/app/models/concerns/avatarable.rb b/app/models/concerns/avatarable.rb
index 2ec70203710..10659030910 100644
--- a/app/models/concerns/avatarable.rb
+++ b/app/models/concerns/avatarable.rb
@@ -4,15 +4,26 @@ module Avatarable
   def avatar_path(only_path: true)
     return unless self[:avatar].present?
 
-    # If only_path is true then use the relative path of avatar.
-    # Otherwise use full path (including host).
     asset_host = ActionController::Base.asset_host
-    gitlab_host = only_path ? gitlab_config.relative_url_root : gitlab_config.url
+    use_asset_host = asset_host.present?
 
-    # If asset_host is set then it is expected that assets are handled by a standalone host.
-    # That means we do not want to get GitLab's relative_url_root option anymore.
-    host = (asset_host.present? && (!respond_to?(:public?) || public?)) ? asset_host : gitlab_host
+    # Avatars for private and internal groups and projects require authentication to be viewed,
+    # which means they can only be served by Rails, on the regular GitLab host.
+    # If an asset host is configured, we need to return the fully qualified URL
+    # instead of only the avatar path, so that Rails doesn't prefix it with the asset host.
+    if use_asset_host && respond_to?(:public?) && !public?
+      use_asset_host = false
+      only_path = false
+    end
 
-    [host, avatar.url].join
+    url_base = ""
+    if use_asset_host
+      url_base << asset_host unless only_path
+    else
+      url_base << gitlab_config.base_url unless only_path
+      url_base << gitlab_config.relative_url_root
+    end
+
+    url_base + avatar.url
   end
 end
diff --git a/changelogs/unreleased/dm-avatarable-with-asset-host.yml b/changelogs/unreleased/dm-avatarable-with-asset-host.yml
new file mode 100644
index 00000000000..6cf8d719afb
--- /dev/null
+++ b/changelogs/unreleased/dm-avatarable-with-asset-host.yml
@@ -0,0 +1,6 @@
+---
+title: Always return full avatar URL for private/internal groups/projects when asset
+  host is set
+merge_request:
+author:
+type: fixed
diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb
index 7a241b02d28..5c5d53877a6 100644
--- a/spec/helpers/application_helper_spec.rb
+++ b/spec/helpers/application_helper_spec.rb
@@ -4,8 +4,6 @@ require 'spec_helper'
 describe ApplicationHelper do
   include UploadHelpers
 
-  let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
-
   describe 'current_controller?' do
     it 'returns true when controller matches argument' do
       stub_controller_name('foo')
@@ -57,30 +55,11 @@ describe ApplicationHelper do
   end
 
   describe 'project_icon' do
-    let(:asset_host) { 'http://assets' }
-
     it 'returns an url for the avatar' do
       project = create(:project, :public, avatar: File.open(uploaded_image_temp_path))
-      avatar_url = "/uploads/-/system/project/avatar/#{project.id}/banana_sample.gif"
-
-      expect(helper.project_icon(project.full_path).to_s)
-        .to eq "<img data-src=\"#{avatar_url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />"
-
-      allow(ActionController::Base).to receive(:asset_host).and_return(asset_host)
-      avatar_url = "#{asset_host}/uploads/-/system/project/avatar/#{project.id}/banana_sample.gif"
-
-      expect(helper.project_icon(project.full_path).to_s)
-        .to eq "<img data-src=\"#{avatar_url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />"
-    end
-
-    it 'gives uploaded icon when present' do
-      project = create(:project)
 
-      allow_any_instance_of(Project).to receive(:avatar_in_git).and_return(true)
-
-      avatar_url = "#{gitlab_host}#{project_avatar_path(project)}"
       expect(helper.project_icon(project.full_path).to_s)
-        .to eq "<img data-src=\"#{avatar_url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />"
+        .to eq "<img data-src=\"#{project.avatar.url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />"
     end
   end
 
@@ -91,40 +70,7 @@ describe ApplicationHelper do
       context 'when there is a matching user' do
         it 'returns a relative URL for the avatar' do
           expect(helper.avatar_icon(user.email).to_s)
-            .to eq("/uploads/-/system/user/avatar/#{user.id}/banana_sample.gif")
-        end
-
-        context 'when an asset_host is set in the config' do
-          let(:asset_host) { 'http://assets' }
-
-          before do
-            allow(ActionController::Base).to receive(:asset_host).and_return(asset_host)
-          end
-
-          it 'returns an absolute URL on that asset host' do
-            expect(helper.avatar_icon(user.email, only_path: false).to_s)
-              .to eq("#{asset_host}/uploads/-/system/user/avatar/#{user.id}/banana_sample.gif")
-          end
-        end
-
-        context 'when only_path is set to false' do
-          it 'returns an absolute URL for the avatar' do
-            expect(helper.avatar_icon(user.email, only_path: false).to_s)
-              .to eq("#{gitlab_host}/uploads/-/system/user/avatar/#{user.id}/banana_sample.gif")
-          end
-        end
-
-        context 'when the GitLab instance is at a relative URL' do
-          before do
-            stub_config_setting(relative_url_root: '/gitlab')
-            # Must be stubbed after the stub above, and separately
-            stub_config_setting(url: Settings.send(:build_gitlab_url))
-          end
-
-          it 'returns a relative URL with the correct prefix' do
-            expect(helper.avatar_icon(user.email).to_s)
-              .to eq("/gitlab/uploads/-/system/user/avatar/#{user.id}/banana_sample.gif")
-          end
+            .to eq(user.avatar.url)
         end
       end
 
@@ -138,18 +84,9 @@ describe ApplicationHelper do
     end
 
     describe 'using a user' do
-      context 'when only_path is true' do
-        it 'returns a relative URL for the avatar' do
-          expect(helper.avatar_icon(user, only_path: true).to_s)
-            .to eq("/uploads/-/system/user/avatar/#{user.id}/banana_sample.gif")
-        end
-      end
-
-      context 'when only_path is false' do
-        it 'returns an absolute URL for the avatar' do
-          expect(helper.avatar_icon(user, only_path: false).to_s)
-            .to eq("#{gitlab_host}/uploads/-/system/user/avatar/#{user.id}/banana_sample.gif")
-        end
+      it 'returns a relative URL for the avatar' do
+        expect(helper.avatar_icon(user).to_s)
+          .to eq(user.avatar.url)
       end
     end
   end
diff --git a/spec/helpers/groups_helper_spec.rb b/spec/helpers/groups_helper_spec.rb
index 97f0ed4904e..32432ee1e81 100644
--- a/spec/helpers/groups_helper_spec.rb
+++ b/spec/helpers/groups_helper_spec.rb
@@ -3,8 +3,6 @@ require 'spec_helper'
 describe GroupsHelper do
   include ApplicationHelper
 
-  let(:asset_host) { 'http://assets' }
-
   describe 'group_icon' do
     avatar_file_path = File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif')
 
@@ -13,16 +11,8 @@ describe GroupsHelper do
       group.avatar = fixture_file_upload(avatar_file_path)
       group.save!
 
-      avatar_url = "/uploads/-/system/group/avatar/#{group.id}/banana_sample.gif"
-
-      expect(helper.group_icon(group).to_s)
-        .to eq "<img data-src=\"#{avatar_url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />"
-
-      allow(ActionController::Base).to receive(:asset_host).and_return(asset_host)
-      avatar_url = "#{asset_host}/uploads/-/system/group/avatar/#{group.id}/banana_sample.gif"
-
       expect(helper.group_icon(group).to_s)
-        .to eq "<img data-src=\"#{avatar_url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />"
+        .to eq "<img data-src=\"#{group.avatar.url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />"
     end
   end
 
@@ -34,25 +24,7 @@ describe GroupsHelper do
       group.avatar = fixture_file_upload(avatar_file_path)
       group.save!
       expect(group_icon_url(group.path).to_s)
-        .to match("/uploads/-/system/group/avatar/#{group.id}/banana_sample.gif")
-    end
-
-    it 'returns an CDN url for the avatar' do
-      allow(ActionController::Base).to receive(:asset_host).and_return(asset_host)
-      group = create(:group)
-      group.avatar = fixture_file_upload(avatar_file_path)
-      group.save!
-      expect(group_icon_url(group.path).to_s)
-        .to match("#{asset_host}/uploads/-/system/group/avatar/#{group.id}/banana_sample.gif")
-    end
-
-    it 'returns an based url for the avatar if private' do
-      allow(ActionController::Base).to receive(:asset_host).and_return(asset_host)
-      group = create(:group, :private)
-      group.avatar = fixture_file_upload(avatar_file_path)
-      group.save!
-      expect(group_icon_url(group.path).to_s)
-        .to match("/uploads/-/system/group/avatar/#{group.id}/banana_sample.gif")
+        .to match(group.avatar.url)
     end
 
     it 'gives default avatar_icon when no avatar is present' do
diff --git a/spec/models/concerns/avatarable_spec.rb b/spec/models/concerns/avatarable_spec.rb
new file mode 100644
index 00000000000..cbdc438be0b
--- /dev/null
+++ b/spec/models/concerns/avatarable_spec.rb
@@ -0,0 +1,44 @@
+require 'spec_helper'
+
+describe Avatarable do
+  subject { create(:project, avatar: fixture_file_upload(File.join(Rails.root, 'spec/fixtures/dk.png'))) }
+
+  let(:gitlab_host) { "https://gitlab.example.com" }
+  let(:relative_url_root) { "/gitlab" }
+  let(:asset_host) { "https://gitlab-assets.example.com" }
+
+  before do
+    stub_config_setting(base_url: gitlab_host)
+    stub_config_setting(relative_url_root: relative_url_root)
+  end
+
+  describe '#avatar_path' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:has_asset_host, :visibility_level, :only_path, :avatar_path) do
+      true  | Project::PRIVATE  | true  | [gitlab_host, relative_url_root, subject.avatar.url]
+      true  | Project::PRIVATE  | false | [gitlab_host, relative_url_root, subject.avatar.url]
+      true  | Project::INTERNAL | true  | [gitlab_host, relative_url_root, subject.avatar.url]
+      true  | Project::INTERNAL | false | [gitlab_host, relative_url_root, subject.avatar.url]
+      true  | Project::PUBLIC   | true  | [subject.avatar.url]
+      true  | Project::PUBLIC   | false | [asset_host, subject.avatar.url]
+      false | Project::PRIVATE  | true  | [relative_url_root, subject.avatar.url]
+      false | Project::PRIVATE  | false | [gitlab_host, relative_url_root, subject.avatar.url]
+      false | Project::INTERNAL | true  | [relative_url_root, subject.avatar.url]
+      false | Project::INTERNAL | false | [gitlab_host, relative_url_root, subject.avatar.url]
+      false | Project::PUBLIC   | true  | [relative_url_root, subject.avatar.url]
+      false | Project::PUBLIC   | false | [gitlab_host, relative_url_root, subject.avatar.url]
+    end
+
+    with_them do
+      before do
+        allow(ActionController::Base).to receive(:asset_host).and_return(has_asset_host ? asset_host : nil)
+        subject.visibility_level = visibility_level
+      end
+
+      it 'returns the expected avatar path' do
+        expect(subject.avatar_path(only_path: only_path)).to eq(avatar_path.join)
+      end
+    end
+  end
+end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index d4052a64570..5e82a2988ce 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -247,8 +247,6 @@ describe Group do
   describe '#avatar_url' do
     let!(:group) { create(:group, :access_requestable, :with_avatar) }
     let(:user) { create(:user) }
-    let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
-    let(:avatar_path) { "/uploads/-/system/group/avatar/#{group.id}/dk.png" }
 
     context 'when avatar file is uploaded' do
       before do
@@ -256,12 +254,8 @@ describe Group do
       end
 
       it 'shows correct avatar url' do
-        expect(group.avatar_url).to eq(avatar_path)
-        expect(group.avatar_url(only_path: false)).to eq([gitlab_host, avatar_path].join)
-
-        allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host)
-
-        expect(group.avatar_url).to eq([gitlab_host, avatar_path].join)
+        expect(group.avatar_url).to eq(group.avatar.url)
+        expect(group.avatar_url(only_path: false)).to eq([Gitlab.config.gitlab.url, group.avatar.url].join)
       end
     end
   end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index e684389af5d..4db417c8793 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -883,20 +883,14 @@ describe Project do
 
     context 'when avatar file is uploaded' do
       let(:project) { create(:project, :public, :with_avatar) }
-      let(:avatar_path) { "/uploads/-/system/project/avatar/#{project.id}/dk.png" }
-      let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
 
       it 'shows correct url' do
-        expect(project.avatar_url).to eq(avatar_path)
-        expect(project.avatar_url(only_path: false)).to eq([gitlab_host, avatar_path].join)
-
-        allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host)
-
-        expect(project.avatar_url).to eq([gitlab_host, avatar_path].join)
+        expect(project.avatar_url).to eq(project.avatar.url)
+        expect(project.avatar_url(only_path: false)).to eq([Gitlab.config.gitlab.url, project.avatar.url].join)
       end
     end
 
-    context 'When avatar file in git' do
+    context 'when avatar file in git' do
       before do
         allow(project).to receive(:avatar_in_git) { true }
       end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index a3ebf649aa8..65dccf9638d 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1149,16 +1149,9 @@ describe User do
     let(:user) { create(:user, :with_avatar) }
 
     context 'when avatar file is uploaded' do
-      let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
-      let(:avatar_path) { "/uploads/-/system/user/avatar/#{user.id}/dk.png" }
-
       it 'shows correct avatar url' do
-        expect(user.avatar_url).to eq(avatar_path)
-        expect(user.avatar_url(only_path: false)).to eq([gitlab_host, avatar_path].join)
-
-        allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host)
-
-        expect(user.avatar_url).to eq([gitlab_host, avatar_path].join)
+        expect(user.avatar_url).to eq(user.avatar.url)
+        expect(user.avatar_url(only_path: false)).to eq([Gitlab.config.gitlab.url, user.avatar.url].join)
       end
     end
   end