SECURITY: Attach DiscourseConnect (SSO) nonce to current session (#12124)

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

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}")

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
 

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)

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

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$$!!"

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

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