diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 0f0dbe98982..1f7cdc81659 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -59,9 +59,14 @@ class SessionController < ApplicationController end if data[:no_current_user] - cookies[:sso_payload] = payload || request.query_string - redirect_to path("/login") - return + if data[:prompt] == "none" + redirect_to data[:sso_redirect_url], allow_other_host: true + return + else + cookies[:sso_payload] = payload || request.query_string + redirect_to path("/login") + return + end end if request.xhr? @@ -88,6 +93,8 @@ class SessionController < ApplicationController render plain: I18n.t("discourse_connect.login_error"), status: 422 rescue DiscourseConnectProvider::BlankReturnUrl render plain: "return_sso_url is blank, it must be provided", status: 400 + rescue DiscourseConnectProvider::InvalidParameterValueError => e + render plain: I18n.t("discourse_connect.invalid_parameter_value", param: e.param), status: 400 end # For use in development mode only when login options could be limited or disabled. diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index a1fda8e86f0..528b4fff1e8 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2543,6 +2543,7 @@ en: email_error: "An account could not be registered with the email address %{email}. Please contact the site's administrator." missing_secret: "Authentication failed due to missing secret. Contact the site administrators to fix this problem." invite_redeem_failed: "Invite redemption failed. Please contact the site's administrator." + invalid_parameter_value: "Authentication failed due to invalid value for `%{param}` parameter. Contact the site administrators to fix this problem." original_poster: "Original Poster" most_recent_poster: "Most Recent Poster" diff --git a/lib/discourse_connect_base.rb b/lib/discourse_connect_base.rb index f39c2556fb6..4cd51f6a327 100644 --- a/lib/discourse_connect_base.rb +++ b/lib/discourse_connect_base.rb @@ -7,7 +7,6 @@ class DiscourseConnectBase ACCESSORS = %i[ add_groups admin - moderator avatar_force_update avatar_url bio @@ -15,14 +14,17 @@ class DiscourseConnectBase confirmed_2fa email external_id + failed groups locale locale_force_update location logout + moderator name no_2fa_methods nonce + prompt profile_background_url remove_groups require_2fa @@ -40,6 +42,7 @@ class DiscourseConnectBase admin avatar_force_update confirmed_2fa + failed locale_force_update logout moderator diff --git a/lib/discourse_connect_provider.rb b/lib/discourse_connect_provider.rb index 76f46989df7..7625b8b5c47 100644 --- a/lib/discourse_connect_provider.rb +++ b/lib/discourse_connect_provider.rb @@ -5,8 +5,17 @@ class DiscourseConnectProvider < DiscourseConnectBase end class BlankReturnUrl < RuntimeError end + class InvalidParameterValueError < RuntimeError + attr_reader :param + def initialize(param) + @param = param + super("Invalid value for parameter `#{param}`") + end + end def self.parse(payload, sso_secret = nil, **init_kwargs) + # We extract the return_sso_url parameter early; we need the URL's host + # in order to lookup the correct SSO secret in our site settings. parsed_payload = Rack::Utils.parse_query(payload) return_sso_url = lookup_return_sso_url(parsed_payload) @@ -32,7 +41,12 @@ class DiscourseConnectProvider < DiscourseConnectBase raise BlankSecret end - super(payload, sso_secret, **init_kwargs) + sso = super(payload, sso_secret, **init_kwargs) + + # Do general parameter validation now, after signature-verification has succeeded. + raise InvalidParameterValueError.new("prompt") if (sso.prompt != nil) && (sso.prompt != "none") + + sso end def self.lookup_return_sso_url(parsed_payload) diff --git a/lib/second_factor/actions/discourse_connect_provider.rb b/lib/second_factor/actions/discourse_connect_provider.rb index bee50463c35..ecc6ce17e52 100644 --- a/lib/second_factor/actions/discourse_connect_provider.rb +++ b/lib/second_factor/actions/discourse_connect_provider.rb @@ -10,7 +10,22 @@ module SecondFactor::Actions def second_factor_auth_skipped!(params) sso = get_sso(payload(params)) return { logout: true, return_sso_url: sso.return_sso_url } if sso.logout - return { no_current_user: true } if !current_user + if !current_user + if sso.prompt == "none" + # 'prompt=none' was requested, so just return a failed authentication + # without putting up a login dialog and interrogating the user. + sso.failed = true + return( + { + no_current_user: true, + prompt: sso.prompt, + sso_redirect_url: sso.to_url(sso.return_sso_url), + } + ) + end + # ...otherwise, trigger the usual redirect to login dialog. + return { no_current_user: true } + end populate_user_data(sso) sso.confirmed_2fa = true if @opts[:confirmed_2fa_during_login] { sso_redirect_url: sso.to_url(sso.return_sso_url) } diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 81304c1b1a9..b95e4bd6b08 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -1462,7 +1462,7 @@ RSpec.describe SessionController do expect(redirect_query["sig"][0]).to eq(expected_sig) end - it "it fails to log in if secret is wrong" do + it "fails to log in if secret is wrong" do get "/session/sso_provider", params: Rack::Utils.parse_query(@sso.payload("secretForRandomSite")) expect(response.status).to eq(422) @@ -1515,6 +1515,47 @@ RSpec.describe SessionController do expect(sso2.no_2fa_methods).to eq(nil) end + it "fails with a nice error message if `prompt` parameter has an invalid value" do + @sso.prompt = "xyzpdq" + + get "/session/sso_provider", + params: Rack::Utils.parse_query(@sso.payload("secretForOverRainbow")) + + expect(response.status).to eq(400) + expect(response.body).to eq( + I18n.t("discourse_connect.invalid_parameter_value", param: "prompt"), + ) + end + + it "redirects browser to return_sso_url with auth failure when prompt=none is requested and the user is not logged in" do + @sso.prompt = "none" + + get "/session/sso_provider", + params: Rack::Utils.parse_query(@sso.payload("secretForOverRainbow")) + + location = response.header["Location"] + expect(location).to match(%r{^http://somewhere.over.rainbow/sso}) + + payload = location.split("?")[1] + sso2 = DiscourseConnectProvider.parse(payload) + + expect(sso2.failed).to eq(true) + + expect(sso2.email).to eq(nil) + expect(sso2.name).to eq(nil) + expect(sso2.username).to eq(nil) + expect(sso2.external_id).to eq(nil) + expect(sso2.admin).to eq(nil) + expect(sso2.moderator).to eq(nil) + expect(sso2.groups).to eq(nil) + + expect(sso2.avatar_url).to eq(nil) + expect(sso2.profile_background_url).to eq(nil) + expect(sso2.card_background_url).to eq(nil) + expect(sso2.confirmed_2fa).to eq(nil) + expect(sso2.no_2fa_methods).to eq(nil) + end + it "handles non local content correctly" do SiteSetting.avatar_sizes = "100|49" setup_s3