DEV: Refactor rp_id and rp_name (#23339)

They're both constant per-instance values, there is no need to store them
in the session. This also makes the code a bit more readable by moving
the `session_challenge_key` method up to the `DiscourseWebauthn` module.

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_id: DiscourseWebauthn.rp_id,
-               rp_name: challenge_session.rp_name,
+               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

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

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:
+      challenge: secure_session[self.session_challenge_key(user)],
-        secure_session[
-          DiscourseWebauthn::ChallengeGenerator::ChallengeSession.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[
+    secure_session[self.session_challenge_key(user)]
-      DiscourseWebauthn::ChallengeGenerator::ChallengeSession.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

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[DiscourseWebauthn.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
 
         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(
+      ChallengeSession.new(challenge: SecureRandom.hex(30))
-        challenge: SecureRandom.hex(30),
-        rp_id: Discourse.current_hostname,
-        rp_name: SiteSetting.title,
-      )
     end
   end
 end

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

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

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_id"]).to eq(DiscourseWebauthn.rp_id)
-      expect(response_parsed["rp_name"]).to eq(DiscourseWebauthn.rp_name(user1, secure_session))
+      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,
       )

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