diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 26299bcb8fd..15f7d4368f2 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -79,7 +79,7 @@ class SessionController < ApplicationController end rescue DiscourseConnectProvider::BlankSecret render plain: I18n.t("discourse_connect.missing_secret"), status: 400 - rescue DiscourseConnectProvider::ParseError => e + rescue DiscourseConnectProvider::ParseError # Do NOT pass the error text to the client, it would give them the correct signature render plain: I18n.t("discourse_connect.login_error"), status: 422 rescue DiscourseConnectProvider::BlankReturnUrl diff --git a/lib/discourse_connect_base.rb b/lib/discourse_connect_base.rb index f2e7a7b8207..3a503ddcbe3 100644 --- a/lib/discourse_connect_base.rb +++ b/lib/discourse_connect_base.rb @@ -122,9 +122,13 @@ class DiscourseConnectBase @custom_fields ||= {} end + def self.sign(payload, secret) + OpenSSL::HMAC.hexdigest("sha256", secret, payload) + end + def sign(payload, secret = nil) secret = secret || sso_secret - OpenSSL::HMAC.hexdigest("sha256", secret, payload) + self.class.sign(payload, secret) end def to_json diff --git a/lib/discourse_connect_provider.rb b/lib/discourse_connect_provider.rb index 8ddf51975c4..218d081b10a 100644 --- a/lib/discourse_connect_provider.rb +++ b/lib/discourse_connect_provider.rb @@ -4,38 +4,59 @@ class DiscourseConnectProvider < DiscourseConnectBase class BlankSecret < RuntimeError; end class BlankReturnUrl < RuntimeError; end - def self.parse(payload, sso_secret = nil) - set_return_sso_url(payload) - if sso_secret.blank? && self.sso_secret.blank? - host = URI.parse(@return_sso_url).host - Rails.logger.warn("SSO failed; website #{host} is not in the `discourse_connect_provider_secrets` site settings") + def self.parse(payload, sso_secret = nil, **init_kwargs) + parsed_payload = Rack::Utils.parse_query(payload) + return_sso_url = lookup_return_sso_url(parsed_payload) + + raise ParseError if !return_sso_url + + sso_secret ||= lookup_sso_secret(return_sso_url, parsed_payload) + + if sso_secret.blank? + begin + host = URI.parse(return_sso_url).host + Rails.logger.warn("SSO failed; website #{host} is not in the `discourse_connect_provider_secrets` site settings") + rescue StandardError => e + # going for StandardError cause URI::Error may not be enough, eg it parses to something not + # responding to host + Discourse.warn_exception(e, message: "SSO failed; invalid or missing return_sso_url in SSO payload") + end + raise BlankSecret end - super + super(payload, sso_secret, **init_kwargs) end - def self.set_return_sso_url(payload) - parsed = Rack::Utils.parse_query(payload) - decoded = Base64.decode64(parsed["sso"]) + def self.lookup_return_sso_url(parsed_payload) + decoded = Base64.decode64(parsed_payload["sso"]) decoded_hash = Rack::Utils.parse_query(decoded) - - raise ParseError unless decoded_hash.key? 'return_sso_url' - @return_sso_url = decoded_hash['return_sso_url'] + decoded_hash['return_sso_url'] end - def self.sso_secret - return nil unless @return_sso_url && SiteSetting.enable_discourse_connect_provider + def self.lookup_sso_secret(return_sso_url, parsed_payload) + return nil unless return_sso_url && SiteSetting.enable_discourse_connect_provider - provider_secrets = SiteSetting.discourse_connect_provider_secrets.split(/[|\n]/) - provider_secrets_hash = Hash[*provider_secrets] - return_url_host = URI.parse(@return_sso_url).host - # moves wildcard domains to the end of hash - sorted_secrets = provider_secrets_hash.sort_by { |k, _| k }.reverse.to_h + return_url_host = URI.parse(return_sso_url).host - secret = sorted_secrets.select do |domain, _| - WildcardDomainChecker.check_domain(domain, return_url_host) + provider_secrets = SiteSetting + .discourse_connect_provider_secrets + .split("\n") + .map { |row| row.split("|", 2) } + .sort_by { |k, _| k } + .reverse + + first_domain_match = nil + + pair = provider_secrets.find do |domain, configured_secret| + if WildcardDomainChecker.check_domain(domain, return_url_host) + first_domain_match ||= configured_secret + sign(parsed_payload["sso"], configured_secret) == parsed_payload["sig"] + end end - secret.present? ? secret.values.first : nil + + # falls back to a secret which will fail to validate in DiscourseConnectBase + # this ensures error flow is correct + pair.present? ? pair[1] : first_domain_match end end diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index d40472dd8d2..b3a160864ab 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -1188,7 +1188,9 @@ describe SessionController do "*|secret,forAll", "*.rainbow|wrongSecretForOverRainbow", "www.random.site|secretForRandomSite", + "somewhere.over.rainbow|oldSecretForOverRainbow", "somewhere.over.rainbow|secretForOverRainbow", + "somewhere.over.rainbow|newSecretForOverRainbow", ].join("\n") @sso = DiscourseConnectProvider.new @@ -1245,9 +1247,28 @@ describe SessionController do expect(sso2.no_2fa_methods).to eq(nil) end + it "correctly logs in for secondary domain secrets" do + sign_in @user + + get "/session/sso_provider", params: Rack::Utils.parse_query(@sso.payload("newSecretForOverRainbow")) + expect(response.status).to eq(302) + redirect_uri = URI.parse(response.location) + expect(redirect_uri.host).to eq("somewhere.over.rainbow") + redirect_query = CGI.parse(redirect_uri.query) + expected_sig = DiscourseConnectBase.sign(redirect_query["sso"][0], "newSecretForOverRainbow") + expect(redirect_query["sig"][0]).to eq(expected_sig) + + get "/session/sso_provider", params: Rack::Utils.parse_query(@sso.payload("oldSecretForOverRainbow")) + expect(response.status).to eq(302) + redirect_uri = URI.parse(response.location) + expect(redirect_uri.host).to eq("somewhere.over.rainbow") + redirect_query = CGI.parse(redirect_uri.query) + expected_sig = DiscourseConnectBase.sign(redirect_query["sso"][0], "oldSecretForOverRainbow") + expect(redirect_query["sig"][0]).to eq(expected_sig) + end + it "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) end @@ -1263,7 +1284,6 @@ describe SessionController do it "returns a 422 if no return_sso_url" do SiteSetting.discourse_connect_provider_secrets = "abcdefghij" - sso = DiscourseConnectProvider.new get "/session/sso_provider?sso=asdf&sig=abcdefghij" expect(response.status).to eq(422) end