Merge branch 'use_relative_path_for_project_avatars' into 'master'

Use relative paths for group/project/user avatars

Closes #13418 and #19662

See merge request !11001

17 files changed, 107 insertions, 44 deletions

diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 6d6bcbaf88a..97cf4863ddc 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -77,7 +77,7 @@ module ApplicationHelper
       end
 
     if user
-      user.avatar_url(size) || default_avatar
+      user.avatar_url(size: size) || default_avatar
     else
       gravatar_icon(user_or_email, size, scale)
     end
diff --git a/app/models/concerns/avatarable.rb b/app/models/concerns/avatarable.rb
new file mode 100644
index 00000000000..8fbfed11bdf
--- /dev/null
+++ b/app/models/concerns/avatarable.rb
@@ -0,0 +1,18 @@
+module Avatarable
+  extend ActiveSupport::Concern
+
+  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
+
+    # 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? ? asset_host : gitlab_host
+
+    [host, avatar.url].join
+  end
+end
diff --git a/app/models/group.rb b/app/models/group.rb
index cbc10b00cf5..6aab477f431 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -4,6 +4,7 @@ class Group < Namespace
   include Gitlab::ConfigHelper
   include Gitlab::VisibilityLevel
   include AccessRequestable
+  include Avatarable
   include Referable
   include SelectForProjectAuthorization
 
@@ -111,10 +112,10 @@ class Group < Namespace
     allowed_by_projects
   end
 
-  def avatar_url(size = nil)
-    if self[:avatar].present?
-      [gitlab_config.url, avatar.url].join
-    end
+  def avatar_url(**args)
+    # We use avatar_path instead of overriding avatar_url because of carrierwave.
+    # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
+    avatar_path(args)
   end
 
   def lfs_enabled?
diff --git a/app/models/project.rb b/app/models/project.rb
index a0413b4e651..1f550cc02e2 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -6,6 +6,7 @@ class Project < ActiveRecord::Base
   include Gitlab::VisibilityLevel
   include Gitlab::CurrentSettings
   include AccessRequestable
+  include Avatarable
   include CacheMarkdownField
   include Referable
   include Sortable
@@ -798,12 +799,10 @@ class Project < ActiveRecord::Base
     repository.avatar
   end
 
-  def avatar_url
-    if self[:avatar].present?
-      [gitlab_config.url, avatar.url].join
-    elsif avatar_in_git
-      Gitlab::Routing.url_helpers.namespace_project_avatar_url(namespace, self)
-    end
+  def avatar_url(**args)
+    # We use avatar_path instead of overriding avatar_url because of carrierwave.
+    # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
+    avatar_path(args) || (Gitlab::Routing.url_helpers.namespace_project_avatar_url(namespace, self) if avatar_in_git)
   end
 
   # For compatibility with old code
diff --git a/app/models/user.rb b/app/models/user.rb
index 77b2b12ee0b..f713a20233c 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -5,6 +5,7 @@ class User < ActiveRecord::Base
 
   include Gitlab::ConfigHelper
   include Gitlab::CurrentSettings
+  include Avatarable
   include Referable
   include Sortable
   include CaseSensitivity
@@ -784,12 +785,10 @@ class User < ActiveRecord::Base
     email.start_with?('temp-email-for-oauth')
   end
 
-  def avatar_url(size = nil, scale = 2)
-    if self[:avatar].present?
-      [gitlab_config.url, avatar.url].join
-    else
-      GravatarService.new.execute(email, size, scale)
-    end
+  def avatar_url(size: nil, scale: 2, **args)
+    # We use avatar_path instead of overriding avatar_url because of carrierwave.
+    # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
+    avatar_path(args) || GravatarService.new.execute(email, size, scale)
   end
 
   def all_emails
diff --git a/changelogs/unreleased/use_relative_path_for_project_avatars.yml b/changelogs/unreleased/use_relative_path_for_project_avatars.yml
new file mode 100644
index 00000000000..e3d0c0e1187
--- /dev/null
+++ b/changelogs/unreleased/use_relative_path_for_project_avatars.yml
@@ -0,0 +1,4 @@
+---
+title: Use relative paths for group/project/user avatars
+merge_request: 11001
+author: blackst0ne
diff --git a/config/initializers/doorkeeper_openid_connect.rb b/config/initializers/doorkeeper_openid_connect.rb
index 700ca25b884..4ff9019c43c 100644
--- a/config/initializers/doorkeeper_openid_connect.rb
+++ b/config/initializers/doorkeeper_openid_connect.rb
@@ -30,7 +30,7 @@ Doorkeeper::OpenidConnect.configure do
       o.claim(:email_verified) { |user| true if user.public_email? }
       o.claim(:website)        { |user| user.full_website_url if user.website_url? }
       o.claim(:profile)        { |user| Rails.application.routes.url_helpers.user_url user }
-      o.claim(:picture)        { |user| user.avatar_url }
+      o.claim(:picture)        { |user| user.avatar_url(only_path: false) }
     end
   end
 end
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index f8f5548d23d..00d494f02f5 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -5,7 +5,10 @@ module API
     end
 
     class UserBasic < UserSafe
-      expose :id, :state, :avatar_url
+      expose :id, :state
+      expose :avatar_url do |user, options|
+        user.avatar_url(only_path: false)
+      end
 
       expose :web_url do |user, options|
         Gitlab::Routing.url_helpers.user_url(user)
@@ -97,7 +100,9 @@ module API
       expose :creator_id
       expose :namespace, using: 'API::Entities::Namespace'
       expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda{ |project, options| project.forked? }
-      expose :avatar_url
+      expose :avatar_url do |user, options|
+        user.avatar_url(only_path: false)
+      end
       expose :star_count, :forks_count
       expose :open_issues_count, if: lambda { |project, options| project.feature_available?(:issues, options[:current_user]) && project.default_issues_tracker? }
       expose :runners_token, if: lambda { |_project, options| options[:user_can_admin_project] }
@@ -141,7 +146,9 @@ module API
     class Group < Grape::Entity
       expose :id, :name, :path, :description, :visibility
       expose :lfs_enabled?, as: :lfs_enabled
-      expose :avatar_url
+      expose :avatar_url do |user, options|
+        user.avatar_url(only_path: false)
+      end
       expose :web_url
       expose :request_access_enabled
       expose :full_name, :full_path
diff --git a/lib/api/v3/entities.rb b/lib/api/v3/entities.rb
index 7c8be7e51db..56a9b019f1b 100644
--- a/lib/api/v3/entities.rb
+++ b/lib/api/v3/entities.rb
@@ -69,7 +69,9 @@ module API
         expose :creator_id
         expose :namespace, using: 'API::Entities::Namespace'
         expose :forked_from_project, using: ::API::Entities::BasicProjectDetails, if: lambda{ |project, options| project.forked? }
-        expose :avatar_url
+        expose :avatar_url do |user, options|
+          user.avatar_url(only_path: false)
+        end
         expose :star_count, :forks_count
         expose :open_issues_count, if: lambda { |project, options| project.feature_available?(:issues, options[:current_user]) && project.default_issues_tracker? }
         expose :runners_token, if: lambda { |_project, options| options[:user_can_admin_project] }
@@ -129,7 +131,9 @@ module API
       class Group < Grape::Entity
         expose :id, :name, :path, :description, :visibility_level
         expose :lfs_enabled?, as: :lfs_enabled
-        expose :avatar_url
+        expose :avatar_url do |user, options|
+          user.avatar_url(only_path: false)
+        end
         expose :web_url
         expose :request_access_enabled
         expose :full_name, :full_path
diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb
index 01bdf01ad22..785fb724132 100644
--- a/spec/helpers/application_helper_spec.rb
+++ b/spec/helpers/application_helper_spec.rb
@@ -3,6 +3,8 @@ 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')
@@ -56,8 +58,14 @@ describe ApplicationHelper do
   describe 'project_icon' do
     it 'returns an url for the avatar' do
       project = create(:empty_project, avatar: File.open(uploaded_image_temp_path))
+      avatar_url = "/uploads/project/avatar/#{project.id}/banana_sample.gif"
+
+      expect(helper.project_icon(project.full_path).to_s).
+        to eq "<img src=\"#{avatar_url}\" alt=\"Banana sample\" />"
+
+      allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host)
+      avatar_url = "#{gitlab_host}/uploads/project/avatar/#{project.id}/banana_sample.gif"
 
-      avatar_url = "http://#{Gitlab.config.gitlab.host}/uploads/project/avatar/#{project.id}/banana_sample.gif"
       expect(helper.project_icon(project.full_path).to_s).
         to eq "<img src=\"#{avatar_url}\" alt=\"Banana sample\" />"
     end
@@ -67,9 +75,8 @@ describe ApplicationHelper do
 
       allow_any_instance_of(Project).to receive(:avatar_in_git).and_return(true)
 
-      avatar_url = "http://#{Gitlab.config.gitlab.host}#{namespace_project_avatar_path(project.namespace, project)}"
-      expect(helper.project_icon(project.full_path).to_s).to match(
-        image_tag(avatar_url))
+      avatar_url = "#{gitlab_host}#{namespace_project_avatar_path(project.namespace, project)}"
+      expect(helper.project_icon(project.full_path).to_s).to match(image_tag(avatar_url))
     end
   end
 
@@ -77,8 +84,14 @@ describe ApplicationHelper do
     it 'returns an url for the avatar' do
       user = create(:user, avatar: File.open(uploaded_image_temp_path))
 
-      expect(helper.avatar_icon(user.email).to_s).
-        to match("/uploads/user/avatar/#{user.id}/banana_sample.gif")
+      avatar_url = "/uploads/user/avatar/#{user.id}/banana_sample.gif"
+
+      expect(helper.avatar_icon(user.email).to_s).to match(avatar_url)
+
+      allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host)
+      avatar_url = "#{gitlab_host}/uploads/user/avatar/#{user.id}/banana_sample.gif"
+
+      expect(helper.avatar_icon(user.email).to_s).to match(avatar_url)
     end
 
     it 'returns an url for the avatar with relative url' do
diff --git a/spec/helpers/avatars_helper_spec.rb b/spec/helpers/avatars_helper_spec.rb
index 581726c1d0e..6157abfe339 100644
--- a/spec/helpers/avatars_helper_spec.rb
+++ b/spec/helpers/avatars_helper_spec.rb
@@ -15,7 +15,7 @@ describe AvatarsHelper do
     end
 
     it "contains the user's avatar image" do
-      is_expected.to include(CGI.escapeHTML(user.avatar_url(16)))
+      is_expected.to include(CGI.escapeHTML(user.avatar_url(size: 16)))
     end
   end
 end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 3d60e52f23f..6ca1eb0374d 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -178,16 +178,20 @@ describe Group, models: true do
   describe '#avatar_url' do
     let!(:group) { create(:group, :access_requestable, :with_avatar) }
     let(:user) { create(:user) }
-    subject { group.avatar_url }
+    let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
+    let(:avatar_path) { "/uploads/group/avatar/#{group.id}/dk.png" }
 
     context 'when avatar file is uploaded' do
-      before do
-        group.add_master(user)
-      end
+      before { group.add_master(user) }
 
-      let(:avatar_path) { "/uploads/group/avatar/#{group.id}/dk.png" }
+      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)
 
-      it { should eq "http://#{Gitlab.config.gitlab.host}#{avatar_path}" }
+        allow(ActionController::Base).to receive(:asset_host).and_return(gitlab_host)
+
+        expect(group.avatar_url).to eq([gitlab_host, avatar_path].join)
+      end
     end
   end
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 429b3dd83af..28aa44d8458 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -813,8 +813,16 @@ describe Project, models: true do
     context 'when avatar file is uploaded' do
       let(:project) { create(:empty_project, :with_avatar) }
       let(:avatar_path) { "/uploads/project/avatar/#{project.id}/dk.png" }
+      let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
 
-      it { should eq "http://#{Gitlab.config.gitlab.host}#{avatar_path}" }
+      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)
+      end
     end
 
     context 'When avatar file in git' do
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index c7ddd17872b..b845e85b295 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -964,12 +964,19 @@ describe User, models: true do
 
   describe '#avatar_url' do
     let(:user) { create(:user, :with_avatar) }
-    subject { user.avatar_url }
 
     context 'when avatar file is uploaded' do
+      let(:gitlab_host) { "http://#{Gitlab.config.gitlab.host}" }
       let(:avatar_path) { "/uploads/user/avatar/#{user.id}/dk.png" }
 
-      it { should eq "http://#{Gitlab.config.gitlab.host}#{avatar_path}" }
+      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)
+      end
     end
   end
 
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index 3e27a3bee77..ed93a8815d3 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/spec/requests/api/groups_spec.rb
@@ -178,7 +178,7 @@ describe API::Groups do
         expect(json_response['path']).to eq(group1.path)
         expect(json_response['description']).to eq(group1.description)
         expect(json_response['visibility']).to eq(Gitlab::VisibilityLevel.string_level(group1.visibility_level))
-        expect(json_response['avatar_url']).to eq(group1.avatar_url)
+        expect(json_response['avatar_url']).to eq(group1.avatar_url(only_path: false))
         expect(json_response['web_url']).to eq(group1.web_url)
         expect(json_response['request_access_enabled']).to eq(group1.request_access_enabled)
         expect(json_response['full_name']).to eq(group1.full_name)
diff --git a/spec/requests/api/v3/groups_spec.rb b/spec/requests/api/v3/groups_spec.rb
index 2862580cc70..065cb7ecfe4 100644
--- a/spec/requests/api/v3/groups_spec.rb
+++ b/spec/requests/api/v3/groups_spec.rb
@@ -176,7 +176,7 @@ describe API::V3::Groups do
         expect(json_response['path']).to eq(group1.path)
         expect(json_response['description']).to eq(group1.description)
         expect(json_response['visibility_level']).to eq(group1.visibility_level)
-        expect(json_response['avatar_url']).to eq(group1.avatar_url)
+        expect(json_response['avatar_url']).to eq(group1.avatar_url(only_path: false))
         expect(json_response['web_url']).to eq(group1.web_url)
         expect(json_response['request_access_enabled']).to eq(group1.request_access_enabled)
         expect(json_response['full_name']).to eq(group1.full_name)
diff --git a/spec/services/projects/participants_service_spec.rb b/spec/services/projects/participants_service_spec.rb
index 063b3bd76eb..0657b7e93fe 100644
--- a/spec/services/projects/participants_service_spec.rb
+++ b/spec/services/projects/participants_service_spec.rb
@@ -6,7 +6,6 @@ describe Projects::ParticipantsService, services: true do
       let(:project) { create(:empty_project, :public) }
       let(:group) { create(:group, avatar: fixture_file_upload(Rails.root + 'spec/fixtures/dk.png')) }
       let(:user) { create(:user) }
-      let(:base_url) { Settings.send(:build_base_gitlab_url) }
       let!(:group_member) { create(:group_member, group: group, user: user) }
 
       it 'should return an url for the avatar' do
@@ -14,7 +13,7 @@ describe Projects::ParticipantsService, services: true do
         groups = participants.groups
 
         expect(groups.size).to eq 1
-        expect(groups.first[:avatar_url]).to eq "#{base_url}/uploads/group/avatar/#{group.id}/dk.png"
+        expect(groups.first[:avatar_url]).to eq("/uploads/group/avatar/#{group.id}/dk.png")
       end
 
       it 'should return an url for the avatar with relative url' do
@@ -25,7 +24,7 @@ describe Projects::ParticipantsService, services: true do
         groups = participants.groups
 
         expect(groups.size).to eq 1
-        expect(groups.first[:avatar_url]).to eq "#{base_url}/gitlab/uploads/group/avatar/#{group.id}/dk.png"
+        expect(groups.first[:avatar_url]).to eq("/gitlab/uploads/group/avatar/#{group.id}/dk.png")
       end
     end
   end