dev-xo / remix-auth-totp

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

[ Feat ]: Pass a pre-read FormData object via context. #60

Closed themkvz closed 6 months ago

themkvz commented 6 months ago

When I try to read FormData (for validation or something else) in action I get an error FetchError: Invalid response body while trying to fetch http://localhost:5173/login: Invalid state: ReadableStream is locked

login.tsx

export async function action ({ request }: ActionFunctionArgs) {
  const formData = await request.formData()
  const type = formData.get('type') as string

  if (!type) {
    return json({ message: 'Type is required' }, { status: 400 })
  }

  const successRedirect = type === 'form' ? '/account' : '/verify'

  await authenticator.authenticate(type, request, {
    successRedirect,
    failureRedirect: '/login',
    context: { formData }
  })
}

As a solution, I added simple checking if we have already passed formData via options

private async _readFormData(request: Request, options: AuthenticateOptions) {
    if (request.method !== 'POST') {
      return new FormData()
    }

    if (options.context?.formData instanceof FormData) {
      return options.context.formData
    }

    return await request.formData()
  }
dev-xo commented 6 months ago

Hello @themkvz.

We used to have an example on how to handle errors with remix-auth, sadly removed it some time ago. Probably looking into remix-auth examples and how to handle errors could help with your current situation?

Can you please provide more details of your case and how the Pull Request will affect the library?

Two more things:

mw10013 commented 6 months ago

@themkvz: Thank you for the PR. remix-auth-totp is designed to take in a pristine request. We're cautious about providing another mechanism to convey the formData so as not to add more complexity to the API.

I wonder if Request.clone() may be helpful for your use case (https://developer.mozilla.org/en-US/docs/Web/API/Request/clone). Please share your thoughts.

It is curious to see logic in the action of your login route that may redirect to an /account route. Upon success the login action would generally redirect to a verify route so that the user can input the totp code as in the example project snippet below.

https://github.com/dev-xo/totp-starter-example/blob/4f14dbc1696a81827f2708a88e1e16beec176edc/app/routes/login.tsx#L26-L39

themkvz commented 6 months ago

Thank you for your response!

@dev-xo I will revert pnpm-lock.yaml. Which indents do you mean? My VSCode uses your . prettierrc config, with "tabWidth": 2 and "singleQuote": true

@mw10013 I use multiple forms on a single remix page following the next way https://sergiodxa.com/articles/multiple-forms-per-route-in-remix (using hidden input with name="type"), I need two forms because, in my Login page, I have two auth methods/strategy (authenticator.use(totpStrategy, "TOTP").use(formStrategy, "form");) – first is a traditional method with user email and user password, and second one is your based on the magic link. Depending on the filled form we need to pass the correct type to authenticator.authenticate Provided solution with a pre-read FormData object I use from this package https://github.com/sergiodxa/remix-auth-form?tab=readme-ov-file#passing-a-pre-read-formdata-object

mw10013 commented 6 months ago

@themkvz: Thanks for providing more color on your use case. It is helpful and compelling.

Can you add a test for this? Also, a small section at the end of docs/customization.md explaining that this exists and why you might need it. Thanks!

dev-xo commented 6 months ago

Any news on this @themkvz? Otherwise I think we can close it for now, and get back to it later at some point.

themkvz commented 6 months ago

Any news on this @themkvz? Otherwise I think we can close it for now, and get back to it later at some point.

I have already added two simple tests and I plan to add a short section with an explanation in the docs

dev-xo commented 6 months ago

Thanks @themkvz, happy to know this is still active. Take your time and let us know when we can review it.

themkvz commented 6 months ago

@dev-xo I've added the brief description to the README.md Before pushing I ran format script (pnpm run format) and I see some additional changes in README.md, if it is a problem – let me know

About test: I've already added two short tests

dev-xo commented 6 months ago

To be clear @themkvz, your current issue (and PR) could be solved by cloning the request like request.clone(), or not?

Also, these changes will now imply that passing a pre-read FormData to the authenticate method will be required, right?

themkvz commented 6 months ago

To be clear @themkvz, your current issue (and PR) could be solved by cloning the request like request.clone(), or not?

It's an additional option

Because in the Remix docs, for example, here we have an example of how to use request.formData(), on the other hand, the search through the Remix docs for request.clone() does not give anything, in the whole the Remix docs any mentions about request.clone() (I understand that is the native Web API).

The user has two options (if he has this specific case with more than one strategy on the same page and needs validate some form fields before passing the request):

  1. Use const formData = await request.clone().formData()
  2. Use the usual const formData = await request.formData() and pass a pre-read FormData to the context

Also, these changes will now imply that passing a pre-read FormData to the authenticate method will be required, right?

If you need to validate FormData, yes – it will be required


I opened this PR because I saw the section on how of works with a pre-read FormData in another remix-auth strategy, that's all)

dev-xo commented 6 months ago

All good @themkvz.

I was wondering if we could simply document this in the docs, letting users know that if they are looking to handle multiple auth strategies or validate the form, they could simply clone the request.

Ideally, the simpler we keep the strategy, the fewer bugs we could introduce, and the easier it will be to maintain it.

I appreciate the efforts in improving the strategy and making it simpler for everyone else to handle this situation or similar ones, so thank you!

themkvz commented 6 months ago

All good @themkvz.

I was wondering if we could simply document this in the docs, letting users know that if they are looking to handle multiple auth strategies or validate the form, they could simply clone the request.

Ideally, the simpler we keep the strategy, the fewer bugs we could introduce, and the easier it will be to maintain it.

  • I'll leave this decision to @mw10013, as he probably has more insights on how useful this could be overall.

I appreciate the efforts in improving the strategy and making it simpler for everyone else to handle this situation or similar ones, so thank you!

Yeah, sure, it will be maybe the best way to post this possibility in the docs I had some struggle when I got the next error FetchError: Invalid response body while trying to fetch http://localhost:5173/login: Invalid state: ReadableStream is locked :)

mw10013 commented 6 months ago

@themkvz: Thanks for the additions. We'd like to proceed with this. I ran gh pr checkout 60 locally and made a few local edits that I want to push to your PR with git push https://github.com/themkvz/remix-auth-totp.git HEAD. I'm getting

 ! [remote rejected] HEAD -> themkvz/main (permission denied)
error: failed to push some refs to 'https://github.com/themkvz/remix-auth-totp.git'

Can you grant me permission? Thanks.

dev-xo commented 6 months ago

Reverted it back, as it was not squashed and the whole history commit was cluttered with small unnecessary commits.

Otherwise, I will open a PR myself with the updated changes and push it.

dev-xo commented 6 months ago

64 Squashed, merged & published v3.3.0.

Let me know if this settles the issue for you @themkvz. Also, thanks for the contribution! 👍🏻