From 619d43ea4759038f0802bd31e457355fb7ad0e72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matt=20Marjanovi=C4=87?= Date: Thu, 28 Sep 2023 04:53:28 -0700 Subject: [PATCH] FEATURE: Add `prompt=none` functionality to SSO Provider protocol (#22393) This commit adds support for an optional `prompt` parameter in the payload of the /session/sso_provider endpoint. If an SSO Consumer adds a `prompt=none` parameter to the encoded/signed `sso` payload, then Discourse will avoid trying to login a not-logged-in user: * If the user is already logged in, Discourse will immediately redirect back to the Consumer with the user's credentials in a signed payload, as usual. * If the user is not logged in, Discourse will immediately redirect back to the Consumer with a signed payload bearing the parameter `failed=true`. This allows the SSO Consumer to simply test whether or not a user is logged in, without forcing the user to try to log in. This is useful when the SSO Consumer allows both anonymous and authenticated access. (E.g., users that are already logged-in to Discourse can be seamlessly logged-in to the Consumer site, and anonymous users can remain anonymous until they explicitly ask to log in.) This feature is similar to the `prompt=none` functionality in an OpenID Connect Authentication Request; see https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest --- app/controllers/session_controller.rb | 13 ++++-- config/locales/server.en.yml | 1 + lib/discourse_connect_base.rb | 5 ++- lib/discourse_connect_provider.rb | 16 ++++++- .../actions/discourse_connect_provider.rb | 17 +++++++- spec/requests/session_controller_spec.rb | 43 ++++++++++++++++++- 6 files changed, 88 insertions(+), 7 deletions(-) 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