From 2fbbba9a2958d51f9a6d8e0a7c4e06ec95ae18c0 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 9 Nov 2017 15:40:41 +0000 Subject: Always return full avatar URL for private/internal groups/projects when asset host is set --- app/models/concerns/avatarable.rb | 25 +++++--- .../unreleased/dm-avatarable-with-asset-host.yml | 6 ++ spec/helpers/application_helper_spec.rb | 73 ++-------------------- spec/helpers/groups_helper_spec.rb | 32 +--------- spec/models/concerns/avatarable_spec.rb | 44 +++++++++++++ spec/models/group_spec.rb | 10 +-- spec/models/project_spec.rb | 12 +--- spec/models/user_spec.rb | 11 +--- 8 files changed, 82 insertions(+), 131 deletions(-) create mode 100644 changelogs/unreleased/dm-avatarable-with-asset-host.yml create mode 100644 spec/models/concerns/avatarable_spec.rb 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 "" - - 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 "" - 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 "" + .to eq "" 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 "" - - 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 "" + .to eq "" 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 -- cgit v1.2.1