FIX: Redirects containing Unicode usernames didn't work

FIX: Redirects containing Unicode usernames didn’t work

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 2ce9f87..c59266e 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -760,7 +760,7 @@ class ApplicationController < ActionController::Base
     return if !current_user
     return if !should_enforce_2fa?
 
-    redirect_path = "#{GlobalSetting.relative_url_root}/u/#{current_user.username}/preferences/second-factor"
+    redirect_path = path("/u/#{current_user.encoded_username}/preferences/second-factor")
     if !request.fullpath.start_with?(redirect_path)
       redirect_to path(redirect_path)
       nil
diff --git a/app/controllers/email_controller.rb b/app/controllers/email_controller.rb
index 5e8b8d6..a7d283a 100644
--- a/app/controllers/email_controller.rb
+++ b/app/controllers/email_controller.rb
@@ -7,7 +7,7 @@ class EmailController < ApplicationController
   before_action :ensure_logged_in, only: :preferences_redirect
 
   def preferences_redirect
-    redirect_to(email_preferences_path(current_user.username_lower))
+    redirect_to path("/u/#{current_user.encoded_username}/preferences/emails")
   end
 
   def unsubscribe
diff --git a/app/controllers/user_avatars_controller.rb b/app/controllers/user_avatars_controller.rb
index 13fc1f8..3b0dd70 100644
--- a/app/controllers/user_avatars_controller.rb
+++ b/app/controllers/user_avatars_controller.rb
@@ -109,7 +109,7 @@ class UserAvatarsController < ApplicationController
 
     if !Discourse.avatar_sizes.include?(size) && Discourse.store.external?
       closest = Discourse.avatar_sizes.to_a.min { |a, b| (size - a).abs <=> (size - b).abs }
-      avatar_url = UserAvatar.local_avatar_url(hostname, user.username_lower, upload_id, closest)
+      avatar_url = UserAvatar.local_avatar_url(hostname, user.encoded_username(lower: true), upload_id, closest)
       return redirect_to cdn_path(avatar_url)
     end
 
@@ -117,7 +117,7 @@ class UserAvatarsController < ApplicationController
     upload ||= user.uploaded_avatar if user.uploaded_avatar_id == upload_id
 
     if user.uploaded_avatar && !upload
-      avatar_url = UserAvatar.local_avatar_url(hostname, user.username_lower, user.uploaded_avatar_id, size)
+      avatar_url = UserAvatar.local_avatar_url(hostname, user.encoded_username(lower: true), user.uploaded_avatar_id, size)
       return redirect_to cdn_path(avatar_url)
     elsif upload && optimized = get_optimized_image(upload, size)
       if optimized.local?
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 12d9643..4230335 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -130,7 +130,7 @@ class UsersController < ApplicationController
   end
 
   def user_preferences_redirect
-    redirect_to email_preferences_path(current_user.username_lower)
+    redirect_to path("/u/#{current_user.encoded_username}/preferences")
   end
 
   def update
@@ -273,7 +273,7 @@ class UsersController < ApplicationController
       cookies[:destination_url] = path("/my/#{params[:path]}")
       redirect_to path("/login-preferences")
     else
-      redirect_to(path("/u/#{current_user.username}/#{params[:path]}"))
+      redirect_to(path("/u/#{current_user.encoded_username}/#{params[:path]}"))
     end
   end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 0147c62..fdade36 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -1302,6 +1302,10 @@ class User < ActiveRecord::Base
       .pluck(:credential_id)
   end
 
+  def encoded_username(lower: false)
+    UrlHelper.encode_component(lower ? username_lower : username)
+  end
+
   protected
 
   def badge_grant
diff --git a/spec/fabricators/user_fabricator.rb b/spec/fabricators/user_fabricator.rb
index 6e6e462..759a5a4 100644
--- a/spec/fabricators/user_fabricator.rb
+++ b/spec/fabricators/user_fabricator.rb
@@ -112,3 +112,7 @@ end
 Fabricator(:staged, from: :user) do
   staged true
 end
+
+Fabricator(:unicode_user, from: :user) do
+  username { sequence(:username) { |i| "Löwe#{i}" } }
+end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 4d2592c..2a50cd8 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -2373,4 +2373,19 @@ describe User do
   def reset_last_seen_cache!(user)
     Discourse.redis.del("user:#{user.id}:#{Time.zone.now.to_date}")
   end
+
+  describe ".encoded_username" do
+    it "doesn't encoded ASCII usernames" do
+      user = Fabricate(:user, username: "John")
+      expect(user.encoded_username).to eq("John")
+      expect(user.encoded_username(lower: true)).to eq("john")
+    end
+
+    it "encodes Unicode characters" do
+      SiteSetting.unicode_usernames = true
+      user = Fabricate(:user, username: "Löwe")
+      expect(user.encoded_username).to eq("L%C3%B6we")
+      expect(user.encoded_username(lower: true)).to eq("l%C3%B6we")
+    end
+  end
 end
diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb
index 8e4dc43..0e3fe9a 100644
--- a/spec/requests/application_controller_spec.rb
+++ b/spec/requests/application_controller_spec.rb
@@ -162,6 +162,15 @@ RSpec.describe ApplicationController do
       expect(response.status).to eq(200)
     end
 
+    it "correctly redirects for Unicode usernames" do
+      SiteSetting.enforce_second_factor = "all"
+      SiteSetting.unicode_usernames = true
+      user = sign_in(Fabricate(:unicode_user))
+
+      get "/"
+      expect(response).to redirect_to("/u/#{user.encoded_username}/preferences/second-factor")
+    end
+
     context "when enforcing second factor for staff" do
       before do
         SiteSetting.enforce_second_factor = "staff"
diff --git a/spec/requests/email_controller_spec.rb b/spec/requests/email_controller_spec.rb
index ec60265..9c9ab97 100644
--- a/spec/requests/email_controller_spec.rb
+++ b/spec/requests/email_controller_spec.rb
@@ -176,11 +176,17 @@ RSpec.describe EmailController do
     end
 
     context 'when logged in' do
-      let!(:user) { sign_in(Fabricate(:user)) }
-
       it 'redirects to your user preferences' do
+        user = sign_in(Fabricate(:user))
+        get "/email_preferences.json"
+        expect(response).to redirect_to("/u/#{user.username}/preferences/emails")
+      end
+
+      it "correctly redirects for Unicode usernames" do
+        SiteSetting.unicode_usernames = true
+        user = sign_in(Fabricate(:unicode_user))
         get "/email_preferences.json"
-        expect(response).to redirect_to("/u/#{user.username}/preferences")
+        expect(response).to redirect_to("/u/#{user.encoded_username}/preferences/emails")
       end
     end
   end
diff --git a/spec/requests/user_avatars_controller_spec.rb b/spec/requests/user_avatars_controller_spec.rb
index 9772fe4..4f639fd 100644
--- a/spec/requests/user_avatars_controller_spec.rb
+++ b/spec/requests/user_avatars_controller_spec.rb
@@ -75,10 +75,10 @@ describe UserAvatarsController do
       SiteSetting.s3_secret_access_key = "XXX"
       SiteSetting.s3_upload_bucket = "test"
       SiteSetting.s3_cdn_url = "http://cdn.com"
+      SiteSetting.unicode_usernames = true
 
       stub_request(:get, "http://cdn.com/something/else").to_return(body: 'image')
-
-      GlobalSetting.stubs(:cdn_url).returns("http://awesome.com/boom")
+      set_cdn_url("http://awesome.com/boom")
 
       upload = Fabricate(:upload, url: "//test.s3.dualstack.us-east-1.amazonaws.com/something")
 
@@ -103,6 +103,11 @@ describe UserAvatarsController do
       expect(response.body).to eq("image")
       expect(response.headers["Cache-Control"]).to eq('max-age=31556952, public, immutable')
       expect(response.headers["Last-Modified"]).to eq(optimized_image.upload.created_at.httpdate)
+
+      user.update!(username: "Löwe")
+
+      get "/user_avatar/default/#{user.encoded_username}/97/#{upload.id}.png"

[... diff too long, it was truncated ...]

GitHub sha: 8c6a42c5

This commit appears in #9989 which was approved by eviltrout and eviltrout. It was merged by gschlager.

This commit has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/404-after-mailing-list-unsub-to-redirect-to-prefs-unicode-issue/153662/4