diff --git a/app/assets/javascripts/discourse/app/controllers/discovery/login-required.js b/app/assets/javascripts/discourse/app/controllers/discovery/login-required.js new file mode 100644 index 00000000000..022c1613c14 --- /dev/null +++ b/app/assets/javascripts/discourse/app/controllers/discovery/login-required.js @@ -0,0 +1,5 @@ +import Controller, { inject as controller } from "@ember/controller"; + +export default class LoginRequiredController extends Controller { + @controller application; +} diff --git a/app/assets/javascripts/discourse/app/lib/homepage-router-overrides.js b/app/assets/javascripts/discourse/app/lib/homepage-router-overrides.js index d8a0def3d53..f78162c61b3 100644 --- a/app/assets/javascripts/discourse/app/lib/homepage-router-overrides.js +++ b/app/assets/javascripts/discourse/app/lib/homepage-router-overrides.js @@ -21,7 +21,7 @@ export default function applyRouterHomepageOverrides(router) { } } -const homepageRewriteParam = "_discourse_homepage_rewrite"; +export const homepageRewriteParam = "_discourse_homepage_rewrite"; /** * Returns a magic URL which `discovery-index` will redirect to. @@ -35,6 +35,7 @@ function rewriteIfNeeded(url, transition) { const intentUrl = transition?.intent?.url; if ( intentUrl?.startsWith(homepageDestination()) || + intentUrl?.startsWith("/login-required") || (transition?.intent.name === `discovery.${defaultHomepage()}` && transition?.intent.queryParams[homepageRewriteParam]) ) { diff --git a/app/assets/javascripts/discourse/app/routes/app-route-map.js b/app/assets/javascripts/discourse/app/routes/app-route-map.js index d83c3dcf779..3321c871b8f 100644 --- a/app/assets/javascripts/discourse/app/routes/app-route-map.js +++ b/app/assets/javascripts/discourse/app/routes/app-route-map.js @@ -68,6 +68,7 @@ export default function () { this.route("category", { path: "/c/*category_slug_path_with_id" }); this.route("custom"); + this.route("login-required"); }); this.route("groups", { resetNamespace: true, path: "/g" }, function () { diff --git a/app/assets/javascripts/discourse/app/routes/application.js b/app/assets/javascripts/discourse/app/routes/application.js index 81f4bcb7cd8..7454793a2ed 100644 --- a/app/assets/javascripts/discourse/app/routes/application.js +++ b/app/assets/javascripts/discourse/app/routes/application.js @@ -271,9 +271,6 @@ export default class ApplicationRoute extends DiscourseRoute { } else { this.router.transitionTo("login").then((login) => { login.controller.set("canSignUp", this.controller.canSignUp); - if (this.siteSettings.login_required) { - login.controller.set("showLogin", true); - } }); } } diff --git a/app/assets/javascripts/discourse/app/routes/discourse.js b/app/assets/javascripts/discourse/app/routes/discourse.js index 8cbeb029ab1..4ffba9a2c76 100644 --- a/app/assets/javascripts/discourse/app/routes/discourse.js +++ b/app/assets/javascripts/discourse/app/routes/discourse.js @@ -36,13 +36,6 @@ export default class DiscourseRoute extends Route { once(this, this._refreshTitleOnce); } - redirectIfLoginRequired() { - const app = this.controllerFor("application"); - if (app.get("loginRequired")) { - this.router.replaceWith("login"); - } - } - isCurrentUser(user) { if (!this.currentUser) { return false; // the current user is anonymous diff --git a/app/assets/javascripts/discourse/app/routes/discovery-index.js b/app/assets/javascripts/discourse/app/routes/discovery-index.js index 5786cde0eb5..48772209e1a 100644 --- a/app/assets/javascripts/discourse/app/routes/discovery-index.js +++ b/app/assets/javascripts/discourse/app/routes/discovery-index.js @@ -1,11 +1,16 @@ import { service } from "@ember/service"; -import { homepageDestination } from "discourse/lib/homepage-router-overrides"; +import { + homepageDestination, + homepageRewriteParam, +} from "discourse/lib/homepage-router-overrides"; import { disableImplicitInjections } from "discourse/lib/implicit-injections"; import DiscourseRoute from "./discourse"; @disableImplicitInjections export default class DiscoveryIndex extends DiscourseRoute { @service router; + @service currentUser; + @service siteSettings; beforeModel(transition) { const url = transition.intent.url; @@ -14,6 +19,11 @@ export default class DiscoveryIndex extends DiscourseRoute { if (params) { destination += `&${params}`; } + + if (this.siteSettings.login_required && !this.currentUser) { + destination = `/login-required?${homepageRewriteParam}=1`; + } + this.router.transitionTo(destination); } } diff --git a/app/assets/javascripts/discourse/app/routes/discovery-login-required.js b/app/assets/javascripts/discourse/app/routes/discovery-login-required.js new file mode 100644 index 00000000000..927432856dc --- /dev/null +++ b/app/assets/javascripts/discourse/app/routes/discovery-login-required.js @@ -0,0 +1,8 @@ +import StaticPage from "discourse/models/static-page"; +import DiscourseRoute from "discourse/routes/discourse"; + +export default class LoginRequiredRoute extends DiscourseRoute { + model() { + return StaticPage.find("login"); + } +} diff --git a/app/assets/javascripts/discourse/app/routes/discovery.js b/app/assets/javascripts/discourse/app/routes/discovery.js index 87d745e4e3c..349b70aa717 100644 --- a/app/assets/javascripts/discourse/app/routes/discovery.js +++ b/app/assets/javascripts/discourse/app/routes/discovery.js @@ -16,10 +16,6 @@ export default class DiscoveryRoute extends DiscourseRoute { filter: { refreshModel: true }, }; - redirect() { - return this.redirectIfLoginRequired(); - } - beforeModel(transition) { const url = transition.intent.url; let matches; diff --git a/app/assets/javascripts/discourse/app/routes/login.js b/app/assets/javascripts/discourse/app/routes/login.js index ac6d35cc3ff..9077202282d 100644 --- a/app/assets/javascripts/discourse/app/routes/login.js +++ b/app/assets/javascripts/discourse/app/routes/login.js @@ -58,10 +58,6 @@ export default class LoginRoute extends DiscourseRoute { ); } - if (this.siteSettings.login_required) { - controller.set("showLogin", false); - } - if (this.login.isOnlyOneExternalLoginMethod) { if (this.siteSettings.auth_immediately) { controller.set("isRedirectingToExternalAuth", true); diff --git a/app/assets/javascripts/discourse/app/routes/topic.js b/app/assets/javascripts/discourse/app/routes/topic.js index 4ac337ccbdc..e072fda8856 100644 --- a/app/assets/javascripts/discourse/app/routes/topic.js +++ b/app/assets/javascripts/discourse/app/routes/topic.js @@ -48,10 +48,6 @@ export default class TopicRoute extends DiscourseRoute { }; } - redirect() { - return this.redirectIfLoginRequired(); - } - titleToken() { const model = this.modelFor("topic"); if (model) { diff --git a/app/assets/javascripts/discourse/app/templates/discovery/login-required.gjs b/app/assets/javascripts/discourse/app/templates/discovery/login-required.gjs new file mode 100644 index 00000000000..201796744b7 --- /dev/null +++ b/app/assets/javascripts/discourse/app/templates/discovery/login-required.gjs @@ -0,0 +1,62 @@ +import { hash } from "@ember/helper"; +import { htmlSafe } from "@ember/template"; +import RouteTemplate from "ember-route-template"; +import DButton from "discourse/components/d-button"; +import PluginOutlet from "discourse/components/plugin-outlet"; +import bodyClass from "discourse/helpers/body-class"; +import hideApplicationHeaderButtons from "discourse/helpers/hide-application-header-buttons"; +import hideApplicationSidebar from "discourse/helpers/hide-application-sidebar"; +import routeAction from "discourse/helpers/route-action"; + +export default RouteTemplate( + +); diff --git a/app/assets/javascripts/discourse/app/templates/login.gjs b/app/assets/javascripts/discourse/app/templates/login.gjs index c180854188d..89b0e18dd3e 100644 --- a/app/assets/javascripts/discourse/app/templates/login.gjs +++ b/app/assets/javascripts/discourse/app/templates/login.gjs @@ -1,8 +1,7 @@ import { hash } from "@ember/helper"; import { htmlSafe } from "@ember/template"; import RouteTemplate from "ember-route-template"; -import { and, not, or } from "truth-helpers"; -import DButton from "discourse/components/d-button"; +import { and } from "truth-helpers"; import FlashMessage from "discourse/components/flash-message"; import LocalLoginForm from "discourse/components/local-login-form"; import LoginButtons from "discourse/components/login-buttons"; @@ -14,7 +13,6 @@ import concatClass from "discourse/helpers/concat-class"; import hideApplicationHeaderButtons from "discourse/helpers/hide-application-header-buttons"; import hideApplicationSidebar from "discourse/helpers/hide-application-sidebar"; import loadingSpinner from "discourse/helpers/loading-spinner"; -import routeAction from "discourse/helpers/route-action"; import { i18n } from "discourse-i18n"; export default RouteTemplate( @@ -28,201 +26,143 @@ export default RouteTemplate( {{! authentication method and is being automatically redirected to it }} {{loadingSpinner}} {{else}} - {{#if - (or @controller.showLogin (not @controller.siteSettings.login_required)) - }} - {{! Show the full page login form }} -
- + + +
+ -
- - - {{#if @controller.hasNoLoginOptions}} -
- + {{#if @controller.hasNoLoginOptions}} +
+ - {{else}} - {{#if @controller.site.mobileView}} - - + {{else}} + {{#if @controller.site.mobileView}} + + + + {{#if @controller.showLoginButtons}} + + {{/if}} + {{/if}} + + {{#if @controller.canLoginLocal}} +
+ {{#if @controller.site.desktopView}} + + + + {{/if}} + + {{#if @controller.site.desktopView}} + - - {{#if @controller.showLoginButtons}} + {{/if}} +
+ {{/if}} + + {{#if + (and @controller.showLoginButtons @controller.site.desktopView) + }} + {{#unless @controller.canLoginLocal}} + + {{/unless}} + {{#if @controller.hasAtLeastOneLoginButton}} + + {{#if @controller.site.mobileView}} + {{#unless @controller.hasNoLoginOptions}} + + {{/unless}} + {{/if}}
- - {{else}} - {{! Show the login-required splash screen }} - {{bodyClass "static-login"}} -
-
- -
-
- {{/if}} +
{{/if}} ); diff --git a/app/assets/javascripts/discourse/tests/acceptance/login-required-test.js b/app/assets/javascripts/discourse/tests/acceptance/login-required-test.js index 85c600e0262..f78b0f7ead6 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/login-required-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/login-required-test.js @@ -9,8 +9,8 @@ acceptance("Login Required - Full page login", function (needs) { await visit("/"); assert.strictEqual( currentRouteName(), - "login", - "it redirects them to login" + "discovery.login-required", + "it shows the login required splash" ); await click(".login-button"); diff --git a/app/assets/javascripts/discourse/tests/acceptance/static-test.js b/app/assets/javascripts/discourse/tests/acceptance/static-test.js index c89102f576a..ab4d1852ac6 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/static-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/static-test.js @@ -43,11 +43,19 @@ acceptance("Static pages", function () { test("Login-required page", async function (assert) { this.siteSettings.login_required = true; - await visit("/login"); + await visit("/"); - assert.strictEqual(currentRouteName(), "login"); + assert.strictEqual(currentRouteName(), "discovery.login-required"); assert.dom(".body-page").exists("The content is present"); assert.dom(".sign-up-button").exists(); assert.dom(".login-button").exists(); }); + + test("Login-required - Login Route", async function (assert) { + this.siteSettings.login_required = true; + await visit("/login"); + + assert.strictEqual(currentRouteName(), "login"); + assert.dom(".login-fullpage").exists("The login full page form is shown"); + }); }); diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 84a6ec562bb..9952dae0167 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -753,6 +753,7 @@ class ApplicationController < ActionController::Base cookies[:destination_url] = destination_url redirect_to path("/auth/#{Discourse.enabled_authenticators.first.name}") else + return if request.path == path("/") && !cookies[:authentication_data] # save original URL in a cookie (javascript redirects after login in this case) cookies[:destination_url] = destination_url redirect_to path("/login") diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index f17f7cc2616..f6837f1fe55 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -18,6 +18,7 @@ class CategoriesController < ApplicationController before_action :fetch_category, only: %i[show update destroy visible_groups] before_action :initialize_staff_action_logger, only: %i[create update destroy] + skip_before_action :check_xhr, only: %i[ index diff --git a/app/controllers/custom_homepage_controller.rb b/app/controllers/custom_homepage_controller.rb deleted file mode 100644 index a2473beffe2..00000000000 --- a/app/controllers/custom_homepage_controller.rb +++ /dev/null @@ -1,9 +0,0 @@ -# frozen_string_literal: true - -class CustomHomepageController < ApplicationController - skip_before_action :check_xhr, only: [:index] - - def index - respond_to { |format| format.html { render :index } } - end -end diff --git a/app/controllers/home_page_controller.rb b/app/controllers/home_page_controller.rb new file mode 100644 index 00000000000..f2f9f1acf9b --- /dev/null +++ b/app/controllers/home_page_controller.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class HomePageController < ApplicationController + skip_before_action :check_xhr, only: %i[custom blank] + + def custom + respond_to { |format| format.html { render :custom } } + end + + def blank + respond_to { |format| format.html { render "default/blank" } } + end +end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 72a22ec0806..fc49b2788ad 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -456,6 +456,8 @@ module ApplicationHelper if current_user && !crawler_layout? params.key?(:print) else + return false if !current_user && SiteSetting.login_required? + crawler_layout? || !mobile_view? || !modern_mobile_device? end end diff --git a/app/views/default/blank.html.erb b/app/views/default/blank.html.erb new file mode 100644 index 00000000000..e69de29bb2d diff --git a/app/views/custom_homepage/index.erb b/app/views/home_page/custom.erb similarity index 100% rename from app/views/custom_homepage/index.erb rename to app/views/home_page/custom.erb diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index ee76fc80b07..048a01ab0db 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -4620,7 +4620,7 @@ en: offline_page_message: "It looks like you are offline! Please check your network connection and try again." login_required: - welcome_message: "# [Welcome to %{title}](#welcome)" + welcome_message: "# Welcome to %{title}" upload: edit_reason: "downloaded local copies of images" diff --git a/config/routes.rb b/config/routes.rb index 4465269db40..e12f3170d35 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -551,6 +551,7 @@ Discourse::Application.routes.draw do post "login" => "static#enter" get "login" => "static#show", :id => "login" + get "login-required" => "static#show", :id => "login" get "login-preferences" => "static#show", :id => "login" get "signup" => "static#show", :id => "signup" get "password-reset" => "static#show", :id => "password_reset" @@ -1707,11 +1708,13 @@ Discourse::Application.routes.draw do constraints: HomePageConstraint.new("finish_installation"), as: "installation_redirect" - root to: "custom_homepage#index", + root to: "home_page#custom", constraints: HomePageConstraint.new("custom"), - as: "custom_index" + as: "home_page_custom" - get "/custom" => "custom_homepage#index" + root to: "home_page#blank", constraints: HomePageConstraint.new("blank"), as: "home_page_blank" + + get "/custom" => "home_page#custom" get "/user-api-key/new" => "user_api_keys#new" post "/user-api-key" => "user_api_keys#create" diff --git a/lib/homepage_helper.rb b/lib/homepage_helper.rb index 93919cf04ab..8c301970f55 100644 --- a/lib/homepage_helper.rb +++ b/lib/homepage_helper.rb @@ -2,6 +2,8 @@ class HomepageHelper def self.resolve(request = nil, current_user = nil) + return "blank" if !current_user && SiteSetting.login_required? + return "custom" if ThemeModifierHelper.new(request: request).custom_homepage enabled = false diff --git a/spec/lib/homepage_helper_spec.rb b/spec/lib/homepage_helper_spec.rb index 334f20ce2b2..b35bde83c2a 100644 --- a/spec/lib/homepage_helper_spec.rb +++ b/spec/lib/homepage_helper_spec.rb @@ -48,5 +48,17 @@ RSpec.describe HomepageHelper do expect(HomepageHelper.resolve).to eq("top") end end + + context "with login required" do + before do + SiteSetting.login_required = true + SiteSetting.top_menu = "new|top|latest|unread" + end + + it "returns a blank route for anon, first result from top menu for authenticated user" do + expect(HomepageHelper.resolve).to eq("blank") + expect(HomepageHelper.resolve(nil, user)).to eq("new") + end + end end end diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index 230af6201fa..4a5c7048b43 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -14,9 +14,10 @@ RSpec.describe ApplicationController do expect(response.headers["Cache-Control"]).to eq("no-cache, no-store") end - it "should redirect to login normally" do + it "should not redirect to login" do get "/" - expect(response).to redirect_to("/login") + expect(response).not_to redirect_to("/login") + expect(response.status).to eq(200) end it "should redirect to SSO if enabled" do @@ -27,10 +28,11 @@ RSpec.describe ApplicationController do end it "should redirect to authenticator if only one, and local logins disabled" do - # Local logins and google enabled, direct to login UI + # Local logins and google enabled, show login UI SiteSetting.enable_google_oauth2_logins = true get "/" - expect(response).to redirect_to("/login") + expect(response).not_to redirect_to("/login") + expect(response.status).to eq(200) # Only google enabled, login immediately SiteSetting.enable_local_logins = false @@ -40,7 +42,8 @@ RSpec.describe ApplicationController do # Google and GitHub enabled, direct to login UI SiteSetting.enable_github_logins = true get "/" - expect(response).to redirect_to("/login") + expect(response).not_to redirect_to("/login") + expect(response.status).to eq(200) end it "should not redirect to SSO when auth_immediately is disabled" do @@ -49,7 +52,8 @@ RSpec.describe ApplicationController do SiteSetting.enable_discourse_connect = true get "/" - expect(response).to redirect_to("/login") + expect(response).not_to redirect_to("/login") + expect(response.status).to eq(200) end it "should not redirect to authenticator when auth_immediately is disabled" do @@ -58,7 +62,8 @@ RSpec.describe ApplicationController do SiteSetting.enable_local_logins = false get "/" - expect(response).to redirect_to("/login") + expect(response).not_to redirect_to("/login") + expect(response.status).to eq(200) end context "with omniauth in test mode" do diff --git a/spec/requests/custom_homepage_controller_spec.rb b/spec/requests/home_page_controller_spec.rb similarity index 95% rename from spec/requests/custom_homepage_controller_spec.rb rename to spec/requests/home_page_controller_spec.rb index 6b9213c2983..6ab95889d28 100644 --- a/spec/requests/custom_homepage_controller_spec.rb +++ b/spec/requests/home_page_controller_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true -RSpec.describe CustomHomepageController do - describe "#index" do +RSpec.describe HomePageController do + describe "#custom" do context "with crawler view" do it "should display the menu by default" do get "/custom", headers: { "HTTP_USER_AGENT" => "Googlebot" } diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb index 8c5afc40b28..dcccee56870 100644 --- a/spec/requests/list_controller_spec.rb +++ b/spec/requests/list_controller_spec.rb @@ -356,6 +356,20 @@ RSpec.describe ListController do expect(response.parsed_body["topic_list"]["categories"]).to eq(nil) end end + + context "when login required" do + before do + SiteSetting.login_required = true + SiteSetting.bootstrap_mode_enabled = false + SiteSetting.has_login_hint = false + end + + it "returns nothing from topic list on homepage for login required" do + get "/" + expect(response.status).to eq(200) + expect(response.body).not_to include(topic.title) + end + end end describe "categories and X" do diff --git a/spec/system/login_spec.rb b/spec/system/login_spec.rb index 7ecbdf73629..f3f07e189a7 100644 --- a/spec/system/login_spec.rb +++ b/spec/system/login_spec.rb @@ -7,6 +7,9 @@ shared_examples "login scenarios" do |login_page_object| let(:activate_account) { PageObjects::Pages::ActivateAccount.new } let(:user_preferences_security_page) { PageObjects::Pages::UserPreferencesSecurity.new } fab!(:user) { Fabricate(:user, username: "john", password: "supersecurepassword") } + fab!(:category) + fab!(:topic) { Fabricate(:topic, user: user, category: category) } + fab!(:topic2) { Fabricate(:topic, user: user) } fab!(:admin) { Fabricate(:admin, username: "admin", password: "supersecurepassword") } let(:user_menu) { PageObjects::Components::UserMenu.new } @@ -134,13 +137,30 @@ shared_examples "login scenarios" do |login_page_object| 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 + + it "does not leak topics" do + visit "/" + + expect(page).to have_css(".login-welcome") + + expect(page.body).not_to include(topic.title) + expect(page.body).not_to include(topic2.title) + end + + it "does not leak category metadata if homepage is /categories" do + SiteSetting.top_menu = "categories|latest|new|unread|top" + visit "/" + + expect(page).to have_css(".login-welcome") + + expect(page.body).not_to include(category.name) + end end context "when login is not required" do @@ -359,11 +379,11 @@ shared_examples "login scenarios" do |login_page_object| end describe "Login", type: :system do - context "when fullpage desktop" do + context "when desktop" do include_examples "login scenarios", PageObjects::Pages::Login.new end - context "when fullpage mobile", mobile: true do + context "when mobile", mobile: true do include_examples "login scenarios", PageObjects::Pages::Login.new end end diff --git a/spec/system/social_authentication_spec.rb b/spec/system/social_authentication_spec.rb index 2625b3b5f14..80d1cc15e28 100644 --- a/spec/system/social_authentication_spec.rb +++ b/spec/system/social_authentication_spec.rb @@ -272,8 +272,7 @@ shared_examples "social authentication scenarios" do |signup_page_object, login_ mock_google_auth visit("/login") - expect(page).to have_css(".login-welcome") - expect(page).to have_css(".site-logo") + expect(page).to have_css(".btn-social") visit("/") expect(page).to have_css(".login-welcome")