Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reauthentication documentation #25

Merged

Conversation

Vagab
Copy link
Contributor

@Vagab Vagab commented Jun 23, 2023

resolves #24

@Vagab Vagab marked this pull request as ready for review June 23, 2023 11:07
@Vagab
Copy link
Contributor Author

Vagab commented Jun 23, 2023

@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?

@Vagab Vagab changed the title Add top level documentation for Reauthentication concern Reauthentication documentation Jun 23, 2023
@tcannonfodder
Copy link
Contributor

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
Copy link
Contributor Author

Vagab commented Jun 23, 2023

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
Copy link
Contributor

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

@tcannonfodder tcannonfodder force-pushed the documentation/reauthentication branch from 6146edc to e7ddbc3 Compare June 24, 2023 21:57
* expanding explanatiuon, copy edits, adding references & symbols
* Outlining the controller actions
* Updating example code
* Adding references to other methods in both `Warden::WebAuthn` and the
	included concerns
* Making protected methods visibile in the documentation to highlight
	their importance and purpose
Copy link
Contributor

@tcannonfodder tcannonfodder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just waiting on CI, but this is good to go when green!

Thanks so much for doing the heavy lifting here; there were only a few edits & symbol references to add. And we made some great refactors through the process 💪🎉💪

Looking forward to the next PR!

@tcannonfodder tcannonfodder merged commit b00ffe6 into ruby-passkeys:main Jun 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation for reauthentication
2 participants