dev-xo / remix-auth-totp

A Time-Based One-Time Password (TOTP) Authentication Strategy for Remix-Auth.
https://totp.fly.dev
MIT License
398 stars 26 forks source link

[ Feat ] Support AppLoadContext as context to sendTOTP and verify. #53

Closed ryan0x44 closed 5 months ago

ryan0x44 commented 5 months ago

I believe this will solve for #52 by allowing users to pass context down to their sendTOTP and verify functions. I've tested this with success, although I had to remember to pass it in when invoking the authenticate() method.

I've temporarily published this patch to npm at https://www.npmjs.com/package/ryan0x44-remix-auth-totp so if someone wants to quickly test just npm i ryan0x44-remix-auth-totp.

Open to any feedback in helping to get this merged :)

dev-xo commented 5 months ago

Hello @ryan0x44. Thanks for contributing with this PR.

Wasn't the answer from @mw10013 https://github.com/dev-xo/remix-auth-totp/issues/52#issuecomment-2057026776 good enough to solve the current issue? We also provide a Cloudflare template that could help with it.

The current context issue seems to be something that people will reiterate with Cloudflare and remix-auth-totp, so probably merging it will avoid getting into the same thread over and over.


remix-auth seems to pass context to AuthenticateOptions, and if I'm not wrong, this PR passes it to both SendTOTPOptions and TOTPVerifyParams (for convenience of the case I guess).

Will consult this with mw10013, as he was in charge of the v3 and based on that we will take an action on this.

mw10013 commented 5 months ago

@ryan0x44: Thanks for the PR. Can you update the comment for the optional context in both SendTOTPOptions and TOTPVerifyParams to

/**
   * The context object received by the loader or action.
   * Defaults to undefined. 
   * Explicitly include it in the options to authenticate if you need it.
   */

For reference from remix-auth: https://github.com/sergiodxa/remix-auth/blob/610b4852f3e126e8710c2590c3d4c1e290d7e6d2/src/strategy.ts#L47-L51

Can you also update a test(s) to check that a provided context gets passed all the way through? Thanks!

ryan0x44 commented 5 months ago

Thanks for the prompt reply @dev-xo @mw10013 ! I've made the changes requested. Let me know if you'd like to see any other changes.

ryan0x44 commented 5 months ago

Thanks for the merge! I have removed the temporary npm package.