FIX: move sso provider into its own class so it doesn't interfere with sso client (#6767)

app/controllers/session_controller.rb

@@ -1,5 +1,6 @@
 require_dependency 'rate_limiter'
 require_dependency 'single_sign_on'
+require_dependency 'single_sign_on_provider'
 require_dependency 'url_helper'
 
 class SessionController < ApplicationController
@@ -46,7 +47,7 @@ class SessionController < ApplicationController
     payload ||= request.query_string
 
     if SiteSetting.enable_sso_provider
-      sso = SingleSignOn.parse(payload)
+      sso = SingleSignOnProvider.parse(payload)
 
       if sso.return_sso_url.blank?
         render plain: "return_sso_url is blank, it must be provided", status: 400

lib/single_sign_on.rb

@@ -52,13 +52,13 @@ class SingleSignOn
 
   def self.parse(payload, sso_secret = nil)
     sso = new
+    sso.sso_secret = sso_secret if sso_secret
 
     parsed = Rack::Utils.parse_query(payload)
     decoded = Base64.decode64(parsed["sso"])
     decoded_hash = Rack::Utils.parse_query(decoded)
 
     return_sso_url = decoded_hash['return_sso_url']
-    sso.sso_secret = sso_secret || (provider_secret(return_sso_url) if return_sso_url)
 
     if sso.sign(parsed["sso"]) != parsed["sig"]
       diags = "\n\nsso: #{parsed["sso"]}\n\nsig: #{parsed["sig"]}\n\nexpected sig: #{sso.sign(parsed["sso"])}"
@@ -69,9 +69,6 @@ class SingleSignOn
       end
     end
 
-    decoded = Base64.decode64(parsed["sso"])
-    decoded_hash = Rack::Utils.parse_query(decoded)
-
     ACCESSORS.each do |k|
       val = decoded_hash[k.to_s]
       val = val.to_i if FIXNUMS.include? k
@@ -90,19 +87,6 @@ class SingleSignOn
     sso
   end
 
-  def self.provider_secret(return_sso_url)
-    provider_secrets = SiteSetting.sso_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
-
-    secret = sorted_secrets.select do |domain, _|
-      WildcardDomainChecker.check_domain(domain, return_url_host)
-    end
-    secret.present? ? secret.values.first : nil
-  end
-
   def diagnostics
     SingleSignOn::ACCESSORS.map { |a| "#{a}: #{send(a)}" }.join("\n")
   end
@@ -119,8 +103,8 @@ class SingleSignOn
     @custom_fields ||= {}
   end
 
-  def sign(payload, provider_secret = nil)
+  def sign(payload, secret = nil)
-    secret = provider_secret || sso_secret
+    secret = secret || sso_secret
     OpenSSL::HMAC.hexdigest("sha256", secret, payload)
   end
 
@@ -129,9 +113,9 @@ class SingleSignOn
     "#{base}#{base.include?('?') ? '&' : '?'}#{payload}"
   end
 
-  def payload(provider_secret = nil)
+  def payload(secret = nil)
     payload = Base64.strict_encode64(unsigned_payload)
-    "sso=#{CGI::escape(payload)}&sig=#{sign(payload, provider_secret)}"
+    "sso=#{CGI::escape(payload)}&sig=#{sign(payload, secret)}"
   end
 
   def unsigned_payload

lib/single_sign_on_provider.rb

@@ -0,0 +1,33 @@
+require_dependency 'single_sign_on'
+
+class SingleSignOnProvider < SingleSignOn
+
+  def self.parse(payload, sso_secret = nil)
+    set_return_sso_url(payload)
+
+    super
+  end
+
+  def self.set_return_sso_url(payload)
+    parsed = Rack::Utils.parse_query(payload)
+    decoded = Base64.decode64(parsed["sso"])
+    decoded_hash = Rack::Utils.parse_query(decoded)
+
+    @return_sso_url = decoded_hash['return_sso_url']
+  end
+
+  def self.sso_secret
+    return nil unless @return_sso_url && SiteSetting.enable_sso_provider
+
+    provider_secrets = SiteSetting.sso_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
+
+    secret = sorted_secrets.select do |domain, _|
+      WildcardDomainChecker.check_domain(domain, return_url_host)
+    end
+    secret.present? ? secret.values.first : nil
+  end
+end

spec/requests/session_controller_spec.rb

@@ -557,6 +557,38 @@ RSpec.describe SessionController do
       expect(response.status).to eq(419)
     end
 
+    context "when sso provider is enabled" do
+      before do
+        SiteSetting.enable_sso_provider = true
+        SiteSetting.sso_provider_secrets = [
+          "*|secret,forAll",
+          "*.rainbow|wrongSecretForOverRainbow",
+          "www.random.site|secretForRandomSite",
+          "somewhere.over.rainbow|secretForOverRainbow",
+        ].join("\n")
+      end
+
+      it "doesn't break" do
+        sso = get_sso('/hello/world')
+        sso.external_id = '997'
+        sso.sso_url = "http://somewhere.over.com/sso_login"
+        sso.return_sso_url = "http://someurl.com"
+
+        user = Fabricate(:user)
+        user.create_single_sign_on_record(external_id: '997', last_payload: '')
+
+        get "/session/sso_login", params: Rack::Utils.parse_query(sso.payload), headers: headers
+
+        user.single_sign_on_record.reload
+        expect(user.single_sign_on_record.last_payload).to eq(sso.unsigned_payload)
+
+        expect(response).to redirect_to('/hello/world')
+        logged_on_user = Discourse.current_user_provider.new(request.env).current_user
+
+        expect(user.id).to eq(logged_on_user.id)
+      end
+    end
+
     it 'returns the correct error code for invalid signature' do
       sso = get_sso('/hello/world')
       sso.external_id = '997'
@@ -652,7 +684,7 @@ RSpec.describe SessionController do
           "somewhere.over.rainbow|secretForOverRainbow",
         ].join("\n")
 
-        @sso = SingleSignOn.new
+        @sso = SingleSignOnProvider.new
         @sso.nonce = "mynonce"
         @sso.return_sso_url = "http://somewhere.over.rainbow/sso"
 
@@ -684,7 +716,7 @@ RSpec.describe SessionController do
         expect(location).to match(/^http:\/\/somewhere.over.rainbow\/sso/)
 
         payload = location.split("?")[1]
-        sso2 = SingleSignOn.parse(payload)
+        sso2 = SingleSignOnProvider.parse(payload)
 
         expect(sso2.email).to eq(@user.email)
         expect(sso2.name).to eq(@user.name)
@@ -718,7 +750,7 @@ RSpec.describe SessionController do
         expect(location).to match(/^http:\/\/somewhere.over.rainbow\/sso/)
 
         payload = location.split("?")[1]
-        sso2 = SingleSignOn.parse(payload)
+        sso2 = SingleSignOnProvider.parse(payload)
 
         expect(sso2.email).to eq(@user.email)
         expect(sso2.name).to eq(@user.name)
@@ -781,7 +813,7 @@ RSpec.describe SessionController do
         expect(location).to match(/^http:\/\/somewhere.over.rainbow\/sso/)
 
         payload = location.split("?")[1]
-        sso2 = SingleSignOn.parse(payload)
+        sso2 = SingleSignOnProvider.parse(payload)
 
         expect(sso2.avatar_url.blank?).to_not eq(true)
         expect(sso2.profile_background_url.blank?).to_not eq(true)