From 29a3203b3fe08649b80def65b7750a866454d3d6 Mon Sep 17 00:00:00 2001 From: blackst0ne Date: Wed, 10 May 2017 15:26:17 +1100 Subject: Use relative paths for group/project/user avatars --- app/helpers/application_helper.rb | 2 +- app/models/concerns/avatarable.rb | 18 ++++++++++++++++ app/models/group.rb | 9 ++++---- app/models/project.rb | 11 +++++----- app/models/user.rb | 11 +++++----- .../use_relative_path_for_project_avatars.yml | 4 ++++ config/initializers/doorkeeper_openid_connect.rb | 2 +- lib/api/entities.rb | 13 ++++++++--- lib/api/v3/entities.rb | 8 +++++-- spec/helpers/application_helper_spec.rb | 25 ++++++++++++++++------ spec/helpers/avatars_helper_spec.rb | 2 +- spec/models/group_spec.rb | 16 ++++++++------ spec/models/project_spec.rb | 10 ++++++++- spec/models/user_spec.rb | 11 ++++++++-- spec/requests/api/groups_spec.rb | 2 +- spec/requests/api/v3/groups_spec.rb | 2 +- .../services/projects/participants_service_spec.rb | 5 ++--- 17 files changed, 107 insertions(+), 44 deletions(-) create mode 100644 app/models/concerns/avatarable.rb create mode 100644 changelogs/unreleased/use_relative_path_for_project_avatars.yml 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 "\"Banana" + + 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 "\"Banana" 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 -- cgit v1.2.1