WIP: fixing destination_url #33072

ZogStriP · 2025-06-04T16:23:27Z

Since the introduction of dedicated login and signup pages (as opposed to modals), we've been seeing reports of issues where visitors aren't redirected back to the "page" they were at when they initiated the authentication process.

Since we have a bazillion of ways a user might authenticate (credentials, social logins, SSO, passkeys, discourse connect, etc...), it's really hard to know what a change will impact.

The goal of this PR is to "simplify" the way we handle this "redirection back to origin" by leveraging the use of a single destination_url cookie set on the client-side.

ZogStriP · 2025-06-04T16:30:45Z

app/assets/javascripts/discourse/app/models/login-method.js

@@ -67,6 +68,13 @@ export default class LoginMethod extends EmberObject {
      params["signup"] = true;
    }

+    const destinationUrl = cookie("destination_url");


When using an "external login" method (aka. social logins), we pass the "origin" to the server (omniauth).

ZogStriP · 2025-06-04T16:31:27Z

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

            removeCookie("destination_url");
            window.location.assign(destinationUrl);
-          } else if (this.referrerTopicUrl) {


this.referrerTopic is redundant now that we use the destination_url cookie.

ZogStriP · 2025-06-04T16:32:03Z

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


-          if (ssoDestinationUrl) {


Not 100% sure why this uses a different cookie 🤔 This is the main reason this PR is a draft because I don't understand the consequences of this change.

Curious if @OsamaSayegh or @davidtaylorhq remember the reason for having sso_destination_url vs destination_url.

ZogStriP · 2025-06-04T16:32:30Z

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

@@ -453,7 +453,7 @@ export default class SignupPageController extends Controller {
    const destinationUrl = this.authOptions?.destination_url;

    if (!isEmpty(destinationUrl)) {
-      cookie("destination_url", destinationUrl, { path: "/" });
+      cookie("destination_url", destinationUrl);


{ path: "/" } is the default.

Can we also ensure that destinationUrl is subfolder aware?

Unless there's a piece of logic somewhere else modifying the cookie, it doesn't seem like it takes subfolders into account:

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

Lines 155 to 162 in a9cf9ae

if (ssoDestinationUrl) {

removeCookie("sso_destination_url");

window.location.assign(ssoDestinationUrl);

} else if (destinationUrl) {

removeCookie("destination_url");

window.location.assign(destinationUrl);

} else if (this.referrerTopicUrl) {

window.location.assign(this.referrerTopicUrl);

What makes you think the behavior changed when it comes to handling subfolder? 🤔

ZogStriP · 2025-06-04T16:34:08Z

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

@@ -278,11 +278,12 @@ export default class ApplicationRoute extends DiscourseRoute {

  handleShowCreateAccount(createAccountProps) {
    if (this.siteSettings.enable_discourse_connect) {
-      const returnPath = encodeURIComponent(window.location.pathname);
+      const returnPath = cookie("destination_url")


When Discourse Connect is enabled, we have a slightly different behavior for the return path when there's a destination_url cookie. I applied the same behavior when going through the signup process too.

This is the 2nd reason why this PR is a draft as I'm not 100% sure the impact this change has 🤔

ZogStriP · 2025-06-04T16:35:10Z

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

-        "referrerTopicUrl",
-        this.internalReferrer || document.referrer
-      );
+      cookie("destination_url", this.internalReferrer || document.referrer);


Use the destination_url cookie instead of relying on a controller's property.

Makes sense to me 👍

ZogStriP · 2025-06-04T16:36:02Z

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

@@ -10,7 +12,11 @@ export default class SignupRoute extends DiscourseRoute {

  authComplete = false;

-  beforeModel() {
+  beforeModel({ from }) {
+    if (from) {


Applies the same "store the origin url in a cookie" behavior we do when login in, but for the signup process.

ZogStriP · 2025-06-04T16:36:19Z

config/initializers/009-omniauth.rb

@@ -21,7 +21,7 @@
  request.session[:auth_reconnect] = !!request.params["reconnect"]

  # If the client provided an origin, store in session to redirect back
-  request.session[:destination_url] = request.params["origin"]
+  request.session[:destination_url] = request.params["origin"] if request.params["origin"].present?


Just QoL change.

ZogStriP · 2025-06-04T16:36:36Z

lib/auth/google_oauth2_authenticator.rb

@@ -32,6 +32,9 @@ def register_middleware(omniauth)
          opts[:client_id] = SiteSetting.google_oauth2_client_id
          opts[:client_secret] = SiteSetting.google_oauth2_client_secret

+          # NOTE: uncomment when testing locally
+          # opts[:redirect_uri] = "http://localhost:4200/auth/google_oauth2/callback"


Took me a while to figure that one out :|

I think we can fix this properly: #33102

(and then drop these comments)

pmusaraj

Looks like the right approach to me.

pmusaraj · 2025-06-04T17:04:34Z

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


-          if (ssoDestinationUrl) {


Curious if @OsamaSayegh or @davidtaylorhq remember the reason for having sso_destination_url vs destination_url.

pmusaraj · 2025-06-04T17:07:26Z

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

-        "referrerTopicUrl",
-        this.internalReferrer || document.referrer
-      );
+      cookie("destination_url", this.internalReferrer || document.referrer);


Makes sense to me 👍

pmusaraj

@ZogStriP let's try this out!

Any chance we can add a system spec here for:

visit a topic page or category
click login
auth using an oauth provider
once auth completes, expect user to land in the topic page or category

…rnal login is enabled

ZogStriP commented Jun 4, 2025

View reviewed changes

pmusaraj reviewed Jun 4, 2025

View reviewed changes

ZogStriP force-pushed the fix-destination-url branch from edd138c to 6d92925 Compare June 5, 2025 13:29

pmusaraj approved these changes Jun 6, 2025

View reviewed changes

ZogStriP added 4 commits June 6, 2025 19:55

WIP: fixing destination_url

bc78439

FIX: qunit acceptance test

f96f2fb

FIX: the user_preferences_security_spec

6f8dc45

FIX: prevent loop when there's an error during auth and only one exte…

1f37a23

…rnal login is enabled

ZogStriP force-pushed the fix-destination-url branch from 6d92925 to 1f37a23 Compare June 6, 2025 17:57

	if (ssoDestinationUrl) {
	removeCookie("sso_destination_url");
	window.location.assign(ssoDestinationUrl);
	} else if (destinationUrl) {
	removeCookie("destination_url");
	window.location.assign(destinationUrl);
	} else if (this.referrerTopicUrl) {
	window.location.assign(this.referrerTopicUrl);

WIP: fixing destination_url #33072

Are you sure you want to change the base?

WIP: fixing destination_url #33072

Conversation

ZogStriP commented Jun 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZogStriP Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmusaraj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmusaraj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZogStriP Jun 4, 2025 •

edited

Loading