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 ]: Phone Support #65

Closed shawnmclean closed 4 months ago

shawnmclean commented 5 months ago

Summary

Adding phone number support for whatsapp and sms TOTP verification.

Notes

This is the first draft of the model changes for fast feedback.

References

49

shawnmclean commented 5 months ago

Hi guys,

Submitting a draft of the proposed model changes.

This is a pretty simple idea that does not break anything, but adds an additional property for the phone number support.

Please give your thoughts on the go-ahead of full implementation.

dev-xo commented 5 months ago

Hello @shawnmclean!

Will share my thoughts after checking the implementation, but overall I would let @mw10013 take the final decision as he understands the whole authentication strategy better than me at this point.

Thank you for the PR!

dev-xo commented 5 months ago

Checked the implementation @shawnmclean, looks good to me.

Any way we could have a phone number validator and the tests for the implementation? For me, this could be a merge if we are able to ensure:

https://github.com/dev-xo/remix-auth-totp/blob/d98d89fcd47e9ef27c36e08684c9fc843659a5d9/src/index.ts#L239 https://github.com/dev-xo/remix-auth-totp/blob/d98d89fcd47e9ef27c36e08684c9fc843659a5d9/src/index.ts#L312

mw10013 commented 5 months ago

@shawnmclean: Thanks for the PR! I apologize for being a little late to the discussion.

Can you outline your use case for this so I have a better understanding. remix-auth-totp is intended to authenticate email addresses by having the user prove they have access to the email sent out. If email and phone are specified, a user could specify a phone unrelated to the email and gain access to the account through an unauthenticated email.

shawnmclean commented 5 months ago

@mw10013 Thanks for that clarity.

The intention is to add support to authenticate phone numbers, similar to emails.

The change is to make accepting emails and phone numbers optional.

To make the change unbreaking, if both are passed in, it will prioritize sending the TOTP to the email. If no email, it will use the phone to authenticate.

The usage of which one to pass in, is up to how the consumer of the lib uses it, and how their UI is structured. Then how they store the identifiers, ie. multiple emails/phone numbers per account.

The change is to add an additional use case, so the lib now becomes:

remix-auth-totp is intended to authenticate email addresses or phone numbers by having the user prove they have access to the medium it was sent, such as email, sms, whatsapp or any other sending strategy they wish.

My question would be these:

What happens if the user passes in both email and phone? Should we allow that? Both are optional, but we must have one. If allow both, is there an order of priority?

From my analysis of the code, it may be easy to allow only 1.

dev-xo commented 5 months ago

Allowing only one will be our best move here, either email or phone, in order to keep a simple UI and avoid possible issues that could arise from having both.

We will need to document all this by providing the user a few options they could leverage:

As I mentioned before, this looks good to me, but we'll need to pass these checks in order to merge it:

Optional: Creating a new template that could implement these changes in order to make it easy for the user to get started with it (Starter Template could be used for this).


I think having this option for the strategy will not cause anything harmful and could simply improve it and also avoid future requests as there have been a few already on the same topic.


A question:

mw10013 commented 5 months ago

@shawnmclean: Thanks for clarifying.

If I understand correctly, you are not trying to authenticate an email and phone number at the same time. And also not trying to authenticate an email with a phone number or vice-versa. You seem to want to authenticate either an email or a phone number.

Taking a step back, I'm wondering if you were able to authenticate a phone number alone using remix-auth-totp and configuring the regex and error messages to reflect a phone number instead of email. And in the sendTOTP(), you treat the email parameter as a phone number. Please share if you were able to achieve that or what experiements you have conducted in that area.

If that's the case, then I wonder if you are able to run two instances to TOTP in parallel. One for email and the other for phone.

I'd probably lean more toward making remix-auth-totp more generic in the sense that it's simply authenticating a string. Currently, it treats the string as an email, but it could be agnostic about that.

Adding on another specialized string such as phone seems to lead to someone else wanting a different specialized string and so on. If remix-auth-totp were generic then the app could decide what specialized string it wanted to authenticate.

chiptus commented 5 months ago

@mw10013 I was also leaning toward just validating a string. for separation of concerns, I think phone and email (and whatsapp) should have separate config.

if we can use different startegies this might make it simpler. just need to remove the "email" word from the config.

how do we make remix auth use one startegy or the other?

mw10013 commented 5 months ago

@chiptus: Apologies for delayed response. It would be helpful if you could share your specific use case, if you have one. For instance, I'm trying to authenticate email and phone. I tried running 2 instances of remix-auth-totp and these were the speed bumps and roadblocks.

Have you been able to use remix-auth-totp to authenticate a phone by changing options into remix-auth and remix-auth-totp? We'd love to have some field reports on what users have been able to accomplish in this area or where things fell short.

I believe remix-auth can handle running multiple strategies in parallel. And we would love to have some field reports running 2 instances of 'remix-auth-totp` in parallel.

chiptus commented 5 months ago

hey, currently we have only whatsapp verification. this is the code I use:

import type { User } from "@prisma/client";
import { isAxiosError } from "axios";
import { Authenticator } from "remix-auth";
import type { SendTOTPOptions, TOTPVerifyParams } from "remix-auth-totp";
import { TOTPStrategy } from "remix-auth-totp";
import { z } from "zod";

import { TOTP_ENCRYPTION_SECRET } from "~/utils/config.ts";
import { prisma } from "~/utils/prisma.server.ts";
import { sessionStorage } from "./session.server.ts";
// import { sendEmail } from './email.server'
import { sendWhatsapp } from "./totp.server.js";

export let authenticator = new Authenticator<User>(sessionStorage, {
  throwOnError: true,
});

authenticator.use(
  new TOTPStrategy(
    {

      secret: TOTP_ENCRYPTION_SECRET,
      totpGeneration: {
        charSet: "0123456789",
      },
      sendTOTP,
      emailFieldKey: "phone",
      validateEmail: async () => true,
    },
    verifyTOTP,
  ),
);

async function sendTOTP({ code, formData }: SendTOTPOptions): Promise<void> {
  try {
    const method = z
      .union([z.literal("email"), z.literal("whatsapp")])
      .parse(formData?.get("method") ?? "whatsapp");

    // Send the TOTP code to the user.
    if (method === "whatsapp") {
      let phone = z.string().parse(formData?.get("phone"));
      await sendWhatsapp(phone, code);
    }
  } catch (err) {
    if (!isAxiosError(err)) {
      // eslint-disable-next-line no-console
      console.error("sendTOTP", err);
      throw err;
    }
    throw new Error(err.response?.data?.message ?? err.message);
  }
}

async function verifyTOTP({
  formData,
  email: phone,
}: TOTPVerifyParams): Promise<User> {
  const method = z
    .union([z.literal("email"), z.literal("whatsapp")])
    .parse(formData?.get("method") ?? "whatsapp");

  if (method === "whatsapp") {
    let user = await prisma.user.findFirst({
      where: { phone },
    });

    if (!user) {
      user = await prisma.user.create({
        data: {
          phone,
          subscription: {
            create: {
              startDate: new Date(),
              status: "active",
              subscription: {
                connect: {
                  name: "Trial",
                },
              },
            },
          },
        },
      });
    }

    return user;
  }

  throw new Error("Invalid method");
}

I would like to add also email, and Product also wants to replace whatsapp with regular sms

mw10013 commented 5 months ago

@chiptus: Thanks for sharing some code. It really helps to make things concrete.

remix-auth is designed to take multiple strategies and you can, for example, use two instance of remix-auth-totp and just give the second one a different name.

https://github.com/sergiodxa/remix-auth/blob/ab7aef1212680edc0642b3d14a64fdf2a250951b/src/authenticator.ts#L71-L83

/**

Wondering if you can have 1 instance for email and the other for regular sms. If you do try this, please let us know how it goes and if you hit any obstacles. We'd love to see this working.