Proxy user avatar images from gravatar (#4599)

Author: segiddins

app/controllers/avatars_controller.rb

@@ -0,0 +1,67 @@
+class AvatarsController < ApplicationController
+  before_action :find_user
+  before_action :set_size
+  before_action :set_theme
+
+  def show
+    gravatar_url = @user.gravatar_url(size: @size, default: "404", secure: true)
+
+    resp = gravatar_client
+      .get(gravatar_url, nil,
+           { "Accept" => "image/png", "Connection" => "close", "User-Agent" => "RubyGems.org avatar proxy" })
+
+    if resp.success?
+      fastly_expires_in(5.minutes)
+
+      # don't copy other headers, since they might leak user info
+      response.headers["last-modified"] = resp.headers["last-modified"] if resp.headers["last-modified"]
+      filename = "#{@user.display_id}_avatar_#{@size}.#{params[:format]}"
+      send_data(resp.body, type: resp.headers["content-type"], disposition: "inline", filename:)
+    elsif resp.status == 404
+      fastly_expires_in(5.minutes)
+
+      # means gravatar doesn't have an avatar for this user
+      # we'll just redirect to our default avatar instead, so everything is cachable
+      redirect_to default_avatar_url
+    else
+      # any other error, just redirect to our default avatar
+      # this includes 400, 429, 500s, etc
+      logger.warn(message: "Failed to fetch gravatar", status: resp.status, url: gravatar_url, user_id: @user.id)
+      redirect_to default_avatar_url
+    end
+  end
+
+  private
+
+  def find_user
+    @user = User.find_by_slug(params[:id])
+    return if @user
+    render_not_found
+  end
+
+  def set_size
+    @size = params.permit(:size).fetch(:size, 64).to_i
+    return unless @size < 1 || @size > 2048
+    render plain: "Invalid size", status: :bad_request
+  end
+
+  def set_theme
+    @theme = params.permit(:theme).fetch(:theme, "light")
+    return if %w[light dark].include?(@theme)
+    render plain: "Invalid theme", status: :bad_request
+  end
+
+  def default_avatar_url
+    case @theme
+    when "light" then "/images/avatar.svg"
+    when "dark" then "/images/avatar_inverted.svg"
+    else raise "invalid default avatar theme, only light and dark are suported"
+    end
+  end
+
+  def gravatar_client
+    Faraday.new(nil, request: { timeout: 2 }) do |f|
+      f.response :logger, logger, headers: false, errors: true
+    end
+  end
+end

app/helpers/application_helper.rb

@@ -35,7 +35,9 @@ def gem_info(rubygem)
   end
 
   def avatar(size, id = "gravatar", user = current_user, theme: :light, **)
-    url = user.gravatar_url(size: size, secure: true) || default_avatar(theme: theme)
+    raise ArgumentError, "invalid default avatar theme, only light and dark are suported" unless %i[light dark].include? theme
+
+    url = avatar_user_path(user.id, params: { size: size, theme: theme })
     image_tag(url,
       id: id,
       width: size,
@@ -98,14 +100,4 @@ def search_field(home: false)
       data: data
     )
   end
-
-  private
-
-  def default_avatar(theme:)
-    case theme
-    when :light then "/images/avatar.svg"
-    when :dark then "/images/avatar_inverted.svg"
-    else raise "invalid default avatar theme, only light and dark are suported"
-    end
-  end
 end

app/models/user.rb

@@ -145,10 +145,6 @@ def self.security_user
     find_by!(email: "security@rubygems.org")
   end
 
-  def gravatar_url(*)
-    public_email ? super : nil
-  end
-
   def name
     handle || email
   end

config/routes.rb

@@ -280,6 +280,15 @@
     end
   end
 
+  ################################################################################
+  # UI Images
+
+  scope constraints: { format: /jpe?g/ }, defaults: { format: :jpeg } do
+    resources :users, only: [] do
+      get 'avatar', on: :member, to: 'avatars#show', format: true
+    end
+  end
+
   ################################################################################
   # high_voltage static routes
   get 'pages/*id' => 'high_voltage/pages#show', constraints: { id: Regexp.union(HighVoltage.page_ids) }, as: :page

test/integration/avatars_test.rb

@@ -0,0 +1,77 @@
+require "test_helper"
+
+class AvatarsTest < ActionDispatch::IntegrationTest
+  test "returns 404 when no user is found" do
+    get avatar_user_path("user", size: 64)
+
+    assert_response :not_found
+  end
+
+  test "redirects to default avatar when gravatar returns 404" do
+    stub_request(:get, Addressable::Template.new("https://secure.gravatar.com/avatar/{hash}.png?d=404&r=PG&s={size}"))
+      .to_return(status: 404)
+
+    user = create(:user)
+    get avatar_user_path(user.id, size: 64)
+
+    assert_response :found
+    assert_equal "http://localhost/images/avatar.svg", response.headers["Location"]
+  end
+
+  test "serves gravatar response on 200" do
+    stub_request(:get, Addressable::Template.new("https://secure.gravatar.com/avatar/{hash}.png?d=404&r=PG&s=64"))
+      .to_return(status: 200, body: "image", headers: {
+                   "Content-Type" => "image/jpeg",
+                   "Last-Modified" => "Wed, 21 Oct 2015 07:28:00 GMT",
+                   "Link" => "foo"
+                 })
+
+    user = create(:user)
+    get avatar_user_path(user.id, size: 64)
+
+    assert_response :success
+    assert_equal "image/jpeg", response.headers["Content-Type"]
+    assert_equal "image", response.body
+    assert_nil response.headers["Link"]
+  end
+
+  test "serves default avatar with theme when user has no gravatar" do
+    user = create(:user)
+    get avatar_user_path(user.id, size: 64, theme: "dark")
+
+    assert_response :found
+    assert_equal "http://localhost/images/avatar_inverted.svg", response.headers["Location"]
+  end
+
+  test "falls back to default avatar when gravatar returns 500" do
+    stub_request(:get, Addressable::Template.new("https://secure.gravatar.com/avatar/{hash}.png?d=404&r=PG&s=64"))
+      .to_return(status: 500)
+
+    user = create(:user)
+    get avatar_user_path(user.id, size: 64)
+
+    assert_response :found
+    assert_equal "http://localhost/images/avatar.svg", response.headers["Location"]
+  end
+
+  test "returns 400 when size is invalid" do
+    user = create(:user)
+    get avatar_user_path(user.id, size: 0)
+
+    assert_response :bad_request
+    assert_equal "Invalid size", response.body
+
+    get avatar_user_path(user.id, size: 2049)
+
+    assert_response :bad_request
+    assert_equal "Invalid size", response.body
+  end
+
+  test "returns 400 when theme is invalid" do
+    user = create(:user)
+    get avatar_user_path(user.id, size: 64, theme: "unknown")
+
+    assert_response :bad_request
+    assert_equal "Invalid theme", response.body
+  end
+end

test/integration/routing_test.rb

@@ -2,7 +2,7 @@
 
 class RoutingTest < ActionDispatch::IntegrationTest
   def contoller_in_ui?(controller)
-    !controller.nil? && controller !~ %r{^api|internal|sendgrid_events.*|view_components(_system_test)?|turbo|admin/admin$}
+    !controller.nil? && controller !~ %r{^api|internal|sendgrid_events.*|view_components(_system_test)?|turbo|admin/admin|avatars$}
   end
 
   setup do
@@ -22,7 +22,7 @@ def contoller_in_ui?(controller)
     @ui_paths_verb.each do |path, verb|
       next if path == "/" # adding random format after root (/) gives 404
 
-      assert_raises(ActionController::RoutingError) do
+      assert_raises(ActionController::RoutingError, "#{verb} #{path} should raise") do
         # ex: get(/password/new.json)
         send(verb.downcase, path.gsub("(.:format)", ".something"))
       end

test/models/user_test.rb

@@ -877,14 +877,4 @@ class UserTest < ActiveSupport::TestCase
       assert_equal "", User.normalize_email("\u9999".force_encoding("ascii"))
     end
   end
-
-  context "#gravatar_url" do
-    should "return gravatar if email is publicly visible" do
-      assert_includes User.new(public_email: true, email: "text@example.com").gravatar_url, "gravatar.com"
-    end
-
-    should "return nil if email is publicly hidden" do
-      assert_nil User.new(public_email: false, email: "text@example.com").gravatar_url
-    end
-  end
 end

test/test_helper.rb

@@ -40,12 +40,16 @@
     "chromedriver.storage.googleapis.com"
   ]
 )
-WebMock.globally_stub_request do |request|
+WebMock.globally_stub_request(:after_local_stubs) do |request|
   avo_request_pattern = WebMock::RequestPattern.new(:post, "https://avohq.io/api/v1/licenses/check")
   if avo_request_pattern.matches?(request)
     { status: 200, body: { id: :pro, valid: true, payload: {} }.to_json,
       headers: { "Content-Type" => "application/json" } }
   end
+
+  if WebMock::RequestPattern.new(:get, Addressable::Template.new("https://secure.gravatar.com/avatar/{hash}.png?d=404&r=PG&s={size}")).matches?(request)
+    { status: 404, body: "", headers: {} }
+  end
 end
 
 Capybara.default_max_wait_time = 2

test/unit/helpers/application_helper_test.rb

@@ -78,7 +78,7 @@ class ApplicationHelperTest < ActionView::TestCase
 
   context "avatar" do
     setup do
-      @user = build(:user, email: "email@example.com")
+      @user = create(:user, email: "email@example.com")
     end
 
     should "raise when invalid theme is requested" do
@@ -92,7 +92,7 @@ class ApplicationHelperTest < ActionView::TestCase
 
       should "return gravatar" do
         url = avatar(160, "id", @user)
-        expected_uri = "https://secure.gravatar.com/avatar/5658ffccee7f0ebfda2b226238b1eb6e.png?d=retro&amp;r=PG&amp;s=160"
+        expected_uri = "/users/#{@user.id}/avatar.jpeg?size=160&amp;theme=light"
 
         assert_equal "<img id=\"id\" width=\"160\" height=\"160\" src=\"#{expected_uri}\" />", url
       end
@@ -106,19 +106,19 @@ class ApplicationHelperTest < ActionView::TestCase
       should "return light themed default avatar" do
         url = avatar(160, "id", @user, theme: :light)
 
-        assert_equal "<img id=\"id\" width=\"160\" height=\"160\" src=\"/images/avatar.svg\" />", url
+        assert_equal "<img id=\"id\" width=\"160\" height=\"160\" src=\"/users/#{@user.id}/avatar.jpeg?size=160&amp;theme=light\" />", url
       end
 
       should "return light themed default avatar by default" do
         url = avatar(160, "id", @user)
 
-        assert_equal "<img id=\"id\" width=\"160\" height=\"160\" src=\"/images/avatar.svg\" />", url
+        assert_equal "<img id=\"id\" width=\"160\" height=\"160\" src=\"/users/#{@user.id}/avatar.jpeg?size=160&amp;theme=light\" />", url
       end
 
       should "return dark themed default avatar" do
         url = avatar(160, "id", @user, theme: :dark)
 
-        assert_equal "<img id=\"id\" width=\"160\" height=\"160\" src=\"/images/avatar_inverted.svg\" />", url
+        assert_equal "<img id=\"id\" width=\"160\" height=\"160\" src=\"/users/#{@user.id}/avatar.jpeg?size=160&amp;theme=dark\" />", url
       end
     end
   end