FIX: avatar, profile & card backgrounds url in DiscourseConnect (#31956)

The URLs returned by DiscourseConnect for the user's avatar, profile and
card backgrounds were not always correctly handling CDN.

This make use of the `GlobalPath.full_cdn_url` helper method which has
been battle-tested.

Ref - https://meta.discourse.org/t/-/356599

lib/second_factor/actions/discourse_connect_provider.rb

@@ -79,27 +79,19 @@ module SecondFactor::Actions
       sso.admin = current_user.admin?
       sso.moderator = current_user.moderator?
       sso.groups = current_user.groups.pluck(:name).join(",")
-
-      if current_user.uploaded_avatar.present?
-        base_url =
-          Discourse.store.external? ? "#{Discourse.store.absolute_base_url}/" : Discourse.base_url
-        avatar_url =
-          "#{base_url}#{Discourse.store.get_path_for_upload(current_user.uploaded_avatar)}"
-        sso.avatar_url = UrlHelper.absolute Discourse.store.cdn_url(avatar_url)
-      end
+      sso.avatar_url =
+        GlobalPath.full_cdn_url(
+          current_user.uploaded_avatar.url,
+        ) if current_user.uploaded_avatar.present?
 
       if current_user.user_profile.profile_background_upload.present?
         sso.profile_background_url =
-          UrlHelper.absolute(
-            GlobalPath.upload_cdn_path(current_user.user_profile.profile_background_upload.url),
-          )
+          GlobalPath.full_cdn_url(current_user.user_profile.profile_background_upload.url)
       end
 
       if current_user.user_profile.card_background_upload.present?
         sso.card_background_url =
-          UrlHelper.absolute(
-            GlobalPath.upload_cdn_path(current_user.user_profile.card_background_upload.url),
-          )
+          GlobalPath.full_cdn_url(current_user.user_profile.card_background_upload.url)
       end
     end
 

spec/lib/second_factor/actions/discourse_connect_provider_spec.rb

@@ -2,14 +2,16 @@
 
 RSpec.describe SecondFactor::Actions::DiscourseConnectProvider do
   fab!(:user)
-  sso_secret = "mysecretmyprecious"
+
+  let(:sso_secret) { "mysecretmyprecious" }
+
   let!(:sso) do
-    sso = ::DiscourseConnectProvider.new
+    ::DiscourseConnectProvider.new.tap do |sso|
       sso.nonce = "mysecurenonce"
       sso.return_sso_url = "http://hobbit.shire.com/sso"
       sso.sso_secret = sso_secret
       sso.require_2fa = true
-    sso
+    end
   end
 
   before do
@@ -17,10 +19,6 @@ RSpec.describe SecondFactor::Actions::DiscourseConnectProvider do
     SiteSetting.discourse_connect_provider_secrets = "hobbit.shire.com|#{sso_secret}"
   end
 
-  def params(hash)
-    ActionController::Parameters.new(hash)
-  end
-
   def create_request(query_string)
     ActionDispatch::TestRequest.create(
       { "REQUEST_METHOD" => "GET", "PATH_INFO" => "/", "QUERY_STRING" => query_string },
@@ -206,7 +204,6 @@ RSpec.describe SecondFactor::Actions::DiscourseConnectProvider do
   describe "#second_factor_auth_completed!" do
     let(:output) do
       request = create_request("")
-      params = params_from_payload("")
       action = create_instance(user, request)
       action.second_factor_auth_completed!(payload: sso.payload)
     end
@@ -223,7 +220,6 @@ RSpec.describe SecondFactor::Actions::DiscourseConnectProvider do
       wrong_sso.require_2fa = true
 
       request = create_request(wrong_sso.payload)
-      params = params_from_payload(wrong_sso.payload)
       action = create_instance(user, request)
       redirect_url = action.second_factor_auth_completed!(payload: sso.payload)[:sso_redirect_url]
       response_payload = ::DiscourseConnectProvider.parse(URI(redirect_url).query)
@@ -241,8 +237,26 @@ RSpec.describe SecondFactor::Actions::DiscourseConnectProvider do
     end
 
     it "the response payload contains the user details" do
+      user.update!(uploaded_avatar: Fabricate(:upload))
+      user.user_profile.update!(
+        profile_background_upload: Fabricate(:upload),
+        card_background_upload: Fabricate(:upload),
+      )
+
+      expect(response_payload.name).to eq(user.name)
       expect(response_payload.username).to eq(user.username)
       expect(response_payload.email).to eq(user.email)
+      expect(response_payload.external_id).to eq(user.id.to_s)
+      expect(response_payload.admin).to eq(user.admin?)
+      expect(response_payload.moderator).to eq(user.moderator?)
+      expect(response_payload.groups).to eq(user.groups.pluck(:name).join(","))
+      expect(response_payload.avatar_url).to eq(GlobalPath.full_cdn_url(user.uploaded_avatar.url))
+      expect(response_payload.profile_background_url).to eq(
+        GlobalPath.full_cdn_url(user.user_profile.profile_background_upload.url),
+      )
+      expect(response_payload.card_background_url).to eq(
+        GlobalPath.full_cdn_url(user.user_profile.card_background_upload.url),
+      )
     end
   end
 end

spec/requests/session_controller_spec.rb

@@ -1576,73 +1576,29 @@ RSpec.describe SessionController do
       end
 
       it "handles non local content correctly" do
-        SiteSetting.avatar_sizes = "100|49"
         setup_s3
         SiteSetting.s3_cdn_url = "http://cdn.com"
 
-        stub_request(:any, /s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/).to_return(
-          status: 200,
-          body: "",
-          headers: {
-            referer: "fgdfds",
-          },
-        )
-
-        @user.create_user_avatar!
-        upload =
-          Fabricate(
-            :upload,
-            url: "//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/something",
-          )
-
-        Fabricate(
-          :optimized_image,
-          sha1: SecureRandom.hex << "A" * 8,
-          upload: upload,
-          width: 98,
-          height: 98,
-          url: "//s3-upload-bucket.s3.amazonaws.com/something/else",
-        )
-
-        @user.update_columns(uploaded_avatar_id: upload.id)
-
-        upload1 = Fabricate(:upload_s3)
-        upload2 = Fabricate(:upload_s3)
-
+        @user.update!(uploaded_avatar: Fabricate(:upload_s3))
         @user.user_profile.update!(
-          profile_background_upload: upload1,
-          card_background_upload: upload2,
+          profile_background_upload: Fabricate(:upload_s3),
+          card_background_upload: Fabricate(:upload_s3),
         )
 
-        @user.reload
-        @user.user_avatar.reload
-        @user.user_profile.reload
-
         sign_in(@user)
 
-        stub_request(:get, "http://cdn.com/something/else").to_return(
-          body: lambda { |request| File.new(Rails.root + "spec/fixtures/images/logo.png") },
-        )
-
         get "/session/sso_provider",
             params: Rack::Utils.parse_query(@sso.payload("secretForOverRainbow"))
 
         location = response.header["Location"]
-        # javascript code will handle redirection of user to return_sso_url
         expect(location).to match(%r{^http://somewhere.over.rainbow/sso})
 
         payload = location.split("?")[1]
         sso2 = DiscourseConnectProvider.parse(payload)
 
-        expect(sso2.avatar_url.blank?).to_not eq(true)
-        expect(sso2.profile_background_url.blank?).to_not eq(true)
-        expect(sso2.card_background_url.blank?).to_not eq(true)
-
-        expect(sso2.avatar_url).to start_with("#{SiteSetting.s3_cdn_url}/original")
+        expect(sso2.avatar_url).to start_with(SiteSetting.s3_cdn_url)
         expect(sso2.profile_background_url).to start_with(SiteSetting.s3_cdn_url)
         expect(sso2.card_background_url).to start_with(SiteSetting.s3_cdn_url)
-        expect(sso2.confirmed_2fa).to eq(nil)
-        expect(sso2.no_2fa_methods).to eq(nil)
       end
 
       it "successfully logs out and redirects user to return_sso_url when the user is logged in" do