Skip to content

Commit 9f1e741

Browse files
nullchristopmusaraj
nullchristo
and
pmusaraj
authored
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>
1 parent 2f86ded commit 9f1e741

File tree

11 files changed

+270
-32
lines changed

11 files changed

+270
-32
lines changed

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,8 @@ export default class Login extends Component {
137137
} else if (destinationUrl) {
138138
removeCookie("destination_url");
139139
window.location.assign(destinationUrl);
140+
} else if (this.args.model.referrerUrl) {
141+
window.location.assign(this.args.model.referrerUrl);
140142
} else {
141143
window.location.reload();
142144
}
@@ -288,6 +290,8 @@ export default class Login extends Component {
288290
removeCookie("destination_url");
289291

290292
applyHiddenFormInputValue(destinationUrl, "redirect");
293+
} else if (this.args.model.referrerUrl) {
294+
applyHiddenFormInputValue(this.args.model.referrerUrl, "redirect");
291295
} else {
292296
applyHiddenFormInputValue(window.location.href, "redirect");
293297
}

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,8 @@ export default class LoginPageController extends Controller {
160160
} else if (destinationUrl) {
161161
removeCookie("destination_url");
162162
window.location.assign(destinationUrl);
163+
} else if (this.referrerUrl) {
164+
window.location.assign(this.referrerUrl);
163165
} else {
164166
window.location.reload();
165167
}
@@ -337,6 +339,8 @@ export default class LoginPageController extends Controller {
337339
removeCookie("destination_url");
338340

339341
applyHiddenFormInputValue(destinationUrl, "redirect");
342+
} else if (this.referrerUrl) {
343+
applyHiddenFormInputValue(this.referrerUrl, "redirect");
340344
} else {
341345
applyHiddenFormInputValue(window.location.href, "redirect");
342346
}

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -298,6 +298,9 @@ export default class ApplicationRoute extends DiscourseRoute {
298298
showNotActivated: (props) => this.send("showNotActivated", props),
299299
showCreateAccount: (props) => this.send("showCreateAccount", props),
300300
canSignUp: this.controller.canSignUp,
301+
referrerUrl: DiscourseURL.isInternal(document.referrer)
302+
? document.referrer
303+
: null,
301304
},
302305
});
303306
}

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { next } from "@ember/runloop";
22
import { service } from "@ember/service";
3+
import DiscourseURL from "discourse/lib/url";
34
import { defaultHomepage } from "discourse/lib/utilities";
45
import StaticPage from "discourse/models/static-page";
56
import DiscourseRoute from "discourse/routes/discourse";
@@ -9,7 +10,11 @@ export default class LoginRoute extends DiscourseRoute {
910
@service router;
1011
@service login;
1112

12-
beforeModel() {
13+
beforeModel(transition) {
14+
if (transition.from) {
15+
this.internalReferrer = this.router.urlFor(transition.from.name);
16+
}
17+
1318
if (this.siteSettings.login_required) {
1419
if (
1520
this.login.isOnlyOneExternalLoginMethod &&
@@ -49,6 +54,10 @@ export default class LoginRoute extends DiscourseRoute {
4954
controller.set("flashType", "");
5055
controller.set("flash", "");
5156

57+
if (this.internalReferrer || DiscourseURL.isInternal(document.referrer)) {
58+
controller.set("referrerUrl", this.internalReferrer || document.referrer);
59+
}
60+
5261
if (this.siteSettings.login_required) {
5362
controller.set("showLogin", false);
5463
}

‎app/controllers/static_controller.rb

Lines changed: 30 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,36 @@ class StaticController < ApplicationController
2727
}
2828
CUSTOM_PAGES = {} # Add via `#add_topic_static_page` in plugin API
2929

30+
def extract_redirect_param
31+
redirect_path = params[:redirect]
32+
if redirect_path.present?
33+
begin
34+
forum_host = URI(Discourse.base_url).host
35+
uri = URI(redirect_path)
36+
37+
if uri.path.present? && !uri.path.starts_with?(login_path) &&
38+
(uri.host.blank? || uri.host == forum_host) && uri.path =~ %r{\A\/{1}[^\.\s]*\z}
39+
return "#{uri.path}#{uri.query ? "?#{uri.query}" : ""}"
40+
end
41+
rescue URI::Error, ArgumentError
42+
# If the URI is invalid, return "/" below
43+
end
44+
end
45+
46+
"/"
47+
end
48+
3049
def show
31-
if current_user && (params[:id] == "login" || params[:id] == "signup")
32-
return redirect_to(path "/")
50+
if params[:id] == "login"
51+
destination = extract_redirect_param
52+
53+
if current_user
54+
return redirect_to(path(destination), allow_other_host: false)
55+
elsif destination != "/"
56+
cookies[:destination_url] = path(destination)
57+
end
58+
elsif params[:id] == "signup" && current_user
59+
return redirect_to path("/")
3360
end
3461

3562
if SiteSetting.login_required? && current_user.nil? && %w[faq guidelines].include?(params[:id])
@@ -123,26 +150,7 @@ def enter
123150
params.delete(:username)
124151
params.delete(:password)
125152

126-
destination = path("/")
127-
128-
redirect_location = params[:redirect]
129-
if redirect_location.present? && !redirect_location.is_a?(String)
130-
raise Discourse::InvalidParameters.new(:redirect)
131-
elsif redirect_location.present? &&
132-
begin
133-
forum_uri = URI(Discourse.base_url)
134-
uri = URI(redirect_location)
135-
136-
if uri.path.present? && !uri.path.starts_with?(login_path) &&
137-
(uri.host.blank? || uri.host == forum_uri.host) &&
138-
uri.path =~ %r{\A\/{1}[^\.\s]*\z}
139-
destination = "#{uri.path}#{uri.query ? "?#{uri.query}" : ""}"
140-
end
141-
rescue URI::Error
142-
# Do nothing if the URI is invalid
143-
end
144-
end
145-
153+
destination = extract_redirect_param
146154
redirect_to(destination, allow_other_host: false)
147155
end
148156

‎app/controllers/users/omniauth_callbacks_controller.rb

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,12 @@ def complete
8686

8787
cookies["_bypass_cache"] = true
8888
cookies[:authentication_data] = { value: client_hash.to_json, path: Discourse.base_path("/") }
89-
redirect_to @origin
89+
90+
if !current_user && @origin.start_with?("/user-api-key/new")
91+
redirect_to path("/")
92+
else
93+
redirect_to @origin
94+
end
9095
end
9196

9297
def valid_origin?(uri)

‎lib/auth/result.rb

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,8 @@ def apply_associated_attributes!
123123
user.update(associated_group_ids: associated_group_ids)
124124
AssociatedGroup.where(id: associated_group_ids).update_all("last_used = CURRENT_TIMESTAMP")
125125
end
126+
127+
Group.user_trust_level_change!(user.id, user.trust_level) if user && !user.staged
126128
end
127129

128130
def can_edit_name

‎spec/requests/static_controller_spec.rb

Lines changed: 54 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@
150150
end
151151
end
152152

153-
it "should redirect to / when logged in and path is /login" do
153+
it "should redirect to / when logged in and path is /login without redirect" do
154154
sign_in(Fabricate(:user))
155155
get "/login"
156156
expect(response).to redirect_to("/")
@@ -252,6 +252,58 @@
252252
get "/login"
253253
end.to_not change { SiteSetting.title }
254254
end
255+
256+
context "without a subfolder" do
257+
it "redirects as requested when logged in and path is /login with valid redirect param" do
258+
sign_in(Fabricate(:user))
259+
get "/login", params: { redirect: "/foo" }
260+
expect(response).to redirect_to("/foo")
261+
end
262+
263+
it "redirects to / when logged in and path is /login with invalid redirect param" do
264+
sign_in(Fabricate(:user))
265+
get "/login", params: { redirect: "//foo" }
266+
expect(response).to redirect_to("/")
267+
get "/login", params: { redirect: "foo" }
268+
expect(response).to redirect_to("/")
269+
get "/login", params: { redirect: "http://foo" }
270+
expect(response).to redirect_to("/")
271+
get "/login", params: { redirect: "www.foo.bar" }
272+
expect(response).to redirect_to("/")
273+
end
274+
275+
it "sets the destination_url cookie when not logged in and path is /login with valid redirect param" do
276+
get "/login", params: { redirect: "/foo" }
277+
expect(response.cookies["destination_url"]).to eq("/foo")
278+
end
279+
end
280+
281+
context "with a subfolder" do
282+
before { set_subfolder "/sub_test" }
283+
284+
it "redirects as requested when logged in and path is /login with valid redirect param" do
285+
sign_in(Fabricate(:user))
286+
get "/login", params: { redirect: "/foo" }
287+
expect(response).to redirect_to("/sub_test/foo")
288+
end
289+
290+
it "redirects to / when logged in and path is /login with invalid redirect param" do
291+
sign_in(Fabricate(:user))
292+
get "/login", params: { redirect: "//foo" }
293+
expect(response).to redirect_to("/sub_test/")
294+
get "/login", params: { redirect: "foo" }
295+
expect(response).to redirect_to("/sub_test/")
296+
get "/login", params: { redirect: "http://foo" }
297+
expect(response).to redirect_to("/sub_test/")
298+
get "/login", params: { redirect: "www.foo.bar" }
299+
expect(response).to redirect_to("/sub_test/")
300+
end
301+
302+
it "sets the destination_url cookie when not logged in and path is /login with valid redirect param" do
303+
get "/login", params: { redirect: "/foo" }
304+
expect(response.cookies["destination_url"]).to eq("/sub_test/foo")
305+
end
306+
end
255307
end
256308

257309
describe "#enter" do
@@ -307,10 +359,7 @@
307359
context "with an array" do
308360
it "redirects to the root" do
309361
post "/login.json", params: { redirect: ["/foo"] }
310-
expect(response.status).to eq(400)
311-
json = response.parsed_body
312-
expect(json["errors"]).to be_present
313-
expect(json["errors"]).to include(I18n.t("invalid_params", message: "redirect"))
362+
expect(response).to redirect_to("/")
314363
end
315364
end
316365

0 commit comments

Comments
 (0)