Minor fix and increase test coverage.

[joel]

The User should be created only if we are sure the registration is successful.

[santiagorodriguez96]

Hey @joel!

Thank you so much for the fix and also for taking the time to add more tests! 💯

Left a couple of comments but looks good to me overall 🙂

[santiagorodriguez96]

What do you think about using a similar approach to the one used in the already existent tests?

  test "should register successfully" do
    raw_challenge = SecureRandom.random_bytes(32)
    challenge = WebAuthn.configuration.encoder.encode(raw_challenge)

    WebAuthn::PublicKeyCredential::CreationOptions.stub_any_instance(:raw_challenge, raw_challenge) do
      post registration_url, params: { registration: { username: "alice" }, format: :json }

      assert_response :success
    end

    public_key_credential =
      WebAuthn::FakeClient
      .new(Rails.configuration.webauthn_origin)
      .create(challenge:, user_verified: true)

    webauthn_credential = WebAuthn::Credential.from_create(public_key_credential)

    assert_difference 'User.count', +1 do
      assert_difference 'Credential.count', +1 do
        post(
          callback_registration_url,
          params: { credential_nickname: "USB Key" }.merge(public_key_credential)
        )
      end
    end

    assert_response :success
  end

[joel]

Sure, we can re-use your way for consistency's sake.

That PR is an extract from my rewrite of your demo. I'm trying to understand how Passkey is implemented.

In my test, I mocked the relying_party itself so I could call the controller as normal.

In your test, you mock WebAuthn::PublicKeyCredential::CreationOptions, which is unclear to me how it makes verify_registration pass as that is an internal component of WebAuthn that the test should not be aware of.

describe "POST #callback" do

  context "with valid params" do

    let(:fake_rp) do
      WebAuthn::RelyingParty.new(
        origin: "https://fake.relying_party.test",
        id: "fake.relying_party.test",
        name: "Fake RelyingParty"
      )
    end

    let(:fake_client) do
      WebAuthn::FakeClient.new("https://fake.relying_party.test", authenticator:)
    end

    it "registers successfully" do
      fake_challenge = WebAuthn.configuration.encoder.encode(SecureRandom.random_bytes(32))

      webauthn_credential = fake_client.create(challenge: fake_challenge, rp_id: fake_rp.id, user_verified: true)

      allow(controller).to receive(:relying_party).and_return(fake_rp)

      post(:callback,
              params: {
                credential_nickname: "John's Security Key",
                user: user.to_h.slice(:id, :name),
                authenticator_selection: { user_verification: "required" }
              }.merge(webauthn_credential),
              session:,
              format: :json
            )

      expect(response).to be_successful
    end
  end
end

[santiagorodriguez96]

Hey!

Sure, I can explain 🙂

In essence, both our approaches are just different approaches for mocking the challenge that's essential for the verifying process of WebAuthn to succeed. The challenge is a critical part of the registration and authentication process, as it used for preventing replying attacks.

In the tests that we have on the demo, we are mocking WebAuthn::PublicKeyCredential::CreationOptions because we are hitting the create action of the RegistrationController which calls RelyingParty#options_for_registration, asks for the challenge to the returned object and stores that challenge on the session. That 'returned object' is simply a WebAuthn::PublicKeyCredential::CreationOptions (RelyingParty#options_for_registration calls WebAuthn::Credential.options_for_create which returns a new WebAuthn::PublicKeyCredential::CreationOptions with the params received) and that is why we mock any instance of it to receive challenge and return a fixed challenge that we have created before.

The crucial point to understand here is that by mocking WebAuthn::PublicKeyCredential::CreationOptions, we directly influence the challenge that's going to be stored in the session. This is essential as we then use the fake client to create a credential using that same challenge and send those params to the callback action, ensuring that both the credential that will come in the request params and the session will have the same challenge.

On the other hand, in your approach, you achieve the same result by simply mocking the session to have the same fixed challenge that you use for creating the credential (using the fake client) so that you ensure that both the credential that will come in the request params and the session will have the same challenge.

Worth noting that the reason that you mock the relying party is because you need the relying party's origin to match your fake client's origin. We are currently doing that by simply instantiating the fake client with our environment's origin which is the same that will be used for instantiating the relying party in the controller.

Having said that, the important thing to note is that both approaches work so feel free to use the one that works best for you 🙂

I hope I explained myself! Feel free to reach out if you have any more questions or need further clarification.

[santiagorodriguez96]

What do you think about also adding a test where the crendential.verify method fails and ensure that the user is not created?

[joel]

I've covered it in my test, I just wasn't at ease doing it in Minitest.

describe "POST #callback" do
  context "with invalid params" do
    let(:fake_rp) do
      instance_double(WebAuthn::FakeRelyingParty)
    end

    it "renders a response with 422 status" do
      allow(fake_rp).to receive(:verify_registration).and_raise(WebAuthn::Error, "WebAuthn::UserVerifiedVerificationError")
      allow(controller).to receive(:relying_party).and_return(fake_rp)

      session = { current_registration: { challenge: "Challenge" } }

      expect do
        expect do
          post :callback, params: { registration: invalid_attributes }, session:
        end.not_to change(User, :count)
      end.not_to change(Credential, :count)

      expect(response).to have_http_status(:unprocessable_entity)

      expect(response.parsed_body).to eq({ "error" => "Verification failed: WebAuthn::UserVerifiedVerificationError" })
    end
  end
end

[santiagorodriguez96]

Cool, then don't worry about it. I'll see what I can do 🙂

[joel]

Hey @joel!

Thank you so much for the fix and also for taking the time to add more tests! 💯

Left a couple of comments but looks good to me overall 🙂

Thanks to you for providing a working example that is cover by tests too 🙂

[joel]

@santiagorodriguez96 feel free to adapt that PR to your code style, so it ensure consistency and clarity.

[santiagorodriguez96]

Cool! Please just update the test in order to be consistent with the other test and I'll proceed to merge 🙂

The CI is failing too so we should probably fix that too!

[joel]

Thanks for the explanations; indeed, that requires knowing and understanding WebAuthn internally.

Cool! Please just update the test in order to be consistent with the other test, and I'll proceed to merge 🙂

The CI is failing, too, so we should probably fix that, too!

I will, likely on Friday. Thanks 🙏

[joel]

@santiagorodriguez96, I've slightly tweaked @EmilioCristalli's work on Use Rails 7.1 defaults #172 as part of that PR, as that code was conflicting and I feel like is better to keep hash access consistent. NOTE: We could use ActiveSupport::HashWithIndifferentAccess too.

both

current_registration = session[:current_registration].deep_symbolize_keys

current_registration = session[:current_registration].with_indifferent_access

would work the same.

[santiagorodriguez96]

@santiagorodriguez96, I've slightly tweaked @EmilioCristalli's work on Use Rails 7.1 defaults #172 as part of that PR, as that code was conflicting and I feel like is better to keep hash access consistent. NOTE: We could use ActiveSupport::HashWithIndifferentAccess too.

both

current_registration = session[:current_registration].deep_symbolize_keys

current_registration = session[:current_registration].with_indifferent_access

would work the same.

The tests are failing after those changes 😕

I understand that there's an inconsistency and I appreciate you taking the time to fixing it, but to be honest neither using deep_symbolize_keys or with_indifferent_access seems like the right approach for me. If we want to keep it consistent I'd simply go with strings for all the keys. Either way, there's still more controllers that have this inconsistency (so we need more changes) so what do you think about just leave it as it was in master so that we make the tests pass again and then I'll open up a follow up PR for removing this inconsistency?

[joel]

As the CI is not triggered when I push the changes, but only when you approve the PR I can't do it on the fly. tests passed on my machine.

(improve-tests)> bundle exec rake
Run options: --seed 30948

# Running:

........

Finished in 0.120516s, 66.3812 runs/s, 174.2507 assertions/s.
8 runs, 21 assertions, 0 failures, 0 errors, 0 skips
Running RuboCop...
The following cops were added to RuboCop, but are not configured. Please set Enabled to either `true` or `false` in your `.rubocop.yml` file.

Please also note that you can opt-in to new cops by default by adding this to your config:
  AllCops:
    NewCops: enable

Lint/UselessNumericOperation: # new in 1.66
  Enabled: true
Rails/EnumSyntax: # new in 2.26
  Enabled: true
For more information: https://docs.rubocop.org/rubocop/versioning.html
Inspecting 33 files
.................................

33 files inspected, no offenses detected

Tip: Based on detected gems, the following RuboCop extension libraries might be helpful:
  * rubocop-capybara (https://rubygems.org/gems/rubocop-capybara)

You can opt out of this message by adding the following to your config (see https://docs.rubocop.org/rubocop/extensions.html#extension-suggestions for more options):
  AllCops:
    SuggestExtensions: false

I will see if I have some slack this week to change that.

[EmilioCristalli]

@joel you might need to run system tests locally to see the failure bin/rails test:system

[joel]

@EmilioCristalli good shot

bin/rails test:system
Run options: --seed 12042

# Running:

Capybara starting Puma...
* Version 6.4.3, codename: The Eagle of Durango
* Min threads: 0, max threads: 4
* Listening on http://127.0.0.1:3030
* Listening on http://[::1]:3030
.2024-10-14 16:32:19 +0200 Rack app ("POST /session/callback" - (::1)): #<NoMethodError: undefined method `deep_symbolize_keys' for nil>
[Screenshot Image]: /Users/joel/Workspace/Workanywhere/webauthn-rails-demo-app/tmp/screenshots/failures_test_register_and_then_sign_in.png
E

Error:
SignInTest#test_register_and_then_sign_in:
NoMethodError: undefined method `deep_symbolize_keys' for nil
    app/controllers/sessions_controller.rb:29:in `callback'


bin/rails test test/system/sign_in_test.rb:7

.

Finished in 3.891466s, 0.7709 runs/s, 2.3128 assertions/s.
3 runs, 9 assertions, 0 failures, 1 errors, 0 skips

That will be easier to fix with it.

[joel]

@santiagorodriguez96, Okay, I removed the change, and tests passed on my machine and with the system, too.

[joel]

I recommend to squash this one, I don't think I can squash the commits from a Fork

[EmilioCristalli]

thanks @joel !

[santiagorodriguez96]

Thank you so much @joel!