From b4e4833d2a693537c5cbc47f74a475473a2b8639 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= Date: Thu, 18 Sep 2025 14:28:00 +0200 Subject: [PATCH] DEV: Rename `SecureSession` to `ServerSession` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch will be followed by https://github.com/discourse/discourse/pull/34747. `SecureSession` doesn’t make a lot of sense anymore and can be confusing as the current cookie store used for the session is actually secure since it’s encrypted. Renaming it to `ServerSession` better conveys what it does: providing a session but on the server side only. This patch also makes some improvements, like injecting that server session into Rack-like request objects, allowing the server session to be available virtually everywhere. --- app/controllers/application_controller.rb | 7 ++- app/controllers/session_controller.rb | 8 +-- lib/discourse_webauthn.rb | 20 +++---- lib/discourse_webauthn/challenge_generator.rb | 4 +- lib/freedom_patches/request_server_session.rb | 10 ++++ lib/{secure_session.rb => server_session.rb} | 29 +++++----- .../lib/concern/second_factor_manager_spec.rb | 53 ++++++++++--------- .../authentication_service_spec.rb | 6 +-- .../challenge_generator_spec.rb | 8 +-- .../discourse_webauthn_spec.rb | 18 +++---- .../registration_service_spec.rb | 10 ++-- spec/lib/secure_session_spec.rb | 27 ---------- spec/lib/server_session_spec.rb | 26 +++++++++ spec/models/discourse_connect_spec.rb | 32 +++++------ spec/requests/session_controller_spec.rb | 4 +- spec/requests/users_controller_spec.rb | 15 +++--- spec/support/integration_helpers.rb | 4 +- 17 files changed, 145 insertions(+), 136 deletions(-) create mode 100644 lib/freedom_patches/request_server_session.rb rename lib/{secure_session.rb => server_session.rb} (59%) delete mode 100644 spec/lib/secure_session_spec.rb create mode 100644 spec/lib/server_session_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index a52c9d2c8f0..408a46ae486 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -14,6 +14,9 @@ class ApplicationController < ActionController::Base attr_reader :theme_id + delegate :server_session, to: :request + alias_method :secure_session, :server_session + serialization_scope :guardian protect_from_forgery @@ -568,10 +571,6 @@ class ApplicationController < ActionController::Base request.session_options[:skip] = true end - def secure_session - SecureSession.new(session["secure_session_id"] ||= SecureRandom.hex) - end - def handle_permalink(path) permalink = Permalink.find_by_url(path) if permalink && permalink.target_url diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 6ca52feb269..ab7641d4b68 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -36,7 +36,7 @@ class SessionController < ApplicationController session.delete(:destination_url) cookies.delete(:destination_url) - sso = DiscourseConnect.generate_sso(return_path, secure_session: secure_session) + sso = DiscourseConnect.generate_sso(return_path, secure_session:) connect_verbose_warn { "Verbose SSO log: Started SSO process\n\n#{sso.diagnostics}" } redirect_to sso_url(sso), allow_other_host: true end @@ -698,13 +698,13 @@ class SessionController < ApplicationController end def get_honeypot_value - secure_session.set(HONEYPOT_KEY, honeypot_value, expires: 1.hour) - secure_session.set(CHALLENGE_KEY, challenge_value, expires: 1.hour) + server_session[HONEYPOT_KEY] = honeypot_value + server_session[CHALLENGE_KEY] = challenge_value render json: { value: honeypot_value, challenge: challenge_value, - expires_in: SecureSession.expiry, + expires_in: ServerSession.expiry, } end diff --git a/lib/discourse_webauthn.rb b/lib/discourse_webauthn.rb index 8bf603fe101..6c302137b9c 100644 --- a/lib/discourse_webauthn.rb +++ b/lib/discourse_webauthn.rb @@ -67,10 +67,10 @@ module DiscourseWebauthn # credentials. # # @param user [User] the user to stage the challenge for - # @param secure_session [SecureSession] the session to store the challenge in - def self.stage_challenge(user, secure_session) + # @param server_session [ServerSession] the session to store the challenge in + def self.stage_challenge(user, server_session) ::DiscourseWebauthn::ChallengeGenerator.generate.commit_to_session( - secure_session, + server_session, user, expires: CHALLENGE_EXPIRY, ) @@ -80,22 +80,22 @@ module DiscourseWebauthn # Clears the challenge from the user's secure session. # # @param user [User] the user to clear the challenge for - # @param secure_session [SecureSession] the session to clear the challenge from - def self.clear_challenge(user, secure_session) - secure_session[self.session_challenge_key(user)] = nil + # @param server_session [ServerSession] the session to clear the challenge from + def self.clear_challenge(user, server_session) + server_session[self.session_challenge_key(user)] = nil end - def self.allowed_credentials(user, secure_session) + def self.allowed_credentials(user, server_session) return {} if !user.security_keys_enabled? { allowed_credential_ids: user.second_factor_security_key_credential_ids, - challenge: self.challenge(user, secure_session), + challenge: self.challenge(user, server_session), } end - def self.challenge(user, secure_session) - secure_session[self.session_challenge_key(user)] + def self.challenge(user, server_session) + server_session[self.session_challenge_key(user)] end def self.rp_id diff --git a/lib/discourse_webauthn/challenge_generator.rb b/lib/discourse_webauthn/challenge_generator.rb index dcc3f064999..086ab30afba 100644 --- a/lib/discourse_webauthn/challenge_generator.rb +++ b/lib/discourse_webauthn/challenge_generator.rb @@ -8,8 +8,8 @@ module DiscourseWebauthn @challenge = params[:challenge] end - def commit_to_session(secure_session, user, expires: nil) - secure_session.set(DiscourseWebauthn.session_challenge_key(user), @challenge, expires:) + def commit_to_session(server_session, user, expires: server_session.expiry) + server_session.set(DiscourseWebauthn.session_challenge_key(user), @challenge, expires:) self end end diff --git a/lib/freedom_patches/request_server_session.rb b/lib/freedom_patches/request_server_session.rb new file mode 100644 index 00000000000..6a850279027 --- /dev/null +++ b/lib/freedom_patches/request_server_session.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +module RequestServerSession + def server_session + session[:server_session_id] ||= (session.delete(:secure_session_id) || SecureRandom.hex) + ServerSession.new(session[:server_session_id]) + end +end + +Rack::Request::Helpers.prepend(RequestServerSession) diff --git a/lib/secure_session.rb b/lib/server_session.rb similarity index 59% rename from lib/secure_session.rb rename to lib/server_session.rb index eedddfa37ae..456a6911296 100644 --- a/lib/secure_session.rb +++ b/lib/server_session.rb @@ -1,21 +1,24 @@ # frozen_string_literal: true -# session that is not stored in cookie, expires after 1.hour unconditionally -class SecureSession +# Session that is not stored in cookie, expires after 1.hour unconditionally +class ServerSession + delegate :expiry, to: :class + + class << self + def expiry + @expiry ||= 1.hour.to_i + end + + def expiry=(val) + @expiry = val + end + end + def initialize(prefix) @prefix = prefix end - def self.expiry - @expiry ||= 1.hour.to_i - end - - def self.expiry=(val) - @expiry = val - end - - def set(key, val, expires: nil) - expires ||= SecureSession.expiry + def set(key, val, expires: expiry) Discourse.redis.setex(prefixed_key(key), expires.to_i, val.to_s) true end @@ -32,7 +35,7 @@ class SecureSession if val == nil Discourse.redis.del(prefixed_key(key)) else - Discourse.redis.setex(prefixed_key(key), SecureSession.expiry.to_i, val.to_s) + Discourse.redis.setex(prefixed_key(key), expiry.to_i, val.to_s) end end diff --git a/spec/lib/concern/second_factor_manager_spec.rb b/spec/lib/concern/second_factor_manager_spec.rb index d29ad6b9c01..765a538d5cc 100644 --- a/spec/lib/concern/second_factor_manager_spec.rb +++ b/spec/lib/concern/second_factor_manager_spec.rb @@ -165,16 +165,17 @@ RSpec.describe SecondFactorManager do describe "#authenticate_second_factor" do let(:params) { {} } - let(:secure_session) { SecureSession.new("some-prefix") } + let(:server_session) { ServerSession.new("some-prefix") } context "when neither security keys nor totp/backup codes are enabled" do before { disable_security_key && disable_totp } + it "returns OK, because it doesn't need to authenticate" do - expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true) + expect(user.authenticate_second_factor(params, server_session).ok).to eq(true) end it "keeps used_2fa_method nil because no authentication is done" do - expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq(nil) + expect(user.authenticate_second_factor(params, server_session).used_2fa_method).to eq(nil) end end @@ -182,7 +183,7 @@ RSpec.describe SecondFactorManager do before do disable_totp simulate_localhost_webauthn_challenge - DiscourseWebauthn.stage_challenge(user, secure_session) + DiscourseWebauthn.stage_challenge(user, server_session) DiscourseWebauthn.stubs(:origin).returns("http://localhost:3000") end @@ -194,11 +195,11 @@ RSpec.describe SecondFactorManager do } end it "returns OK" do - expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true) + expect(user.authenticate_second_factor(params, server_session).ok).to eq(true) end it "sets used_2fa_method to security keys" do - expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq( + expect(user.authenticate_second_factor(params, server_session).used_2fa_method).to eq( UserSecondFactor.methods[:security_key], ) end @@ -217,7 +218,7 @@ RSpec.describe SecondFactorManager do } end it "returns not OK" do - result = user.authenticate_second_factor(params, secure_session) + result = user.authenticate_second_factor(params, server_session) expect(result.ok).to eq(false) expect(result.error).to eq(I18n.t("webauthn.validation.not_found_error")) expect(result.used_2fa_method).to eq(nil) @@ -236,11 +237,11 @@ RSpec.describe SecondFactorManager do } end it "returns OK" do - expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true) + expect(user.authenticate_second_factor(params, server_session).ok).to eq(true) end it "sets used_2fa_method to totp" do - expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq( + expect(user.authenticate_second_factor(params, server_session).used_2fa_method).to eq( UserSecondFactor.methods[:totp], ) end @@ -251,7 +252,7 @@ RSpec.describe SecondFactorManager do { second_factor_token: "blah", second_factor_method: UserSecondFactor.methods[:totp] } end it "returns not OK" do - result = user.authenticate_second_factor(params, secure_session) + result = user.authenticate_second_factor(params, server_session) expect(result.ok).to eq(false) expect(result.error).to eq(I18n.t("login.invalid_second_factor_code")) expect(result.used_2fa_method).to eq(nil) @@ -265,13 +266,13 @@ RSpec.describe SecondFactorManager do before do simulate_localhost_webauthn_challenge - DiscourseWebauthn.stage_challenge(user, secure_session) + DiscourseWebauthn.stage_challenge(user, server_session) DiscourseWebauthn.stubs(:origin).returns("http://localhost:3000") end context "when method selected is invalid" do it "returns an error" do - result = user.authenticate_second_factor(params, secure_session) + result = user.authenticate_second_factor(params, server_session) expect(result.ok).to eq(false) expect(result.error).to eq(I18n.t("login.invalid_second_factor_method")) expect(result.used_2fa_method).to eq(nil) @@ -286,11 +287,11 @@ RSpec.describe SecondFactorManager do let(:params) { { second_factor_token: token, second_factor_method: method } } it "validates totp OK" do - expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true) + expect(user.authenticate_second_factor(params, server_session).ok).to eq(true) end it "sets used_2fa_method to totp" do - expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq( + expect(user.authenticate_second_factor(params, server_session).used_2fa_method).to eq( UserSecondFactor.methods[:totp], ) end @@ -300,7 +301,7 @@ RSpec.describe SecondFactorManager do before { user.totps.destroy_all } it "returns an error" do - result = user.authenticate_second_factor(params, secure_session) + result = user.authenticate_second_factor(params, server_session) expect(result.ok).to eq(false) expect(result.error).to eq(I18n.t("login.not_enabled_second_factor_method")) expect(result.used_2fa_method).to eq(nil) @@ -314,7 +315,7 @@ RSpec.describe SecondFactorManager do before do simulate_localhost_webauthn_challenge - DiscourseWebauthn.stage_challenge(user, secure_session) + DiscourseWebauthn.stage_challenge(user, server_session) end context "when security key params are valid" do @@ -322,11 +323,11 @@ RSpec.describe SecondFactorManager do { second_factor_token: valid_security_key_auth_post_data, second_factor_method: method } end it "returns OK" do - expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true) + expect(user.authenticate_second_factor(params, server_session).ok).to eq(true) end it "sets used_2fa_method to security keys" do - expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq( + expect(user.authenticate_second_factor(params, server_session).used_2fa_method).to eq( UserSecondFactor.methods[:security_key], ) end @@ -335,7 +336,7 @@ RSpec.describe SecondFactorManager do before { user.security_keys.destroy_all } it "returns an error" do - result = user.authenticate_second_factor(params, secure_session) + result = user.authenticate_second_factor(params, server_session) expect(result.ok).to eq(false) expect(result.error).to eq(I18n.t("login.not_enabled_second_factor_method")) expect(result.used_2fa_method).to eq(nil) @@ -355,11 +356,11 @@ RSpec.describe SecondFactorManager do context "when backup codes enabled" do it "validates codes OK" do - expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true) + expect(user.authenticate_second_factor(params, server_session).ok).to eq(true) end it "sets used_2fa_method to backup codes" do - expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq( + expect(user.authenticate_second_factor(params, server_session).used_2fa_method).to eq( UserSecondFactor.methods[:backup_codes], ) end @@ -369,7 +370,7 @@ RSpec.describe SecondFactorManager do before { user.user_second_factors.backup_codes.destroy_all } it "returns an error" do - result = user.authenticate_second_factor(params, secure_session) + result = user.authenticate_second_factor(params, server_session) expect(result.ok).to eq(false) expect(result.error).to eq(I18n.t("login.not_enabled_second_factor_method")) expect(result.used_2fa_method).to eq(nil) @@ -387,11 +388,11 @@ RSpec.describe SecondFactorManager do end it "validates the security key OK" do - expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true) + expect(user.authenticate_second_factor(params, server_session).ok).to eq(true) end it "sets used_2fa_method to security keys" do - expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq( + expect(user.authenticate_second_factor(params, server_session).used_2fa_method).to eq( UserSecondFactor.methods[:security_key], ) end @@ -406,11 +407,11 @@ RSpec.describe SecondFactorManager do end it "validates totp OK" do - expect(user.authenticate_second_factor(params, secure_session).ok).to eq(true) + expect(user.authenticate_second_factor(params, server_session).ok).to eq(true) end it "sets used_2fa_method to totp" do - expect(user.authenticate_second_factor(params, secure_session).used_2fa_method).to eq( + expect(user.authenticate_second_factor(params, server_session).used_2fa_method).to eq( UserSecondFactor.methods[:totp], ) end diff --git a/spec/lib/discourse_webauthn/authentication_service_spec.rb b/spec/lib/discourse_webauthn/authentication_service_spec.rb index ccb851e13f8..47fcb38e33d 100644 --- a/spec/lib/discourse_webauthn/authentication_service_spec.rb +++ b/spec/lib/discourse_webauthn/authentication_service_spec.rb @@ -58,7 +58,7 @@ RSpec.describe DiscourseWebauthn::AuthenticationService do let(:credential_id) do "mJAJ4CznTO0SuLkJbYwpgK75ao4KMNIPlU5KWM92nq39kRbXzI9mSv6GxTcsMYoiPgaouNw7b7zBiS4vsQaO6A==" end - let(:secure_session) { SecureSession.new("tester") } + let(:server_session) { ServerSession.new("tester") } let(:challenge) { "81d4acfbd69eafa8f02bc2ecbec5267be8c9b28c1e0ba306d52b79f0f13d" } let(:client_data_challenge) { Base64.strict_encode64(challenge) } let(:client_data_webauthn_type) { "webauthn.get" } @@ -92,7 +92,7 @@ RSpec.describe DiscourseWebauthn::AuthenticationService do end let(:options) do - { session: secure_session, factor_type: UserSecurityKey.factor_types[:second_factor] } + { session: server_session, factor_type: UserSecurityKey.factor_types[:second_factor] } end let(:current_user) { Fabricate(:user) } @@ -273,7 +273,7 @@ RSpec.describe DiscourseWebauthn::AuthenticationService do describe "authenticating passkeys" do let(:options) do - { factor_type: UserSecurityKey.factor_types[:first_factor], session: secure_session } + { factor_type: UserSecurityKey.factor_types[:first_factor], session: server_session } end ## diff --git a/spec/lib/discourse_webauthn/challenge_generator_spec.rb b/spec/lib/discourse_webauthn/challenge_generator_spec.rb index 4e7df133540..29953d2c650 100644 --- a/spec/lib/discourse_webauthn/challenge_generator_spec.rb +++ b/spec/lib/discourse_webauthn/challenge_generator_spec.rb @@ -10,13 +10,13 @@ RSpec.describe DiscourseWebauthn::ChallengeGenerator do describe "ChallengeSession" do describe "#commit_to_session" do let(:user) { Fabricate(:user) } + let(:server_session) { ServerSession.new("some-prefix") } + let(:generated_session) { DiscourseWebauthn::ChallengeGenerator.generate } it "stores the challenge in the provided session object" do - secure_session = SecureSession.new("some-prefix") - generated_session = DiscourseWebauthn::ChallengeGenerator.generate - generated_session.commit_to_session(secure_session, user) + generated_session.commit_to_session(server_session, user) - expect(secure_session["staged-webauthn-challenge-#{user&.id}"]).to eq( + expect(server_session["staged-webauthn-challenge-#{user&.id}"]).to eq( generated_session.challenge, ) end diff --git a/spec/lib/discourse_webauthn/discourse_webauthn_spec.rb b/spec/lib/discourse_webauthn/discourse_webauthn_spec.rb index f5b6731d4d3..e4c1e001f26 100644 --- a/spec/lib/discourse_webauthn/discourse_webauthn_spec.rb +++ b/spec/lib/discourse_webauthn/discourse_webauthn_spec.rb @@ -17,32 +17,32 @@ RSpec.describe DiscourseWebauthn do end describe ".stage_challenge" do - let(:secure_session) { SecureSession.new("some-prefix") } + let(:server_session) { ServerSession.new("some-prefix") } it "stores the challenge in the provided session object with the right expiry" do - described_class.stage_challenge(user, secure_session) + described_class.stage_challenge(user, server_session) key = described_class.session_challenge_key(user) - expect(secure_session[key]).to be_present + expect(server_session[key]).to be_present - expect(secure_session.ttl(key)).to be_within_one_second_of( + expect(server_session.ttl(key)).to be_within_one_second_of( DiscourseWebauthn::CHALLENGE_EXPIRY, ) end end describe ".clear_challenge" do - let(:secure_session) { SecureSession.new("some-prefix") } + let(:server_session) { ServerSession.new("some-prefix") } it "clears the challenge from the provided session object" do - described_class.stage_challenge(user, secure_session) + described_class.stage_challenge(user, server_session) key = described_class.session_challenge_key(user) - expect(secure_session[key]).to be_present + expect(server_session[key]).to be_present - described_class.clear_challenge(user, secure_session) + described_class.clear_challenge(user, server_session) - expect(secure_session[key]).to be_nil + expect(server_session[key]).to be_nil end end end diff --git a/spec/lib/discourse_webauthn/registration_service_spec.rb b/spec/lib/discourse_webauthn/registration_service_spec.rb index 21816b5e4d7..422dc8f826c 100644 --- a/spec/lib/discourse_webauthn/registration_service_spec.rb +++ b/spec/lib/discourse_webauthn/registration_service_spec.rb @@ -4,7 +4,7 @@ require "discourse_webauthn" RSpec.describe DiscourseWebauthn::RegistrationService do subject(:service) { described_class.new(current_user, params, **options) } - let(:secure_session) { SecureSession.new("tester") } + let(:server_session) { ServerSession.new("tester") } let(:client_data_challenge) { Base64.encode64(challenge) } let(:client_data_webauthn_type) { "webauthn.create" } let(:client_data_origin) { "http://test.localhost" } @@ -34,10 +34,10 @@ RSpec.describe DiscourseWebauthn::RegistrationService do # The above attestation was generated in localhost; Discourse.current_hostname # returns test.localhost which we do not want let(:options) do - { session: secure_session, factor_type: UserSecurityKey.factor_types[:second_factor] } + { session: server_session, factor_type: UserSecurityKey.factor_types[:second_factor] } end - let(:challenge) { DiscourseWebauthn.stage_challenge(current_user, secure_session).challenge } + let(:challenge) { DiscourseWebauthn.stage_challenge(current_user, server_session).challenge } let(:current_user) { Fabricate(:user) } @@ -178,7 +178,7 @@ RSpec.describe DiscourseWebauthn::RegistrationService do describe "registering a second factor key as first factor" do let(:options) do - { factor_type: UserSecurityKey.factor_types[:first_factor], session: secure_session } + { factor_type: UserSecurityKey.factor_types[:first_factor], session: server_session } end it "does not work since second-factor key does not have the user verification flag" do @@ -191,7 +191,7 @@ RSpec.describe DiscourseWebauthn::RegistrationService do describe "registering a passkey" do let(:options) do - { factor_type: UserSecurityKey.factor_types[:first_factor], session: secure_session } + { factor_type: UserSecurityKey.factor_types[:first_factor], session: server_session } end ## diff --git a/spec/lib/secure_session_spec.rb b/spec/lib/secure_session_spec.rb deleted file mode 100644 index 5ff2e3b4a91..00000000000 --- a/spec/lib/secure_session_spec.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe SecureSession do - it "operates correctly" do - s = SecureSession.new("abc") - - s["hello"] = "world" - s["foo"] = "bar" - expect(s["hello"]).to eq("world") - expect(s["foo"]).to eq("bar") - - s["hello"] = nil - expect(s["hello"]).to eq(nil) - end - - it "can override expiry" do - s = SecureSession.new("abc") - key = SecureRandom.hex - - s.set(key, "test2", expires: 5.minutes) - expect(s.ttl(key)).to be_within(1.second).of(5.minutes) - - key = SecureRandom.hex - s.set(key, "test2") - expect(s.ttl(key)).to be_within(1.second).of(SecureSession.expiry) - end -end diff --git a/spec/lib/server_session_spec.rb b/spec/lib/server_session_spec.rb new file mode 100644 index 00000000000..eebe35958d0 --- /dev/null +++ b/spec/lib/server_session_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +RSpec.describe ServerSession do + subject(:session) { described_class.new("abc") } + + it "operates correctly" do + session["hello"] = "world" + session["foo"] = "bar" + expect(session["hello"]).to eq("world") + expect(session["foo"]).to eq("bar") + + session["hello"] = nil + expect(session["hello"]).to be_nil + end + + it "can override expiry" do + key = SecureRandom.hex + + session.set(key, "test2", expires: 5.minutes) + expect(session.ttl(key)).to be_within(1.second).of(5.minutes) + + key = SecureRandom.hex + session.set(key, "test2") + expect(session.ttl(key)).to be_within(1.second).of(described_class.expiry) + end +end diff --git a/spec/models/discourse_connect_spec.rb b/spec/models/discourse_connect_spec.rb index bb17c732523..4b0251e1ad7 100644 --- a/spec/models/discourse_connect_spec.rb +++ b/spec/models/discourse_connect_spec.rb @@ -39,7 +39,7 @@ RSpec.describe DiscourseConnect do end def new_discourse_sso - DiscourseConnect.new(secure_session: secure_session) + DiscourseConnect.new(secure_session: server_session) end def test_parsed(parsed, sso) @@ -77,7 +77,7 @@ RSpec.describe DiscourseConnect do end let(:ip_address) { "127.0.0.1" } - let(:secure_session) { SecureSession.new("abc") } + let(:server_session) { ServerSession.new("abc") } it "bans bad external id" do sso = new_discourse_sso @@ -756,13 +756,13 @@ RSpec.describe DiscourseConnect do end it "validates nonce" do - _, payload = DiscourseConnect.generate_url(secure_session: secure_session).split("?") + _, payload = DiscourseConnect.generate_url(secure_session: server_session).split("?") - sso = DiscourseConnect.parse(payload, secure_session: secure_session) + sso = DiscourseConnect.parse(payload, secure_session: server_session) expect(sso.nonce_valid?).to eq true other_session_sso = - DiscourseConnect.parse(payload, secure_session: SecureSession.new("differentsession")) + DiscourseConnect.parse(payload, secure_session: ServerSession.new("differentsession")) expect(other_session_sso.nonce_valid?).to eq false sso.expire_nonce! @@ -772,13 +772,13 @@ RSpec.describe DiscourseConnect do it "allows disabling CSRF protection" do SiteSetting.discourse_connect_csrf_protection = false - _, payload = DiscourseConnect.generate_url(secure_session: secure_session).split("?") + _, payload = DiscourseConnect.generate_url(secure_session: server_session).split("?") - sso = DiscourseConnect.parse(payload, secure_session: secure_session) + sso = DiscourseConnect.parse(payload, secure_session: server_session) expect(sso.nonce_valid?).to eq true other_session_sso = - DiscourseConnect.parse(payload, secure_session: SecureSession.new("differentsession")) + DiscourseConnect.parse(payload, secure_session: ServerSession.new("differentsession")) expect(other_session_sso.nonce_valid?).to eq true sso.expire_nonce! @@ -787,18 +787,18 @@ RSpec.describe DiscourseConnect do end it "generates a correct sso url" do - url, payload = DiscourseConnect.generate_url(secure_session: secure_session).split("?") + url, payload = DiscourseConnect.generate_url(secure_session: server_session).split("?") expect(url).to eq discourse_connect_url - sso = DiscourseConnect.parse(payload, secure_session: secure_session) + sso = DiscourseConnect.parse(payload, secure_session: server_session) expect(sso.nonce).to_not be_nil end describe "nonce error" do it "generates correct error message when nonce has already been used" do - _, payload = DiscourseConnect.generate_url(secure_session: secure_session).split("?") + _, payload = DiscourseConnect.generate_url(secure_session: server_session).split("?") - sso = DiscourseConnect.parse(payload, secure_session: secure_session) + sso = DiscourseConnect.parse(payload, secure_session: server_session) expect(sso.nonce_valid?).to eq true sso.expire_nonce! @@ -806,9 +806,9 @@ RSpec.describe DiscourseConnect do end it "generates correct error message when nonce is expired" do - _, payload = DiscourseConnect.generate_url(secure_session: secure_session).split("?") + _, payload = DiscourseConnect.generate_url(secure_session: server_session).split("?") - sso = DiscourseConnect.parse(payload, secure_session: secure_session) + sso = DiscourseConnect.parse(payload, secure_session: server_session) expect(sso.nonce_valid?).to eq true Discourse.cache.delete(sso.used_nonce_key) @@ -819,9 +819,9 @@ RSpec.describe DiscourseConnect do it "generates correct error message when nonce is expired, and csrf protection disabled" do SiteSetting.discourse_connect_csrf_protection = false - _, payload = DiscourseConnect.generate_url(secure_session: secure_session).split("?") + _, payload = DiscourseConnect.generate_url(secure_session: server_session).split("?") - sso = DiscourseConnect.parse(payload, secure_session: secure_session) + sso = DiscourseConnect.parse(payload, secure_session: server_session) expect(sso.nonce_valid?).to eq true Discourse.cache.delete(sso.used_nonce_key) diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 23184da4c19..1562cd578b4 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -119,10 +119,8 @@ RSpec.describe SessionController do expect(response_body_parsed["allowed_credential_ids"]).to eq( [user_security_key.credential_id], ) - secure_session = SecureSession.new(session["secure_session_id"]) - expect(response_body_parsed["challenge"]).to eq( - DiscourseWebauthn.challenge(user, secure_session), + DiscourseWebauthn.challenge(user, request.server_session), ) expect(DiscourseWebauthn.rp_id).to eq("localhost") end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 42b7dce41c7..87e319c9294 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -22,6 +22,8 @@ RSpec.describe UsersController do before { SiteSetting.hide_email_address_taken = false } describe "#full account registration flow" do + let(:server_session) { request.server_session } + it "will correctly handle honeypot and challenge" do get "/session/hp.json" expect(response.status).to eq(200) @@ -37,10 +39,8 @@ RSpec.describe UsersController do password: SecureRandom.hex, } - secure_session = SecureSession.new(session["secure_session_id"]) - - expect(secure_session[UsersController::HONEYPOT_KEY]).to eq(json["value"]) - expect(secure_session[UsersController::CHALLENGE_KEY]).to eq(json["challenge"]) + expect(server_session[UsersController::HONEYPOT_KEY]).to eq(json["value"]) + expect(server_session[UsersController::CHALLENGE_KEY]).to eq(json["challenge"]) post "/u.json", params: params @@ -50,8 +50,8 @@ RSpec.describe UsersController do expect(jane.email).to eq("jane@jane.com") - expect(secure_session[UsersController::HONEYPOT_KEY]).to eq(nil) - expect(secure_session[UsersController::CHALLENGE_KEY]).to eq(nil) + expect(server_session[UsersController::HONEYPOT_KEY]).to eq(nil) + expect(server_session[UsersController::CHALLENGE_KEY]).to eq(nil) end end @@ -627,8 +627,7 @@ RSpec.describe UsersController do end it "stages a webauthn challenge for the user" do - secure_session = SecureSession.new(session["secure_session_id"]) - expect(DiscourseWebauthn.challenge(user1, secure_session)).not_to eq(nil) + expect(DiscourseWebauthn.challenge(user1, request.server_session)).not_to be_blank end it "changes password with valid security key challenge and authentication" do diff --git a/spec/support/integration_helpers.rb b/spec/support/integration_helpers.rb index 3937edecd65..1190d018d64 100644 --- a/spec/support/integration_helpers.rb +++ b/spec/support/integration_helpers.rb @@ -38,7 +38,7 @@ module IntegrationHelpers def read_secure_session id = begin - session[:secure_session_id] + session[:server_session_id] rescue NoMethodError nil end @@ -46,7 +46,7 @@ module IntegrationHelpers # This route will init the secure_session for us get "/session/hp.json" if id.nil? - SecureSession.new(session[:secure_session_id]) + ServerSession.new(session[:server_session_id]) end def write_secure_session(key, value)