diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 33f2db264c8..ba705c123bf 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1550,8 +1550,8 @@ class UsersController < ApplicationController render json: success_json.merge( challenge: challenge_session.challenge, - rp_id: challenge_session.rp_id, - rp_name: challenge_session.rp_name, + rp_id: DiscourseWebauthn.rp_id, + rp_name: DiscourseWebauthn.rp_name, supported_algorithms: ::DiscourseWebauthn::SUPPORTED_ALGORITHMS, user_secure_id: current_user.create_or_fetch_secure_identifier, existing_active_credential_ids: @@ -1568,7 +1568,7 @@ class UsersController < ApplicationController current_user, params, challenge: DiscourseWebauthn.challenge(current_user, secure_session), - rp_id: DiscourseWebauthn.rp_id(current_user, secure_session), + rp_id: DiscourseWebauthn.rp_id, origin: Discourse.base_url, ).register_second_factor_security_key render json: success_json diff --git a/app/models/concerns/second_factor_manager.rb b/app/models/concerns/second_factor_manager.rb index d173a3d7dc3..68b3c48f6e0 100644 --- a/app/models/concerns/second_factor_manager.rb +++ b/app/models/concerns/second_factor_manager.rb @@ -167,7 +167,7 @@ module SecondFactorManager self, security_key_credential, challenge: DiscourseWebauthn.challenge(self, secure_session), - rp_id: DiscourseWebauthn.rp_id(self, secure_session), + rp_id: DiscourseWebauthn.rp_id, origin: Discourse.base_url, ).authenticate_security_key end diff --git a/lib/discourse_webauthn.rb b/lib/discourse_webauthn.rb index 800c60931b8..2742cfd51b1 100644 --- a/lib/discourse_webauthn.rb +++ b/lib/discourse_webauthn.rb @@ -59,26 +59,23 @@ module DiscourseWebauthn credential_ids = user.second_factor_security_key_credential_ids { allowed_credential_ids: credential_ids, - challenge: - secure_session[ - DiscourseWebauthn::ChallengeGenerator::ChallengeSession.session_challenge_key(user) - ], + challenge: secure_session[self.session_challenge_key(user)], } end - def self.rp_id(user, secure_session) - secure_session[DiscourseWebauthn::ChallengeGenerator::ChallengeSession.session_rp_id_key(user)] - end - - def self.rp_name(user, secure_session) - secure_session[ - DiscourseWebauthn::ChallengeGenerator::ChallengeSession.session_rp_name_key(user) - ] - end - def self.challenge(user, secure_session) - secure_session[ - DiscourseWebauthn::ChallengeGenerator::ChallengeSession.session_challenge_key(user) - ] + secure_session[self.session_challenge_key(user)] + end + + def self.rp_id + Discourse.current_hostname + end + + def self.rp_name + SiteSetting.title + end + + def self.session_challenge_key(user) + "staged-webauthn-challenge-#{user&.id}" end end diff --git a/lib/webauthn/challenge_generator.rb b/lib/webauthn/challenge_generator.rb index f6704bd94dc..988efedc367 100644 --- a/lib/webauthn/challenge_generator.rb +++ b/lib/webauthn/challenge_generator.rb @@ -2,41 +2,21 @@ module DiscourseWebauthn class ChallengeGenerator class ChallengeSession - attr_reader :challenge, :rp_id, :rp_name + attr_reader :challenge def initialize(params) @challenge = params[:challenge] - @rp_id = params[:rp_id] - @rp_name = params[:rp_name] end def commit_to_session(secure_session, user) - secure_session[self.class.session_challenge_key(user)] = @challenge - secure_session[self.class.session_rp_id_key(user)] = @rp_id - secure_session[self.class.session_rp_name_key(user)] = @rp_name + secure_session[DiscourseWebauthn.session_challenge_key(user)] = @challenge self end - - def self.session_challenge_key(user) - "staged-webauthn-challenge-#{user&.id}" - end - - def self.session_rp_id_key(user) - "staged-webauthn-rp-id-#{user&.id}" - end - - def self.session_rp_name_key(user) - "staged-webauthn-rp-name-#{user&.id}" - end end def self.generate - ChallengeSession.new( - challenge: SecureRandom.hex(30), - rp_id: Discourse.current_hostname, - rp_name: SiteSetting.title, - ) + ChallengeSession.new(challenge: SecureRandom.hex(30)) end end end diff --git a/spec/lib/webauthn/challenge_generator_spec.rb b/spec/lib/webauthn/challenge_generator_spec.rb index faa0bb5f156..c505bcbdd85 100644 --- a/spec/lib/webauthn/challenge_generator_spec.rb +++ b/spec/lib/webauthn/challenge_generator_spec.rb @@ -1,19 +1,17 @@ # frozen_string_literal: true RSpec.describe DiscourseWebauthn::ChallengeGenerator do - it "generates a DiscourseWebauthn::ChallengeGenerator::ChallengeSession with correct params" do + it "generates a DiscourseWebauthn::ChallengeGenerator::ChallengeSession with a challenge" do session = DiscourseWebauthn::ChallengeGenerator.generate expect(session).to be_a(DiscourseWebauthn::ChallengeGenerator::ChallengeSession) expect(session.challenge).not_to eq(nil) - expect(session.rp_id).to eq(Discourse.current_hostname) - expect(session.rp_name).to eq(SiteSetting.title) end describe "ChallengeSession" do describe "#commit_to_session" do let(:user) { Fabricate(:user) } - it "stores the challenge, rp id, and rp name in the provided session object" do + it "stores the challenge in the provided session object" do secure_session = {} generated_session = DiscourseWebauthn::ChallengeGenerator.generate generated_session.commit_to_session(secure_session, user) @@ -21,10 +19,6 @@ RSpec.describe DiscourseWebauthn::ChallengeGenerator do expect(secure_session["staged-webauthn-challenge-#{user&.id}"]).to eq( generated_session.challenge, ) - expect(secure_session["staged-webauthn-rp-id-#{user&.id}"]).to eq(generated_session.rp_id) - expect(secure_session["staged-webauthn-rp-name-#{user&.id}"]).to eq( - generated_session.rp_name, - ) end end end diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 36f8c2382f9..81304c1b1a9 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -120,7 +120,7 @@ RSpec.describe SessionController do expect(response_body_parsed["challenge"]).to eq( DiscourseWebauthn.challenge(user, secure_session), ) - expect(DiscourseWebauthn.rp_id(user, secure_session)).to eq(Discourse.current_hostname) + expect(DiscourseWebauthn.rp_id).to eq(Discourse.current_hostname) end end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index d84d4a446c3..410e9b1542f 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -457,10 +457,9 @@ RSpec.describe UsersController do end end - it "stages a webauthn challenge and rp-id for the user" do + 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.rp_id(user1, secure_session)).to eq(Discourse.current_hostname) end it "changes password with valid security key challenge and authentication" do @@ -5659,8 +5658,8 @@ RSpec.describe UsersController do secure_session = read_secure_session response_parsed = response.parsed_body expect(response_parsed["challenge"]).to eq(DiscourseWebauthn.challenge(user1, secure_session)) - expect(response_parsed["rp_id"]).to eq(DiscourseWebauthn.rp_id(user1, secure_session)) - expect(response_parsed["rp_name"]).to eq(DiscourseWebauthn.rp_name(user1, secure_session)) + expect(response_parsed["rp_id"]).to eq(DiscourseWebauthn.rp_id) + expect(response_parsed["rp_name"]).to eq(DiscourseWebauthn.rp_name) expect(response_parsed["user_secure_id"]).to eq( user1.reload.create_or_fetch_secure_identifier, ) diff --git a/spec/support/webauthn_integration_helpers.rb b/spec/support/webauthn_integration_helpers.rb index e5648f4649f..a7a646721ab 100644 --- a/spec/support/webauthn_integration_helpers.rb +++ b/spec/support/webauthn_integration_helpers.rb @@ -16,7 +16,7 @@ module DiscourseWebauthnIntegrationHelpers # # This is because the challenge is embedded # in the post data's authenticatorData and must match up. See - # simulate_localhost_webautn_challenge for a real example. + # simulate_localhost_webauthn_challenge for a real example. def valid_security_key_data { credential_id: @@ -67,7 +67,6 @@ module DiscourseWebauthnIntegrationHelpers DiscourseWebauthn::ChallengeGenerator.stubs(:generate).returns( DiscourseWebauthn::ChallengeGenerator::ChallengeSession.new( challenge: valid_security_key_challenge_data[:challenge], - rp_id: Discourse.current_hostname, ), ) end