DEV: backport login redirect param to stable (#32865)

This backports to stable my changes to add a redirect queryParam to the
/login route.

Pulled in a couple other commits that affect the login process and the
same spec that I modified.

Had to add a commit cleaning up the modal login helper for login_spec.rb
to pass here since the modal login was already removed in main.

---------

Co-authored-by: Penar Musaraj <pmusaraj@gmail.com>

app/assets/javascripts/discourse/app/components/modal/login.js

@@ -137,6 +137,8 @@ export default class Login extends Component {
           } else if (destinationUrl) {
             removeCookie("destination_url");
             window.location.assign(destinationUrl);
+          } else if (this.args.model.referrerUrl) {
+            window.location.assign(this.args.model.referrerUrl);
           } else {
             window.location.reload();
           }
@@ -288,6 +290,8 @@ export default class Login extends Component {
           removeCookie("destination_url");
 
           applyHiddenFormInputValue(destinationUrl, "redirect");
+        } else if (this.args.model.referrerUrl) {
+          applyHiddenFormInputValue(this.args.model.referrerUrl, "redirect");
         } else {
           applyHiddenFormInputValue(window.location.href, "redirect");
         }

app/assets/javascripts/discourse/app/controllers/login.js

@@ -160,6 +160,8 @@ export default class LoginPageController extends Controller {
           } else if (destinationUrl) {
             removeCookie("destination_url");
             window.location.assign(destinationUrl);
+          } else if (this.referrerUrl) {
+            window.location.assign(this.referrerUrl);
           } else {
             window.location.reload();
           }
@@ -337,6 +339,8 @@ export default class LoginPageController extends Controller {
           removeCookie("destination_url");
 
           applyHiddenFormInputValue(destinationUrl, "redirect");
+        } else if (this.referrerUrl) {
+          applyHiddenFormInputValue(this.referrerUrl, "redirect");
         } else {
           applyHiddenFormInputValue(window.location.href, "redirect");
         }

app/assets/javascripts/discourse/app/routes/application.js

@@ -298,6 +298,9 @@ export default class ApplicationRoute extends DiscourseRoute {
             showNotActivated: (props) => this.send("showNotActivated", props),
             showCreateAccount: (props) => this.send("showCreateAccount", props),
             canSignUp: this.controller.canSignUp,
+            referrerUrl: DiscourseURL.isInternal(document.referrer)
+              ? document.referrer
+              : null,
           },
         });
       }

app/assets/javascripts/discourse/app/routes/login.js

@@ -1,5 +1,6 @@
 import { next } from "@ember/runloop";
 import { service } from "@ember/service";
+import DiscourseURL from "discourse/lib/url";
 import { defaultHomepage } from "discourse/lib/utilities";
 import StaticPage from "discourse/models/static-page";
 import DiscourseRoute from "discourse/routes/discourse";
@@ -9,7 +10,11 @@ export default class LoginRoute extends DiscourseRoute {
   @service router;
   @service login;
 
-  beforeModel() {
+  beforeModel(transition) {
+    if (transition.from) {
+      this.internalReferrer = this.router.urlFor(transition.from.name);
+    }
+
     if (this.siteSettings.login_required) {
       if (
         this.login.isOnlyOneExternalLoginMethod &&
@@ -49,6 +54,10 @@ export default class LoginRoute extends DiscourseRoute {
     controller.set("flashType", "");
     controller.set("flash", "");
 
+    if (this.internalReferrer || DiscourseURL.isInternal(document.referrer)) {
+      controller.set("referrerUrl", this.internalReferrer || document.referrer);
+    }
+
     if (this.siteSettings.login_required) {
       controller.set("showLogin", false);
     }

app/controllers/static_controller.rb

@@ -27,9 +27,36 @@ class StaticController < ApplicationController
   }
   CUSTOM_PAGES = {} # Add via `#add_topic_static_page` in plugin API
 
+  def extract_redirect_param
+    redirect_path = params[:redirect]
+    if redirect_path.present?
+      begin
+        forum_host = URI(Discourse.base_url).host
+        uri = URI(redirect_path)
+
+        if uri.path.present? && !uri.path.starts_with?(login_path) &&
+             (uri.host.blank? || uri.host == forum_host) && uri.path =~ %r{\A\/{1}[^\.\s]*\z}
+          return "#{uri.path}#{uri.query ? "?#{uri.query}" : ""}"
+        end
+      rescue URI::Error, ArgumentError
+        # If the URI is invalid, return "/" below
+      end
+    end
+
+    "/"
+  end
+
   def show
-    if current_user && (params[:id] == "login" || params[:id] == "signup")
+    if params[:id] == "login"
-      return redirect_to(path "/")
+      destination = extract_redirect_param
+
+      if current_user
+        return redirect_to(path(destination), allow_other_host: false)
+      elsif destination != "/"
+        cookies[:destination_url] = path(destination)
+      end
+    elsif params[:id] == "signup" && current_user
+      return redirect_to path("/")
     end
 
     if SiteSetting.login_required? && current_user.nil? && %w[faq guidelines].include?(params[:id])
@@ -123,26 +150,7 @@ class StaticController < ApplicationController
     params.delete(:username)
     params.delete(:password)
 
-    destination = path("/")
+    destination = extract_redirect_param
-
-    redirect_location = params[:redirect]
-    if redirect_location.present? && !redirect_location.is_a?(String)
-      raise Discourse::InvalidParameters.new(:redirect)
-    elsif redirect_location.present? &&
-          begin
-            forum_uri = URI(Discourse.base_url)
-            uri = URI(redirect_location)
-
-            if uri.path.present? && !uri.path.starts_with?(login_path) &&
-                 (uri.host.blank? || uri.host == forum_uri.host) &&
-                 uri.path =~ %r{\A\/{1}[^\.\s]*\z}
-              destination = "#{uri.path}#{uri.query ? "?#{uri.query}" : ""}"
-            end
-          rescue URI::Error
-            # Do nothing if the URI is invalid
-          end
-    end
-
     redirect_to(destination, allow_other_host: false)
   end
 

app/controllers/users/omniauth_callbacks_controller.rb

@@ -86,8 +86,13 @@ class Users::OmniauthCallbacksController < ApplicationController
 
     cookies["_bypass_cache"] = true
     cookies[:authentication_data] = { value: client_hash.to_json, path: Discourse.base_path("/") }
+
+    if !current_user && @origin.start_with?("/user-api-key/new")
+      redirect_to path("/")
+    else
       redirect_to @origin
     end
+  end
 
   def valid_origin?(uri)
     return false if uri.nil?

lib/auth/result.rb

@@ -123,6 +123,8 @@ class Auth::Result
       user.update(associated_group_ids: associated_group_ids)
       AssociatedGroup.where(id: associated_group_ids).update_all("last_used = CURRENT_TIMESTAMP")
     end
+
+    Group.user_trust_level_change!(user.id, user.trust_level) if user && !user.staged
   end
 
   def can_edit_name

spec/requests/static_controller_spec.rb

@@ -150,7 +150,7 @@ RSpec.describe StaticController do
       end
     end
 
-    it "should redirect to / when logged in and path is /login" do
+    it "should redirect to / when logged in and path is /login without redirect" do
       sign_in(Fabricate(:user))
       get "/login"
       expect(response).to redirect_to("/")
@@ -252,6 +252,58 @@ RSpec.describe StaticController do
         get "/login"
       end.to_not change { SiteSetting.title }
     end
+
+    context "without a subfolder" do
+      it "redirects as requested when logged in and path is /login with valid redirect param" do
+        sign_in(Fabricate(:user))
+        get "/login", params: { redirect: "/foo" }
+        expect(response).to redirect_to("/foo")
+      end
+
+      it "redirects to / when logged in and path is /login with invalid redirect param" do
+        sign_in(Fabricate(:user))
+        get "/login", params: { redirect: "//foo" }
+        expect(response).to redirect_to("/")
+        get "/login", params: { redirect: "foo" }
+        expect(response).to redirect_to("/")
+        get "/login", params: { redirect: "http://foo" }
+        expect(response).to redirect_to("/")
+        get "/login", params: { redirect: "www.foo.bar" }
+        expect(response).to redirect_to("/")
+      end
+
+      it "sets the destination_url cookie when not logged in and path is /login with valid redirect param" do
+        get "/login", params: { redirect: "/foo" }
+        expect(response.cookies["destination_url"]).to eq("/foo")
+      end
+    end
+
+    context "with a subfolder" do
+      before { set_subfolder "/sub_test" }
+
+      it "redirects as requested when logged in and path is /login with valid redirect param" do
+        sign_in(Fabricate(:user))
+        get "/login", params: { redirect: "/foo" }
+        expect(response).to redirect_to("/sub_test/foo")
+      end
+
+      it "redirects to / when logged in and path is /login with invalid redirect param" do
+        sign_in(Fabricate(:user))
+        get "/login", params: { redirect: "//foo" }
+        expect(response).to redirect_to("/sub_test/")
+        get "/login", params: { redirect: "foo" }
+        expect(response).to redirect_to("/sub_test/")
+        get "/login", params: { redirect: "http://foo" }
+        expect(response).to redirect_to("/sub_test/")
+        get "/login", params: { redirect: "www.foo.bar" }
+        expect(response).to redirect_to("/sub_test/")
+      end
+
+      it "sets the destination_url cookie when not logged in and path is /login with valid redirect param" do
+        get "/login", params: { redirect: "/foo" }
+        expect(response.cookies["destination_url"]).to eq("/sub_test/foo")
+      end
+    end
   end
 
   describe "#enter" do
@@ -307,10 +359,7 @@ RSpec.describe StaticController do
     context "with an array" do
       it "redirects to the root" do
         post "/login.json", params: { redirect: ["/foo"] }
-        expect(response.status).to eq(400)
+        expect(response).to redirect_to("/")
-        json = response.parsed_body
-        expect(json["errors"]).to be_present
-        expect(json["errors"]).to include(I18n.t("invalid_params", message: "redirect"))
       end
     end
 

spec/system/login_spec.rb

@@ -36,6 +36,16 @@ shared_examples "login scenarios" do |login_page_object|
       expect(page).to have_css(".header-dropdown-toggle.current-user")
     end
 
+    it "can login with redirect" do
+      EmailToken.confirm(Fabricate(:email_token, user: user).token)
+
+      login_form
+        .open_with_redirect("/about")
+        .fill(username: "john", password: "supersecurepassword")
+        .click_login
+      expect(page).to have_current_path("/about")
+    end
+
     it "can login and activate account" do
       login_form.open.fill(username: "john", password: "supersecurepassword").click_login
       expect(page).to have_css(".not-activated-modal")
@@ -88,9 +98,6 @@ shared_examples "login scenarios" do |login_page_object|
 
       # TODO: prefill username when fullpage
       if find("#username-or-email").value.blank?
-        if page.has_css?("html.mobile-view", wait: 0)
-          expect(page).to have_no_css(".d-modal.is-animating")
-        end
         find("#username-or-email").fill_in(with: user.username)
       end
 
@@ -126,6 +133,128 @@ shared_examples "login scenarios" do |login_page_object|
       login_form.fill(username: "john", password: "supersecurepassword").click_login
       expect(page).to have_css(".header-dropdown-toggle.current-user")
     end
+
+    it "redirects to a PM after login" do
+      EmailToken.confirm(Fabricate(:email_token, user: user).token)
+
+      group = Fabricate(:group, publish_read_state: true)
+      Fabricate(:group_user, group: group, user: user)
+      pm = Fabricate(:private_message_topic, allowed_groups: [group])
+      Fabricate(:post, topic: pm, user: user, reads: 2, created_at: 1.day.ago)
+      Fabricate(:group_private_message_topic, user: user, recipient_group: group)
+
+      visit "/t/#{pm.id}"
+      find(".login-welcome .login-button").click
+      login_form.fill(username: "john", password: "supersecurepassword").click_login
+
+      expect(page).to have_css(".header-dropdown-toggle.current-user")
+      expect(page).to have_css("#topic-title")
+      expect(page).to have_css(".private_message")
+    end
+  end
+
+  context "when login is not required" do
+    before { SiteSetting.login_required = false }
+
+    it "redirects to a PM after authentication" do
+      EmailToken.confirm(Fabricate(:email_token, user: user).token)
+      group = Fabricate(:group, publish_read_state: true)
+      Fabricate(:group_user, group: group, user: user)
+      pm = Fabricate(:private_message_topic, allowed_groups: [group])
+      Fabricate(:post, topic: pm, user: user, reads: 2, created_at: 1.day.ago)
+      Fabricate(:group_private_message_topic, user: user, recipient_group: group)
+
+      visit "/t/#{pm.id}"
+      find(".btn.login-button").click
+
+      login_form.fill(username: "john", password: "supersecurepassword").click_login
+      expect(page).to have_css(".header-dropdown-toggle.current-user")
+
+      expect(page).to have_css("#topic-title")
+      expect(page).to have_css(".private_message")
+    end
+
+    it "redirects to a public topic when hitting Reply then logging in" do
+      EmailToken.confirm(Fabricate(:email_token, user: user).token)
+      topic = Fabricate(:topic)
+      Fabricate(:post, topic: topic, created_at: 1.day.ago)
+
+      visit "/t/#{topic.id}"
+      find(".topic-footer-main-buttons .btn-primary").click
+
+      login_form.fill(username: "john", password: "supersecurepassword").click_login
+      expect(page).to have_css(".header-dropdown-toggle.current-user")
+
+      expect(page).to have_css("#topic-title")
+    end
+
+    context "with user api key and omniauth" do
+      include OmniauthHelpers
+
+      let :public_key do
+        <<~TXT
+    -----BEGIN PUBLIC KEY-----
+    MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDh7BS7Ey8hfbNhlNAW/47pqT7w
+    IhBz3UyBYzin8JurEQ2pY9jWWlY8CH147KyIZf1fpcsi7ZNxGHeDhVsbtUKZxnFV
+    p16Op3CHLJnnJKKBMNdXMy0yDfCAHZtqxeBOTcCo1Vt/bHpIgiK5kmaekyXIaD0n
+    w0z/BYpOgZ8QwnI5ZwIDAQAB
+    -----END PUBLIC KEY-----
+    TXT
+      end
+
+      let :private_key do
+        <<~TXT
+    -----BEGIN RSA PRIVATE KEY-----
+    MIICWwIBAAKBgQDh7BS7Ey8hfbNhlNAW/47pqT7wIhBz3UyBYzin8JurEQ2pY9jW
+    WlY8CH147KyIZf1fpcsi7ZNxGHeDhVsbtUKZxnFVp16Op3CHLJnnJKKBMNdXMy0y
+    DfCAHZtqxeBOTcCo1Vt/bHpIgiK5kmaekyXIaD0nw0z/BYpOgZ8QwnI5ZwIDAQAB
+    AoGAeHesbjzCivc+KbBybXEEQbBPsThY0Y+VdgD0ewif2U4UnNhzDYnKJeTZExwQ
+    vAK2YsRDV3KbhljnkagQduvmgJyCKuV/CxZvbJddwyIs3+U2D4XysQp3e1YZ7ROr
+    YlOIoekHCx1CNm6A4iImqGxB0aJ7Owdk3+QSIaMtGQWaPTECQQDz2UjJ+bomguNs
+    zdcv3ZP7W3U5RG+TpInSHiJXpt2JdNGfHItozGJCxfzDhuKHK5Cb23bgldkvB9Xc
+    p/tngTtNAkEA7S4cqUezA82xS7aYPehpRkKEmqzMwR3e9WeL7nZ2cdjZAHgXe49l
+    3mBhidEyRmtPqbXo1Xix8LDuqik0IdnlgwJAQeYTnLnHS8cNjQbnw4C/ECu8Nzi+
+    aokJ0eXg5A0tS4ttZvGA31Z0q5Tz5SdbqqnkT6p0qub0JZiZfCNNdsBe9QJAaGT5
+    fJDwfGYW+YpfLDCV1bUFhMc2QHITZtSyxL0jmSynJwu02k/duKmXhP+tL02gfMRy
+    vTMorxZRllgYeCXeXQJAEGRXR8/26jwqPtKKJzC7i9BuOYEagqj0nLG2YYfffCMc
+    d3JGCf7DMaUlaUE8bJ08PtHRJFSGkNfDJLhLKSjpbw==
+    -----END RSA PRIVATE KEY-----
+    TXT
+      end
+
+      let :args do
+        {
+          scopes: "one_time_password",
+          client_id: "x" * 32,
+          auth_redirect: "discourse://auth_redirect",
+          application_name: "foo",
+          public_key: public_key,
+          nonce: SecureRandom.hex,
+        }
+      end
+
+      before do
+        OmniAuth.config.test_mode = true
+        SiteSetting.auth_skip_create_confirm = true
+        SiteSetting.enable_google_oauth2_logins = true
+        SiteSetting.enable_local_logins = false
+      end
+
+      after { reset_omniauth_config(:google_oauth2) }
+
+      it "completes signup and redirects to the user api key authorization form" do
+        mock_google_auth
+        visit("/user-api-key/new?#{args.to_query}")
+
+        expect(page).to have_css(".authorize-api-key .scopes")
+      end
+
+      it "redirects when navigating to login with redirect param" do
+        mock_google_auth
+        login_form.open_with_redirect("/about")
+        expect(page).to have_current_path("/about")
+      end
+    end
   end
 
   context "with two-factor authentication" do
@@ -161,6 +290,21 @@ shared_examples "login scenarios" do |login_page_object|
       expect(page).to have_css(".header-dropdown-toggle.current-user")
     end
 
+    it "can login with totp and redirect" do
+      login_form
+        .open_with_redirect("/about")
+        .fill(username: "john", password: "supersecurepassword")
+        .click_login
+
+      expect(page).to have_css(".second-factor")
+
+      totp = ROTP::TOTP.new(user_second_factor.data).now
+      find("#login-second-factor").fill_in(with: totp)
+      login_form.click_login
+
+      expect(page).to have_current_path("/about")
+    end
+
     it "can login with backup code" do
       login_form.open.fill(username: "john", password: "supersecurepassword").click_login
 

spec/system/page_objects/modals/login.rb

@@ -16,6 +16,11 @@ module PageObjects
         self
       end
 
+      def open_with_redirect(redirect_path)
+        visit("/login?redirect=#{redirect_path}")
+        self
+      end
+
       def open_from_header
         find(".login-button").click
       end

spec/system/page_objects/pages/login.rb

@@ -16,6 +16,11 @@ module PageObjects
         self
       end
 
+      def open_with_redirect(redirect_path)
+        visit("/login?redirect=#{redirect_path}")
+        self
+      end
+
       def open_from_header
         find(".login-button").click
       end