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

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

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 5815b0c..256da94 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_sso
 
     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("sso.login_error")), status: 422
     end
diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb
index b3079d5..4a1013d 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_sso?
-      sso = DiscourseSingleSignOn.generate_sso(return_path)
+      sso = DiscourseSingleSignOn.generate_sso(return_path, secure_session: secure_session)
       if SiteSetting.verbose_sso_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_sso_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 b28f04b..6ec4cd5 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.sso_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 e1436ce..729b3df 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 fb318d1..961ad51 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.sso_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 sso_overrides_username is set but username is missing or blank' do
     SiteSetting.sso_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.sso_overrides_email = true
     SiteSetting.sso_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
 

[... diff too long, it was truncated ...]

GitHub sha: 5489a1c4

1 Like

This commit has been mentioned on Discourse Meta. There might be relevant details there: