FEATURE: allow for overlapping DiscourseConnect secrets per domain (#16915)

Previously we limited Discourse Connect provider to 1 secret per domain.

This made it pretty awkward to cycle secrets in environments where config
takes time to propagate

This change allows for the same domain to have multiple secrets

Also fixes internal implementation on DiscourseConnectProvider which was
not thread safe as it leaned on class variables to ferry data around

Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Co-authored-by: David Taylor <david@taylorhq.com>

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

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

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)
+  def self.parse(payload, sso_secret = nil, **init_kwargs)
-    set_return_sso_url(payload)
+    parsed_payload = Rack::Utils.parse_query(payload)
-    if sso_secret.blank? && self.sso_secret.blank?
+    return_sso_url = lookup_return_sso_url(parsed_payload)
-      host = URI.parse(@return_sso_url).host
+
-      Rails.logger.warn("SSO failed; website #{host} is not in the `discourse_connect_provider_secrets` site settings")
+    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)
+  def self.lookup_return_sso_url(parsed_payload)
-    parsed = Rack::Utils.parse_query(payload)
+    decoded = Base64.decode64(parsed_payload["sso"])
-    decoded = Base64.decode64(parsed["sso"])
     decoded_hash = Rack::Utils.parse_query(decoded)
-
+    decoded_hash['return_sso_url']
-    raise ParseError unless decoded_hash.key? 'return_sso_url'
-    @return_sso_url = decoded_hash['return_sso_url']
   end
 
-  def self.sso_secret
+  def self.lookup_sso_secret(return_sso_url, parsed_payload)
-    return nil unless @return_sso_url && SiteSetting.enable_discourse_connect_provider
+    return nil unless return_sso_url && SiteSetting.enable_discourse_connect_provider
 
-    provider_secrets = SiteSetting.discourse_connect_provider_secrets.split(/[|\n]/)
+    return_url_host = URI.parse(return_sso_url).host
-    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
 
-    secret = sorted_secrets.select do |domain, _|
+    provider_secrets = SiteSetting
-      WildcardDomainChecker.check_domain(domain, return_url_host)
+      .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

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