diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 195ad63831e..0c0d7e61057 100644 --- a/app/controllers/session_controller.rb +++ b/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 diff --git a/lib/single_sign_on.rb b/lib/single_sign_on.rb index fe9d2bf1be9..c1f1074b195 100644 --- a/lib/single_sign_on.rb +++ b/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) - secret = provider_secret || sso_secret + def sign(payload, secret = nil) + 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 diff --git a/lib/single_sign_on_provider.rb b/lib/single_sign_on_provider.rb new file mode 100644 index 00000000000..a6b6a9e39d9 --- /dev/null +++ b/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 diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index f81105ec6fe..afc5a1e1de0 100644 --- a/spec/requests/session_controller_spec.rb +++ b/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)