From 67dec38f314b877e35be56d21f47f297799ea903 Mon Sep 17 00:00:00 2001
From: Blake Erickson <o.blakeerickson@gmail.com>
Date: Wed, 2 Sep 2020 20:12:24 -0600
Subject: [PATCH] FIX: Gravatar download attempt if user is missing their email

It is possible that a user could exist without an email, if so we should
not enqueue a job to download their gravatar.

This commit resolves this error that can occur:

```
Job exception: undefined method `email' for nil:NilClass
/var/www/discourse/app/models/user.rb:1204:in `email'
/var/www/discourse/app/jobs/regular/update_gravatar.rb:12:in `execute'
```

This commit also fixes the original spec which actually was wrong. The
job never enqueued in the original spec and so the gravatar was never
actually updated and the test was checking if the two values were the
same, but they were both null and never updated, so of course they were
the same!

A new test has also been added to make sure the gravatar job isn't
enqueued when a user's email is missing.
---
 app/jobs/regular/update_gravatar.rb |  2 +-
 app/models/user.rb                  |  5 +++-
 spec/jobs/update_gravatar_spec.rb   | 36 +++++++++++++++++++++++++----
 3 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/app/jobs/regular/update_gravatar.rb b/app/jobs/regular/update_gravatar.rb
index 7ae721759d..de954b41ee 100644
--- a/app/jobs/regular/update_gravatar.rb
+++ b/app/jobs/regular/update_gravatar.rb
@@ -9,7 +9,7 @@ module Jobs
       user = User.find_by(id: args[:user_id])
       avatar = UserAvatar.find_by(id: args[:avatar_id])
 
-      if user && avatar && avatar.user&.id == user.id && user.email.present?
+      if user && avatar && avatar.user&.id == user.id && user.primary_email.present?
         avatar.update_gravatar!
         if !user.uploaded_avatar_id && avatar.gravatar_upload_id
           user.update_column(:uploaded_avatar_id, avatar.gravatar_upload_id)
diff --git a/app/models/user.rb b/app/models/user.rb
index b5d4d63ab5..865b6676fa 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -1077,7 +1077,10 @@ class User < ActiveRecord::Base
 
     avatar = user_avatar || create_user_avatar
 
-    if SiteSetting.automatically_download_gravatars? && !avatar.last_gravatar_download_attempt
+    if self.primary_email.present? &&
+        SiteSetting.automatically_download_gravatars? &&
+        !avatar.last_gravatar_download_attempt
+
       Jobs.cancel_scheduled_job(:update_gravatar, user_id: self.id, avatar_id: avatar.id)
       Jobs.enqueue_in(1.second, :update_gravatar, user_id: self.id, avatar_id: avatar.id)
     end
diff --git a/spec/jobs/update_gravatar_spec.rb b/spec/jobs/update_gravatar_spec.rb
index 3be1af2615..0f0de83105 100644
--- a/spec/jobs/update_gravatar_spec.rb
+++ b/spec/jobs/update_gravatar_spec.rb
@@ -3,22 +3,48 @@
 require 'rails_helper'
 
 describe Jobs::UpdateGravatar do
+  fab!(:user) { Fabricate(:user) }
+  let(:temp) { Tempfile.new('test') }
+  fab!(:upload) { Fabricate(:upload, user: user) }
+  let(:avatar) { user.create_user_avatar! }
 
   it "picks gravatar if system avatar is picked and gravatar was just downloaded" do
-    user = User.create!(username: "bob", name: "bob", email: "a@a.com")
+    temp.binmode
+    # tiny valid png
+    temp.write(Base64.decode64("iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg=="))
+    temp.rewind
+    FileHelper.expects(:download).returns(temp)
+
+    Jobs.run_immediately!
+
     expect(user.uploaded_avatar_id).to eq(nil)
     expect(user.user_avatar.gravatar_upload_id).to eq(nil)
 
-    png = Base64.decode64("R0lGODlhAQABALMAAAAAAIAAAACAAICAAAAAgIAAgACAgMDAwICAgP8AAAD/AP//AAAA//8A/wD//wBiZCH5BAEAAA8ALAAAAAABAAEAAAQC8EUAOw==")
-    url = "https://www.gravatar.com/avatar/d10ca8d11301c2f4993ac2279ce4b930.png?s=360&d=404"
-    stub_request(:get, url).to_return(body: png)
-
     SiteSetting.automatically_download_gravatars = true
 
     user.refresh_avatar
     user.reload
 
+    expect(user.uploaded_avatar_id).to_not eq(nil)
     expect(user.uploaded_avatar_id).to eq(user.user_avatar.gravatar_upload_id)
+
+    temp.unlink
   end
 
+  it "does not enqueue a job when user is missing their email" do
+    user.primary_email.destroy
+    user.reload
+
+    expect(user.uploaded_avatar_id).to eq(nil)
+    expect(user.user_avatar.gravatar_upload_id).to eq(nil)
+
+    SiteSetting.automatically_download_gravatars = true
+
+    expect { user.refresh_avatar }
+      .to change { Jobs::UpdateGravatar.jobs.count }.by(0)
+    user.reload
+
+    expect(user.uploaded_avatar_id).to eq(nil)
+    expect(user.user_avatar.gravatar_upload_id).to eq(nil)
+  end
 end