summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2017-11-09 15:40:41 +0000
committerWinnie Hellmann <winnie@gitlab.com>2017-11-17 11:55:21 +0000
commitabb6de2245d2aba9e0242ca3709a04f648a099e8 (patch)
tree671651a550391842ecfa9215d16b5e4319a62eab
parent24290d5da5c4a7abb8d7130266e594566f7c0654 (diff)
downloadgitlab-ce-abb6de2245d2aba9e0242ca3709a04f648a099e8.tar.gz
Merge branch 'dm-avatarable-with-asset-host' into 'master'
Always return full avatar URL for private/internal groups/projects when asset host is set See merge request gitlab-org/gitlab-ce!15288 (cherry picked from commit f24a78f2801608be7c3b9d3a0dad90656880f097) 0cf25867 Always return full avatar URL for private/internal groups/projects when asset host is set d4babd26 Remove specs that are testing behavior already tested in Avatarable unit tests
-rw-r--r--app/models/concerns/avatarable.rb25
-rw-r--r--changelogs/unreleased/dm-avatarable-with-asset-host.yml6
-rw-r--r--spec/helpers/application_helper_spec.rb73
-rw-r--r--spec/helpers/groups_helper_spec.rb32
-rw-r--r--spec/models/concerns/avatarable_spec.rb44
-rw-r--r--spec/models/group_spec.rb10
-rw-r--r--spec/models/project_spec.rb12
-rw-r--r--spec/models/user_spec.rb11
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 0f706732a9f..f7f19d464d1 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 34f3fb3248f..3b5ee70ae7c 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1155,16 +1155,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