diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 68dd6da72ac..69dfc97392e 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -442,7 +442,7 @@ class Admin::UsersController < Admin::AdminController return render body: nil, status: 404 unless SiteSetting.enable_discourse_connect begin - sso = DiscourseSingleSignOn.parse("sso=#{params[:sso]}&sig=#{params[:sig]}") + sso = DiscourseSingleSignOn.parse("sso=#{params[:sso]}&sig=#{params[:sig]}", secure_session: secure_session) rescue DiscourseSingleSignOn::ParseError => e return render json: failed_json.merge(message: I18n.t("discourse_connect.login_error")), status: 422 end diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 0c190147747..1b5cf7c2761 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -25,7 +25,7 @@ class SessionController < ApplicationController cookies.delete(:destination_url) if SiteSetting.enable_discourse_connect? - sso = DiscourseSingleSignOn.generate_sso(return_path) + sso = DiscourseSingleSignOn.generate_sso(return_path, secure_session: secure_session) if SiteSetting.verbose_discourse_connect_logging Rails.logger.warn("Verbose SSO log: Started SSO process\n\n#{sso.diagnostics}") end @@ -144,7 +144,7 @@ class SessionController < ApplicationController params.require(:sig) begin - sso = DiscourseSingleSignOn.parse(request.query_string) + sso = DiscourseSingleSignOn.parse(request.query_string, secure_session: secure_session) rescue DiscourseSingleSignOn::ParseError => e if SiteSetting.verbose_discourse_connect_logging Rails.logger.warn("Verbose SSO log: Signature parse error\n\n#{e.message}\n\n#{sso&.diagnostics}") diff --git a/app/models/discourse_single_sign_on.rb b/app/models/discourse_single_sign_on.rb index c40b04b1b4b..3daa48dee36 100644 --- a/app/models/discourse_single_sign_on.rb +++ b/app/models/discourse_single_sign_on.rb @@ -13,35 +13,39 @@ class DiscourseSingleSignOn < SingleSignOn SiteSetting.discourse_connect_secret end - def self.generate_sso(return_path = "/") - sso = new + def self.generate_sso(return_path = "/", secure_session:) + sso = new(secure_session: secure_session) sso.nonce = SecureRandom.hex sso.register_nonce(return_path) sso.return_sso_url = Discourse.base_url + "/session/sso_login" sso end - def self.generate_url(return_path = "/") - generate_sso(return_path).to_url + def self.generate_url(return_path = "/", secure_session:) + generate_sso(return_path, secure_session: secure_session).to_url + end + + def initialize(secure_session:) + @secure_session = secure_session end def register_nonce(return_path) if nonce - Discourse.cache.write(nonce_key, return_path, expires_in: SingleSignOn.nonce_expiry_time) + @secure_session.set(nonce_key, return_path, expires: SingleSignOn.nonce_expiry_time) end end def nonce_valid? - nonce && Discourse.cache.read(nonce_key).present? + nonce && @secure_session[nonce_key].present? end def return_path - Discourse.cache.read(nonce_key) || "/" + @secure_session[nonce_key] || "/" end def expire_nonce! if nonce - Discourse.cache.delete nonce_key + @secure_session[nonce_key] = nil end end diff --git a/lib/single_sign_on.rb b/lib/single_sign_on.rb index e1436ced9ff..729b3df2006 100644 --- a/lib/single_sign_on.rb +++ b/lib/single_sign_on.rb @@ -61,8 +61,8 @@ class SingleSignOn raise RuntimeError, "sso_url not implemented on class, be sure to set it on instance" end - def self.parse(payload, sso_secret = nil) - sso = new + def self.parse(payload, sso_secret = nil, **init_kwargs) + sso = new(**init_kwargs) sso.sso_secret = sso_secret if sso_secret parsed = Rack::Utils.parse_query(payload) diff --git a/spec/models/discourse_single_sign_on_spec.rb b/spec/models/discourse_single_sign_on_spec.rb index 7867cfd38e1..edf3b856abc 100644 --- a/spec/models/discourse_single_sign_on_spec.rb +++ b/spec/models/discourse_single_sign_on_spec.rb @@ -37,6 +37,10 @@ describe DiscourseSingleSignOn do sso end + def new_discourse_sso + DiscourseSingleSignOn.new(secure_session: secure_session) + end + def test_parsed(parsed, sso) expect(parsed.nonce).to eq sso.nonce expect(parsed.email).to eq sso.email @@ -72,9 +76,10 @@ describe DiscourseSingleSignOn do end let(:ip_address) { "127.0.0.1" } + let(:secure_session) { SecureSession.new("abc") } it "bans bad external id" do - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "test" sso.name = "" sso.email = "test@test.com" @@ -102,7 +107,7 @@ describe DiscourseSingleSignOn do end it "can lookup or create user when name is blank" do - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "test" sso.name = "" sso.email = "test@test.com" @@ -119,7 +124,7 @@ describe DiscourseSingleSignOn do email = "staged@user.com" Fabricate(:user, staged: true, email: email) - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "staged" sso.name = "Bob O'Bob" sso.email = email @@ -136,7 +141,7 @@ describe DiscourseSingleSignOn do context "reviewables" do let(:sso) do - DiscourseSingleSignOn.new.tap do |sso| + new_discourse_sso.tap do |sso| sso.username = "staged" sso.name = "Bob O'Bob" sso.email = "bob@obob.com" @@ -164,7 +169,7 @@ describe DiscourseSingleSignOn do mod_group = Group[:moderators] staff_group = Group[:staff] - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "misteradmin" sso.name = "Bob Admin" sso.email = "admin@admin.com" @@ -186,7 +191,7 @@ describe DiscourseSingleSignOn do group1 = Fabricate(:group, name: 'group1') group2 = Fabricate(:group, name: 'group2') - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "bobsky" sso.name = "Bob" sso.email = user.email @@ -232,7 +237,7 @@ describe DiscourseSingleSignOn do add_group1.add(user) existing_group.save! - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "bobsky" sso.name = "Bob" sso.email = user.email @@ -262,7 +267,7 @@ describe DiscourseSingleSignOn do it 'can override username properly when only the case changes' do SiteSetting.auth_overrides_username = true - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "testuser" sso.name = "test user" sso.email = "test@test.com" @@ -283,7 +288,7 @@ describe DiscourseSingleSignOn do it 'behaves properly when auth_overrides_username is set but username is missing or blank' do SiteSetting.auth_overrides_username = true - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "testuser" sso.name = "test user" sso.email = "test@test.com" @@ -314,7 +319,7 @@ describe DiscourseSingleSignOn do SiteSetting.auth_overrides_email = true SiteSetting.auth_overrides_username = true - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "bob%the$admin" sso.name = "Bob Admin" sso.email = admin.email @@ -365,9 +370,9 @@ describe DiscourseSingleSignOn do end it "validates nonce" do - _ , payload = DiscourseSingleSignOn.generate_url.split("?") + _ , payload = DiscourseSingleSignOn.generate_url(secure_session: secure_session).split("?") - sso = DiscourseSingleSignOn.parse(payload) + sso = DiscourseSingleSignOn.parse(payload, secure_session: secure_session) expect(sso.nonce_valid?).to eq true sso.expire_nonce! @@ -377,10 +382,10 @@ describe DiscourseSingleSignOn do end it "generates a correct sso url" do - url, payload = DiscourseSingleSignOn.generate_url.split("?") + url, payload = DiscourseSingleSignOn.generate_url(secure_session: secure_session).split("?") expect(url).to eq @discourse_connect_url - sso = DiscourseSingleSignOn.parse(payload) + sso = DiscourseSingleSignOn.parse(payload, secure_session: secure_session) expect(sso.nonce).to_not be_nil end @@ -388,7 +393,7 @@ describe DiscourseSingleSignOn do it 'sets default user locale if specified' do SiteSetting.allow_user_locale = true - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "test" sso.name = "test" sso.email = "test@test.com" @@ -416,7 +421,7 @@ describe DiscourseSingleSignOn do context 'trusting emails' do let(:sso) do - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "test" sso.name = "test" sso.email = "test@example.com" @@ -489,7 +494,7 @@ describe DiscourseSingleSignOn do context 'welcome emails' do let(:sso) { - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "test" sso.name = "test" sso.email = "test@example.com" @@ -511,7 +516,7 @@ describe DiscourseSingleSignOn do context 'setting title for a user' do let(:sso) { - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = 'test' sso.name = 'test' sso.email = 'test@test.com' @@ -538,7 +543,7 @@ describe DiscourseSingleSignOn do context 'setting bio for a user' do let(:sso) do - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "test" sso.name = "test" sso.email = "test@test.com" @@ -578,7 +583,7 @@ describe DiscourseSingleSignOn do context 'when discourse_connect_overrides_avatar is not enabled' do it "correctly handles provided avatar_urls" do - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.external_id = 666 sso.email = "sam@sam.com" sso.name = "sam" @@ -639,7 +644,7 @@ describe DiscourseSingleSignOn do fab!(:sso_record) { Fabricate(:single_sign_on_record, external_avatar_url: "http://example.com/an_image.png") } let!(:sso) { - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "test" sso.name = "test" sso.email = sso_record.user.email @@ -686,7 +691,7 @@ describe DiscourseSingleSignOn do context 'when discourse_connect_overrides_profile_background is not enabled' do it "correctly handles provided profile_background_urls" do - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.external_id = 666 sso.email = "sam@sam.com" sso.name = "sam" @@ -719,7 +724,7 @@ describe DiscourseSingleSignOn do fab!(:sso_record) { Fabricate(:single_sign_on_record, external_profile_background_url: "http://example.com/an_image.png") } let!(:sso) { - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "test" sso.name = "test" sso.email = sso_record.user.email @@ -758,7 +763,7 @@ describe DiscourseSingleSignOn do context 'when discourse_connect_overrides_card_background is not enabled' do it "correctly handles provided card_background_urls" do - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.external_id = 666 sso.email = "sam@sam.com" sso.name = "sam" @@ -791,7 +796,7 @@ describe DiscourseSingleSignOn do fab!(:sso_record) { Fabricate(:single_sign_on_record, external_card_background_url: "http://example.com/an_image.png") } let!(:sso) { - sso = DiscourseSingleSignOn.new + sso = new_discourse_sso sso.username = "test" sso.name = "test" sso.email = sso_record.user.email diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index 96b3e482336..68d470eb7d9 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -877,7 +877,7 @@ RSpec.describe Admin::UsersController do sso.email = "bob@bob.com" sso.external_id = "1" - user = DiscourseSingleSignOn.parse(sso.payload).lookup_or_create_user + user = DiscourseSingleSignOn.parse(sso.payload, secure_session: read_secure_session).lookup_or_create_user sso.name = "Bill" sso.username = "Hokli$$!!" diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 84420d9d011..9fd1ae9edd0 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -533,7 +533,7 @@ RSpec.describe SessionController do def get_sso(return_path) nonce = SecureRandom.hex - dso = DiscourseSingleSignOn.new + dso = DiscourseSingleSignOn.new(secure_session: read_secure_session) dso.nonce = nonce dso.register_nonce(return_path) @@ -682,7 +682,7 @@ RSpec.describe SessionController do ScreenedIpAddress.all.destroy_all get "/" sso = sso_for_ip_specs - DiscourseSingleSignOn.parse(sso.payload).lookup_or_create_user(request.remote_ip) + DiscourseSingleSignOn.parse(sso.payload, secure_session: read_secure_session).lookup_or_create_user(request.remote_ip) sso = sso_for_ip_specs _screened_ip = Fabricate(:screened_ip_address, ip_address: request.remote_ip, action_type: ScreenedIpAddress.actions[:block]) @@ -891,6 +891,21 @@ RSpec.describe SessionController do expect(response.status).to eq(419) end + it 'associates the nonce with the current session' do + sso = get_sso('/hello/world') + sso.external_id = '997' + sso.sso_url = "http://somewhere.over.com/sso_login" + + user = Fabricate(:user) + user.create_single_sign_on_record(external_id: '997', last_payload: '') + + # Establish a fresh session + cookies.to_hash.keys.each { |k| cookies.delete(k) } + + get "/session/sso_login", params: Rack::Utils.parse_query(sso.payload), headers: headers + expect(response.status).to eq(419) + end + context "when sso provider is enabled" do before do SiteSetting.enable_discourse_connect_provider = true diff --git a/spec/support/integration_helpers.rb b/spec/support/integration_helpers.rb index ece5b8a686c..182a5514ee4 100644 --- a/spec/support/integration_helpers.rb +++ b/spec/support/integration_helpers.rb @@ -35,6 +35,15 @@ module IntegrationHelpers end def read_secure_session + id = begin + session[:secure_session_id] + rescue NoMethodError + nil + end + + # This route will init the secure_session for us + get "/session/hp.json" if id.nil? + SecureSession.new(session[:secure_session_id]) end end