FIX: Redirects containing Unicode usernames didn't work

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

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

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?

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
 

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

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

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

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"

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")
+        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.encoded_username}/preferences/emails")
       end
     end
   end

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"
+      expect(response).to redirect_to("http://awesome.com/boom/user_avatar/default/#{user.encoded_username(lower: true)}/98/#{upload.id}_#{OptimizedImage::VERSION}.png")
     end
 
     it 'serves new version for old urls' do
@@ -149,5 +154,22 @@ describe UserAvatarsController do
 
       expect(response.status).to eq(200)
     end
+
+    it "serves the correct image when the upload id changed" do
+      SiteSetting.avatar_sizes = "50"
+      SiteSetting.unicode_usernames = true
+
+      upload = Fabricate(:upload)
+      another_upload = Fabricate(:upload)
+      user = Fabricate(:user, uploaded_avatar_id: upload.id)
+
+      get "/user_avatar/default/#{user.username}/50/#{another_upload.id}.png"
+      expect(response).to redirect_to("http://test.localhost/user_avatar/default/#{user.username_lower}/50/#{upload.id}_#{OptimizedImage::VERSION}.png")
+
+      user.update!(username: "Löwe")
+
+      get "/user_avatar/default/#{user.encoded_username}/50/#{another_upload.id}.png"
+      expect(response).to redirect_to("http://test.localhost/user_avatar/default/#{user.encoded_username(lower: true)}/50/#{upload.id}_#{OptimizedImage::VERSION}.png")
+    end
   end
 end

spec/requests/users_controller_spec.rb

@@ -2433,8 +2433,10 @@ describe UsersController do
 
   describe '#my_redirect' do
     it "redirects if the user is not logged in" do
-      get "/my/wat.json"
-      expect(response).to be_redirect
+      get "/my/wat"
+      expect(response).to redirect_to("/login-preferences")
+      expect(response.cookies).to have_key("destination_url")
+      expect(response.cookies["destination_url"]).to eq("/my/wat")
       expect(response.headers['X-Robots-Tag']).to eq('noindex')
     end
 
@@ -2447,13 +2449,21 @@ describe UsersController do
       end
 
       it "will redirect to an valid path" do
-        get "/my/preferences.json"
-        expect(response).to be_redirect
+        get "/my/preferences"
+        expect(response).to redirect_to("/u/#{user.username}/preferences")
       end
 
       it "permits forward slashes" do
-        get "/my/activity/posts.json"
-        expect(response).to be_redirect
+        get "/my/activity/posts"
+        expect(response).to redirect_to("/u/#{user.username}/activity/posts")
+      end
+
+      it "correctly redirects for Unicode usernames" do
+        SiteSetting.unicode_usernames = true
+        user = sign_in(Fabricate(:unicode_user))
+
+        get "/my/preferences"
+        expect(response).to redirect_to("/u/#{user.encoded_username}/preferences")
       end
     end
   end
@@ -3495,7 +3505,14 @@ describe UsersController do
     it "redirects to their profile when logged in" do
       sign_in(user)
       get '/user_preferences'
-      expect(response).to redirect_to("/u/#{user.username_lower}/preferences")
+      expect(response).to redirect_to("/u/#{user.username}/preferences")
+    end
+
+    it "correctly redirects for Unicode usernames" do
+      SiteSetting.unicode_usernames = true
+      user = sign_in(Fabricate(:unicode_user))
+      get '/user_preferences'
+      expect(response).to redirect_to("/u/#{user.encoded_username}/preferences")
     end
   end
 

spec/support/integration_helpers.rb

@@ -26,7 +26,7 @@ module IntegrationHelpers
   end
 
   def sign_in(user)
-    get "/session/#{user.username}/become"
+    get "/session/#{user.encoded_username}/become"
     user
   end