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

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

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

ryan0x44
Copy link
Contributor

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

dev-xo commented Apr 18, 2024

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

mw10013 commented Apr 18, 2024

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

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.

@dev-xo dev-xo requested review from mw10013 and dev-xo April 19, 2024 03:15
@mw10013 mw10013 merged commit d1f10a6 into dev-xo:main Apr 19, 2024
4 checks passed
@ryan0x44 ryan0x44 deleted the cloudflare-context branch April 22, 2024 04:29
@ryan0x44
Copy link
Contributor Author

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

@dev-xo dev-xo changed the title Support passing AppLoadContext as context to sendTOTP and verify [ Feat ] Support AppLoadContext as context to sendTOTP and verify. May 6, 2024
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.

None yet

3 participants