From 9f1e7415fdbb08de694dba7bf554b1de938e9ac3 Mon Sep 17 00:00:00 2001 From: Chris Alberti Date: Thu, 22 May 2025 16:20:21 -0500 Subject: [PATCH] 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 --- .../discourse/app/components/modal/login.js | 4 + .../discourse/app/controllers/login.js | 4 + .../discourse/app/routes/application.js | 3 + .../javascripts/discourse/app/routes/login.js | 11 +- app/controllers/static_controller.rb | 52 +++--- .../users/omniauth_callbacks_controller.rb | 7 +- lib/auth/result.rb | 2 + spec/requests/static_controller_spec.rb | 59 ++++++- spec/system/login_spec.rb | 150 +++++++++++++++++- spec/system/page_objects/modals/login.rb | 5 + spec/system/page_objects/pages/login.rb | 5 + 11 files changed, 270 insertions(+), 32 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/modal/login.js b/app/assets/javascripts/discourse/app/components/modal/login.js index d50dbd292a0..1d895b21fdc 100644 --- a/app/assets/javascripts/discourse/app/components/modal/login.js +++ b/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"); } diff --git a/app/assets/javascripts/discourse/app/controllers/login.js b/app/assets/javascripts/discourse/app/controllers/login.js index 559ae4370b6..17fe482b46d 100644 --- a/app/assets/javascripts/discourse/app/controllers/login.js +++ b/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"); } diff --git a/app/assets/javascripts/discourse/app/routes/application.js b/app/assets/javascripts/discourse/app/routes/application.js index d680a6ac790..58076ee7338 100644 --- a/app/assets/javascripts/discourse/app/routes/application.js +++ b/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, }, }); } diff --git a/app/assets/javascripts/discourse/app/routes/login.js b/app/assets/javascripts/discourse/app/routes/login.js index 988a3e6024b..8c6c79a6b74 100644 --- a/app/assets/javascripts/discourse/app/routes/login.js +++ b/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); } diff --git a/app/controllers/static_controller.rb b/app/controllers/static_controller.rb index dd1198a2f26..fe658a0c210 100644 --- a/app/controllers/static_controller.rb +++ b/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") - return redirect_to(path "/") + if params[:id] == "login" + 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("/") - - 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 - + destination = extract_redirect_param redirect_to(destination, allow_other_host: false) end diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index ea6be324361..a699f64e627 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -86,7 +86,12 @@ class Users::OmniauthCallbacksController < ApplicationController cookies["_bypass_cache"] = true cookies[:authentication_data] = { value: client_hash.to_json, path: Discourse.base_path("/") } - redirect_to @origin + + if !current_user && @origin.start_with?("/user-api-key/new") + redirect_to path("/") + else + redirect_to @origin + end end def valid_origin?(uri) diff --git a/lib/auth/result.rb b/lib/auth/result.rb index 8e83fea8ba2..9b25181003e 100644 --- a/lib/auth/result.rb +++ b/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 diff --git a/spec/requests/static_controller_spec.rb b/spec/requests/static_controller_spec.rb index e57cbe4862e..c5c61ba9738 100644 --- a/spec/requests/static_controller_spec.rb +++ b/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) - json = response.parsed_body - expect(json["errors"]).to be_present - expect(json["errors"]).to include(I18n.t("invalid_params", message: "redirect")) + expect(response).to redirect_to("/") end end diff --git a/spec/system/login_spec.rb b/spec/system/login_spec.rb index 87ed75f0679..adda393e2af 100644 --- a/spec/system/login_spec.rb +++ b/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 diff --git a/spec/system/page_objects/modals/login.rb b/spec/system/page_objects/modals/login.rb index ae7947f95b0..77a7831ab9c 100644 --- a/spec/system/page_objects/modals/login.rb +++ b/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 diff --git a/spec/system/page_objects/pages/login.rb b/spec/system/page_objects/pages/login.rb index d03135f72c7..20817dd67ae 100644 --- a/spec/system/page_objects/pages/login.rb +++ b/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