ruby-passkeys / devise-passkeys

Devise extension to use passkeys instead of passwords
MIT License
161 stars 12 forks source link

`Reauthentication` documentation #25

Closed Vagab closed 1 year ago

Vagab commented 1 year ago

resolves #24

Vagab commented 1 year ago

@tcannonfodder while working on this pr, I've stumbled a question: I see we're relying on the users defining the relying_party method(pun intended), which should return the new relying party. However, in WebAuthn::StrategyHelpers there's a defined relying_party which looks into the env. Since the users are going to define the relying_party themselves, what's the purpose of including it in helpers?

tcannonfodder commented 1 year ago

IIRC, the Warden strategy operates in an entirely different context, so it doesn't have access to the relying_party method the user defines. Instead, it pulls from the request's env hash to get the relying party that the user set up as part of the request's preamble

Vagab commented 1 year ago

I think normally that's exactly right, but in our case it seems that the relying_party method is actually included into the controller. Moreover, if you remove the relying_party concern from the template you will get a confusing error:

NameError (undefined local variable or method `env' for #<Users::SessionsController:0x00000000010180>)

which happens bc the #env method was deprecated in rails 5. But that's not the point, the point is that when the relying_party method is missing from the controller, the warden will call it's own relying_party method, and since it's clear that the end user has to define the relying_party method themselves, the warden's relying_party method's purpose eludes me. So with the limited time I had to look at the code I propose the following changes, lmk wyt:

  1. Rename the relying_party method in Warden::WebAuthn
  2. Maybe alias env to be request.env? Or am I missing smth? Taking a quick look at the warden code, the env is supposed to be exactly rack env, so I think either changing env to request.env or just alias_method :env, :request_env if we need it to be precisely env should be good.
tcannonfodder commented 1 year ago

I'm digging into the code, for both; and an alternative is breaking up the Warden::WebAuthn::StrategyHelpers; or adjusting how it's used in devise-passkeys

Technically; the env is a special variable passed to strategies when they're initialized: https://github.com/wardencommunity/warden/blob/88d2f59adf5d650238c1e93072635196aea432dc/lib/warden/strategies/base.rb#L45

What's happening here is that we're including Warden::WebAuthn::StrategyHelpers in non-strategy parts of the codebase because the module has useful helper method, and the idea is:

If we inherit from the StrategyHelpers to get the authentication_challenge_key, parsed_credential, relying_party_key, etc. ; we'll help maintain consistency between the two Rack middlewares.

However, obviously that approach has some sharp edges. From Warden::WebAuthn's perspective; we want to keep the relying_party method the way it is, because in the context of that gem, the naming convention makes sense.

But breaking stuff up too much makes the code a maintenance nightmare. I'm going to see about finding a middle ground; possibly with a Warden::WebAuthn::RackHelpers that is automatically included when StrategyHelpers is included